Skip to content

Commit 98184f1

Browse files
rochalarochala
authored
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.
1 parent d14f5c7 commit 98184f1

File tree

10 files changed

+118
-93
lines changed

10 files changed

+118
-93
lines changed

Diff for: ‎compiler/src/dotty/tools/dotc/parsing/Parsers.scala

+1-1
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,7 @@ object Parsers {
399399
false
400400
}
401401

402-
def errorTermTree(start: Offset): Literal = atSpan(start, in.offset, in.offset) { Literal(Constant(null)) }
402+
def errorTermTree(start: Offset): Tree = atSpan(start, in.offset, in.offset) { unimplementedExpr }
403403

404404
private var inFunReturnType = false
405405
private def fromWithinReturnType[T](body: => T): T = {

Diff for: ‎compiler/src/dotty/tools/dotc/util/Signatures.scala

+29-9
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,17 @@ object Signatures {
9797
case tp @ TypeApply(fun, types) => applyCallInfo(span, types, fun, true)
9898
case _ => (0, 0, Nil)
9999

100+
101+
def isEnclosingApply(tree: tpd.Tree, span: Span)(using Context): Boolean =
102+
tree match
103+
case apply @ Apply(fun, _) => !fun.span.contains(span) && isValid(apply)
104+
case unapply @ UnApply(fun, _, _) =>
105+
!fun.span.contains(span) && !ctx.definitions.isFunctionNType(tree.tpe) // we want to show tuples in unapply
106+
case typeTree @ AppliedTypeTree(fun, _) => !fun.span.contains(span) && isValid(typeTree)
107+
case typeApply @ TypeApply(fun, _) => !fun.span.contains(span) && isValid(typeApply)
108+
case _ => false
109+
110+
100111
/**
101112
* Finds enclosing application from given `path` for `span`.
102113
*
@@ -108,17 +119,26 @@ object Signatures {
108119
* next subsequent application exists, it returns the latter
109120
*/
110121
private def findEnclosingApply(path: List[tpd.Tree], span: Span)(using Context): tpd.Tree =
111-
path.filterNot {
112-
case apply @ Apply(fun, _) => fun.span.contains(span) || isValid(apply)
113-
case unapply @ UnApply(fun, _, _) => fun.span.contains(span) || isValid(unapply)
114-
case typeTree @ AppliedTypeTree(fun, _) => fun.span.contains(span) || isValid(typeTree)
115-
case typeApply @ TypeApply(fun, _) => fun.span.contains(span) || isValid(typeApply)
116-
case _ => true
117-
} match {
122+
import tpd.TreeOps
123+
124+
val filteredPath = path.filter:
125+
case block @ Block(stats, expr) =>
126+
block.existsSubTree(tree => isEnclosingApply(tree, span) && tree.span.contains(span))
127+
case other => isEnclosingApply(other, span)
128+
129+
filteredPath match
118130
case Nil => tpd.EmptyTree
131+
case tpd.Block(stats, expr) :: _ => // potential block containing lifted args
132+
133+
val enclosingFunction = stats.collectFirst:
134+
case defdef: tpd.DefDef if defdef.rhs.span.contains(span) => defdef
135+
136+
val enclosingTree = enclosingFunction.getOrElse(expr)
137+
findEnclosingApply(Interactive.pathTo(enclosingTree, span), span)
138+
119139
case direct :: enclosing :: _ if isClosingSymbol(direct.source(span.end -1)) => enclosing
120140
case direct :: _ => direct
121-
}
141+
122142

123143
private def isClosingSymbol(ch: Char) = ch == ')' || ch == ']'
124144

@@ -303,7 +323,7 @@ object Signatures {
303323
* @param tree tree to validate
304324
*/
305325
private def isValid(tree: tpd.Tree)(using Context): Boolean =
306-
ctx.definitions.isTupleNType(tree.tpe) || ctx.definitions.isFunctionNType(tree.tpe)
326+
!ctx.definitions.isTupleNType(tree.tpe) && !ctx.definitions.isFunctionNType(tree.tpe)
307327

308328
/**
309329
* Get unapply method result type omiting unknown types and another method calls.

Diff for: ‎language-server/test/dotty/tools/languageserver/SignatureHelpTest.scala

+5-4
Original file line numberDiff line numberDiff line change
@@ -516,13 +516,14 @@ class SignatureHelpTest {
516516
.signatureHelp(m2, List(signature), Some(0), 1)
517517
}
518518

519-
@Test def noUnapplyForTuple: Unit = {
519+
@Test def unapplyForTuple: Unit = {
520+
val signature = S("", Nil, List(List(P("", "Int"), P("", "Int"))), None)
520521
code"""object Main {
521522
| (1, 2) match
522523
| case (x${m1}, ${m2}) =>
523524
|}"""
524-
.signatureHelp(m1, Nil, Some(0), 0)
525-
.signatureHelp(m2, Nil, Some(0), 0)
525+
.signatureHelp(m1, List(signature), Some(0), 0)
526+
.signatureHelp(m2, List(signature), Some(0), 1)
526527
}
527528

528529
@Test def unapplyCaseClass: Unit = {
@@ -693,7 +694,7 @@ class SignatureHelpTest {
693694
.signatureHelp(m1, sigs, None, 0)
694695
.signatureHelp(m2, sigs, None, 0)
695696
.signatureHelp(m3, sigs, Some(2), 0)
696-
.signatureHelp(m4, sigs, None, 1)
697+
.signatureHelp(m4, List(sig0, sig1), None, 1)
697698
.signatureHelp(m5, sigs, Some(2), 1)
698699
.signatureHelp(m6, sigs, Some(0), 1)
699700
.signatureHelp(m7, sigs, Some(1), 1)

Diff for: ‎presentation-compiler/src/main/dotty/tools/pc/SignatureHelpProvider.scala

+1-28
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,7 @@ object SignatureHelpProvider:
3737
val pos = driver.sourcePosition(params)
3838
val trees = driver.openedTrees(uri.nn)
3939

40-
val path =
41-
Interactive.pathTo(trees, pos).dropWhile(t => notCurrentApply(t, pos))
40+
val path = Interactive.pathTo(trees, pos)
4241

4342
val (paramN, callableN, alternatives) =
4443
Signatures.signatureHelp(path, pos.span)
@@ -70,32 +69,6 @@ object SignatureHelpProvider:
7069
)
7170
end signatureHelp
7271

73-
private def isValid(tree: tpd.Tree)(using Context): Boolean =
74-
ctx.definitions.isTupleClass(
75-
tree.symbol.owner.companionClass
76-
) || ctx.definitions.isFunctionType(tree.tpe)
77-
78-
private def notCurrentApply(
79-
tree: tpd.Tree,
80-
pos: SourcePosition
81-
)(using Context): Boolean =
82-
tree match
83-
case unapply: tpd.UnApply =>
84-
unapply.fun.span.contains(pos.span) || isValid(unapply)
85-
case typeTree @ AppliedTypeTree(fun, _) =>
86-
fun.span.contains(pos.span) || isValid(typeTree)
87-
case typeApply @ TypeApply(fun, _) =>
88-
fun.span.contains(pos.span) || isValid(typeApply)
89-
case appl: tpd.GenericApply =>
90-
/* find first apply that the cursor is located in arguments and not at function name
91-
* for example in:
92-
* `Option(1).fold(2)(@@_ + 1)`
93-
* we want to find the tree responsible for the entire location, not just `_ + 1`
94-
*/
95-
appl.fun.span.contains(pos.span)
96-
97-
case _ => true
98-
9972
private def withDocumentation(
10073
info: SymbolDocumentation,
10174
signature: Signatures.Signature,

Diff for: ‎presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpPatternSuite.scala

+2-2
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ class SignatureHelpPatternSuite extends BaseSignatureHelpSuite:
1515
| }
1616
|}
1717
|""".stripMargin,
18-
"""|map[B](f: ((Int, Int)) => B): List[B]
19-
| ^^^^^^^^^^^^^^^^^^^^
18+
"""|(Int, Int)
19+
| ^^^
2020
|""".stripMargin
2121
)
2222

Diff for: ‎presentation-compiler/test/dotty/tools/pc/tests/signaturehelp/SignatureHelpSuite.scala

+76-4
Original file line numberDiff line numberDiff line change
@@ -703,7 +703,7 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
703703
|""".stripMargin
704704
)
705705

706-
@Test def `local-method` =
706+
@Test def `local-method` =
707707
check(
708708
"""
709709
|object Main {
@@ -721,7 +721,7 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
721721
|""".stripMargin,
722722
)
723723

724-
@Test def `local-method2` =
724+
@Test def `local-method2` =
725725
check(
726726
"""
727727
|object Main {
@@ -739,7 +739,7 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
739739
|""".stripMargin,
740740
)
741741

742-
@Test def `local-method3` =
742+
@Test def `local-method3` =
743743
check(
744744
"""
745745
|object Main {
@@ -756,4 +756,76 @@ class SignatureHelpSuite extends BaseSignatureHelpSuite:
756756
| ^^^^^^^^^
757757
|apply(a: Int): Int
758758
|""".stripMargin
759-
)
759+
)
760+
761+
@Test def `instantiated-type-var-old-ext-1` =
762+
check(
763+
"""|object O:
764+
| implicit class Test[T](xs: List[T]):
765+
| def test(x: T): List[T] = ???
766+
| List(1,2,3).test(@@""".stripMargin,
767+
"""|test(x: Int): List[Int]
768+
| ^^^^^^
769+
|""".stripMargin
770+
)
771+
772+
@Test def `instantiated-type-var-old-ext-2` =
773+
check(
774+
"""|object O:
775+
| implicit class Test[T](xs: List[T]):
776+
| def test(x: T): List[T] = ???
777+
| List(1,2,3).test(s@@""".stripMargin,
778+
"""|test(x: Int): List[Int]
779+
| ^^^^^^
780+
|""".stripMargin
781+
)
782+
783+
@Test def `instantiated-type-var-old-ext-3` =
784+
check(
785+
"""|object O:
786+
| implicit class Test[T](xs: List[T]):
787+
| def test(x: T): List[T] = ???
788+
| List(1,2,3).test(@@)""".stripMargin,
789+
"""|test(x: Int): List[Int]
790+
| ^^^^^^
791+
|""".stripMargin
792+
)
793+
794+
@Test def `instantiated-type-var-old-ext-4` =
795+
check(
796+
"""|object O:
797+
| implicit class Test[T](xs: List[T]):
798+
| def test(x: T): List[T] = ???
799+
| List(1,2,3).test(
800+
| @@
801+
| )""".stripMargin,
802+
"""|test(x: Int): List[Int]
803+
| ^^^^^^
804+
|""".stripMargin
805+
)
806+
807+
@Test def `instantiated-type-var-old-ext-5` =
808+
check(
809+
"""|object O:
810+
| implicit class Test[T](xs: List[T]):
811+
| def test(x: T): List[T] = ???
812+
| List(1,2,3).test(
813+
| @@
814+
| println("test")
815+
|""".stripMargin,
816+
"""|test(x: Int | Unit): List[Int | Unit]
817+
| ^^^^^^^^^^^^^
818+
|""".stripMargin
819+
)
820+
821+
@Test def `instantiated-type-var-old-ext-6` =
822+
check(
823+
"""|object O:
824+
| implicit class Test[T](xs: List[T]):
825+
| def test(y: String, x: T): List[T] = ???
826+
| List(1,2,3).test("", @@)
827+
|""".stripMargin,
828+
"""|test(y: String, x: Int): List[Int]
829+
| ^^^^^^
830+
|""".stripMargin
831+
)

Diff for: ‎tests/neg/i5004.scala

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
object i0 {
22
1 match {
33
def this(): Int // error
4-
def this() // error
5-
}
4+
def this()
5+
} // error
66
}

Diff for: ‎tests/neg/i5498-postfixOps.check

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
-- [E172] Type Error: tests/neg/i5498-postfixOps.scala:6:0 -------------------------------------------------------------
1212
6 | Seq(1, 2).filter(List(1,2) contains) // error: usage of postfix operator // error // error (type error) // error (type error)
1313
|^
14-
|No given instance of type scala.concurrent.duration.DurationConversions.Classifier[Null] was found for parameter ev of method second in trait DurationConversions
14+
|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
1515
-- [E007] Type Mismatch Error: tests/neg/i5498-postfixOps.scala:6:24 ---------------------------------------------------
1616
6 | Seq(1, 2).filter(List(1,2) contains) // error: usage of postfix operator // error // error (type error) // error (type error)
1717
| ^

Diff for: ‎tests/neg/parser-stability-1.scala

+1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
object x0 {
22
x1 match // error
33
def this // error
4+
// error

Diff for: ‎tests/neg/syntax-error-recovery.check

-42
Original file line numberDiff line numberDiff line change
@@ -94,48 +94,6 @@
9494
| Not found: bam
9595
|
9696
| longer explanation available when compiling with `-explain`
97-
-- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:7:2 -------------------------------------------
98-
6 | 2
99-
7 | }
100-
| ^
101-
| Discarded non-Unit value of type Null. You may want to use `()`.
102-
|
103-
| longer explanation available when compiling with `-explain`
104-
-- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:15:2 ------------------------------------------
105-
14 | 2
106-
15 | println(baz) // error
107-
| ^
108-
| Discarded non-Unit value of type Null. You may want to use `()`.
109-
|
110-
| longer explanation available when compiling with `-explain`
111-
-- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:27:2 ------------------------------------------
112-
26 | 2
113-
27 | println(bam) // error
114-
| ^
115-
| Discarded non-Unit value of type Null. You may want to use `()`.
116-
|
117-
| longer explanation available when compiling with `-explain`
118-
-- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:36:2 ------------------------------------------
119-
35 | 2
120-
36 | }
121-
| ^
122-
| Discarded non-Unit value of type Null. You may want to use `()`.
123-
|
124-
| longer explanation available when compiling with `-explain`
125-
-- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:44:2 ------------------------------------------
126-
43 | 2
127-
44 | println(baz) // error
128-
| ^
129-
| Discarded non-Unit value of type Null. You may want to use `()`.
130-
|
131-
| longer explanation available when compiling with `-explain`
132-
-- [E190] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:56:2 ------------------------------------------
133-
55 | 2
134-
56 | println(bam) // error
135-
| ^
136-
| Discarded non-Unit value of type Null. You may want to use `()`.
137-
|
138-
| longer explanation available when compiling with `-explain`
13997
-- Warning: tests/neg/syntax-error-recovery.scala:61:2 -----------------------------------------------------------------
14098
61 | println(bam)
14199
| ^^^^^^^

0 commit comments

Comments
 (0)