Report human-readable names for shadowed parameters (#8115)

Improved warning reporting by not reporting IR in user-directed warning messages. Made sure that fresh names retain some knowledge of the name which they were created from. Closes #7963.
2024-11-23 08:08:34 +03:00 · 2023-10-20 11:01:52 +02:00 · 2023-10-20 11:01:52 +02:00 · cd94b388d1
commit cd94b388d1
parent a9387cd051
18 changed files with 113 additions and 39 deletions
--- a/engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java
+++ b/engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java
@ -458,7 +458,8 @@ final class TreeToIr {
              l.copy$default$3(),
              l.copy$default$4(),
              l.copy$default$5(),
-              l.copy$default$6()
+              l.copy$default$6(),
+              l.copy$default$7()
            );
        } else {
            args = cons(typeArg, args);
@ -471,7 +472,7 @@ final class TreeToIr {
        case 1 -> fullQualifiedNames.head();
        default -> {
          var name = fullQualifiedNames.head();
-          name = new Name.Literal(name.name(), true, name.location(), name.passData(), name.diagnostics());
+          name = new Name.Literal(name.name(), true, name.location(), Option.empty(), name.passData(), name.diagnostics());
          List<Name> tail = (List<Name>)fullQualifiedNames.tail();
          tail = tail.reverse();
          final Option<IdentifiedLocation> loc = getIdentifiedLocation(app);
@ -746,7 +747,7 @@ final class TreeToIr {
            var lhs = unnamedCallArgument(app.getLhs());
            var rhs = unnamedCallArgument(app.getRhs());
            var name = new Name.Literal(
-              op.codeRepr(), true, getIdentifiedLocation(op), meta(), diag()
+              op.codeRepr(), true, getIdentifiedLocation(op), Option.empty(), meta(), diag()
            );
            var loc = getIdentifiedLocation(app);
            if (lhs == null && rhs == null) {
@ -803,7 +804,7 @@ final class TreeToIr {
          var subexpression = Objects.requireNonNullElse(applySkip(body), body);
          yield translateExpression(subexpression, false);
        }
-        var fn = new Name.Literal(fullName, true, Option.empty(), meta(), diag());
+        var fn = new Name.Literal(fullName, true, Option.empty(), Option.empty(), meta(), diag());
        if (!checkArgs(args)) {
          yield translateSyntaxError(app, Syntax.UnexpectedExpression$.MODULE$);
        }
@ -952,7 +953,7 @@ final class TreeToIr {
            n.copy$default$6()
          );
          case Expression expr -> {
-            var negate = new Name.Literal("negate", true, Option.empty(), meta(), diag());
+            var negate = new Name.Literal("negate", true, Option.empty(), Option.empty(), meta(), diag());
            var arg = new CallArgument.Specified(Option.empty(), expr, expr.location(), meta(), diag());
            yield new Application.Prefix(negate, cons(arg, nil()), false, expr.location(), meta(), diag());
          }
@ -1104,6 +1105,7 @@ final class TreeToIr {
            var name = new Name.Literal(
                    op.codeRepr(), true,
                    getIdentifiedLocation(app),
+                    Option.empty(),
                    meta(), diag()
            );
            var loc = getIdentifiedLocation(app);
@ -1144,7 +1146,7 @@ final class TreeToIr {
  Expression translateTypeAnnotated(Tree.TypeAnnotated anno) {
    var type = translateTypeCallArgument(anno.getType());
    var expr = translateCallArgument(anno.getExpression());
-    var opName = new Name.Literal(anno.getOperator().codeRepr(), true, Option.empty(), meta(), diag());
+    var opName = new Name.Literal(anno.getOperator().codeRepr(), true, Option.empty(), Option.empty(), meta(), diag());
    return new Operator.Binary(
            expr,
            opName,
@ -1666,7 +1668,7 @@ final class TreeToIr {

  private Name.Literal buildName(Option<IdentifiedLocation> loc, Token id, boolean isMethod) {
    final String name = id.codeRepr();
-    return new Name.Literal(name, isMethod, loc, meta(), diag());
+    return new Name.Literal(name, isMethod, loc, Option.empty(), meta(), diag());
  }

  private Name sanitizeName(Name.Literal id) {
--- 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
@ -436,6 +436,7 @@ object Name {
    * @param name        the literal text of the name
    * @param isMethod    is this a method call name
    * @param location    the source location that the node corresponds to
+    * @param originalName the name which this literal has replaced, if any
    * @param passData    the pass metadata associated with this node
    * @param diagnostics compiler diagnostics for this node
    */
@ -443,6 +444,7 @@ object Name {
    override val name: String,
    override val isMethod: Boolean,
    override val location: Option[IdentifiedLocation],
+    originalName: Option[Name]                  = None,
    override val passData: MetadataStorage      = MetadataStorage(),
    override val diagnostics: DiagnosticStorage = DiagnosticStorage()
  ) extends Name {
@ -453,6 +455,7 @@ object Name {
      * @param name        the literal text of the name
      * @param isMethod    is this a method call name
      * @param location    the source location that the node corresponds to
+      * @param originalName the name which this literal has replaced, if any
      * @param passData    the pass metadata associated with this node
      * @param diagnostics compiler diagnostics for this node
      * @param id          the identifier for the new node
@ -462,12 +465,13 @@ object Name {
      name: String                         = name,
      isMethod: Boolean                    = isMethod,
      location: Option[IdentifiedLocation] = location,
+      originalName: Option[Name]           = originalName,
      passData: MetadataStorage            = passData,
      diagnostics: DiagnosticStorage       = diagnostics,
      id: Identifier                       = id
    ): Literal = {
      val res =
-        Literal(name, isMethod, location, passData, diagnostics)
+        Literal(name, isMethod, location, originalName, passData, diagnostics)
      res.id = id
      res
    }
--- a/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/warnings/Shadowed.scala
+++ b/engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/warnings/Shadowed.scala
@ -27,7 +27,7 @@ object Shadowed {
    override val location: Option[IdentifiedLocation]
  ) extends Shadowed {
    override def message(source: Source): String =
-      s"The argument $shadowedName is shadowed by $shadower"
+      s"The argument '$shadowedName' is shadowed by another one with the same name."

    override def diagnosticKeys(): Array[Any] =
      Array(shadowedName, shadower)
@ -46,7 +46,7 @@ object Shadowed {
    override val location: Option[IdentifiedLocation]
  ) extends Shadowed {
    override def message(source: Source): String =
-      s"The pattern field $shadowedName is shadowed by $shadower."
+      s"The pattern field '$shadowedName' is shadowed by another one with the same name."

    override def diagnosticKeys(): Array[Any] =
      Array(shadowedName, shadower)
--- a/engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
@ -1585,7 +1585,7 @@ class IrToTruffle(
      */
    def processName(name: Name): RuntimeExpression = {
      val nameExpr = name match {
-        case Name.Literal(nameStr, _, _, _, _) =>
+        case Name.Literal(nameStr, _, _, _, _, _) =>
          val useInfo = name
            .unsafeGetMetadata(
              AliasAnalysis,
@ -1613,6 +1613,7 @@ class IrToTruffle(
              Constants.Names.SELF_ARGUMENT,
              isMethod = false,
              location,
+              None,
              passData
            )
          )
--- a/engine/runtime/src/main/scala/org/enso/compiler/context/FreshNameSupply.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/context/FreshNameSupply.scala
@ -10,26 +10,29 @@ class FreshNameSupply {

  private def mkName(
    numId: Long,
-    isMethod: Boolean
+    isMethod: Boolean,
+    from: Option[Name]
  ): Name.Literal = {
    Name.Literal(
      s"<internal-${numId}>",
      isMethod,
-      None
+      None,
+      from
    )
  }

  /** Generates a name guaranteed not to exist in this program.
    *
    * @param isMethod whether or not the name should represent a method name.
+    * @param from the original name which the fresh name will substitute for
    * @return a new name
    */
  def newName(
-    isMethod: Boolean = false
+    isMethod: Boolean  = false,
+    from: Option[Name] = None
  ): Name.Literal = {
    val num = counter
    counter += 1
-
-    mkName(num, isMethod)
+    mkName(num, isMethod, from)
  }
 }
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AutomaticParallelism.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/AutomaticParallelism.scala
@ -285,7 +285,7 @@ object AutomaticParallelism extends IRPass {
    val refVars = threadBlocks.values.flatten
      .map(_.ir)
      .collect { case bind: Expression.Binding =>
-        bind.name -> freshNameSupply.newName()
+        bind.name -> freshNameSupply.newName(from = Some(bind.name))
      }
      .toMap

--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/BindingAnalysis.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/BindingAnalysis.scala
@ -82,9 +82,9 @@ case object BindingAnalysis extends IRPass {
            if (!shadowed && n.name == moduleContext.getName().item)
              Some(ref.methodName.name)
            else None
-          case Some(Name.Literal(n, _, _, _, _)) =>
-            val shadowed = definedSumTypes.exists(_.name == n)
-            if (!shadowed && n == moduleContext.getName().item)
+          case Some(literal: Name.Literal) =>
+            val shadowed = definedSumTypes.exists(_.name == literal.name)
+            if (!shadowed && literal.name == moduleContext.getName().item)
              Some(ref.methodName.name)
            else None
          case None => Some(ref.methodName.name)
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/ComplexType.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/ComplexType.scala
@ -168,8 +168,8 @@ case object ComplexType extends IRPass {
      val sig = lastSignature match {
        case Some(Type.Ascription(typed, _, _, _, _)) =>
          typed match {
-            case Name.Literal(nameStr, _, _, _, _) =>
-              if (name.name == nameStr) {
+            case literal: Name.Literal =>
+              if (name.name == literal.name) {
                lastSignature
              } else {
                unusedSig = lastSignature
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/NestedPatternMatch.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/desugar/NestedPatternMatch.scala
@ -204,12 +204,12 @@ case object NestedPatternMatch extends IRPass {
  ): Case.Branch = {
    if (containsNestedPatterns(branch.pattern)) {
      branch.pattern match {
-        case cons @ Pattern.Constructor(_, fields, _, _, _) =>
+        case cons @ Pattern.Constructor(constrName, fields, _, _, _) =>
          // Note [Unsafe Getting the Nested Field]
          val (lastNestedPattern, nestedPosition) =
            fields.zipWithIndex.findLast { case (pat, _) => isNested(pat) }.get

-          val newName  = freshNameSupply.newName()
+          val newName  = freshNameSupply.newName(from = Some(constrName))
          val newField = Pattern.Name(newName, None)
          val nestedScrutinee =
            newName.duplicate()
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/lint/UnusedBindings.scala
@ -203,9 +203,14 @@ case object UnusedBindings extends IRPass {
        s
      case s @ DefinitionArgument.Specified(name, _, default, _, _, _, _) =>
        if (!isIgnored && !isUsed) {
+          val nameToReport = name match {
+            case literal: Name.Literal =>
+              literal.originalName.getOrElse(literal)
+            case _ => name
+          }
          s.copy(
            defaultValue = default.map(runExpression(_, context))
-          ).addDiagnostic(warnings.Unused.FunctionArgument(name))
+          ).addDiagnostic(warnings.Unused.FunctionArgument(nameToReport))
        } else s
    }
  }
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/LambdaConsolidate.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/optimise/LambdaConsolidate.scala
@ -406,7 +406,7 @@ case object LambdaConsolidate extends IRPass {
    * @param argsWithShadowed the args with whether or not they are shadowed
    * @return a set of argument names, with shadowed arguments replaced
    */
-  def generateNewNames(
+  private def generateNewNames(
    argsWithShadowed: List[(DefinitionArgument, Boolean)],
    freshNameSupply: FreshNameSupply
  ): List[DefinitionArgument] = {
@ -418,7 +418,7 @@ case object LambdaConsolidate extends IRPass {
        val newName =
          if (isShadowed) {
            freshNameSupply
-              .newName()
+              .newName(from = Some(name))
              .copy(
                location    = name.location,
                passData    = name.passData,
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/FullyQualifiedNames.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/FullyQualifiedNames.scala
@ -322,7 +322,7 @@ case object FullyQualifiedNames extends IRPass {
              err => Some(err),
              _.map(resolvedMod =>
                freshNameSupply
-                  .newName()
+                  .newName(from = Some(name))
                  .updateMetadata(this -->> resolvedMod)
                  .setLocation(name.location)
              )
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/IgnoredBindings.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/IgnoredBindings.scala
@ -140,7 +140,7 @@ case object IgnoredBindings extends IRPass {
  ): Expression.Binding = {
    if (isIgnore(binding.name)) {
      val newName = supply
-        .newName()
+        .newName(from = Some(binding.name))
        .copy(
          location    = binding.name.location,
          passData    = binding.name.passData,
@ -226,7 +226,7 @@ case object IgnoredBindings extends IRPass {
      case spec: DefinitionArgument.Specified =>
        if (isIgnored) {
          val newName = freshNameSupply
-            .newName()
+            .newName(from = Some(spec.name))
            .copy(
              location    = arg.name.location,
              passData    = arg.name.passData,
@ -271,9 +271,9 @@ case object IgnoredBindings extends IRPass {
    */
  def isIgnore(ir: Name): Boolean = {
    ir match {
-      case _: Name.Blank                  => true
-      case Name.Literal(name, _, _, _, _) => name == "_"
-      case _                              => false
+      case _: Name.Blank         => true
+      case literal: Name.Literal => literal.name == "_"
+      case _                     => false
    }
  }

@ -327,7 +327,7 @@ case object IgnoredBindings extends IRPass {
      case named @ Pattern.Name(name, _, _, _) =>
        if (isIgnore(name)) {
          val newName = supply
-            .newName()
+            .newName(from = Some(name))
            .copy(
              location    = name.location,
              passData    = name.passData,
@ -351,7 +351,7 @@ case object IgnoredBindings extends IRPass {
      case typed @ Pattern.Type(name, _, _, _, _) =>
        if (isIgnore(name)) {
          val newName = supply
-            .newName()
+            .newName(from = Some(name))
            .copy(
              location    = name.location,
              passData    = name.passData,
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/MethodDefinitions.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/MethodDefinitions.scala
@ -141,8 +141,8 @@ case object MethodDefinitions extends IRPass {
    typePointer match {
      case _: Name.Qualified | _: Name.Literal =>
        val items = typePointer match {
-          case Name.Qualified(names, _, _, _) => names.map(_.name)
-          case Name.Literal(name, _, _, _, _) => List(name)
+          case qualName: Name.Qualified => qualName.parts.map(_.name)
+          case literal: Name.Literal    => List(literal.name)
          case _ =>
            throw new CompilerError("Impossible to reach.")
        }
--- a/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/SuspendedArguments.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/SuspendedArguments.scala
@ -262,8 +262,8 @@ case object SuspendedArguments extends IRPass {
    */
  def representsSuspended(value: Expression): Boolean = {
    value match {
-      case Name.Literal("Suspended", _, _, _, _) => true
-      case _                                     => false
+      case Name.Literal("Suspended", _, _, _, _, _) => true
+      case _                                        => false
    }
  }

--- a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/ShadowedPatternFieldsTest.scala
+++ b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/ShadowedPatternFieldsTest.scala
@ -83,6 +83,9 @@ class ShadowedPatternFieldsTest extends CompilerTest {

      warning.shadowedName shouldEqual "a"
      warning.shadower shouldEqual pattern.fields(2)
+      warning.message(
+        null
+      ) shouldBe "The pattern field 'a' is shadowed by another one with the same name."
    }
  }
 }
--- a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/UnusedBindingsTest.scala
+++ b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/lint/UnusedBindingsTest.scala
@ -207,5 +207,58 @@ class UnusedBindingsTest extends CompilerTest with Inside {

      lintMeta2 shouldBe empty
    }
+
+    "report human-readable names for shadowed arguments" in {
+      implicit val ctx: ModuleContext = mkModuleContext
+
+      val ir =
+        """
+          |f a a = 10
+          |main =
+          |    f 0 1
+          |""".stripMargin.preprocessModule.lint
+
+      inside(ir.bindings.head) { case definition: definition.Method.Explicit =>
+        inside(definition.body) { case f: Function.Lambda =>
+          val lintMeta = f.arguments(1).diagnostics.collect {
+            case u: warnings.Unused.FunctionArgument => u
+          }
+
+          lintMeta should not be empty
+          lintMeta.head shouldBe an[warnings.Unused.FunctionArgument]
+          lintMeta.head.name.name shouldEqual "a"
+        }
+      }
+    }
+
+    "report human-readable names for shadowed bindings in patterns" in {
+      implicit val ctx: InlineContext = mkInlineContext
+
+      val ir =
+        """
+          |case x of
+          |    Cons a a -> 10
+          |""".stripMargin.preprocessExpression.get.lint
+          .asInstanceOf[Expression.Block]
+          .returnValue
+          .asInstanceOf[Case.Expr]
+
+      val pattern = ir.branches.head.pattern.asInstanceOf[Pattern.Constructor]
+      val field1  = pattern.fields.head.asInstanceOf[Pattern.Name]
+      val field2  = pattern.fields(1).asInstanceOf[Pattern.Name]
+
+      val lintMeta1 = field1.diagnostics.collect { case u: warnings.Unused =>
+        u
+      }
+      val lintMeta2 = field2.diagnostics.collect { case u: warnings.Unused =>
+        u
+      }
+
+      lintMeta2 should not be empty
+      lintMeta2.head shouldBe an[warnings.Unused.PatternBinding]
+      lintMeta2.head.name.name shouldEqual "a"
+
+      lintMeta1 shouldBe empty
+    }
  }
 }
--- a/engine/runtime/src/test/scala/org/enso/compiler/test/pass/optimise/LambdaConsolidateTest.scala
+++ b/engine/runtime/src/test/scala/org/enso/compiler/test/pass/optimise/LambdaConsolidateTest.scala
@ -297,6 +297,9 @@ class LambdaConsolidateTest extends CompilerTest {
      ws should not be empty
      ws.head.shadowedName shouldEqual "x"
      ws.head.shadower shouldBe ir.arguments(1)
+      ws.head.message(
+        null
+      ) shouldBe "The argument 'x' is shadowed by another one with the same name."
    }

    "consolidate chained lambdas if the chaining occurs via a single-lined block" in {