Fix #4373: reject wildcard types in syntactically invalid positions #4422

Blaisorblade · 2018-04-30T18:34:15Z

The testcase is from examples in #4373, variations and tests for types with wildcards. I'm unsure for a couple cases.

The strategy I use comes from generalizing existing code.

A separate commit avoids cascade errors. That was the trickiest part of this PR, but pretty instructive. I now sometimes use Ident(nme.ERROR) instead of scalaAny to replace illegal wildcards in extends clauses.

Action items:

rebase
merge Create {And,Or}TypeTree in Parsers, not desugar #4424 since it was approved
make checkWildcard private to Parsers again
go through A variety of bugs (42 or so). #4389 and try all reported issues in Bugs.txt that contain a _ somewhere.
test and fix with too (noticed from A variety of bugs (42 or so). #4389)
add testcases with annotations

smarter · 2018-04-30T18:41:01Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -2279,7 +2279,8 @@ object Parsers {
    /** ConstrApp         ::=  SimpleType {ParArgumentExprs}
     */
    val constrApp = () => {
-      val t = checkWildcard(annotType())
+      // Using Ident(nme.ERROR) to avoid causing cascade errors on non-user-written code
+      val t = checkWildcard(annotType(), fallbackTree = Ident(nme.ERROR))


In extends clauses you could use AnyRef instead of Any

That didn't work, because I end up with error messages like this, even though https://github.com/scala/scala/blob/2.13.x/src/library-aux/scala/AnyRef.scala#L15 talks about a trait AnyRef:

scala> class A extends AnyRef with AnyRef 1 |class A extends AnyRef with AnyRef | ^^^^^^ | class Object is not a trait

smarter · 2018-04-30T18:49:48Z

tests/neg/i4373.scala

+trait Base
+trait TypeConstr[X]
+
+class X1[A >: _ | X1[_]] // error


Maybe add some examples involving annotations too? @_, _ @x, ...

smarter

Otherwise LGTM

smarter · 2018-04-30T18:53:38Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -1114,8 +1117,8 @@ object desugar {
        Apply(Select(Apply(Ident(nme.StringContext), strs), id), elems)
      case InfixOp(l, op, r) =>
        if (ctx.mode is Mode.Type)
-          if (!op.isBackquoted && op.name == tpnme.raw.AMP) AndTypeTree(l, r)     // l & r
-          else if (!op.isBackquoted && op.name == tpnme.raw.BAR) OrTypeTree(l, r) // l | r
+          if (!op.isBackquoted && op.name == tpnme.raw.AMP) AndTypeTree(checkWildcard(l), checkWildcard(r))     // l & r


I wonder if we shouldn't do this in Parser directly. This desugaring is kind of weird anyway, since it's not really about sugar but about how to parse something: we could just emit AndTypeTree or OrTypeTree as appropriate directly in Parser

The reason to do it this way is that op is an identifier with a position. Converted to And/OrTypeTree, that information is lost.

👍, trying this out.

It took me a while to give up searching OrTypeTree in the Parser, and honestly I was very confused that wasn't already the case.

The reason to do it this way is that op is an identifier with a position. Converted to And/OrTypeTree, that information is lost.

But is that information useful for anything?

FWIW, changing this passes CI, see #4424.

allanrenucci · 2018-04-30T18:58:26Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

+   *  If it is, returns the [[Position]] where the wildcard occurs.
+   */
+  @tailrec
+  final def findWildcardType(t: Tree): Option[Position] = t match {


You could avoid using an Option here by returning NoPosition. You can then call exists on Position.

Makes sense, but is that a good idea even if the existing code used Option[Position]?

The existing code is not always right :)

I guess it was a defensive move to accommodate the fact that the returned position could be NoPosition. So, yes, I'd leave it as is.

~~Actually, self-answer: probably yes, that's the point of NoPosition, plus it avoids allocations.~~
EDIT: had missed Martin's answer.

odersky · 2018-05-01T08:14:39Z

I think it would be good to go through #4389 and try all reported issues in Bugs.txt that contain a _ somewhere. I fixed all the other ones there but left wildcard bugs alone, awaiting the fixes.

smarter · 2018-05-02T21:53:52Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -1035,6 +1035,19 @@ object Parsers {
      else
        rejectWildcard(t, fallbackTree)

+    def checkAndOrArgument(t: Tree): Tree =
+      findWildcardType(t, true) match {


At this point, findWildcardType is a misnomer.

Blaisorblade · 2018-05-02T23:36:57Z

FWIW, there's a bunch of further errors in #4389 which I'm making progress on — it seems some testcases still fail even when the minimizations were fixed.

Blaisorblade · 2018-05-03T15:50:09Z

Ready for re-review, I've addressed all open action items.

I've looked through bugs.txt and I think I'm done here with it, the leftovers with wildcards are different issues, namely #4382 (again) and #4453. Non-minimized bugs are another matter.

// ==> 09d8cdb2f7be2b81c318d338626268b7ec28ab12.scala <==
trait x0[x0[_]] {
  (??? :x0[_]):x0[Int]
}

Essentially reviewed in scala#4424.

The new API is required for all use cases in this PR.

Fix scala#4373. The testcase is from examples in scala#4373, variations and tests for types with wildcards.

Without this commit, the testcase produces additional spurious errors: ```scala -- [E104] Syntax Error: tests/neg/i4373.scala:10:0 ----------------------------- 10 |class A2 extends _ with _ // error // error |^ |class Any is not a trait longer explanation available when compiling with `-explain` -- [E104] Syntax Error: tests/neg/i4373.scala:11:21 ---------------------------- 11 |class A3 extends Base with _ // error | ^ | class Any is not a trait longer explanation available when compiling with `-explain` -- Error: tests/neg/i4373.scala:12:24 ------------------------------------------ 12 |class A4 extends _ with Base // error | ^^^^ |illegal trait inheritance: superclass Any does not derive from trait Base's superclass Object ``` These errors make absolutely no sense for a user, since they refer to code the user didn't write, but that was generated by the compiler's error correction mechanism. I haven't checked if we could use `Ident(nme.ERROR)` everywhere. I have also tried using `EmptyTree` and `EmptyTree.withPos(wildcardPos)` and `EmptyTree.clone.withPos(wildcardPos)` as fallback trees, but all of them failed in some way.

This is mostly a refactoring, though `findWildcardType(t, alsoNonValue = true)` is new behavior that belongs later. `rejectWildcard` had leftover tortous abstration, which is removed here.

Those types lead to assertion failures later while building OrTypeTree. Reject them, also for AndTypeTree.

smarter reviewed Apr 30, 2018

View reviewed changes

odersky mentioned this pull request Apr 30, 2018

Fix #4382: Disallow wildcard arguments for higher-kinded types #4423

Merged

smarter reviewed Apr 30, 2018

View reviewed changes

smarter approved these changes Apr 30, 2018

View reviewed changes

smarter reviewed Apr 30, 2018

View reviewed changes

allanrenucci reviewed Apr 30, 2018

View reviewed changes

Blaisorblade mentioned this pull request Apr 30, 2018

Create {And,Or}TypeTree in Parsers, not desugar #4424

Closed

1 task

Blaisorblade self-assigned this May 1, 2018

Blaisorblade added the stat:wip label May 1, 2018

Blaisorblade force-pushed the fix-4373 branch 3 times, most recently from 765369d to 24d6c9c Compare May 2, 2018 21:43

smarter reviewed May 2, 2018

View reviewed changes

Blaisorblade mentioned this pull request May 3, 2018

ClassCastException: Types$LambdaParam cannot be cast to Symbol #4452

Closed

Blaisorblade added stat:needs review and removed stat:wip labels May 3, 2018

Blaisorblade removed their assignment May 3, 2018

Blaisorblade added 10 commits May 7, 2018 14:12

Produce AndTypeTree and OrTypeTree directly in Parsers, not Desugar

5b3d81b

Essentially reviewed in scala#4424.

Refactor parser code that rejects forbidden wildcards

9177c15

The new API is required for all use cases in this PR.

Fix scala#4373: reject wildcard types in syntactically invalid positions

3183c50

Fix scala#4373. The testcase is from examples in scala#4373, variations and tests for types with wildcards.

Add and fix missing testcases with with

f4ea23c

Handle by-name types better

95d0901

Refactor, simplify and extend wildcard checking

c17ed3e

This is mostly a refactoring, though `findWildcardType(t, alsoNonValue = true)` is new behavior that belongs later. `rejectWildcard` had leftover tortous abstration, which is removed here.

Reject by-name types as arguments of AndTypeTree and OrTypeTree

3ddd88a

Those types lead to assertion failures later while building OrTypeTree. Reject them, also for AndTypeTree.

Add some testcases

7f3a0cf

Review feedback: findWildcardType -> findNonValueTypeTree

35f09c4

Exhaustive testing of permutations with annotations

0804fde

Blaisorblade force-pushed the fix-4373 branch from 3ee21d5 to 0804fde Compare May 7, 2018 12:12

allanrenucci merged commit 25dd1de into scala:master May 7, 2018

allanrenucci deleted the fix-4373 branch May 7, 2018 14:26

smarter mentioned this pull request Jun 8, 2018

Don't accept wildcard type bounds. scala/scala#6335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4373: reject wildcard types in syntactically invalid positions #4422

Fix #4373: reject wildcard types in syntactically invalid positions #4422

Blaisorblade commented Apr 30, 2018 •

edited

Loading

smarter Apr 30, 2018 •

edited

Loading

Blaisorblade Apr 30, 2018

smarter Apr 30, 2018

Blaisorblade May 3, 2018

smarter left a comment

smarter Apr 30, 2018

odersky Apr 30, 2018

Blaisorblade Apr 30, 2018

smarter Apr 30, 2018

Blaisorblade Apr 30, 2018

allanrenucci Apr 30, 2018

Blaisorblade Apr 30, 2018

smarter Apr 30, 2018

odersky Apr 30, 2018

Blaisorblade Apr 30, 2018 •

edited

Loading

odersky commented May 1, 2018 •

edited

Loading

smarter May 2, 2018

Blaisorblade May 3, 2018

Blaisorblade commented May 2, 2018

Blaisorblade commented May 3, 2018

Fix #4373: reject wildcard types in syntactically invalid positions #4422

Fix #4373: reject wildcard types in syntactically invalid positions #4422

Conversation

Blaisorblade commented Apr 30, 2018 • edited Loading

smarter Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blaisorblade Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

odersky commented May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blaisorblade commented May 2, 2018

Blaisorblade commented May 3, 2018

Blaisorblade commented Apr 30, 2018 •

edited

Loading

smarter Apr 30, 2018 •

edited

Loading

Blaisorblade Apr 30, 2018 •

edited

Loading

odersky commented May 1, 2018 •

edited

Loading