Fix #10888: Avoid A.this.B for Java inner classes #11006
Conversation
compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
Outdated
Show resolved
Hide resolved
compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
Outdated
Show resolved
Hide resolved
liufengyun
force-pushed
the
fix-10888
branch
3 times, most recently
from
c4508d5 to
a6eb9f8
Compare
|
@smarter Could you have a look at the last commit? It took a while to find out the root cause. |
| // not a `JavaArrayType`. | ||
|
|
||
| val annot = atPhase(erasurePhase.next) { cls.getAnnotation(annotCls) } | ||
| val arg = annot.get.argument(0).get |
There was a problem hiding this comment.
It's important to run this line at erasure, otherwise we're not forcing the annotation tree at the erasure phase. But also, why is this test suddenly failing after the change you made? They don't seem related at all, and based on the comment you added I would expect it to fail before too. If you're not sure what's going on let me know and I can try to have a look.
There was a problem hiding this comment.
I don't know why it's not failing before. But the subtyping trace and debugging tells that Array[String] =:= Array[String] is false after erasure due to the cause in the commit message. Just running the subtyping in an early phase is enough.
There was a problem hiding this comment.
@smarter: I've just updated the code so that we only do subtyping test before erasure. If it's green, then please have a look if you have clue why it's not failing before.
There was a problem hiding this comment.
From the investigation so far, the changes in ClassfileParser have changed subtyping path for Array[String] <:< Array[String].
Previously, it does not touch the line TypeComparer.compareAppliedType2, now it reaches that line.
There was a problem hiding this comment.
After some further investigation, we know why it accidentally works before: before the change the two Array[String] and Array[String] point to the same object, thus they go through the short-cut tp1 eq tp2 in TypeComparer, thus never reaches the line TypeComparer.compareAppliedType2.
The changes in ClassfileParser somehow changes the caching behavior.
liufengyun
force-pushed
the
fix-10888
branch
from
e7c84d4 to
9021afb
Compare
For Java inner classes, we should use `A#B` instead of `A.this.B`. Co-authored-by: Guillaume Martres <[email protected]>
We have the following subtyping result:
```scala
Subtype trace:
==> Array#262[String#810] <:< Array#262[String#810]
==> Array#262[String#810] <:< Array#262[String#810] recur
<== Array#262[String#810] <:< Array#262[String#810] recur = false
<== Array#262[String#810] <:< Array#262[String#810] = false
```
In `TypeComparer.compareAppliedType2`, the method just returns false
because `typcon2.typeParams` is empty:
```
def compareAppliedType2(tp2: AppliedType, tycon2: Type, args2: List[Type]): Boolean = {
val tparams = tycon2.typeParams
if (tparams.isEmpty) return false
```
The reason why `ftypcon1.typeParams` is empty is because we are
running the subtyping check after erasure. The following code works
without any problem:
```scala
atPhase(typerPhase) { arrayOfString =:= arg.tpe }
```
Before the change the two `Array[String]` and `Array[String]` point to
the same object, thus they go through the short-cut `tp1 eq tp2` in
`TypeComparer`, thus never reaches the line
`TypeComparer.compareAppliedType2`.
liufengyun
force-pushed
the
fix-10888
branch
from
9021afb to
4ae3d64
Compare
|
Oh, looks like there's a failed test: -- [E007] Type Mismatch Error: tests/pos/i5655/S_2.scala:4:41 ----------------------------------------------------------
4 | val ab: J_1#A[String]#B[String] = j.m()
Compilation failed for: 'tests/pos/i5655'
| ^^^^^
| Found: J_1#A[?]#B[String]
| Required: J_1#A[String]#B[String]Corresponding PR is #5656 |
The commits 5cf9e0b incorrectly throw the prefix away and construct the type from the symbol by ignoring type parameters of generic classes. As prefix is already in the "inner normal form", it suffices to select member on it. The commit 22ab968 calls `processInner`, which is effectively an no-op. Also found that we should reset `skiptvs` in reading method parameter and result types.
Fix #10888: Avoid A.this.B for Java inner classes
For Java inner classes, we should use
A#Binstead ofA.this.B.Co-authored-by: Guillaume Martres [email protected]