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

Wip/mk/statics #3740

Merged
merged 32 commits into from
Sep 27, 2022
Merged

Wip/mk/statics #3740

merged 32 commits into from
Sep 27, 2022

Conversation

kustosz
Copy link
Contributor

@kustosz kustosz commented Sep 26, 2022

Pull Request Description

Makes statics static. A type and its instances have different methods defined on them, as it should be. Constructors are now scoped in types, and can be imported/exported.

Important Notes

The method of fixing stdlib chosen here is to just not. All the conses are exported to make all old code work. All such instances are marked with TODO Dubious constructor export so that it can be found and fixed.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Only checked the Enso stuff - looks a good place for us to work from.

Comment on lines +14 to 15
foreign js my_method self = """
return this.a + this.b;
Copy link
Member

Choose a reason for hiding this comment

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

The self feels odd - I thought we had a special case for this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do? It doesn't work anyway, should be tackled in a separate issue if you think we need it.

Copy link
Member

Choose a reason for hiding this comment

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

No special cases for JavaScript outside of EpbLanguage, please!

}

@Specialization
Text doType(Type type) {
Copy link
Member

Choose a reason for hiding this comment

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

Meta.get_constructor_name works on Type!? Is the get_constructor_name still correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left for backwards compatibility – optimally we should get rid of this, but there is still code in the stdlib that depends on some things being atoms.

@@ -158,7 +158,7 @@ private void generateQualifiedAccessor() {
null,
new FunctionSchema(
new ArgumentDefinition(0, "self", ArgumentDefinition.ExecutionMode.EXECUTE)));
definitionScope.registerMethod(definitionScope.getAssociatedType(), this.name, function);
definitionScope.registerMethod(type.getEigentype(), this.name, function);
Copy link
Member

Choose a reason for hiding this comment

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

What is eigentype? Own type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the type of this type – where statics live. If you can think of a better name, we can switch

@@ -170,4 +227,16 @@ boolean isNull(@CachedLibrary("this") InteropLibrary self) {
public String toString() {
return toDisplayString(true);
}

public Type getEigentype() {
Copy link
Member

Choose a reason for hiding this comment

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

There already is Type getType()...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that is for library usage, and given this is already a type, and there's also it's supertype, I want to be super clear with the getters.

@kustosz kustosz added the CI: Ready to merge This PR is eligible for automatic merge label Sep 27, 2022
@mergify mergify bot merged commit 726bfeb into develop Sep 27, 2022
@mergify mergify bot deleted the wip/mk/statics branch September 27, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants