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

Inconsistent import and FQN #9329

Closed
JaroslavTulach opened this issue Mar 8, 2024 · 16 comments · Fixed by #9539
Closed

Inconsistent import and FQN #9329

JaroslavTulach opened this issue Mar 8, 2024 · 16 comments · Fixed by #9539
Assignees

Comments

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Mar 8, 2024

As of Mar 8, 2024 there seems to be an inconsistency between import and using fully qualified name in certain cases.

When Main Does not Reexport Submodule

When Main in root of library doesn't re-export a submodule, then the module is not going to be accessible via FQN. Example where import works:

import Standard.Table.Data.Table
main = Table

but where FQN access doesn't work:

import Standard.Table
main = Standard.Table.Data.Table

and it yields

Execution finished with an error: Method `Data` of type Main could not be found.
        at <enso> i.main<arg-0>(i.enso:2:8-26)
        at <enso> i.main(i.enso:2:8-32)

The same type of problem is also reported as #6553 - the FQN in question is Standard.Database.Connection.Postgres_Details.Postgres_Details.Postgres in that case.

Any Other Case?

Not known yet...

Requested Behavior

If x.y.z.A can be imported by such name, one shall be able to use the same name as FQN.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Mar 8, 2024

I managed to get the Standard.Table.Data.Table case working with following change:

diff --git engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java
index 0998e55277..fe1cc688d6 100644
--- engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java
+++ engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java
@@ -210,6 +210,25 @@ public abstract class InvokeMethodNode extends BaseNode {
     Type selfTpe = typesLibrary.getType(self);
     Function function = resolveFunction(symbol, selfTpe, methodResolverNode);
     if (function == null) {
+      if (self instanceof Type t && t.getDefinitionScope().getAssociatedType() == t) {
+        CompilerDirectives.transferToInterpreter();
+        // if we represent a Module
+        var scope = t.getDefinitionScope();
+        var module = scope.getModule();
+        var ctx = EnsoContext.get(this);
+        var moduleName =
+            module.getName().item().equals("Main")
+                ? module.getPackage().libraryName().toString()
+                : module.getName().toString();
+        var subModuleName = moduleName + "." + symbol.getName();
+        var optionModule = ctx.getTopScope().getModule(subModuleName);
+        if (optionModule.isPresent()) {
+          var subType = optionModule.get().getScope().getAssociatedType();
+          if (subType != null) {
+            return subType;
+          }
+        }
+      }
       throw methodNotFound(symbol, self);
     }
     var resolvedFuncArgCount = function.getSchema().getArgumentsCount();

I am not really satisfied with the above code - it seems too brutally direct. When investigating what import/export resolution does I found this stack:

ImportResolver.tryResolveImport() (runtime-compiler/src/main/scala/org/enso/compiler/phase/ImportResolver.scala:198)
ImportResolver.$anonfun$mapImports$3() (runtime-compiler/src/main/scala/org/enso/compiler/phase/ImportResolver.scala:85)
ImportResolver$$Lambda/0x00007f7c5c55a728.apply() (Unknown Source:0)
List.map() (List.scala:246)
ImportResolver.analyzeModule$1() (runtime-compiler/src/main/scala/org/enso/compiler/phase/ImportResolver.scala:83)
ImportResolver.go$1() (runtime-compiler/src/main/scala/org/enso/compiler/phase/ImportResolver.scala:150)
ImportResolver.mapImports() (runtime-compiler/src/main/scala/org/enso/compiler/phase/ImportResolver.scala:162)
Compiler.liftedTree1$1() (runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala:456)
Compiler.runImportsAndExportsResolution() (runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala:455)
Compiler.$anonfun$runCompilerPipeline$2() (runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala:273)
Compiler$$Lambda/0x00007f7c5c559120.apply() (Unknown Source:0)
List.flatMap() (List.scala:293)
Compiler.runCompilerPipeline() (runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala:268)
Compiler.go$1() (runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala:233)
Compiler.runInternal() (runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala:240)
Compiler.run() (runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala:131)
Module.compile() (runtime/src/main/java/org/enso/interpreter/runtime/Module.java:392)

What the ImportResolver.tryResolveImport does?

I'd say the functionality of the above diff is similar. It also uses TopScope.getModule - just the way it computes the name to search for (with Main check) seems wild...

@Akirathan
Copy link
Member

Might be a duplicate of #6553

@Akirathan
Copy link
Member

Akirathan commented Mar 8, 2024

I believe the proper fix is in the compilation phase rather than fixing the runtime behavior, namely in org.enso.compiler.pass.resolve.FullyQualifiedNames.scala introduced by #4056. It appears that this compiler pass is inconsistent with the Import/export resolver.

@enso-bot
Copy link

enso-bot bot commented Mar 11, 2024

Pavel Marek reports a new STANDUP for today (2024-03-11):

Progress: - Looking into the problems with consistency between FQN and imports

  • Adding some minimal reproducer tests It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 12, 2024

Pavel Marek reports a new STANDUP for today (2024-03-12):

Progress: - Making sure I correctly understand the requirements and what is expected from the engine.

  • Chatting with Hubert and Adam
  • Created a thorough test that scans all the exported symbols from std libs and makes sure that the FQN/import invariant holds for all the symbols
  • Seems that it should be enough to ensure that synthetic modules are exported. It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 14, 2024

Pavel Marek reports a new STANDUP for today (2024-03-14):

Progress: - New BC assignment.

  • Reporting some issues with the IDE.
  • Dry-run benchmarks fail and fix engine compiler benchmarks in dry-run benchmarks exits when some benchmark fails #9397.
  • Import/FQN
    • Jaroslav's runtime patch seems insufficient, it only deals with types and not with methods.
    • Looking again at FullyQualifiedNames pass. It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 15, 2024

Pavel Marek reports a new STANDUP for today (2024-03-15):

Progress: - Adding more tests that checks resolved symbols in BindingsMap

  • I am getting close to identify the exact place that needs to be modified in order to fix this. It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 18, 2024

Pavel Marek reports a new STANDUP for today (2024-03-18):

Progress: - Debugging the tests and other IR passes. Somehow, the IR pass is called twice on a clone of the expression IR??

  • Debugging is difficult, tracking the modifications of the IR and its metadata is difficult. It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 19, 2024

Pavel Marek reports a new STANDUP for today (2024-03-19):

Progress: - Discovered huge regressions in stdlibs benchmarks.

@hubertp
Copy link
Contributor

hubertp commented Mar 20, 2024

Same situation I see with

Standard.Visualization.Table.Visualization.prepare_visualization

when GUI sends executeExpression

@enso-bot
Copy link

enso-bot bot commented Mar 21, 2024

Pavel Marek reports a new STANDUP for yesterday (2024-03-20):

Progress: - Fixed some tests by modification of FullyQualifiedNames pass, but messed up some other tests.

  • Still struggling with debugging the modifications of IR and associated metadata. It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 22, 2024

Pavel Marek reports a new STANDUP for yesterday (2024-03-21):

Progress: - Comming up with a simple GraphViz visualization tool for our IR.

  • (Reduced hours) It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 22, 2024

Pavel Marek reports a new STANDUP for today (2024-03-22):

Progress: - With the help of my recent IR visualization tool, I am finally able to see what is wrong with some passes.

  • Fixing some tests
  • Will need to work on this issue next week as well, fixing of FQN is more difficult than I thought.
  • Discussion about the future of our ultimate built system. It should be finished by 2024-03-22.

@enso-bot
Copy link

enso-bot bot commented Mar 25, 2024

Pavel Marek reports a new 🔴 DELAY for today (2024-03-25):

Summary: There is 7 days delay in implementation of the Inconsistent import and FQN (#9329) task.
It will cause 7 days delay for the delivery of this weekly plan.

As noted previous week, there is a delay on this issue. In past few days, I was just comming up with a way how to correctly understand the changes done to IR and medata between various Compiler passes.

Delay Cause: Fixes on IR and metadata level are more complex and intertwined than expected.

@enso-bot
Copy link

enso-bot bot commented Mar 25, 2024

Pavel Marek reports a new STANDUP for today (2024-03-25):

Progress: - Ensuring that code coverage is sufficient.

  • Discussing possible runtime-only solution with Jaroslav
  • Still more experimentation - fixing some test cases, and breaking some other. It should be finished by 2024-03-29.

@enso-bot
Copy link

enso-bot bot commented Mar 26, 2024

Pavel Marek reports a new STANDUP for today (2024-03-26):

Progress: - Abandoning the original PR #9367 in favor of Jaroslav's runtime approach at #9539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
3 participants