Fix #2566: Ycheck that New node is always enclosed in a Select

[abeln]

Tested:
Verified that the test case in #2564
triggers the assert.
Verified that the check doesn't run when we ycheck a phase before frontend.

[abeln]

@DarkDimius is this the right place to invoke the new check? Do I need to pass a fresh context?

[DarkDimius]

Yes, this is the right place.
TreeChecker is always after typer, no need for the if.

[abeln]

@DarkDimius The regular tests pass, but the legacy and bootstrap suites fail with what looks like unrelated (to this PR) errors. Are those tests flaky?

[nicolasstucki]

I would put the assertion inside of the TreeAccumulator .apply to have it fail with the accumulator stacktrace. This would allow us to attache the debugger at the moment of the assertion failure and navigate back to the trees that contained this malformed New.

[abeln]

That's a good idea. Will do.

[nicolasstucki]

If you do this you might not even need the parameter of Found and could make it a case object

[nicolasstucki]

Why not just partent: tpd.Tree?

[abeln]

When I invoke the accumulator on the root of the tree, there's no parent node. But I guess I could also pass an empty Thicket?

[nicolasstucki]

But on the root you use NotFound. My comment is only for Found.

For NotFound you could maybe simplify it using some trick like the Ticket or defining a case object Root extends State

[abeln]

Well, when we construct the Found object,we use the optional parent from the NotFound, so the optionality trickles through the hierarchy. We can get rid of it everywhere with the Thicket.

[abeln]

Applied your suggestion and the code is now simpler. Thanks.

[abeln]

@DarkDimius @nicolasstucki
The test failures are because sometimes annotations violate the invariant:

e.g. this test https://github.com/lampepfl/dotty/blob/master/tests/pos/functions1.scala
after tailrec ends up with this node

Annotated(Ident(x),New(TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,scala)),unchecked)]))

So should we relax the invariant? Is it ok for Annotated nodes to wrap New? (if so, which constructor gets chosen?)

Xprint:tailrec shows

val y: PartialFunction[String, String] = 
        {
          def $anonfun(x: String): String = 
            x match 
              {
                case "abc" => ""
                case _ => 
                  x
              }
          def isDefinedAt(x: String): Boolean = 
            x @unchecked match 
              {
                case "abc" => true
                case _ => true
                case _ @ _ => false
              }
          {
            class $anonfun() extends Object() with PartialFunction { 
              def apply(x: String): String = $anonfun(x)
              def isDefinedAt(x: String): Boolean = isDefinedAt(x)
            }
            new Object with PartialFunction[String, String]{...}()
          }
        }

[DarkDimius]

Is it ok for Annotated nodes to wrap New?

Annotations aren't systematically transformed in the pipeline and they also can contain some-what invalid trees. I think you shouldn't recurse into annotations at all.

[abeln]

@DarkDimius We no longer check inside annotations. PTAL.

[abeln]

Friendly ping.

[DarkDimius]

Looks good to me!
@abeln, thanks for making Ycheck better!