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

Import modules' extension methods only with unqualified import statements #3906

Merged
merged 8 commits into from
Dec 1, 2022

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Nov 24, 2022

Previously, a qualified import would always bring extension methods into the scope. This is because extensions methods weren't taking into account the qualifiers at all and imported scope was blindly added to the existing module scope.

For example, given a module with an extension method in one module (A.enso)

type A
    X
    Y
...
Integer.foo self a = ....

we weren't consistent with how extension method foo could be brought into the scope.
All of

  • import project.A
  • import project.A.A
  • from project.A import all
  • from project.A.A import X, Y

would bring it into the scope.

Now, only

  • import project.A
  • from project.A import all

would.

The change limits that by taking into account import qualifiers of the types that are being brought and creating a copy of the original ModuleScope.

This meant that in pretty much every test file of our testsuite we had to import Test.Extensions module.
This fixes problem 2) in https://www.pivotaltracker.com/n/projects/2539304/stories/183833055

Important Notes

Note that one cannot

import Standard.Table as Table_Module

because of the 2-component name restriction that gets desugared to Standard.Table.Main and we have to write

import Standard.Table.Main as Table_Module

in a few places. Once we move Json.to_table extension this can be improved.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • [ x 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.

test/Geo_Tests/src/Geo_Spec.enso Outdated Show resolved Hide resolved
@farmaazon
Copy link
Contributor

farmaazon commented Nov 24, 2022

Can someone explain me what exactly is limited? If someone pick an extension method foo for type Type defined in module Module in component browser, what import shall be added?

@hubertp hubertp force-pushed the wip/hubert/fix-extension-methods-imports-183833055 branch from 2637031 to d09a1b8 Compare November 24, 2022 14:21
@hubertp hubertp changed the title Don't import extension methods by default Import modules' extension methods only with unqualified import statements Nov 24, 2022
@hubertp
Copy link
Contributor Author

hubertp commented Nov 24, 2022

Can someone explain me what exactly is limited? If someone pick an extension method foo for type Type defined in module Module in component browser, what import shall be added.

I updated the description. Let me know if this is still confusing.

@farmaazon
Copy link
Contributor

Thank you. Now I see it. Seemingly, no additional update in Component Browser is required (it uses import Module).

@hubertp hubertp force-pushed the wip/hubert/fix-extension-methods-imports-183833055 branch 2 times, most recently from 9f5d037 to 05c158e Compare November 28, 2022 13:20
@hubertp
Copy link
Contributor Author

hubertp commented Nov 28, 2022

Discovered some more corner cases. I will ping again once I'm done with them.

@hubertp hubertp force-pushed the wip/hubert/fix-extension-methods-imports-183833055 branch from 9b1df42 to caba6e2 Compare November 29, 2022 10:04
@hubertp
Copy link
Contributor Author

hubertp commented Nov 29, 2022

OK, added a bunch of tests that cover exporting as well. Now it really looks like it works as expected.

@JaroslavTulach @4e6 mind having a look?

@@ -1,5 +1,6 @@
import project.Data.Any.Any
import project.Data.Numbers.Integer
import project.Data.Range as Range_Module
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdunkerley something to keep in mind in further refactorings (this is because of up_to)

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

If the tests are working, then the change is likely good. Few (not that important) comments inline.

@hubertp hubertp force-pushed the wip/hubert/fix-extension-methods-imports-183833055 branch from caba6e2 to b600118 Compare December 1, 2022 09:31
Previously, a qualified import would always bring extension methods into
the scope. This is because extensions methods weren't taking into
account the qualifiers at all and imported scope was blindly added to
the existing module scope.

The change limits that by taking into account import qualifiers of the
types that are being brought and creating a copy of the original
`ModuleScope`.

This meant that in pretty much every test file of our testsuite we had
to import `Test` module.
Also, note that one cannot
```
import Standard.Test as Test_Module
```
because of the 2-component name restriction that gets desugared to
`Standard.Test.Main` and we have to write
```
import Standard.Test.Main as Test_Module
```
all over the place.
@hubertp hubertp force-pushed the wip/hubert/fix-extension-methods-imports-183833055 branch from b600118 to 8b43f3d Compare December 1, 2022 09:32
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Dec 1, 2022
@mergify mergify bot merged commit 06bd694 into develop Dec 1, 2022
@mergify mergify bot deleted the wip/hubert/fix-extension-methods-imports-183833055 branch December 1, 2022 10:13
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.

4 participants