Fix #10769: change synthesized type in def ordinal

[liufengyun]

Fix #10769: change synthesized type in def ordinal

As pointed out by @smarter in #10769, for the following code:

package stm

trait STMLike[F[_]] {
  import Internals._

  sealed abstract class Txn[+A] {}

  object Txn {
def abort[A](e: Throwable): Txn[A] = Abort(e)
  }

  object Internals {
case class Abort(error: Throwable) extends Txn[Nothing]
  }

}

The compiler synthesized the following code for the object Txn:

object Txn {
  type MirroredMonoType = STMLike.this.Txn[?]
  def ordinal(x: Txn.MirroredMonoType): Int =
x match {
      case _:stm.STMLike.Internals.Abort => 0
}

  def abort[A](e: Throwable): Txn[A] = Abort(e)
}

In the method ordinal, the type for Internals.Abort is

TypeRef(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class stm)),trait STMLike)),module class Internals$)),class Abort)

This type is incorrect, as we are not in the object Internals. The
explicit outer can only handle such types if it's static. In this
case, the object is not static, thus it crashes the explicit outer.

Co-authored-by: Guillaume Martres [email protected]

[smarter]

• We need reachableTermRef, reachableTypeRef and reachableRawTypeRef, all of those can be useful (and for ordinal we need reachableTermRef too since we can have a child object in an inner object).
• Instead of using a TypeMap, I would suggest trying to define them directly in a recursive manner like termRef and typeRef
• Instead of special-casing objects, I would suggest also replacing references to inner classes A.this.B with A#B, this could be used to fix exception caught when loading class C: Cyclic reference involving class C #10888 (comment) nicely and get rid of processInner in ClassfileParser
• I think this is useful enough to be defined directly in SymDenotations, but that's not super important
• This is likely useful enough to be exposed in tasty reflect, but that can be done later.

[liufengyun]

• Instead of special-casing objects, I would suggest also replacing references to inner classes A.this.B with A#B, this could be used to fix #10888 (comment) nicely and get rid of processInner in ClassfileParser

I'm not sure about this change, as it changes the meaning of the types. It might work for Java, but generally does not work for Dotty.

Updated: #11006 is proposed to fix the issue above separately.

[smarter]

Instead of special-casing objects, I would suggest also replacing references to inner classes A.this.B with A#B, this could be used to fix #10888 (comment) nicely and get rid of processInner in ClassfileParser

I'm not sure about this change, as it changes the meaning of the types. It might work for Java, but generally does not work for Dotty.

The type A.this.B is not meaningful if we're not inside A or a subclass of A. Imagine that we change the test case to use an inner class instead of an inner object:

package stm

trait STMLike[F[_]] {
  import Internals._

  sealed abstract class Txn[+A] {}

  object Txn {
    def abort[A](e: Throwable): Txn[A] = Abort(e)
  }

  class Internals {
    case class Abort(error: Throwable) extends Txn[Nothing]
  }

}

In the generated ordinal method for Txn, we want to have a case _: Internals#Abort, because case _: Internals.this.Abort isn't valid.

[liufengyun]

The type A.this.B is not meaningful if we're not inside A or a subclass of A.

I agree. However, given that we don't know the context where the method is called, it's dangerous. We could make the change A.this.B -> A#B more than necessary, which may potentially change the semantics of typed patterns that requires outer testing.

On the other hand, in the current usage of .reachableTermRef and .reachableTypeRef, there is already an explicit precondition in whyNotGenericSum:

   /**
    *   - all of its children are addressable through a path from its companion object
    */
    def whyNotGenericSum(using Context): String = ...

With such a precondition, the usage of reachableTermRef and reachableTypeRef will be safe (See added test).

Generally, I think similar care should be taken in doing complex things with symbol, as it is really error-prone. The suggested changes don't help avoid such errors, and it may potentially cause other problems, thus I'd suggest stay with the current solution.

(Interestingly, all such use cases I know of are related to children of sealed classes.)

[smarter]

there is already an explicit precondition in whyNotGenericSum

Ah thanks, I had missed that!

[smarter]

We could make the change A.this.B -> A#B more than necessary, which may potentially change the semantics of typed patterns that requires outer testing.

Right, to be correct we would have to check the current ctx.owner to decide whether we should do this change, but if we don't have usecases for that (though it could be useful for tasty-reflection?), we don't have to do it now.

[smarter]

What abotu the ThisType.raw here?

[liufengyun]

This one is different, as we need to call owner. reachableThisType.

[smarter]

These documentation comments need to be updated since we also do this for static objects now.

[liufengyun]

Done.

[smarter]

LGTM, thanks!