Parser simple expression error recovery change from null to ??? (#…

…19103) Previously, simpleExpr was recovered as `Literal(Constant(null))` which led to some errors in interactive. Type inference in Scala 3 works on whole chain, thus type vars were inferred as union type of `Null` because of this very reason. Recovering such errors as `unimplementedExpr` which has a type of `Nothing`, solves the issue. [Cherry-picked 98184f1][modified]
scala · Jun 25, 2024 · e064bf2 · e064bf2
1 parent 5ded2e3
commit e064bf2
Show file tree

Hide file tree

Showing 10 changed files with 118 additions and 93 deletions.
diff --git a/compiler/src/dotty/tools/dotc/parsing/Parsers.scala b/compiler/src/dotty/tools/dotc/parsing/Parsers.scala
@@ -398,7 +398,7 @@ object Parsers {
       false
     }
 
-    def errorTermTree(start: Offset): Literal = atSpan(start, in.offset, in.offset) { Literal(Constant(null)) }
+    def errorTermTree(start: Offset): Tree = atSpan(start, in.offset, in.offset) { unimplementedExpr }
 
     private var inFunReturnType = false
     private def fromWithinReturnType[T](body: => T): T = {

diff --git a/compiler/src/dotty/tools/dotc/util/Signatures.scala b/compiler/src/dotty/tools/dotc/util/Signatures.scala
@@ -97,6 +97,17 @@ object Signatures {
       case tp @ TypeApply(fun, types) => applyCallInfo(span, types, fun, true)
       case _ => (0, 0, Nil)
 
+
+  def isEnclosingApply(tree: tpd.Tree, span: Span)(using Context): Boolean =
+    tree match
+      case apply @ Apply(fun, _) => !fun.span.contains(span) && isValid(apply)
+      case unapply @ UnApply(fun, _, _) =>
+        !fun.span.contains(span) && !ctx.definitions.isFunctionNType(tree.tpe) // we want to show tuples in unapply
+      case typeTree @ AppliedTypeTree(fun, _) => !fun.span.contains(span) && isValid(typeTree)
+      case typeApply @ TypeApply(fun, _) => !fun.span.contains(span) && isValid(typeApply)
+      case _ => false
+
+
   /**
    * Finds enclosing application from given `path` for `span`.
    *
@@ -108,17 +119,26 @@ object Signatures {
    *         next subsequent application exists, it returns the latter
    */
   private def findEnclosingApply(path: List[tpd.Tree], span: Span)(using Context): tpd.Tree =
-    path.filterNot {
-      case apply @ Apply(fun, _) => fun.span.contains(span) || isValid(apply)
-      case unapply @ UnApply(fun, _, _) => fun.span.contains(span) || isValid(unapply)
-      case typeTree @ AppliedTypeTree(fun, _) => fun.span.contains(span) || isValid(typeTree)
-      case typeApply @ TypeApply(fun, _) => fun.span.contains(span) || isValid(typeApply)
-      case _ => true
-    } match {
+    import tpd.TreeOps
+
+    val filteredPath = path.filter:
+      case block @ Block(stats, expr) =>
+        block.existsSubTree(tree => isEnclosingApply(tree, span) && tree.span.contains(span))
+      case other => isEnclosingApply(other, span)
+
+    filteredPath match
       case Nil => tpd.EmptyTree
+      case tpd.Block(stats, expr) :: _ => // potential block containing lifted args
+
+        val enclosingFunction = stats.collectFirst:
+          case defdef: tpd.DefDef if defdef.rhs.span.contains(span) => defdef
+
+        val enclosingTree = enclosingFunction.getOrElse(expr)
+        findEnclosingApply(Interactive.pathTo(enclosingTree, span), span)
+
       case direct :: enclosing :: _ if isClosingSymbol(direct.source(span.end -1)) => enclosing
       case direct :: _ => direct
-    }
+
 
   private def isClosingSymbol(ch: Char) = ch == ')' || ch == ']'
 
@@ -303,7 +323,7 @@ object Signatures {
    * @param tree tree to validate
    */
   private def isValid(tree: tpd.Tree)(using Context): Boolean =
-    ctx.definitions.isTupleNType(tree.tpe) || ctx.definitions.isFunctionNType(tree.tpe)
+    !ctx.definitions.isTupleNType(tree.tpe) && !ctx.definitions.isFunctionNType(tree.tpe)
 
   /**
    * Get unapply method result type omiting unknown types and another method calls.

diff --git a/language-server/test/dotty/tools/languageserver/SignatureHelpTest.scala b/language-server/test/dotty/tools/languageserver/SignatureHelpTest.scala
@@ -516,13 +516,14 @@ class SignatureHelpTest {
       .signatureHelp(m2, List(signature), Some(0), 1)
   }
 
-  @Test def noUnapplyForTuple: Unit = {
+  @Test def unapplyForTuple: Unit = {
+    val signature = S("", Nil, List(List(P("", "Int"), P("", "Int"))), None)
     code"""object Main {
           |  (1, 2) match
           |    case (x${m1}, ${m2}) =>
           |}"""
-      .signatureHelp(m1, Nil, Some(0), 0)
-      .signatureHelp(m2, Nil, Some(0), 0)
+      .signatureHelp(m1, List(signature), Some(0), 0)
+      .signatureHelp(m2, List(signature), Some(0), 1)
   }
 
   @Test def unapplyCaseClass: Unit = {
@@ -693,7 +694,7 @@ class SignatureHelpTest {
       .signatureHelp(m1, sigs, None, 0)
       .signatureHelp(m2, sigs, None, 0)
       .signatureHelp(m3, sigs, Some(2), 0)
-      .signatureHelp(m4, sigs, None, 1)
+      .signatureHelp(m4, List(sig0, sig1), None, 1)
       .signatureHelp(m5, sigs, Some(2), 1)
       .signatureHelp(m6, sigs, Some(0), 1)
       .signatureHelp(m7, sigs, Some(1), 1)

diff --git a/presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala
@@ -37,8 +37,7 @@ object SignatureHelpProvider:
     val pos = driver.sourcePosition(params)
     val trees = driver.openedTrees(uri.nn)
 
-    val path =
-      Interactive.pathTo(trees, pos).dropWhile(t => notCurrentApply(t, pos))
+    val path = Interactive.pathTo(trees, pos)
 
     val (paramN, callableN, alternatives) =
       Signatures.signatureHelp(path, pos.span)
@@ -70,32 +69,6 @@ object SignatureHelpProvider:
     )
   end signatureHelp
 
-  private def isValid(tree: tpd.Tree)(using Context): Boolean =
-    ctx.definitions.isTupleClass(
-      tree.symbol.owner.companionClass
-    ) || ctx.definitions.isFunctionType(tree.tpe)
-
-  private def notCurrentApply(
-      tree: tpd.Tree,
-      pos: SourcePosition
-  )(using Context): Boolean =
-    tree match
-      case unapply: tpd.UnApply =>
-        unapply.fun.span.contains(pos.span) || isValid(unapply)
-      case typeTree @ AppliedTypeTree(fun, _) =>
-        fun.span.contains(pos.span) || isValid(typeTree)
-      case typeApply @ TypeApply(fun, _) =>
-        fun.span.contains(pos.span) || isValid(typeApply)
-      case appl: tpd.GenericApply =>
-        /* find first apply that the cursor is located in arguments and not at function name
-         * for example in:
-         *   `Option(1).fold(2)(@@_ + 1)`
-         * we want to find the tree responsible for the entire location, not just `_ + 1`
-         */
-        appl.fun.span.contains(pos.span)
-
-      case _ => true
-
   private def withDocumentation(
       info: SymbolDocumentation,
       signature: Signatures.Signature,

diff --git a/...entation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpPatternSuite.scala b/...entation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpPatternSuite.scala
@@ -15,8 +15,8 @@ class SignatureHelpPatternSuite extends BaseSignatureHelpSuite:
         |  }
         |}
         |""".stripMargin,
-      """|map[B](f: ((Int, Int)) => B): List[B]
-         |       ^^^^^^^^^^^^^^^^^^^^
+      """|(Int, Int)
+         |      ^^^
          |""".stripMargin
     )
 

diff --git a/presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpSuite.scala b/presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpSuite.scala
@@ -703,7 +703,7 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
          |""".stripMargin
     )
 
-  @Test def `local-method` = 
+  @Test def `local-method` =
     check(
       """
         |object Main {
@@ -721,7 +721,7 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
          |""".stripMargin,
     )
 
-  @Test def `local-method2` = 
+  @Test def `local-method2` =
       check(
         """
           |object Main {
@@ -739,7 +739,7 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
            |""".stripMargin,
       )
 
-  @Test def `local-method3` = 
+  @Test def `local-method3` =
     check(
       """
         |object Main {
@@ -756,4 +756,76 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
          |      ^^^^^^^^^
          |apply(a: Int): Int
          |""".stripMargin
-    )
+    )
+
+  @Test def `instantiated-type-var-old-ext-1` =
+    check(
+      """|object O:
+         |  implicit class Test[T](xs: List[T]):
+         |    def test(x: T): List[T] = ???
+         |  List(1,2,3).test(@@""".stripMargin,
+      """|test(x: Int): List[Int]
+         |     ^^^^^^
+         |""".stripMargin
+    )
+
+  @Test def `instantiated-type-var-old-ext-2` =
+    check(
+      """|object O:
+         |  implicit class Test[T](xs: List[T]):
+         |    def test(x: T): List[T] = ???
+         |  List(1,2,3).test(s@@""".stripMargin,
+      """|test(x: Int): List[Int]
+         |     ^^^^^^
+         |""".stripMargin
+    )
+
+  @Test def `instantiated-type-var-old-ext-3` =
+    check(
+      """|object O:
+         |  implicit class Test[T](xs: List[T]):
+         |    def test(x: T): List[T] = ???
+         |  List(1,2,3).test(@@)""".stripMargin,
+      """|test(x: Int): List[Int]
+         |     ^^^^^^
+         |""".stripMargin
+    )
+
+  @Test def `instantiated-type-var-old-ext-4` =
+    check(
+      """|object O:
+         |  implicit class Test[T](xs: List[T]):
+         |    def test(x: T): List[T] = ???
+         |  List(1,2,3).test(
+         |    @@
+         |  )""".stripMargin,
+      """|test(x: Int): List[Int]
+         |     ^^^^^^
+         |""".stripMargin
+    )
+
+  @Test def `instantiated-type-var-old-ext-5` =
+    check(
+      """|object O:
+         |  implicit class Test[T](xs: List[T]):
+         |    def test(x: T): List[T] = ???
+         |  List(1,2,3).test(
+         |    @@
+         |  println("test")
+         |""".stripMargin,
+      """|test(x: Int | Unit): List[Int | Unit]
+         |     ^^^^^^^^^^^^^
+         |""".stripMargin
+    )
+
+  @Test def `instantiated-type-var-old-ext-6` =
+    check(
+      """|object O:
+         |  implicit class Test[T](xs: List[T]):
+         |    def test(y: String, x: T): List[T] = ???
+         |  List(1,2,3).test("", @@)
+         |""".stripMargin,
+      """|test(y: String, x: Int): List[Int]
+         |                ^^^^^^
+         |""".stripMargin
+    )
diff --git a/tests/neg/i5004.scala b/tests/neg/i5004.scala
@@ -1,6 +1,6 @@
 object i0 {
 1 match {
 def this(): Int  // error
-  def this()  // error
-}
+  def this()
+} // error
 }
diff --git a/tests/neg/i5498-postfixOps.check b/tests/neg/i5498-postfixOps.check
@@ -11,7 +11,7 @@
 -- [E172] Type Error: tests/neg/i5498-postfixOps.scala:6:0 -------------------------------------------------------------
 6 |  Seq(1, 2).filter(List(1,2) contains) // error: usage of postfix operator // error // error (type error) // error (type error)
   |^
-  |No given instance of type scala.concurrent.duration.DurationConversions.Classifier[Null] was found for parameter ev of method second in trait DurationConversions
+  |Ambiguous given instances: both object spanConvert in object DurationConversions and object fromNowConvert in object DurationConversions match type scala.concurrent.duration.DurationConversions.Classifier[C] of parameter ev of method second in trait DurationConversions
 -- [E007] Type Mismatch Error: tests/neg/i5498-postfixOps.scala:6:24 ---------------------------------------------------
 6 |  Seq(1, 2).filter(List(1,2) contains) // error: usage of postfix operator // error // error (type error) // error (type error)
   |                        ^

diff --git a/tests/neg/parser-stability-1.scala b/tests/neg/parser-stability-1.scala
@@ -1,3 +1,4 @@
 object x0 {
 x1 match  // error
 def this // error
+// error
diff --git a/tests/neg/syntax-error-recovery.check b/tests/neg/syntax-error-recovery.check
@@ -94,45 +94,3 @@
    |          Not found: bam
    |
    | longer explanation available when compiling with `-explain`
--- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:7:2 -------------------------------------------
-6 |        2
-7 |  }
-  |         ^
-  |         Discarded non-Unit value of type Null. You may want to use `()`.
-  |
-  | longer explanation available when compiling with `-explain`
--- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:15:2 ------------------------------------------
-14 |        2
-15 |  println(baz) // error
-   |         ^
-   |         Discarded non-Unit value of type Null. You may want to use `()`.
-   |
-   | longer explanation available when compiling with `-explain`
--- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:27:2 ------------------------------------------
-26 |      2
-27 |  println(bam) // error
-   |       ^
-   |       Discarded non-Unit value of type Null. You may want to use `()`.
-   |
-   | longer explanation available when compiling with `-explain`
--- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:36:2 ------------------------------------------
-35 |        2
-36 |  }
-   |         ^
-   |         Discarded non-Unit value of type Null. You may want to use `()`.
-   |
-   | longer explanation available when compiling with `-explain`
--- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:44:2 ------------------------------------------
-43 |        2
-44 |  println(baz) // error
-   |         ^
-   |         Discarded non-Unit value of type Null. You may want to use `()`.
-   |
-   | longer explanation available when compiling with `-explain`
--- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:56:2 ------------------------------------------
-55 |      2
-56 |  println(bam) // error
-   |       ^
-   |       Discarded non-Unit value of type Null. You may want to use `()`.
-   |
-   | longer explanation available when compiling with `-explain`