From 5cf9e0b0e581dc2611d3bd40c723522e04d583e5 Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Wed, 6 Jan 2021 14:42:36 +0100 Subject: [PATCH 1/3] Fix #10888: Avoid A.this.B for Java inner classes For Java inner classes, we should use `A#B` instead of `A.this.B`. Co-authored-by: Guillaume Martres --- .../dotc/core/classfile/ClassfileParser.scala | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index aeaf6f11543b..725eb45a4f61 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -349,12 +349,15 @@ class ClassfileParser( val tag = sig(index); index += 1 (tag: @switch) match { case 'L' => - def processInner(tp: Type): Type = tp match { - case tp: TypeRef if !tp.symbol.owner.is(Flags.ModuleClass) => - TypeRef(processInner(tp.prefix.widen), tp.symbol.asType) - case _ => - tp - } + /** A type representation where inner classes become `A#B` instead of `A.this.B` (like with `typeRef`) */ + def innerType(symbol: Symbol): Type = + if symbol.is(Flags.Package) then + symbol.thisType + else if symbol.isType then + TypeRef(innerType(symbol.owner), symbol) + else + throw new RuntimeException("unexpected term symbol " + symbol) + def processClassType(tp: Type): Type = tp match { case tp: TypeRef => if (sig(index) == '<') { @@ -388,13 +391,13 @@ class ClassfileParser( } val classSym = classNameToSymbol(subName(c => c == ';' || c == '<')) - val classTpe = if (classSym eq defn.ObjectClass) defn.FromJavaObjectType else classSym.typeRef - var tpe = processClassType(processInner(classTpe)) + val classTpe = if (classSym eq defn.ObjectClass) defn.FromJavaObjectType else innerType(classSym) + var tpe = processClassType(classTpe) while (sig(index) == '.') { accept('.') val name = subName(c => c == ';' || c == '<' || c == '.').toTypeName val clazz = tpe.member(name).symbol - tpe = processClassType(processInner(TypeRef(tpe, clazz))) + tpe = processClassType(innerType(clazz)) } accept(';') tpe From 4ae3d64791e23691ec0b76abaf1b615484e68f8f Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Mon, 8 Feb 2021 14:30:16 +0100 Subject: [PATCH 2/3] Fix test 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`. --- compiler/test/dotty/tools/AnnotationsTests.scala | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/compiler/test/dotty/tools/AnnotationsTests.scala b/compiler/test/dotty/tools/AnnotationsTests.scala index a197ca4bc94e..e7fc1242517c 100644 --- a/compiler/test/dotty/tools/AnnotationsTests.scala +++ b/compiler/test/dotty/tools/AnnotationsTests.scala @@ -29,13 +29,20 @@ class AnnotationsTest: val arrayOfString = defn.ArrayType.appliedTo(List(defn.StringType)) atPhase(erasurePhase.next) { - val annot = cls.getAnnotation(annotCls) // Even though we're forcing the annotation after erasure, // the typed trees should be unerased, so the type of // the annotation argument should be `arrayOfString` and // not a `JavaArrayType`. + val annot = cls.getAnnotation(annotCls) val arg = annot.get.argument(0).get - assert(arg.tpe.isInstanceOf[AppliedType] && arg.tpe =:= arrayOfString, + + // If we run the type check after erasure, we will have + // `Array[String] =:= Array[String]` being false. + // The reason is that in `TypeComparer.compareAppliedType2` we have + // `tycon2.typeParams == Nil` after erasure, thus always get false. + val res = atPhase(typerPhase) { arrayOfString =:= arg.tpe } + + assert(arg.tpe.isInstanceOf[AppliedType] && res, s"Argument $arg had type:\n${arg.tpe}\nbut expected type:\n$arrayOfString") } } From b3ba79fce943fec40357374b9c8370977cb3887e Mon Sep 17 00:00:00 2001 From: Liu Fengyun Date: Mon, 15 Feb 2021 18:37:25 +0100 Subject: [PATCH 3/3] Fix tests/pos/i5655 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. --- .../dotc/core/classfile/ClassfileParser.scala | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index 725eb45a4f61..8c5fb61c9da7 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -349,7 +349,10 @@ class ClassfileParser( val tag = sig(index); index += 1 (tag: @switch) match { case 'L' => - /** A type representation where inner classes become `A#B` instead of `A.this.B` (like with `typeRef`) */ + /** A type representation where inner classes become `A#B` instead of `A.this.B` (like with `typeRef`) + * + * Note: the symbol must not be nested in a generic class. + */ def innerType(symbol: Symbol): Type = if symbol.is(Flags.Package) then symbol.thisType @@ -358,7 +361,7 @@ class ClassfileParser( else throw new RuntimeException("unexpected term symbol " + symbol) - def processClassType(tp: Type): Type = tp match { + def processTypeArgs(tp: Type): Type = tp match { case tp: TypeRef => if (sig(index) == '<') { accept('<') @@ -392,12 +395,12 @@ class ClassfileParser( val classSym = classNameToSymbol(subName(c => c == ';' || c == '<')) val classTpe = if (classSym eq defn.ObjectClass) defn.FromJavaObjectType else innerType(classSym) - var tpe = processClassType(classTpe) + var tpe = processTypeArgs(classTpe) while (sig(index) == '.') { accept('.') val name = subName(c => c == ';' || c == '<' || c == '.').toTypeName - val clazz = tpe.member(name).symbol - tpe = processClassType(innerType(clazz)) + val tp = tpe.select(name) + tpe = processTypeArgs(tp) } accept(';') tpe @@ -435,15 +438,15 @@ class ClassfileParser( paramtypes += { if isRepeatedParam(index) then index += 1 - val elemType = sig2type(tparams, skiptvs) + val elemType = sig2type(tparams, skiptvs = false) // `ElimRepeated` is responsible for correctly erasing this. defn.RepeatedParamType.appliedTo(elemType) else - sig2type(tparams, skiptvs) + sig2type(tparams, skiptvs = false) } index += 1 - val restype = sig2type(tparams, skiptvs) + val restype = sig2type(tparams, skiptvs = false) JavaMethodType(paramnames.toList, paramtypes.toList, restype) case 'T' => val n = subName(';'.==).toTypeName