Skip to content

Commit

Permalink
Ignore orphan parameters inside a retains annotation during Ycheck (#…
Browse files Browse the repository at this point in the history
…19684)

Fixes #19661.

## Cause of the issue

As reported in #19661, the following code triggers an assertation
failure during Ycheck:
```scala
import language.experimental.captureChecking

trait MySet[A]:
  def collect[B](pf: PartialFunction[A, B]^): MySet[B]^{this, pf}

class Test:
  def f(xs: MySet[Int]) = xs collect { case x => x }
  def g(xs: MySet[Int]): MySet[Int] = f(xs)
```

The failure happens when checking the tree `f(xs)`, whose type is
`MySet[Int]^{this, PartialFunction[Int, Int]}`. The `checkNoOrphans`
function is invoked on `this`, whose type turns out to be an orphan
parameter reference (`xs`).

We first inspect the tree outputed by `typer`:
```scala
class Test() extends Object() {
  def f(xs: MySet[Int]): MySet[Int]^{this, PartialFunction[Int, Int]} =
    xs.collect[Int](
      {
        def $anonfun(x$1: Int): Int =
          x$1 match
            {
              case x @ _ =>
                x:Int
            }
        closure($anonfun:PartialFunction[Int, Int])
      }
    )
  def g(xs: MySet[Int]): MySet[Int] = this.f(xs)
}
```

The problem roots in the signature of the method `f`: in the capture set
of its result type, the `this` reference is dangling.

How come? It turns out that the `asSeenFrom` map is not working
correctly for the typing of `xs.collect`:
```
(xs.collect : [B](pf: PartialFunction[Int, B]^): MySet[B]^{this, pf})
```
Instead of replacing `this` with `xs`, `asSeenFrom` keeps `this`
untouched. This is what happened:
- When mapping `asSeenFrom` on the method type, the `asSeenFrom` map
recurses and applies on the annotated type.
- When mapping the annotation (`@retains(this, pf)`), the `asSeenFrom`
map derives a `TreeTypeMap` from itself and uses it to map the `tree` of
the annotation.
- During that, the type of `this` is properly mapped to `xs.type` but
the tree `this` is never changed (since the `TreeTypeMap` is an identity
on the structure of trees).

To solve this issue, there are (at least) two possibilities:
- Refactor the `TypeMap` machineries on annotations to enable it to
properly handle these cases. But it is hard: when mapping the capture
annotation, we are at a pre-CC phase, so tools for manipulating capture
sets are not available. And it is unnecessary: even if we compute these
references properly, it gets discarded during CC.
- During Ycheck, ignore orphan parameter references inside a normal
`@retains` annotation (as opposed to an optimised `CaptureAnnotation`).
This feels like a dangerous fix but these `@retains` annotations, even
if they are ill-formed, is already treated as being unreliable in CC and
get rechecked. Also, CC turns these concrete annotations into optimised
`CaptureAnnotation`s, which are not ignored by Ycheck.

## Fix

So this PR implements the second option:
- Ignore orphan parameter errors inside a normal `@retains` annotation
during Ycheck.
- The check for `CaptureAnnotation`s will not be bypassed.
  • Loading branch information
odersky authored Feb 15, 2024
2 parents fc593df + 0f81c2f commit b9f2ef0
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
20 changes: 19 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/TreeChecker.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import collection.mutable
import ProtoTypes.*
import staging.StagingLevel
import inlines.Inlines.inInlineMethod
import cc.{isRetainsLike, CaptureAnnotation}

import dotty.tools.backend.jvm.DottyBackendInterface.symExtensions

Expand Down Expand Up @@ -162,17 +163,34 @@ object TreeChecker {
*/
def checkNoOrphans(tp0: Type, tree: untpd.Tree = untpd.EmptyTree)(using Context): Type = new TypeMap() {
val definedBinders = new java.util.IdentityHashMap[Type, Any]
private var inRetainingAnnot = false

def insideRetainingAnnot[T](op: => T): T =
val saved = inRetainingAnnot
inRetainingAnnot = true
try op finally inRetainingAnnot = saved

def apply(tp: Type): Type = {
tp match {
case tp: BindingType =>
definedBinders.put(tp, tp)
mapOver(tp)
definedBinders.remove(tp)
case tp: ParamRef =>
assert(definedBinders.get(tp.binder) != null, s"orphan param: ${tp.show}, hash of binder = ${System.identityHashCode(tp.binder)}, tree = ${tree.show}, type = $tp0")
val isValidRef =
definedBinders.get(tp.binder) != null
|| inRetainingAnnot
// Inside a normal @retains annotation, the captured references could be ill-formed. See issue #19661.
// But this is ok since capture checking does not rely on them.
assert(isValidRef, s"orphan param: ${tp.show}, hash of binder = ${System.identityHashCode(tp.binder)}, tree = ${tree.show}, type = $tp0")
case tp: TypeVar =>
assert(tp.isInstantiated, s"Uninstantiated type variable: ${tp.show}, tree = ${tree.show}")
apply(tp.underlying)
case tp @ AnnotatedType(underlying, annot) if annot.symbol.isRetainsLike && !annot.isInstanceOf[CaptureAnnotation] =>
val underlying1 = this(underlying)
val annot1 = insideRetainingAnnot:
annot.mapWith(this)
derivedAnnotatedType(tp, underlying1, annot1)
case _ =>
mapOver(tp)
}
Expand Down
8 changes: 8 additions & 0 deletions tests/pos-custom-args/captures/i19661.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import language.experimental.captureChecking

trait MySet[A]:
def collect[B](pf: PartialFunction[A, B]^): MySet[B]^{this, pf}

class Test:
def f(xs: MySet[Int]) = xs collect { case x => x }
def g(xs: MySet[Int]): MySet[Int] = f(xs)

0 comments on commit b9f2ef0

Please sign in to comment.