enso-org / enso

Hybrid visual and textual functional programming.
https://ensoanalytics.com
Apache License 2.0
7.36k stars 324 forks source link

Internal Compiler Error exposed if duplicate field names are provided in a type #7962

Closed radeusgd closed 10 months ago

radeusgd commented 1 year ago

Repro:

from Standard.Base import all

type My_Type
    Value a b c a

main = 42

Actual behaviour

Compiler crash exposing an internal error, impossible to locate which part of the codebase the error refers to without running with a debugger

Execution finished with an error: org.enso.compiler.core.CompilerError: Compiler Internal Error:
                Arguments should never be redefined. This is a bug.
                ()

        at <java> org.enso.compiler.pass.analyse.AliasAnalysis$.$anonfun$analyseArgumentDefs$1(AliasAnalysis.scala:560)
        at <java> scala.collection.immutable.List.map(List.scala:250)
        at <java> org.enso.compiler.pass.analyse.AliasAnalysis$.analyseArgumentDefs(AliasAnalysis.scala:496)
        at <java> org.enso.compiler.pass.analyse.AliasAnalysis$.$anonfun$analyseModuleDefinition$1(AliasAnalysis.scala:284)
        at <java> scala.collection.immutable.List.map(List.scala:246)
        at <java> org.enso.compiler.pass.analyse.AliasAnalysis$.analyseModuleDefinition(AliasAnalysis.scala:278)
        at <java> org.enso.compiler.pass.analyse.AliasAnalysis$.$anonfun$runModule$1(AliasAnalysis.scala:101)
        at <java> scala.collection.immutable.List.map(List.scala:246)
        at <java> org.enso.compiler.pass.analyse.AliasAnalysis$.runModule(AliasAnalysis.scala:101)
        at <java> org.enso.compiler.pass.PassManager.$anonfun$runPassesOnModule$2(PassManager.scala:101)
        at <java> scala.collection.LinearSeqOps.foldLeft(LinearSeq.scala:183)
        at <java> scala.collection.LinearSeqOps.foldLeft$(LinearSeq.scala:179)
        at <java> scala.collection.immutable.List.foldLeft(List.scala:79)
        at <java> org.enso.compiler.pass.PassManager.runPassesOnModule(PassManager.scala:92)
        at <java> org.enso.compiler.Compiler.runMethodBodyPasses(Compiler.scala:850)
        at <java> org.enso.compiler.Compiler.$anonfun$runCompilerPipeline$11(Compiler.scala:377)
        at <java> org.enso.compiler.Compiler.$anonfun$runCompilerPipeline$11$adapted(Compiler.scala:361)
        at <java> scala.collection.immutable.List.foreach(List.scala:333)
        at <java> org.enso.compiler.Compiler.runCompilerPipeline(Compiler.scala:361)
        at <java> org.enso.compiler.Compiler.go$1(Compiler.scala:232)
        at <java> org.enso.compiler.Compiler.runInternal(Compiler.scala:238)
        at <java> org.enso.compiler.Compiler.run(Compiler.scala:135)
        at <java> org.enso.interpreter.runtime.Module.compile(Module.java:372)
        at <java> org.enso.interpreter.runtime.Module.compileScope(Module.java:299)
        at <java> org.enso.interpreter.runtime.Module$InvokeMember.doInvoke(Module.java:644)
        at <java> org.enso.interpreter.runtime.ModuleGen$InteropLibraryExports$Cached.executeAndSpecialize(ModuleGen.java:115)
        at <java> org.enso.interpreter.runtime.ModuleGen$InteropLibraryExports$Cached.invokeMember(ModuleGen.java:104)
        at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotValueDispatch$InteropValue$SharedInvokeNode.doDefault(PolyglotValueDispatch.java:4553)
        at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotValueDispatchFactory$InteropValueFactory$SharedInvokeNodeGen$Inlined.executeAndSpecialize(PolyglotValueDispatchFactory.java:10292)
        at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotValueDispatchFactory$InteropValueFactory$SharedInvokeNodeGen$Inlined.executeShared(PolyglotValueDispatchFactory.java:10244)
        at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotValueDispatch$InteropValue$InvokeNoArgsNode.doDefault(PolyglotValueDispatch.java:4618)
        at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.PolyglotValueDispatchFactory$InteropValueFactory$InvokeNoArgsNodeGen.executeImpl(PolyglotValueDispatchFactory.java:10528)
        at <java> org.graalvm.truffle/com.oracle.truffle.polyglot.HostToGuestRootNode.execute(HostToGuestRootNode.java:124)
        at <java> org.graalvm.sdk/org.graalvm.polyglot.Value.invokeMember(Value.java:972)
        at <java> org.enso.polyglot.Module.getAssociatedType(Module.scala:19)
        at <java> org.enso.runner.Main$.runMain(Main.scala:834)
        at <java> org.enso.runner.Main$.runSingleFile(Main.scala:764)
        at <java> org.enso.runner.Main$.run(Main.scala:646)
        at <java> org.enso.runner.Main$.main(Main.scala:1149)
        at <java> org.enso.runner.Main.main(Main.scala)

Expected behaviour

A friendly error like a syntax error telling the user about a duplicate definition and location of the issue (file, line number), like in most errors.

C:\NBO\repr\bug1.enso:4:5-18: error: Field name `a` is used twice in the same constructor. Field names of a single constructor must be unique.
    4 |     Value a b c a
      |           ^     ^
JaroslavTulach commented 1 year ago

I have a fix that properly reports the error from CLI. I am struggling to write a test in "IDE mode" - e.g. strict.errors=false. I am afraid my idea to associate the error with Argument.defaultValue neither Argument.ascribedType isn't that great idea. Any advices @Akirathan or @hubertp?

diff --git engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Redefined.scala engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Redefined.scala
index 066711d86b..1e4739905c 100644
--- engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Redefined.scala
+++ engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Redefined.scala
@@ -564,6 +564,104 @@ object Redefined {
       s"(Redefined (Atom $typeName))"
   }

+  /** An error representing the redefinition of an atom in a given module.
+    *
+    * @param name    the name of the atom being redefined
+    * @param location    the location in the source to which this error
+    *                    corresponds
+    * @param passData    the pass metadata for the error
+    * @param diagnostics any diagnostics associated with this error.
+    */
+  sealed case class Arg(
+    name: Name,
+    override val location: Option[IdentifiedLocation],
+    override val passData: MetadataStorage      = MetadataStorage(),
+    override val diagnostics: DiagnosticStorage = DiagnosticStorage()
+  ) extends Redefined
+      with Diagnostic.Kind.Interactive
+      with module.scope.Definition
+      with IRKind.Primitive {
+    override protected var id: Identifier = randomId
+
+    /** Creates a copy of `this`.
+      *
+      * @param name    the name of the atom the method was being redefined
+      *                    on
+      * @param location    the location in the source to which this error
+      *                    corresponds
+      * @param passData    the pass metadata for the error
+      * @param diagnostics any diagnostics associated with this error.
+      * @param id          the identifier for the node
+      * @return a copy of `this`, updated with the specified values
+      */
+    def copy(
+      name: Name                       = name,
+      location: Option[IdentifiedLocation] = location,
+      passData: MetadataStorage            = passData,
+      diagnostics: DiagnosticStorage       = diagnostics,
+      id: Identifier                       = id
+    ): Arg = {
+      val res =
+        Arg(name, location, passData, diagnostics)
+      res.id = id
+      res
+    }
+
+    /** @inheritdoc */
+    override def duplicate(
+      keepLocations: Boolean   = true,
+      keepMetadata: Boolean    = true,
+      keepDiagnostics: Boolean = true,
+      keepIdentifiers: Boolean = false
+    ): Arg =
+      copy(
+        name = name.duplicate(
+          keepLocations,
+          keepMetadata,
+          keepDiagnostics,
+          keepIdentifiers
+        ),
+        location = if (keepLocations) location else None,
+        passData = if (keepMetadata) passData.duplicate else MetadataStorage(),
+        diagnostics =
+          if (keepDiagnostics) diagnostics.copy else DiagnosticStorage(),
+        id = if (keepIdentifiers) id else randomId
+      )
+
+    /** @inheritdoc */
+    override def setLocation(location: Option[IdentifiedLocation]): Arg =
+      copy(location = location)
+
+    /** @inheritdoc */
+    override def message(source: Source): String =
+      s"Redefining arguments is not supported: ${name.name} is " +
+      s"defined multiple times."
+
+    override def diagnosticKeys(): Array[Any] = Array(name.name)
+
+    /** @inheritdoc */
+    override def mapExpressions(fn: Expression => Expression): Arg = this
+
+    /** @inheritdoc */
+    override def toString: String =
+      s"""
+         |Error.Redefined.Arg(
+         |name = $name,
+         |location = $location,
+         |passData = ${this.showPassData},
+         |diagnostics = $diagnostics,
+         |id = $id
+         |)
+         |""".stripMargin
+
+    /** @inheritdoc */
+    override def children: List[IR] = List(name)
+
+    /** @inheritdoc */
+    override def showCode(indent: Int): String =
+      s"(Redefined (Argument $name))"
+  }
+
   /** An error representing the redefinition of a binding in a given scope.
     *
     * While bindings in child scopes are allowed to _shadow_ bindings in
diff --git engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
index 9ec93d7350..39e61b35fd 100644
--- engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
+++ engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
@@ -1764,6 +1764,10 @@ class IrToTruffle(
           context.getBuiltins
             .error()
             .makeCompileError(Text.create(err.message(source)))
+        case err: errors.Redefined.Arg =>
+          context.getBuiltins
+            .error()
+            .makeCompileError(Text.create(err.message(source)))
         case err: errors.Unexpected.TypeSignature =>
           context.getBuiltins
             .error()
diff --git engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala
index 21e43a6a38..19deb594d0 100644
--- engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala
+++ engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala
@@ -26,6 +26,7 @@ import org.enso.compiler.core.ir.expression.{
   Operator,
   Section
 }
+import org.enso.compiler.core.ir.expression.errors.Redefined
 import org.enso.compiler.core.ir.`type`
 import org.enso.compiler.pass.IRPass
 import org.enso.compiler.pass.analyse.AliasAnalysis.Graph.{Occurrence, Scope}
@@ -554,12 +555,13 @@ case object AliasAnalysis extends IRPass {
             )
             .updateMetadata(this -->> Info.Occurrence(graph, occurrenceId))
         } else {
-          throw new CompilerError(
-            s"""
-                Arguments should never be redefined. This is a bug.
-                ${}
-                """
+          val f = scope.occurrences.collectFirst {
+            case x if x.symbol == name.name => x
+          }
+          arg.copy(
+            ascribedType = Some(new Redefined.Arg(name, arg.location))
           )
+            .updateMetadata(this -->> Info.Occurrence(graph, f.get.id))
         }
     }
   }
diff --git engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
index a41d9ffdc1..5865b038f6 100644
--- engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
+++ engine/runtime/src/test/java/org/enso/compiler/ExecCompilerTest.java
@@ -72,6 +72,17 @@ public class ExecCompilerTest {
     }
   }

+  @Test
+  public void redefinedArgument() throws Exception {
+    var module = ctx.eval("enso", """
+    type My_Type
+        Value a b c a
+    """);
+    var run = module.invokeMember("eval_expression", "My_Type.Value");
+    var error = run.newInstance(1, 2, 3, 4);
+    assertTrue("We get an error value back: " + error, error.isException());
+  }
+
   @Test
   public void testSelfAssignment() throws Exception {
     var module = ctx.eval("enso", """
Akirathan commented 1 year ago

@JaroslavTulach

I wouldn't put this IR error into ascribedType field, but rather add it as a diagnostic, like so:

diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala
index 21e43a6a3..e18285cda 100644
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala
@@ -26,6 +26,7 @@ import org.enso.compiler.core.ir.expression.{
   Operator,
   Section
 }
+import org.enso.compiler.core.ir.expression.errors.Redefined
 import org.enso.compiler.core.ir.`type`
 import org.enso.compiler.pass.IRPass
 import org.enso.compiler.pass.analyse.AliasAnalysis.Graph.{Occurrence, Scope}
@@ -554,11 +555,8 @@ case object AliasAnalysis extends IRPass {
             )
             .updateMetadata(this -->> Info.Occurrence(graph, occurrenceId))
         } else {
-          throw new CompilerError(
-            s"""
-                Arguments should never be redefined. This is a bug.
-                ${}
-                """
+          arg.copy().addDiagnostic(
+            Redefined.Arg(name, arg.location)
           )
         }
     }
enso-bot[bot] commented 10 months ago

Jaroslav Tulach reports a new STANDUP for yesterday (2023-12-01):

Progress: - #7962 fixed by PR: https://github.com/enso-org/enso/pull/8438

Next Day: Bugfix something

Discord
Discord - A New Way to Chat with Friends & Communities
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.