MetadataStorage is mutable - structural equals in copy isn't enou…

…gh (#11513) `MetadataStorage` is mutable. Using `Object.equals` in `IR.copy` methods to check whether a _copy is needed_ isn't enough. The fact that two storages are `Object.equal` may be just temporary. Replacing the checks in 69 `IR.copy` methods with identity check - e.g. `ne` in Scala which is `==` in Java. Using proper structural check inside of `MetadataStorage` fixes #11171. # Important Notes I [used this regex](#11171 (comment)) to find out 69 instances of `IR.copy`: ![69 copy methods](https://github.com/user-attachments/assets/257580b9-54fc-4199-88ad-a22103b0041f) and I modified all 69 of them.
enso-org · Nov 8, 2024 · 67db825 · 67db825
1 parent f2037ee
commit 67db825
Show file tree

Hide file tree

Showing 36 changed files with 95 additions and 145 deletions.
diff --git a/...integration-tests/src/test/scala/org/enso/compiler/test/core/ir/MetadataStorageTest.scala b/...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 a/engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.java b/engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.java
@@ -1,6 +1,5 @@
 package org.enso.compiler.core;
 
-import java.util.Comparator;
 import java.util.UUID;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -29,24 +28,6 @@
  * <p>See also: Note [IR Equality and hashing]
  */
 public interface IR {
-  /**
-   * Compares IR structure, but not metadata neither diagnostics. A special comparator used by
-   * <em>Persistance API</em> to perform some rare consistency checks.
-   */
-  public static final Comparator<IR> STRUCTURE_COMPARATOR =
-      (aIr, bIr) -> {
-        if (aIr == bIr) {
-          return 0;
-        }
-        var aCopy = aIr.duplicate(true, false, false, true);
-        var bCopy = bIr.duplicate(true, false, false, true);
-
-        if (aCopy.equals(bCopy)) {
-          return 0;
-        }
-        return System.identityHashCode(aIr) - System.identityHashCode(bIr);
-      };
-
   /**
    * Storage for metadata that the node has been tagged with as the result of various compiler
    * passes.

diff --git a/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java b/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java
@@ -3,7 +3,6 @@
 import java.io.IOException;
 import java.util.AbstractList;
 import java.util.ArrayList;
-import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.UUID;
 import org.enso.compiler.core.ir.expression.Application;
@@ -445,33 +444,6 @@ protected Seq readObject(Input in) throws IOException, ClassNotFoundException {
     }
   }
 
-  @ServiceProvider(service = Persistance.class)
-  public static final class PersistMetadataStorage extends Persistance<MetadataStorage> {
-    public PersistMetadataStorage() {
-      super(MetadataStorage.class, false, 389);
-    }
-
-    @Override
-    @SuppressWarnings("unchecked")
-    protected void writeObject(MetadataStorage obj, Output out) throws IOException {
-      var map = new LinkedHashMap<ProcessingPass, ProcessingPass.Metadata>();
-      obj.map(
-          (processingPass, data) -> {
-            map.put(processingPass, data);
-            return null;
-          });
-      out.writeInline(java.util.Map.class, map);
-    }
-
-    @Override
-    @SuppressWarnings("unchecked")
-    protected MetadataStorage readObject(Input in) throws IOException, ClassNotFoundException {
-      var map = in.readInline(java.util.Map.class);
-      var storage = new MetadataStorage(map);
-      return storage;
-    }
-  }
-
   @ServiceProvider(service = Persistance.class)
   public static final class PersistDiagnosticStorage extends Persistance<DiagnosticStorage> {
     public PersistDiagnosticStorage() {

diff --git a/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java b/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/MetadataStorage.java
@@ -11,18 +11,25 @@
 import java.util.function.BiFunction;
 import java.util.stream.Collectors;
 import org.enso.compiler.core.CompilerStub;
+import org.enso.persist.Persistable;
 import scala.Option;
 
 /** Stores metadata for the various passes. */
+@Persistable(id = 398)
 public final class MetadataStorage {
   private Map<ProcessingPass, ProcessingPass.Metadata> metadata;
 
+  /** Constructs empty, ready to be populated metadata. */
   public MetadataStorage() {
     this(Collections.emptyMap());
   }
 
-  public MetadataStorage(Map<ProcessingPass, ProcessingPass.Metadata> init) {
-    this.metadata = init;
+  MetadataStorage(Map<ProcessingPass, ProcessingPass.Metadata> metaValues) {
+    this.metadata = metaValues;
+  }
+
+  final Map<ProcessingPass, ProcessingPass.Metadata> metaValues() {
+    return this.metadata;
   }
 
   /**
@@ -82,6 +89,15 @@ public Option<ProcessingPass.Metadata> get(ProcessingPass pass) {
     return Option.apply(prev);
   }
 
+  /**
+   * Creates a shallow copy of `this`. Use when re-assigning metadata from one IR to another.
+   *
+   * @return a shallow copy of `this`
+   */
+  public MetadataStorage copy() {
+    return new MetadataStorage(metadata);
+  }
+
   /**
    * Creates a deep copy of `this`.
    *
@@ -214,7 +230,7 @@ public boolean equals(Object obj) {
       return true;
     }
     if (obj instanceof MetadataStorage other) {
-      return this.metadata == other.metadata;
+      return this.metadata.equals(other.metadata);
     }
     return false;
   }

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/CallArgument.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/CallArgument.scala
@@ -72,7 +72,7 @@ object CallArgument {
         name != this.name
         || value != this.value
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/DefinitionArgument.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/DefinitionArgument.scala
@@ -123,7 +123,7 @@ object DefinitionArgument {
         || defaultValue != this.defaultValue
         || suspended != this.suspended
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Empty.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Empty.scala
@@ -35,7 +35,7 @@ sealed case class Empty(
   ): Empty = {
     if (
       location != this.location
-      || passData != this.passData
+      || (passData ne this.passData)
       || diagnostics != this.diagnostics
       || id != this.id
     ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Expression.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Expression.scala
@@ -90,7 +90,7 @@ object Expression {
         || returnValue != this.returnValue
         || suspended != this.suspended
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -231,7 +231,7 @@ object Expression {
         name != this.name
         || expression != this.expression
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Function.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Function.scala
@@ -117,7 +117,7 @@ object Function {
         || body != this.body
         || location != this.location
         || canBeTCO != this.canBeTCO
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -283,7 +283,7 @@ object Function {
         || isPrivate != this.isPrivate
         || location != this.location
         || canBeTCO != this.canBeTCO
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Literal.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Literal.scala
@@ -65,7 +65,7 @@ object Literal {
         base != this.base
         || value != this.value
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -197,7 +197,7 @@ object Literal {
       if (
         text != this.text
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Module.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Module.scala
@@ -80,7 +80,7 @@ final case class Module(
       || bindings != this.bindings
       || isPrivate != this.isPrivate
       || location != this.location
-      || passData != this.passData
+      || (passData ne this.passData)
       || diagnostics != this.diagnostics
       || id != this.id
     ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Name.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Name.scala
@@ -76,7 +76,7 @@ object Name {
         typePointer != this.typePointer
         || methodName != this.methodName
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -246,7 +246,7 @@ object Name {
       if (
         parts != this.parts
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -318,7 +318,7 @@ object Name {
     ): Blank = {
       if (
         location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -400,7 +400,7 @@ object Name {
       if (
         specialName != this.specialName
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -506,7 +506,7 @@ object Name {
         || isMethod != this.isMethod
         || location != this.location
         || originalName != this.originalName
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -616,7 +616,7 @@ object Name {
       if (
         name != this.name
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -711,7 +711,7 @@ object Name {
         name != this.name
         || expression != this.expression
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -822,7 +822,7 @@ object Name {
       if (
         synthetic != this.synthetic
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -921,7 +921,7 @@ object Name {
     ): SelfType = {
       if (
         location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Pattern.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Pattern.scala
@@ -66,7 +66,7 @@ object Pattern {
       if (
         name != this.name
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -169,7 +169,7 @@ object Pattern {
         constructor != this.constructor
         || fields != this.fields
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -324,7 +324,7 @@ object Pattern {
       if (
         literal != this.literal
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -429,7 +429,7 @@ object Pattern {
         name != this.name
         || tpe != this.tpe
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -549,7 +549,7 @@ object Pattern {
       if (
         doc != this.doc
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {

diff --git a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Type.scala b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/Type.scala
@@ -54,7 +54,7 @@ object Type {
         args != this.args
         || result != this.result
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -171,7 +171,7 @@ object Type {
         || signature != this.signature
         || comment != this.comment
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -287,7 +287,7 @@ object Type {
         typed != this.typed
         || context != this.context
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {
@@ -399,7 +399,7 @@ object Type {
         typed != this.typed
         || error != this.error
         || location != this.location
-        || passData != this.passData
+        || (passData ne this.passData)
         || diagnostics != this.diagnostics
         || id != this.id
       ) {