Two `BindingsMap$ResolvedImport` aren't equal and raise exception on the CI

[hubertp]

I somehow got an impression that this shouldn't happen anymore?

   INFO ide_ci::program::command: sbt ℹ️ [info] Iteration   1: <failure>
   INFO ide_ci::program::command: sbt ℹ️ [info] java.io.IOException: Adding at 20165 object:
   INFO ide_ci::program::command: sbt ℹ️ [info]   org.enso.compiler.data.BindingsMap$ResolvedImport@7402996f
   INFO ide_ci::program::command: sbt ℹ️ [info] but there already is:
   INFO ide_ci::program::command: sbt ℹ️ [info]   org.enso.compiler.data.BindingsMap$ResolvedImport@4252eec3
   INFO ide_ci::program::command: sbt ℹ️ [info] are they equal: false
   INFO ide_ci::program::command: sbt ℹ️ [info]     at org.enso.runtime/org.enso.persist.PerInputImpl.readIndirect(PerInputImpl.java:229)
   INFO ide_ci::program::command: sbt ℹ️ [info]     at org.enso.runtime/org.enso.persist.PerInputImpl.readObject(PerInputImpl.java:74)
   INFO ide_ci::program::command: sbt ℹ️ [info]     at org.enso.runtime/org.enso.compiler.core.ir.IrPersistance$PersistScalaList.readObject(IrPersistance.java:227)
   INFO ide_ci::program::command: sbt ℹ️ [info]     at org.enso.runtime/org.enso.compiler.core.ir.IrPersistance$PersistScalaList.readObject(IrPersistance.java:207)
   INFO ide_ci::program::command: sbt ℹ️ [info]     at org.enso.runtime/org.enso.persist.Persistance.readWith(Persistance.java:162)
   INFO ide_ci::program::command: sbt ℹ️ [info]     at org.enso.runtime/org.enso.persist.PerInputImpl.readInline(PerInputImpl.java:67)

https://github.com/enso-org/enso/actions/runs/11030572774/job/30635424983#step:7:1461

[JaroslavTulach]

this shouldn't happen anymore?

The test has been disabled in the release branch only by

• 11015

as the fix was integrated only into https://github.com/enso-org/enso/tree/release/2024.4 branch, the failure still appears in develop. The progress is being tracked by:

• 10985

Closing as duplicate. But thanks for the reminder we need a fix for #10985 before next release.

GitHub

GitHub - enso-org/enso at release/2024.4

Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

[hubertp]

I continue to see this failure in CI. Latest one where it happened: https://github.com/enso-org/enso/actions/runs/11211349356/job/31160043979?pr=11250#step:7:1446

GitHub

Do not run visualizations on InterruptedException · enso-org/enso@8f0ced4

Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

[hubertp]

And another https://github.com/enso-org/enso/actions/runs/11563450462/job/32186772474?pr=11375#step:7:1636

GitHub

Don't cancel aborted jobs immediately · enso-org/enso@6a5168c

Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

[enso-bot[bot]]

Jaroslav Tulach reports a new STANDUP for yesterday (2024-11-06):

Progress: .

• IfThenElse with flattenToParent & a test
• more comments on the "logging PR": https://github.com/enso-org/enso/pull/11472#issuecomment-2458943487
• standups & meetings It should be finished by 2024-11-08.

[JaroslavTulach]

I tried to re-enable proper check of metadata.equals(other.metadata) in:

diff --git engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FramePointer.java engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FramePointer.java
index 425af3bf8f..697b769b24 100644
--- engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FramePointer.java
+++ engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FramePointer.java
@@ -1,5 +1,6 @@
 package org.enso.compiler.pass.analyse;

+import java.util.IdentityHashMap;
 import org.enso.compiler.core.CompilerStub;
 import org.enso.compiler.core.ir.ProcessingPass;
 import org.enso.persist.Persistable;
@@ -10,10 +11,20 @@ import scala.Option;
  */
 @Persistable(clazz = FramePointer.class, id = 1283)
 public record FramePointer(int parentLevel, int frameSlotIdx) implements FrameAnalysisMeta {
+  public static final IdentityHashMap<FramePointer, Exception> WHEN = new IdentityHashMap<>();

   public FramePointer {
     assert parentLevel >= 0;
     assert frameSlotIdx >= 0;
+    if (parentLevel == 0 && frameSlotIdx == 3) {
+      //        System.err.println("now Frame for 0, 3");
+    }
+    //    WHEN.put(this, new Exception("Allocated now: " + parentLevel + " " + frameSlotIdx));
+  }
+
+  public Exception when() {
+    var ex = WHEN.get(this);
+    return ex == null ? new Exception("no previous record") : ex;
   }

   @Override
diff --git engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FrameVariableNames.java engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FrameVariableNames.java
index 9d87a57a17..92edc4c9e0 100644
--- engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FrameVariableNames.java
+++ engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FrameVariableNames.java
@@ -1,6 +1,7 @@
 package org.enso.compiler.pass.analyse;

 import java.util.List;
+import java.util.Objects;
 import org.enso.compiler.core.CompilerStub;
 import org.enso.compiler.core.ir.ProcessingPass;
 import org.enso.persist.Persistable;
@@ -11,7 +12,7 @@ import scala.jdk.javaapi.CollectionConverters;
 public final class FrameVariableNames implements FrameAnalysisMeta {
   private final List<String> names;

-  public FrameVariableNames(List<String> variableNames) {
+  FrameVariableNames(List<String> variableNames) {
     this.names = variableNames;
   }

@@ -42,4 +43,31 @@ public final class FrameVariableNames implements FrameAnalysisMeta {
   public Option<ProcessingPass.Metadata> duplicate() {
     return Option.apply(new FrameVariableNames(names));
   }
+
+  @Override
+  public int hashCode() {
+    int hash = 3;
+    hash = 59 * hash + Objects.hashCode(this.names);
+    return hash;
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj) {
+      return true;
+    }
+    if (obj == null) {
+      return false;
+    }
+    if (getClass() != obj.getClass()) {
+      return false;
+    }
+    final FrameVariableNames other = (FrameVariableNames) obj;
+    return Objects.equals(this.names, other.names);
+  }
+
+  @Override
+  public String toString() {
+    return "FrameVariableNames{" + "names=" + names + '}';
+  }
 }
diff --git engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/FramePointerAnalysis.scala engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/FramePointerAnalysis.scala
index 71a1e00b1b..07263105f1 100644
--- engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/FramePointerAnalysis.scala
+++ engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/FramePointerAnalysis.scala
@@ -38,7 +38,10 @@ case object FramePointerAnalysis extends IRPass {
   override val invalidatedPasses: Seq[IRPass] = Seq(this)

   override def runModule(ir: Module, moduleContext: ModuleContext): Module = {
+    FramePointer.WHEN.clear()
+    System.err.println("running a module")
     ir.bindings.foreach(processBinding)
+    System.err.println("finished a module")
     ir
   }

@@ -319,16 +322,21 @@ case object FramePointerAnalysis extends IRPass {
     newMeta: FrameAnalysisMeta
   ): Unit = {
     ir.passData().get(this) match {
-      case None =>
-        ir.passData()
-          .update(this, newMeta)
       case Some(meta) =>
-        val ex = new IllegalStateException(
-          "Unexpected FrameAnalysisMeta associated with IR " + ir + "\nOld: " + meta + " new " + newMeta
-        )
-        ex.setStackTrace(ex.getStackTrace().slice(0, 10))
-        throw ex
+        if (meta != newMeta) {
+          val ex = new IllegalStateException(
+            "Unexpected FrameAnalysisMeta associated with IR " + ir + "\nOld: " + meta + " new " + newMeta
+          )
+//            ex.setStackTrace(ex.getStackTrace().slice(0, 10))
+          if (meta.isInstanceOf[FramePointer]) {
+            ex.printStackTrace()
+            meta.asInstanceOf[FramePointer].when.printStackTrace()
+          }
+          // throw ex
+        }
+      case None =>
     }
+    ir.passData().update(this, newMeta)
   }

   /** Returns the index of the given `defOcc` definition in the given `scope`
diff --git engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/core/ir/MetadataStorageTest.scala engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/core/ir/MetadataStorageTest.scala
index 1f2fdf6558..07a0c187d2 100644
--- engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/core/ir/MetadataStorageTest.scala
+++ engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/core/ir/MetadataStorageTest.scala
@@ -159,7 +159,7 @@ class MetadataStorageTest extends CompilerTest {
       meta1.update(TestPass1, meta)
       meta2.update(TestPass1, meta)

-      meta1 shouldNot equal(meta2)
+      meta1 shouldEqual meta2
     }

     def newMetadataStorage(init: Seq[MetadataPair[_]]): MetadataStorage = {
@@ -201,8 +201,8 @@ class MetadataStorageTest extends CompilerTest {
         )
       )

-      meta.duplicate shouldNot equal(meta)
-      meta.duplicate shouldNot equal(expected)
+      meta.duplicate shouldEqual meta
+      meta.duplicate shouldEqual expected
     }

     "enforce safe construction" in {
diff --git engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
index e0ddd703ae..89f08ccec1 100644
--- engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
+++ engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
@@ -3,10 +3,10 @@ package org.enso.compiler.core.ir;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.function.BiFunction;
 import java.util.stream.Collectors;
@@ -15,14 +15,17 @@ import scala.Option;

 /** Stores metadata for the various passes. */
 public final class MetadataStorage {
-  private Map<ProcessingPass, ProcessingPass.Metadata> metadata;
+  private SortedMap<ProcessingPass, ProcessingPass.Metadata> metadata;

   public MetadataStorage() {
-    this(Collections.emptyMap());
+    this(copyMetaMap(null));
   }

   public MetadataStorage(Map<ProcessingPass, ProcessingPass.Metadata> init) {
-    this.metadata = init;
+    this.metadata =
+        init instanceof SortedMap<ProcessingPass, ProcessingPass.Metadata> sorted
+            ? sorted
+            : copyMetaMap(init);
   }

   /**
@@ -34,7 +37,7 @@ public final class MetadataStorage {
    * @tparam K the concrete type of `pass`
    */
   public void update(ProcessingPass pass, ProcessingPass.Metadata newMeta) {
-    var copy = copyMetaMap();
+    var copy = copyMetaMap(metadata);
     copy.put(pass, newMeta);
     metadata = copy;
   }
@@ -63,7 +66,7 @@ public final class MetadataStorage {
     if (prev == null) {
       return Option.empty();
     } else {
-      var copy = copyMetaMap();
+      var copy = copyMetaMap(metadata);
       copy.remove(pass);
       metadata = copy;
       return Option.apply(prev);
@@ -88,7 +91,7 @@ public final class MetadataStorage {
    * @return a deep copy of `this`
    */
   public MetadataStorage duplicate() {
-    var map = new HashMap<ProcessingPass, ProcessingPass.Metadata>();
+    var map = new TreeMap<ProcessingPass, ProcessingPass.Metadata>(COMPARATOR);
     for (var entry : this.metadata.entrySet()) {
       var key = entry.getKey();
       var meta = (ProcessingPass.Metadata) entry.getValue();
@@ -151,24 +154,17 @@ public final class MetadataStorage {
    * @return `true` if restoration was successful, `false` otherwise
    */
   public boolean restoreFromSerialization(CompilerStub compiler) {
-    var ok = new boolean[] {true};
-    var newMap =
-        metadata.entrySet().stream()
-            .collect(
-                Collectors.toMap(
-                    Map.Entry::getKey,
-                    (en) -> {
-                      var value = en.getValue();
-                      var newOption = value.restoreFromSerialization(compiler);
-                      if (newOption.nonEmpty()) {
-                        return newOption.get();
-                      } else {
-                        ok[0] = false;
-                        return value;
-                      }
-                    }));
+    var newMap = new TreeMap<ProcessingPass, ProcessingPass.Metadata>(COMPARATOR);
+    for (var en : metadata.entrySet()) {
+      var newOption = en.getValue().restoreFromSerialization(compiler);
+      if (newOption.nonEmpty()) {
+        newMap.put(en.getKey(), newOption.get());
+      } else {
+        return false;
+      }
+    }
     this.metadata = newMap;
-    return ok[0];
+    return true;
   }

   @Override
@@ -195,9 +191,12 @@ public final class MetadataStorage {
         return p1.getClass().getName().compareTo(p2.getClass().getName());
       };

-  private Map<ProcessingPass, ProcessingPass.Metadata> copyMetaMap() {
+  private static SortedMap<ProcessingPass, ProcessingPass.Metadata> copyMetaMap(
+      Map<ProcessingPass, ProcessingPass.Metadata> data) {
     var copy = new TreeMap<ProcessingPass, ProcessingPass.Metadata>(COMPARATOR);
-    copy.putAll(metadata);
+    if (data != null) {
+      copy.putAll(data);
+    }
     return copy;
   }

@@ -214,7 +213,7 @@ public final class MetadataStorage {
       return true;
     }
     if (obj instanceof MetadataStorage other) {
-      return this.metadata == other.metadata;
+      return this.metadata.equals(other.metadata);
     }
     return false;
   }

but that fails with StackoverFlowError due to cycles in the IR. Possibly caused by #11509

[enso-bot[bot]]

Jaroslav Tulach reports a new STANDUP for yesterday (2024-11-07):

Progress: .

• fighting with https://github.com/enso-org/enso/issues/11171#issuecomment-2462828693
• Expanding AliasAnalysisTest with if/then section: aa93b06019
• review regex benchmarks: https://github.com/enso-org/enso/pull/11504#discussion_r1832168660
• search order for --jvm option: https://github.com/enso-org/enso/pull/11500#pullrequestreview-2420150162
• review of mini passes: https://github.com/enso-org/enso/pull/11501#pullrequestreview-2420348122
• never ending (useless) dissatisfaction with logging: https://github.com/enso-org/enso/pull/11472#issuecomment-2461849874
• discussing new benchmarks UI: https://github.com/enso-org/enso/issues/10212#issuecomment-2461945145 It should be finished by 2024-11-08.

[JaroslavTulach]

I tried to re-enable proper check of metadata.equals(other.metadata) in:

There are problems with the equals check. I suspect that one of the possible cases is that we sometimes use the same MetadataStorage for multiple IR elements. When that happens we risk a change of metadata in one of the elements will affect other elements. That's probably not desirable. Clear place where unwanted(?) sharing is happening are the IR.copy methods. Let's do a shallow copy in copy methods:

• obviously, this would be way easier to change if all the IR implementations were #11498

CCing @radeusgd

[JaroslavTulach]

Finally found the problem! this.passData ne passData will do the fix! PR #11513 is ready for your consideration.bb