From 2676aa50a3cc86a1c0673a3e60a553134e350b1b Mon Sep 17 00:00:00 2001 From: Dmitry Bushev Date: Tue, 28 Dec 2021 18:09:34 +0300 Subject: [PATCH] fix: method clash error (#3210) --- .../enso/compiler/codegen/IrToTruffle.scala | 7 +- .../scala/org/enso/compiler/core/IR.scala | 120 ++++++++++++++++++ .../pass/resolve/OverloadsResolution.scala | 18 ++- .../resolve/OverloadsResolutionTest.scala | 29 +++++ .../test/instrument/RuntimeServerTest.scala | 60 +++++++++ .../test/semantic/MethodsTest.scala | 1 - .../OverloadsResolutionErrorTest.scala | 25 +++- 7 files changed, 251 insertions(+), 9 deletions(-) diff --git a/engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala b/engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala index 82885c6288..4cfe79f241 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala @@ -464,7 +464,7 @@ class IrToTruffle( "No binding analysis at the point of codegen." ) bindingsMap.exportedSymbols.foreach { - case (name, List(resolution)) => + case (name, resolution :: _) => if (resolution.module.unsafeAsModule() != moduleScope.getModule) { resolution match { case BindingsMap.ResolvedConstructor(definitionModule, cons) => @@ -1100,6 +1100,11 @@ class IrToTruffle( .error() .compileError() .newInstance(Text.create(err.message)) + case err: Error.Redefined.MethodClashWithAtom => + context.getBuiltins + .error() + .compileError() + .newInstance(Text.create(err.message)) case err: Error.Redefined.Conversion => context.getBuiltins .error() diff --git a/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala b/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala index c86e08a0d9..c43396901d 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala @@ -7476,6 +7476,126 @@ object IR { s"(Redefined (Method $atomName.$methodName))" } + /** An error representing the redefinition of a method in a given module, + * when the module defines a method with the same name as an atom. + * This is also known as a name clash. + * + * @param atomName the name of the atom that clashes with the method + * @param methodName the method name being redefined in the module + * @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 MethodClashWithAtom( + atomName: IR.Name, + methodName: IR.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 atomName the name of the atom that clashes with the method + * @param methodName the method name being redefined in the module + * @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( + atomName: IR.Name = atomName, + methodName: IR.Name = methodName, + location: Option[IdentifiedLocation] = location, + passData: MetadataStorage = passData, + diagnostics: DiagnosticStorage = diagnostics, + id: Identifier = id + ): MethodClashWithAtom = { + val res = MethodClashWithAtom( + atomName, + methodName, + location, + passData, + diagnostics + ) + res.id = id + res + } + + /** @inheritdoc */ + override def duplicate( + keepLocations: Boolean = true, + keepMetadata: Boolean = true, + keepDiagnostics: Boolean = true, + keepIdentifiers: Boolean = false + ): MethodClashWithAtom = + copy( + atomName = atomName.duplicate( + keepLocations, + keepMetadata, + keepDiagnostics, + keepIdentifiers + ), + methodName = methodName + .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] + ): MethodClashWithAtom = + copy(location = location) + + /** @inheritdoc */ + override def message: String = + s"Method definitions with the same name as atoms are not supported. " + + s"Method ${methodName.name} clashes with the atom ${atomName.name} in this module." + + /** @inheritdoc */ + override def mapExpressions( + fn: Expression => Expression + ): MethodClashWithAtom = + this + + /** @inheritdoc */ + override def toString: String = + s""" + |IR.Error.Redefined.MethodClashWithAtom( + |atomName = $atomName, + |methodName = $methodName, + |location = $location, + |passData = ${this.showPassData}, + |diagnostics = $diagnostics, + |id = $id + |) + |""".stripMargin + + /** @inheritdoc */ + override def children: List[IR] = List(atomName, methodName) + + /** @inheritdoc */ + override def showCode(indent: Int): String = + s"(Redefined (MethodClash $atomName $methodName))" + } + /** An error representing the redefinition of an atom in a given module. * * @param atomName the name of the atom being redefined diff --git a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/OverloadsResolution.scala b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/OverloadsResolution.scala index 62d1fea9e3..0941f08685 100644 --- a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/OverloadsResolution.scala +++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/OverloadsResolution.scala @@ -74,11 +74,21 @@ case object OverloadsResolution extends IRPass { IR.Error.Redefined .Method(method.typeName, method.methodName, method.location) } else { - val currentMethods = seenMethods(method.typeName.name) - seenMethods = seenMethods + (method.typeName.name -> - (currentMethods + method.methodName.name)) + atoms.find(_.name.name.equalsIgnoreCase(method.methodName.name)) match { + case Some(clashedAtom) + if method.typeName.isInstanceOf[IR.Name.Here] => + IR.Error.Redefined.MethodClashWithAtom( + clashedAtom.name, + method.methodName, + method.location + ) + case _ => + val currentMethods = seenMethods(method.typeName.name) + seenMethods = seenMethods + (method.typeName.name -> + (currentMethods + method.methodName.name)) - method + method + } } }) diff --git a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/OverloadsResolutionTest.scala b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/OverloadsResolutionTest.scala index 391564c01a..fb82262652 100644 --- a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/OverloadsResolutionTest.scala +++ b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/resolve/OverloadsResolutionTest.scala @@ -138,4 +138,33 @@ class OverloadsResolutionTest extends CompilerTest { } } + "Atom overloads method resolution" should { + implicit val ctx: ModuleContext = mkModuleContext + + val atomName = "Foo" + val methodName = atomName.toLowerCase + + val ir = + s"""| + |type $atomName + |$methodName = 0 + |Unit.$methodName = 1 + |""".stripMargin.preprocessModule.resolve + + "detect overloads within a given module" in { + exactly(1, ir.bindings) shouldBe an[ + IR.Error.Redefined.MethodClashWithAtom + ] + } + + "replace all overloads by an error node" in { + ir.bindings(1) shouldBe an[IR.Error.Redefined.MethodClashWithAtom] + ir.bindings(1) + .asInstanceOf[IR.Error.Redefined.MethodClashWithAtom] + .methodName + .name shouldEqual methodName + ir.bindings(2) shouldBe an[IR.Module.Scope.Definition.Method.Explicit] + } + } + } diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala index 49a42015c9..cbbf1b6668 100644 --- a/engine/runtime/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala +++ b/engine/runtime/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala @@ -2512,6 +2512,66 @@ class RuntimeServerTest ) } + it should "return error when method name clashes with atom" in { + val contextId = UUID.randomUUID() + val requestId = UUID.randomUUID() + val moduleName = "Enso_Test.Test.Main" + val metadata = new Metadata + + val code = + """type Foo + |foo = 0 + |main = 0 + |""".stripMargin.linesIterator.mkString("\n") + val contents = metadata.appendToCode(code) + val mainFile = context.writeMain(contents) + + // create context + context.send(Api.Request(requestId, Api.CreateContextRequest(contextId))) + context.receive shouldEqual Some( + Api.Response(requestId, Api.CreateContextResponse(contextId)) + ) + + // Open the new file + context.send( + Api.Request(Api.OpenFileNotification(mainFile, contents)) + ) + context.receiveNone shouldEqual None + + // push main + context.send( + Api.Request( + requestId, + Api.PushContextRequest( + contextId, + Api.StackItem.ExplicitCall( + Api.MethodPointer(moduleName, "Enso_Test.Test.Main", "main"), + None, + Vector() + ) + ) + ) + ) + context.receive(3) should contain theSameElementsAs Seq( + Api.Response(requestId, Api.PushContextResponse(contextId)), + Api.Response( + Api.ExecutionUpdate( + contextId, + Seq( + Api.ExecutionResult.Diagnostic.error( + "Method definitions with the same name as atoms are not supported. Method foo clashes with the atom Foo in this module.", + Some(mainFile), + Some(model.Range(model.Position(1, 0), model.Position(1, 7))), + None, + Vector() + ) + ) + ) + ), + context.executionComplete(contextId) + ) + } + it should "return error with a stack trace" in { val contextId = UUID.randomUUID() val requestId = UUID.randomUUID() diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/MethodsTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/MethodsTest.scala index a619e486b5..733f6e997d 100644 --- a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/MethodsTest.scala +++ b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/MethodsTest.scala @@ -160,7 +160,6 @@ class MethodsTest extends InterpreterTest { |main = | myList = Cons 1 (Cons 2 (Cons 3 Nil)) | myList.sum - | |""".stripMargin eval(code) shouldEqual 6 diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala index 82a8b953f6..55f7625401 100644 --- a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala +++ b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala @@ -18,7 +18,7 @@ class OverloadsResolutionErrorTest extends InterpreterTest { interpreterContext: InterpreterContext ): Unit = { - "result in an error at runtime for methods" in { + "result in an error at runtime for method overloads" in { val code = """from Standard.Builtins import all | @@ -38,7 +38,7 @@ class OverloadsResolutionErrorTest extends InterpreterTest { ) } - "result in an error at runtime for atoms" in { + "result in an error at runtime for atom overloads" in { val code = """ |type MyAtom @@ -56,6 +56,25 @@ class OverloadsResolutionErrorTest extends InterpreterTest { "Test[3:1-3:11]: Redefining atoms is not supported: MyAtom is defined multiple times in this module." ) } - } + "result in an error at runtime for methods overloading atoms" in { + val code = + """ + |type Foo + |foo = 0 + |""".stripMargin.linesIterator.mkString("\n") + + the[InterpreterException] thrownBy eval(code) should have message + "Compilation aborted due to errors." + + val diagnostics = consumeOut + diagnostics + .filterNot(_.contains("Compiler encountered")) + .filterNot(_.contains("In module")) + .toSet shouldEqual Set( + "Test[3:1-3:7]: Method definitions with the same name as atoms are not supported. Method foo clashes with the atom Foo in this module." + ) + } + + } }