Support module imports using a qualified name #3608

hubertp · 2022-07-25T15:02:09Z

Pull Request Description

This change allows for importing modules using a qualified name and deals with any conflicts on the way.
Given a module C defined at A/B/C.enso with

type C
    type C a

it is now possible to import it as

import project.A
...
    val x = A.B.C 10

Given a module located at A/B/C/D.enso, we will generate
intermediate, synthetic, modules that only import and export the successor module along the path.
For example, the contents of a synthetic module B will look like

import <namespace>.<pkg-name>.A.B.C
export <namespace>.<pkg-name>.A.B.C

If module B is defined already by the developer, the compiler will inject the above statements to the IR.

Also removed the last elements of some lowercase name resolution that managed to survive recent
changes (Meta.Enso_Project would now be ambiguous with enso_project method).

Finally, added a pass that detects shadowing of the synthetic module by the type defined along the path.
We print a warning in such a situation.

Related to https://www.pivotaltracker.com/n/projects/2539304

Important Notes

There was an additional request to fix the annoying problem with from imports that would always bring
the module into the scope. The changes in stdlib demonstrate how it is now possible to avoid the workaround of

from X.Y.Z as Z_Module import A, B

(i.e. as Z_Module part is almost always unnecessary).

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 dist and ./run ide watch.

JaroslavTulach

Request: Make sure the submodules setter can be invoked only once and the value cannot change when the system is running.

Advice: Make sure the submodules getter returns immutable array/List.
Advice: If the submodules setter can only be invoked by IR package, consider using friend packages pattern.

Opinion: Avoid Optional in Java and check for null

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

JaroslavTulach · 2022-07-28T05:39:57Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

    this.name = name;
    this.scope = new ModuleScope(this);
    this.pkg = pkg;
-    this.compilationStage = CompilationStage.AFTER_CODEGEN;
+    this.compilationStage = virtual ? CompilationStage.INITIAL : CompilationStage.AFTER_CODEGEN;


Virtual modules, that's something @4e6 may also be interested in for his lazy visualizations.

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

engine/runtime/src/main/scala/org/enso/compiler/PackageRepository.scala

JaroslavTulach · 2022-07-28T05:47:45Z

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala

@@ -77,4 +77,13 @@ class ImportsTest extends PackageTest {
    )) should have message "Compilation aborted due to errors."
    consumeOut should contain("Export statements form a cycle:")
  }
+
+  "Import statements" should "should allow for importing submodules" in {
+    evalTestProject("TestSubmodules") shouldEqual 42


Is the TestSubmodules project evaluated with standard distribution or micro distribution?

It doesn't matter TBH. I only import from stdlib println because I want to do additional verification and not deal with unused variable warnings. Strictly speaking it is not necessary because compiler not complaining is already a progress.

JaroslavTulach · 2022-07-28T06:00:42Z

Btw. the meaning of

Request, Advice, Opinion

is taken from SunARC (Sun Microsystems Architecture Review Committee) guidelines as adopted by NetBeans. I am approving only under the assumption that all Requests are addressed. I'd be glad to see Advices implemented - but you can explain why that's not good idea and I'll live with that. Opinions reflect my personal preferences and you can ignore them.

farmaazon

LGTM

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

jdunkerley

Looks good to me.

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso

radeusgd

Looks good, I really appreciate not having to rename modules when it's not truly necesary.

I think a test for conflicting module names should be added. We should also probably ensure that if any error message is emitted due to that event, it is quite clear to the user what is the issue and how it can be solved.

engine/runtime/src/main/scala/org/enso/compiler/data/BindingsMap.scala

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Ordering/Comparator.enso

radeusgd · 2022-07-28T09:27:46Z

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala

@@ -77,4 +77,13 @@ class ImportsTest extends PackageTest {
    )) should have message "Compilation aborted due to errors."
    consumeOut should contain("Export statements form a cycle:")
  }
+
+  "Import statements" should "should allow for importing submodules" in {


I remember discussions on what to do when we both have stuff like module A/B.enso defining type C and a module A/B/C.enso.

I don't see any tests checking how this conflict-case behaves. Can we add those? (Or did I miss something?)

Yeap, was going to add it. Will need to add some additional check to produce a reasonable message

@radeusgd done

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

Easy path for importing submodules. Given a module located at `A/B/C/D.enso`, we will generate intermediate, virtual, modules that only import and export the succesor. For example, the contents of a virtual module B will look like ``` import <namespace>.<pkg-name>.A.B.C export <namespace>.<pkg-name>.A.B.C ``` This change does not attempt to deal with conflicts, yet.

When a virtual module conflicts with a non-virtual module, magic happens. We enhance the non-virtual module with essentially exactly the same information that the virtual one would carry without loosing anything. In order to distinguish between regular imports and the virtual ones, an additional flag needs to be carried in order to be able to continue to do name resolution properly. Also fixed a long standing problem with imports that include names but also import the name of the module automatically. Binding map was incorrectly always taking into account the name of the import.

Given ``` from X.Y.Z import A, B ``` would always bring `Z` into scope as well which was almost never desirable. As a result stdlib was full of ``` from X.Y.Z as Z_Module import A, B ``` workarounds. This is now no longer necessary.

Made it a warning, since this only manifests itself when one tries to access the conflicting module.

Co-authored-by: James Dunkerley <[email protected]>

…ap.scala Co-authored-by: Radosław Waśko <[email protected]>

hubertp force-pushed the wip/hubert/module-imports-182573549 branch from 82c8c3f to f8b9a44 Compare July 27, 2022 18:29

hubertp changed the title ~~Importing submodules~~ Support import modules using a qualified name Jul 27, 2022

hubertp marked this pull request as ready for review July 27, 2022 19:59

hubertp requested review from 4e6, jdunkerley, radeusgd, JaroslavTulach, MichaelMauderer and PabloBuchu as code owners July 27, 2022 19:59

hubertp changed the title ~~Support import modules using a qualified name~~ Support module imports using a qualified name Jul 27, 2022

hubertp requested review from kustosz and farmaazon July 27, 2022 20:07

hubertp force-pushed the wip/hubert/module-imports-182573549 branch from 4de1c47 to dc70b99 Compare July 27, 2022 20:29

JaroslavTulach approved these changes Jul 28, 2022

View reviewed changes

farmaazon approved these changes Jul 28, 2022

View reviewed changes

JaroslavTulach reviewed Jul 28, 2022

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed Jul 28, 2022

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java Outdated Show resolved Hide resolved

jdunkerley approved these changes Jul 28, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/System/File.enso Outdated Show resolved Hide resolved

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Text/Extensions.enso Outdated Show resolved Hide resolved

4e6 approved these changes Jul 28, 2022

View reviewed changes

radeusgd approved these changes Jul 28, 2022

View reviewed changes

hubertp force-pushed the wip/hubert/module-imports-182573549 branch from 9a76f87 to 7dbb5d7 Compare July 28, 2022 14:33

kustosz approved these changes Jul 28, 2022

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java Outdated Show resolved Hide resolved

hubertp force-pushed the wip/hubert/module-imports-182573549 branch from d6a9d42 to 3e164ed Compare July 29, 2022 09:22

hubertp added 6 commits July 29, 2022 11:47

Remove workarounds for import problems

d404543

Given ``` from X.Y.Z import A, B ``` would always bring `Z` into scope as well which was almost never desirable. As a result stdlib was full of ``` from X.Y.Z as Z_Module import A, B ``` workarounds. This is now no longer necessary.

nit

59d3fc8

Address PR review

7fa1c72

nit

152410e

hubertp and others added 6 commits July 29, 2022 11:47

Linting pass for detecting type/module conflicts

647446e

Made it a warning, since this only manifests itself when one tries to access the conflicting module.

Apply suggestions from code review

6e03779

Co-authored-by: James Dunkerley <[email protected]>

Update engine/runtime/src/main/scala/org/enso/compiler/data/BindingsM…

d9e5416

…ap.scala Co-authored-by: Radosław Waśko <[email protected]>

changelog update

b7999aa

documentation and minor code cleanups

4478d04

s/virtual/synthetic

22e59a9

hubertp force-pushed the wip/hubert/module-imports-182573549 branch from 3e164ed to 22e59a9 Compare July 29, 2022 09:47

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jul 29, 2022

Make windows happy

0389742

mergify bot merged commit d59714a into develop Jul 29, 2022

mergify bot deleted the wip/hubert/module-imports-182573549 branch July 29, 2022 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support module imports using a qualified name #3608

Support module imports using a qualified name #3608

hubertp commented Jul 25, 2022 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Jul 28, 2022

JaroslavTulach Jul 28, 2022

hubertp Jul 28, 2022

JaroslavTulach commented Jul 28, 2022

farmaazon left a comment

jdunkerley left a comment

radeusgd left a comment

radeusgd Jul 28, 2022

hubertp Jul 28, 2022

hubertp Jul 28, 2022

Support module imports using a qualified name #3608

Support module imports using a qualified name #3608

Conversation

hubertp commented Jul 25, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Jul 28, 2022

Choose a reason for hiding this comment

JaroslavTulach Jul 28, 2022

Choose a reason for hiding this comment

hubertp Jul 28, 2022

Choose a reason for hiding this comment

JaroslavTulach commented Jul 28, 2022

farmaazon left a comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

radeusgd Jul 28, 2022

Choose a reason for hiding this comment

hubertp Jul 28, 2022

Choose a reason for hiding this comment

hubertp Jul 28, 2022

Choose a reason for hiding this comment

hubertp commented Jul 25, 2022 •

edited

Loading