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

Simplify BindingsMap by instead relying on StaticModuleScope for atom type resolution #11955

Open
wants to merge 15 commits into
base: wip/radeusgd/9812-infer-method-calls
Choose a base branch
from

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Dec 31, 2024

Pull Request Description

  • Removing AtomTypeInterface - it was a useful temporary hack, but in the longer term it seemed to not really solve the problem we had.
  • Now storing the types of atom constructor arguments in StaticModuleScope, so I can remove the Expression from BindingsMap. I think that storing IR inside of metadata attached to said IR (which was the reason for introducing cycles) was a bad idea after all.
  • Thus, the TypeRepresentation is greatly simplified - it no longer refences complicated objects like AtomTypeInterface (which had a nice API but ugly internals, and the internals were affecting serialization). Now TypeRepresentation becomes just plain data, a much simpler representation and I think it is closer to what it was supposed to be from the beginning.

Important Notes

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.

@radeusgd radeusgd force-pushed the wip/radeusgd/type-checker-atom-type-in-static-module-scope branch from 31934b9 to 919086a Compare January 2, 2025 18:05
Comment on lines -598 to -606
case class Argument(
name: String,
hasDefaultValue: Boolean,
typReference: Reference[Expression]
) {
def typ(): Option[Expression] = Option(
typReference.get(classOf[Expression])
)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the infamous cycle from the IR - the Expression holding the type of the constructor is no longer needed.

That's because we process this type Expression in StaticModuleScopeAnalysis and save it in StaticModuleScope metadata as an already processed TypeRepresentation - so it is no longer IR that is stored inside of metadata, but a simpler data structure representing our types.

registerFieldGetters(scopeBuilder, atomTypeScope, typ);
}

private TypeRepresentation buildAtomConstructorType(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly moved from the TypeResolver.

@@ -51,8 +51,7 @@ public QualifiedName getAssociatedType() {
* <p>Instances that are assigned this type are built with one of the available constructors, but
* statically we do not necessarily know which one.
*/
record AtomType(QualifiedName fqn, AtomTypeInterface typeInterface)
implements TypeRepresentation {
record AtomType(QualifiedName fqn) implements TypeRepresentation {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our type representation is no longer holding references to some complex AtomTypeInterface structure which internally was holding references to BindingsMap and was overall ugly.

Now the TypeRepresentation is a plain data type. The information about Atom type interface is instead encoded in the StaticModuleScope, like information about method already has been.

@radeusgd radeusgd self-assigned this Jan 2, 2025
@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 2, 2025
@radeusgd radeusgd marked this pull request as ready for review January 2, 2025 19:05
@radeusgd radeusgd changed the title [Draft] Simplify BindingsMap by instead relying on StaticModuleScope for atom type resolution Simplify BindingsMap by instead relying on StaticModuleScope for atom type resolution Jan 2, 2025
@radeusgd
Copy link
Member Author

radeusgd commented Jan 2, 2025

@JaroslavTulach shall we maybe disable the support for cycles in IR persistence now?

I think we are no longer relying on it and there were issues with it from time to time so maybe we should change the code to fail on cycles and keep an eye on us accidentally re-introducing these problematic cycles?

btw. I think the cycles could have been also why @Akirathan could not use equality for comparing IR and had to resort to identity. I'm not sure if we had any other cycles than this one? If not, maybe we could try to come back to equality over identity then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant