Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Poc] Generate IR definitions - superclass approach #11770

Draft
wants to merge 136 commits into
base: develop
Choose a base branch
from

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Dec 4, 2024

Alternative approach to #11267

Pull Request Description

The overall description is the same as in #11267, but this approach tries to generate super classes as suggested in https://github.com/enso-org/enso/pull/11267/files#r1869342527

Important Notes

An example of generated code for JExpression is:

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

This will be important for mapExpressions method implementation
This is the only way to retain 100% backward compatibility.
@Akirathan
Copy link
Member Author

As of b8f6b8f all tests in runtime-parser-processor-tests passes.

An example of generated code for JExpression is:

@JaroslavTulach
Copy link
Member

I think the generated code looks reasonably nice, but:

  • when serializing, we do not want to intialize passData() neither diagnostics() if they were not initialized before
  • probably the @Persistance annotation shall be in the same package and have package private access to these special fields
  • JBindingGen should be abstract
  • builder should return JBinding and not the generated class
  • I'd like to remind you of the WhiningBuilder pattern - better than validate() as it does compile type checks
  • the build() method shall throw NameExc, ExpressionExc and the builder type should be Builder<NameExc extends Exception, ExpressionExc extends Exception>
  • empty builder is initialized with as Builder<MissingNameException, MissingExpressionException>
  • Signature of name method is: Builder<RuntimeException, ExpressionExc> name(String name) - to signal that name has already been provided
  • name.duplicate(...) should be overridden in Name to return Name
  • IR setLocation(Option<IdentifiedLocation> location) should return JBinding
  • JExpression duplicate( should return JBinding
  • showCode may be implemented manually in JBinding - another reason for making JBindingGen abstract
  • missing new line in list.add(returnValue);return scala.jdk.javaapi.CollectionConverters.asScala(list).toList();

As a concept it is nice, but the real challenge is going to be the implementation of real IR element(s).

@JaroslavTulach
Copy link
Member

I see one failure related to Empty and EmptyGen. Are there any other important failures?

@Akirathan
Copy link
Member Author

Unapply & apply

In order to make necessary modifications in runtime-parser as minimal as possible, we could implement unapply methods on Java classes to make it work with Scala deconstruct pattern matching.

0a1f5a8 demonstrates how a Java class JSpecified can implement public static unapply method so it works in Scala deconstruct match. The Scala compiler is happy and the tests succeeds. But the IDE is not happy:
image

I assume VSCode with Metals extension will also not be able to deduce that JSpecified java class can be treated as a case class in this particular case.

The question is: is this really worth it? Wouldn't it be better to just incrementally rewrite pattern matching in Scala that uses deconstruct to simpler variants that just matches on types? For example:

    val callArg = new JSpecified(true, None, lit)
    callArg match {
      case JSpecified(isSynthetic, _, _) =>
        isSynthetic shouldEqual true
    }

Could be rewritten to

    callArg match {
      case specified: JSpecified =>
        specified.isSynthetic shouldEqual true
    }

without any behavioral changes. Moreover, the latter is OK in IDE.

I would like to first create a PR that simplifies pattern matching, for CallArgument, and then continue with the migration. Any strong opinions against @JaroslavTulach @hubertp ?

TL;DR;

Don't try to bend Java classes to look like Scala case classes. Just refactor deconstructing pattern matches in Scala to simpler variants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants