Verify ascribed types of parameters really exist #6584

JaroslavTulach · 2023-05-05T14:22:56Z

Pull Request Description

Verify ascribed types (a : Xyz) are checked for existence.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

All code follows the
Scala,
Java,
style guides.
All code has been tested:
- Unit tests have been written where possible.

Akirathan

Adding tests to TypeSignaturesTest.scala would probably be less amount of work, since there are already some tests.

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeNames.scala

JaroslavTulach · 2023-05-09T07:56:51Z

There is a lot of unresolved symbols when parsing the standard library. I was investigating it and it seems to me that BindingMap is properly created for a module:

BindingsMap: Standard.Base.System.Platform
        at org.enso.compiler.data.BindingsMap.<init>(BindingsMap.scala:35)
        at org.enso.compiler.data.BindingsMap.copy(BindingsMap.scala:30)
        at org.enso.compiler.data.BindingsMap.$anonfun$toConcrete$1(BindingsMap.scala:91)
        at scala.Option.map(Option.scala:242)
        at org.enso.compiler.data.BindingsMap.toConcrete(BindingsMap.scala:90)
        at org.enso.compiler.phase.ImportResolver.go$1(ImportResolver.scala:99)
        at org.enso.compiler.phase.ImportResolver.mapImports(ImportResolver.scala:132)
        at org.enso.compiler.Compiler.liftedTree1$1(Compiler.scala:439)
        at org.enso.compiler.Compiler.runImportsAndExportsResolution(Compiler.scala:438)
        at org.enso.compiler.Compiler.$anonfun$runCompilerPipeline$2(Compiler.scala:272)
        at scala.collection.immutable.List.flatMap(List.scala:293)
        at org.enso.compiler.Compiler.runCompilerPipeline(Compiler.scala:271)
        at org.enso.compiler.Compiler.go$1(Compiler.scala:254)
        at org.enso.compiler.Compiler.runInternal(Compiler.scala:260)
        at org.enso.compiler.Compiler.run(Compiler.scala:157)
        at org.enso.interpreter.runtime.Module.compile(Module.java:406)
        at org.enso.interpreter.runtime.Module.compileScope(Module.java:333)

filled with symbols properly, but then it gets created once again and this time the symbols remain empty:

java.lang.Exception: BindingsMap: Standard.Base.System.Platform
        at org.enso.compiler.data.BindingsMap.<init>(BindingsMap.scala:35)
        at org.enso.compiler.pass.analyse.BindingAnalysis$.runModule(BindingAnalysis.scala:99)
        at org.enso.compiler.pass.PassManager.$anonfun$runPassesOnModule$2(PassManager.scala:101)
        at scala.collection.LinearSeqOps.foldLeft(LinearSeq.scala:169)
        at scala.collection.LinearSeqOps.foldLeft$(LinearSeq.scala:165)
        at scala.collection.immutable.List.foldLeft(List.scala:79)
        at org.enso.compiler.pass.PassManager.runPassesOnModule(PassManager.scala:92)
        at org.enso.compiler.Compiler.recognizeBindings(Compiler.scala:796)
        at org.enso.compiler.Compiler.uncachedParseModule(Compiler.scala:595)
        at org.enso.compiler.Compiler.parseModule(Compiler.scala:552)
        at org.enso.compiler.Compiler.ensureParsed(Compiler.scala:634)
        at org.enso.compiler.Compiler.ensureParsedAndAnalyzed(Compiler.scala:472)
        at org.enso.compiler.Compiler.$anonfun$runImportsAndExportsResolution$1(Compiler.scala:456)
        at scala.collection.immutable.List.map(List.scala:250)
        at org.enso.compiler.Compiler.runImportsAndExportsResolution(Compiler.scala:449)
        at org.enso.compiler.Compiler.$anonfun$runCompilerPipeline$2(Compiler.scala:272)
        at scala.collection.immutable.List.flatMap(List.scala:293)
        at org.enso.compiler.Compiler.runCompilerPipeline(Compiler.scala:271)
        at org.enso.compiler.Compiler.go$1(Compiler.scala:254)
        at org.enso.compiler.Compiler.runInternal(Compiler.scala:260)
        at org.enso.compiler.Compiler.run(Compiler.scala:157)
        at org.enso.interpreter.runtime.Module.compile(Module.java:406)
        at org.enso.interpreter.runtime.Module.compileScope(Module.java:333)

Update: solved by 0711186

hubertp

nits

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala

engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeNames.scala

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeNames.scala

JaroslavTulach · 2023-05-09T10:32:15Z

There is a lot of unresolved symbols ....

Thank you all for your help. These three lines are causing all the failures:

enso$ git diff
diff --git engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala
index bbe19d43fc..5be235da81 100644
--- engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala
+++ engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala
@@ -134,9 +134,9 @@ case object TypeSignatures extends IRPass {
         lastSignature = None
         res
       case ut: IR.Module.Scope.Definition.Type =>
-        ut.members.foreach(d => {
-          verifyAscribedArguments(d.arguments)
-        })
+        //ut.members.foreach(d => {
+        //  verifyAscribedArguments(d.arguments)
+        //})
         Some(ut.mapExpressions(resolveExpression))
       case err: IR.Error                  => Some(err)
       case ann: IR.Name.GenericAnnotation => Some(ann)

Update: solved by 0711186

engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java

Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>

JaroslavTulach · 2023-05-10T16:11:37Z

Dropping the caches didn't help - the failure is still there. The following patch seems to help me locally: #6135 (comment)

engine/runtime/src/main/scala/org/enso/compiler/phase/ImportResolver.scala

…scribedTypes_6527

JaroslavTulach · 2023-05-11T14:14:36Z

There is a:

RuntimeErrorsTest failure
TypeNamesTest failure

JaroslavTulach · 2023-05-12T05:43:01Z

TypeNamesTest failure

Looks like the first test in TypeNamesTest now influences the other one. Disabling the first one, or running just the second one makes the test pass. Also allocating different mkModuleContext helps.

enso$ git diff
diff --git engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/TypeNamesTest.scala engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/TypeNamesTest.scala
index a7147210cb..b00cfe4191 100644
--- engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/TypeNamesTest.scala
+++ engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/TypeNamesTest.scala
@@ -76,7 +76,7 @@ class TypeNamesTest extends CompilerTest {
   "Resolution of type names in modules" should {
     implicit val ctx: ModuleContext = mkModuleContext
 
-    "should correctly resolve local type names" in {
+    "should correctly resolve local type names" ignore {
       val ir =
         """
           |type A

Akirathan · 2023-05-12T07:20:27Z

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

@@ -102,6 +108,7 @@ public boolean isBefore(CompilationStage stage) {
  private boolean hasCrossModuleLinks;
  private final boolean synthetic;
  private List<QualifiedName> directModulesRefs;
+  private BindingsMap bindings;


I have hard times understanding why this field is needed. Why can't we operate with the BindingsMap only via ir.unsafeGetMetadata(BindingAnalysis)? How do we ensure that the values of this field will always be exactly the same as the value of this.ir.unsafeGetMetadata(BindingAnalysis)? Note that the BindingAnalysis phase is run on a particular module prior to import resolution, which means that the BindingMap is available soon.

Why cannot the problems with caches be resolved only via this.ir.usafeGetMetadata(BindingAnalysis)?

I am not 100% sure yet, but it seems that when the BindingsMap gets read from stdlib caches, there are some inconsistencies.

When the caches exist the BindingsMap gets created sooner than IR. That's good, for purposes of #6100 we don't want to load IR just to resolve imports. However that means, having BindingsMap inside of IR is problematic in any case.

BindingMap not being part of IR's metadata sounds reasonable. In that case, I would forbid to access BindingMap via ir.getMetadata, and only implement one way of accessing BindingMap - for example via Module.getBindings. I have a feeling that it is a bad idea for BindingMap to extend IRPass.Metadata anyway, since it is somewhat special in a sense that this metadata is filled even before import resolution starts. We should probably refactor BindingMap such that it is an internal part of the compiler and it is tightly coupled with a particular Module. What do you think?

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeNames.scala

JaroslavTulach · 2023-05-14T05:39:04Z

Removing most of own modifications and using #6655 implementation. Looks like SignatureTest and ImportsTest are failing.

…z-6260 * develop: Build nightlies every day. (#6681) Force `newDashboard` default on the CI-built packages. (#6680) Verify ascribed types of parameters really exist (#6584) SuggestionBuilder needs to send ascribedType of constructor parameters (#6655) Improvements to the Table visualization. (#6653) Removing flush (#6670) Improving widgets for take/drop (#6641)

* develop: Build nightlies every day. (#6681) Force `newDashboard` default on the CI-built packages. (#6680) Verify ascribed types of parameters really exist (#6584) SuggestionBuilder needs to send ascribedType of constructor parameters (#6655) Improvements to the Table visualization. (#6653) Removing flush (#6670) Improving widgets for take/drop (#6641) disable authentication and newDashboard by default (#6664) Add COOP+COEP+CORP headers (#6646)

Verify ascribed types of parameters really exist

1cdb009

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label May 5, 2023

JaroslavTulach self-assigned this May 5, 2023

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners May 5, 2023 14:22

Ascriptions in type constructors shall be checked as well

e4a3cd4

Akirathan approved these changes May 6, 2023

View reviewed changes

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeSignatures.scala Outdated Show resolved Hide resolved

JaroslavTulach added 2 commits May 9, 2023 04:46

No need to store result in arr variable

cb6215c

Verify ascribed arguments of atom constructors

e378be5

JaroslavTulach linked an issue May 9, 2023 that may be closed by this pull request

Ascribed type of an argument isn't checked for correctness #6527

Closed

JaroslavTulach commented May 9, 2023

View reviewed changes

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeNames.scala Outdated Show resolved Hide resolved

hubertp reviewed May 9, 2023

View reviewed changes

JaroslavTulach mentioned this pull request May 9, 2023

SuggestionBuilder needs to send ascribedType of constructor parameters #6611

Closed

JaroslavTulach added 3 commits May 9, 2023 14:45

Have to disable verifyAscribedArgument on the Type here

ebb52ce

map the copy of the Type members

0711186

Adding missing imports

6345ee0

JaroslavTulach requested review from jdunkerley, radeusgd and GregoryTravis as code owners May 10, 2023 05:56

JaroslavTulach added 2 commits May 10, 2023 09:13

Allow type parameters as ascribed types

5522b68

More of _ per Hubert's advice

17d7453

JaroslavTulach commented May 10, 2023

View reviewed changes

engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java Show resolved Hide resolved

Less of currly braces

6e2ce12

Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>

JaroslavTulach force-pushed the wip/jtulach/CheckAscribedTypes_6527 branch from c52e045 to 6e2ce12 Compare May 10, 2023 07:24

JaroslavTulach requested a review from hubertp May 10, 2023 07:24

Case with _

e385838

Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>

jdunkerley approved these changes May 10, 2023

View reviewed changes

Forcibly drop BindingsMap caches

91fba7e

JaroslavTulach mentioned this pull request May 10, 2023

Make compilation of stdlib components more resilient to cache corruption #6135

Closed

JaroslavTulach added 2 commits May 10, 2023 18:25

Extend cached BindingsMap, when it exists

0f82e6a

Force loading from cache before asking ensureParsed

67cf459

JaroslavTulach commented May 11, 2023

View reviewed changes

engine/runtime/src/main/scala/org/enso/compiler/phase/ImportResolver.scala Outdated Show resolved Hide resolved

JaroslavTulach added 2 commits May 11, 2023 10:24

Use bindings associated with module

7f6c0d8

scalafmtAll

61fbbda

enso-bot bot mentioned this pull request May 11, 2023

Vector.sort should attach limited number of warnings #6283

Closed

JaroslavTulach added 2 commits May 11, 2023 12:14

Merge remote-tracking branch 'origin/develop' into wip/jtulach/CheckA…

569989b

…scribedTypes_6527

Call to mapImports is causing StackOverflow

478c2be

Akirathan reviewed May 12, 2023

View reviewed changes

enso-bot bot mentioned this pull request May 12, 2023

Project folder renaming stopped working #6515

Closed

2 tasks

Cleaning up bindings when re-assigning new source to a module

f2df70c

JaroslavTulach force-pushed the wip/jtulach/CheckAscribedTypes_6527 branch from 3dddf00 to f2df70c Compare May 12, 2023 08:28

JaroslavTulach mentioned this pull request May 12, 2023

SuggestionBuilder needs to send ascribedType of constructor parameters #6655

Merged

3 tasks

4e6 reviewed May 12, 2023

View reviewed changes

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/TypeNames.scala Outdated Show resolved Hide resolved

4e6 self-requested a review May 12, 2023 13:10

enso-bot bot mentioned this pull request May 13, 2023

Broken redefinition of variable inside if-then expression #6211

Open

JaroslavTulach added 3 commits May 14, 2023 06:35

Merging with Dmitry's #6655

50e1da7

Less changes against develop branch

e9a42f2

Make SignatureTest.ascribedWithAParameter pass

2d21386

JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label May 14, 2023

mergify bot merged commit 41bb52c into develop May 14, 2023

mergify bot deleted the wip/jtulach/CheckAscribedTypes_6527 branch May 14, 2023 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify ascribed types of parameters really exist #6584

Verify ascribed types of parameters really exist #6584

JaroslavTulach commented May 5, 2023 •

edited

Loading

Akirathan left a comment

JaroslavTulach commented May 9, 2023 •

edited

Loading

hubertp left a comment

JaroslavTulach commented May 9, 2023 •

edited

Loading

JaroslavTulach commented May 10, 2023

JaroslavTulach commented May 11, 2023 •

edited

Loading

JaroslavTulach commented May 12, 2023 •

edited

Loading

Akirathan May 12, 2023

JaroslavTulach May 12, 2023

Akirathan May 12, 2023

JaroslavTulach commented May 14, 2023

Verify ascribed types of parameters really exist #6584

Verify ascribed types of parameters really exist #6584

Conversation

JaroslavTulach commented May 5, 2023 • edited Loading

Pull Request Description

Checklist

Akirathan left a comment

Choose a reason for hiding this comment

JaroslavTulach commented May 9, 2023 • edited Loading

hubertp left a comment

Choose a reason for hiding this comment

JaroslavTulach commented May 9, 2023 • edited Loading

JaroslavTulach commented May 10, 2023

JaroslavTulach commented May 11, 2023 • edited Loading

JaroslavTulach commented May 12, 2023 • edited Loading

Akirathan May 12, 2023

Choose a reason for hiding this comment

JaroslavTulach May 12, 2023

Choose a reason for hiding this comment

Akirathan May 12, 2023

Choose a reason for hiding this comment

JaroslavTulach commented May 14, 2023

JaroslavTulach commented May 5, 2023 •

edited

Loading

JaroslavTulach commented May 9, 2023 •

edited

Loading

JaroslavTulach commented May 9, 2023 •

edited

Loading

JaroslavTulach commented May 11, 2023 •

edited

Loading

JaroslavTulach commented May 12, 2023 •

edited

Loading