From 857ed965f9ad2734605f01ca17ab977a932fda3b Mon Sep 17 00:00:00 2001 From: odersky Date: Mon, 18 Apr 2022 21:45:05 +0200 Subject: [PATCH 1/5] Print double definitions signatures at Typer Fixes #14966 --- compiler/src/dotty/tools/dotc/reporting/messages.scala | 10 ++++++---- tests/neg/i14966.check | 10 ++++++++++ tests/neg/i14966.scala | 2 ++ 3 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 tests/neg/i14966.check create mode 100644 tests/neg/i14966.scala diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index 18e2d234452d..36246faaea22 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -2174,10 +2174,12 @@ import transform.SymUtils._ else "Name clash between inherited members" - em"""$clashDescription: - |${previousDecl.showDcl} ${symLocation(previousDecl)} and - |${decl.showDcl} ${symLocation(decl)} - |""" + details + atPhase(typerPhase) { + em"""$clashDescription: + |${previousDecl.showDcl} ${symLocation(previousDecl)} and + |${decl.showDcl} ${symLocation(decl)} + |""" + } + details } def explain = "" } diff --git a/tests/neg/i14966.check b/tests/neg/i14966.check new file mode 100644 index 000000000000..2706d54d64c8 --- /dev/null +++ b/tests/neg/i14966.check @@ -0,0 +1,10 @@ +-- [E120] Naming Error: tests/neg/i14966.scala:2:11 -------------------------------------------------------------------- +2 | export s.* // error + | ^ + | Double definition: + | final def concat[B >: T](suffix: IterableOnce[B]): Set[B] in class B at line 2 and + | final def concat(that: IterableOnce[T]): Set[T] in class B at line 2 + | have the same type after erasure. + | + | Consider adding a @targetName annotation to one of the conflicting definitions + | for disambiguation. diff --git a/tests/neg/i14966.scala b/tests/neg/i14966.scala new file mode 100644 index 000000000000..f8869e9bbac0 --- /dev/null +++ b/tests/neg/i14966.scala @@ -0,0 +1,2 @@ +class B[T](val s: Set[T]): + export s.* // error From 3c6425b9e66f0409f2d41acf3852c8f1165c203e Mon Sep 17 00:00:00 2001 From: odersky Date: Tue, 19 Apr 2022 09:35:48 +0200 Subject: [PATCH 2/5] Fix check file Is this a stability problem? It seems the CI visits Seq definitions in different order than local. --- tests/neg/i14966.check | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/neg/i14966.check b/tests/neg/i14966.check index 2706d54d64c8..6cc8132f17bb 100644 --- a/tests/neg/i14966.check +++ b/tests/neg/i14966.check @@ -2,8 +2,8 @@ 2 | export s.* // error | ^ | Double definition: - | final def concat[B >: T](suffix: IterableOnce[B]): Set[B] in class B at line 2 and - | final def concat(that: IterableOnce[T]): Set[T] in class B at line 2 + | final def ++[B >: T](suffix: IterableOnce[B]): Set[B] in class B at line 2 and + | final def ++(that: IterableOnce[T]): Set[T] in class B at line 2 | have the same type after erasure. | | Consider adding a @targetName annotation to one of the conflicting definitions From 8d1c43578b95e4e608eedd33ec221f140dfd936c Mon Sep 17 00:00:00 2001 From: odersky Date: Wed, 20 Apr 2022 18:41:52 +0200 Subject: [PATCH 3/5] Avoid export clashes Avoid export clashes in most cases by picking the alternative which would be preferred by overloading resolution. If none is, issue a more intelligible error message than before. --- .../src/dotty/tools/dotc/typer/Namer.scala | 59 ++++++++++++++++++- tests/neg/i14966.check | 20 +++---- tests/neg/i14966.scala | 13 +++- tests/pos/i14966.scala | 13 ++++ tests/pos/i14966a.scala | 2 + 5 files changed, 94 insertions(+), 13 deletions(-) create mode 100644 tests/pos/i14966.scala create mode 100644 tests/pos/i14966a.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index aac0288aa771..8a147d388ad0 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -24,6 +24,7 @@ import Inferencing._ import transform.ValueClasses._ import transform.TypeUtils._ import transform.SymUtils._ +import TypeErasure.erasure import reporting._ import config.Feature.sourceVersion import config.SourceVersion._ @@ -1237,8 +1238,64 @@ class Namer { typer: Typer => addForwarders(sels1, sel.name :: seen) case _ => + /** Avoid a clash of export forwarder `forwarder` with other forwarders in `forwarders`. + * @return If `forwarder` clashes, a new leading forwarder and trailing forwarders list + * that avoids the clash according to the scheme described in `avoidClashes`. + * If there's no clash, the inputs as they are in a pair. + */ + def avoidClashWith(forwarder: tpd.DefDef, forwarders: List[tpd.MemberDef]): (tpd.DefDef, List[tpd.MemberDef]) = + def clashes(fwd1: Symbol, fwd2: Symbol) = + fwd1.targetName == fwd2.targetName + && erasure(fwd1.info).signature == erasure(fwd2.info).signature + + forwarders match + case forwarders @ ((forwarder1: tpd.DefDef) :: forwarders1) + if forwarder.name == forwarder1.name => + if clashes(forwarder.symbol, forwarder1.symbol) then + val alt1 = tpd.methPart(forwarder.rhs).tpe + val alt2 = tpd.methPart(forwarder1.rhs).tpe + val cmp = alt1 match + case alt1: TermRef => alt2 match + case alt2: TermRef => compare(alt1, alt2) + case _ => 0 + case _ => 0 + if cmp == 0 then + report.error( + ex"""Clashing exports: The exported + | ${forwarder.rhs.symbol}: ${alt1.widen} + |and ${forwarder1.rhs.symbol}: ${alt2.widen} + |have the same signature after erasure and overloading resolution could not disambiguate.""", + exp.srcPos) + avoidClashWith(if cmp < 0 then forwarder else forwarder1, forwarders1) + else + val (forwarder2, forwarders2) = avoidClashWith(forwarder, forwarders1) + (forwarder2, forwarders.derivedCons(forwarder1, forwarders2)) + case _ => + (forwarder, forwarders) + end avoidClashWith + + /** Avoid clashes of any two export forwarders in `forwarders`. + * A clash is if two forwarders f1 and f2 have the same name and signatures after erasure. + * We try to avoid a clash by dropping one of f1 and f2, keeping the one whose right hand + * side reference would be preferred by overloading resolution. + * If neither of f1 or f2 is preferred over the other, report an error. + * + * The idea is that this simulates the hypothetical case where export forwarders + * are not generated and we treat an export instead more like an import where we + * expand the use site reference. Test cases in {neg,pos}/i14699.scala. + * + * @pre Forwarders with the same name are consecutive in `forwarders`. + */ + def avoidClashes(forwarders: List[tpd.MemberDef]): List[tpd.MemberDef] = forwarders match + case forwarders @ (forwarder :: forwarders1) => + val (forwarder2, forwarders2) = forwarder match + case forwarder: tpd.DefDef => avoidClashWith(forwarder, forwarders1) + case _ => (forwarder, forwarders1) + forwarders.derivedCons(forwarder2, avoidClashes(forwarders2)) + case Nil => forwarders + addForwarders(selectors, Nil) - val forwarders = buf.toList + val forwarders = avoidClashes(buf.toList) exp.pushAttachment(ExportForwarders, forwarders) forwarders end exportForwarders diff --git a/tests/neg/i14966.check b/tests/neg/i14966.check index 6cc8132f17bb..c5227b352b1b 100644 --- a/tests/neg/i14966.check +++ b/tests/neg/i14966.check @@ -1,10 +1,10 @@ --- [E120] Naming Error: tests/neg/i14966.scala:2:11 -------------------------------------------------------------------- -2 | export s.* // error - | ^ - | Double definition: - | final def ++[B >: T](suffix: IterableOnce[B]): Set[B] in class B at line 2 and - | final def ++(that: IterableOnce[T]): Set[T] in class B at line 2 - | have the same type after erasure. - | - | Consider adding a @targetName annotation to one of the conflicting definitions - | for disambiguation. +-- Error: tests/neg/i14966.scala:10:9 ---------------------------------------------------------------------------------- +10 | export s.* // error + | ^^^^^^^^^^ + | Clashing exports: The exported + | method f: (x: List[Int]): Int + | and method f²: (x: List[String]): Int + | have the same signature after erasure and overloading resolution could not disambiguate. + | + | where: f is a method in trait S + | f² is a method in trait I diff --git a/tests/neg/i14966.scala b/tests/neg/i14966.scala index f8869e9bbac0..38c6d99247ea 100644 --- a/tests/neg/i14966.scala +++ b/tests/neg/i14966.scala @@ -1,2 +1,11 @@ -class B[T](val s: Set[T]): - export s.* // error +trait I[A]: + def f(x: List[String]): A + +trait S: + def f(x: List[Int]): Int + +trait T[A] extends I[A], S + +class Test(s: T[Int]): + export s.* // error + diff --git a/tests/pos/i14966.scala b/tests/pos/i14966.scala new file mode 100644 index 000000000000..9ed0e6ce3720 --- /dev/null +++ b/tests/pos/i14966.scala @@ -0,0 +1,13 @@ +trait I[+A] extends IOps[A, I[A]] + +trait S[A] extends I[A], SOps[A, S[A]] + +trait IOps[+A, +C <: I[A]]: + def concat[B >: A](other: IterableOnce[B]): C + +trait SOps[A, +C <: S[A]] extends IOps[A, C]: + def concat(other: IterableOnce[A]): C + +class Test(s: S[Int]): + export s.* + diff --git a/tests/pos/i14966a.scala b/tests/pos/i14966a.scala new file mode 100644 index 000000000000..ad426fc721b2 --- /dev/null +++ b/tests/pos/i14966a.scala @@ -0,0 +1,2 @@ +class B[T](val s: Set[T]): + export s.* From fed27643972efeeee6b83ffb8c11305fcedb501e Mon Sep 17 00:00:00 2001 From: odersky Date: Wed, 20 Apr 2022 19:31:46 +0200 Subject: [PATCH 4/5] Print erased type in double definition message Print erased type if the message says "have the same type after erasure" --- compiler/src/dotty/tools/dotc/reporting/messages.scala | 3 ++- tests/neg/i14966a.check | 10 ++++++++++ tests/neg/i14966a.scala | 4 ++++ 3 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 tests/neg/i14966a.check create mode 100644 tests/neg/i14966a.scala diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index 36246faaea22..13bf2cf7f580 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -2128,6 +2128,7 @@ import transform.SymUtils._ class DoubleDefinition(decl: Symbol, previousDecl: Symbol, base: Symbol)(using Context) extends NamingMsg(DoubleDefinitionID) { def msg = { def nameAnd = if (decl.name != previousDecl.name) " name and" else "" + def erasedType = if ctx.erasedTypes then i" ${decl.info}" else "" def details(using Context): String = if (decl.isRealMethod && previousDecl.isRealMethod) { import Signature.MatchDegree._ @@ -2155,7 +2156,7 @@ import transform.SymUtils._ |Consider adding a @targetName annotation to one of the conflicting definitions |for disambiguation.""" else "" - i"have the same$nameAnd type after erasure.$hint" + i"have the same$nameAnd type$erasedType after erasure.$hint" } } else "" diff --git a/tests/neg/i14966a.check b/tests/neg/i14966a.check new file mode 100644 index 000000000000..777d1ec74955 --- /dev/null +++ b/tests/neg/i14966a.check @@ -0,0 +1,10 @@ +-- [E120] Naming Error: tests/neg/i14966a.scala:3:6 -------------------------------------------------------------------- +3 | def f(x: List[Int]): String = ??? // error + | ^ + | Double definition: + | def f[X <: String](x: List[X]): String in class Test at line 2 and + | def f(x: List[Int]): String in class Test at line 3 + | have the same type (x: scala.collection.immutable.List): String after erasure. + | + | Consider adding a @targetName annotation to one of the conflicting definitions + | for disambiguation. diff --git a/tests/neg/i14966a.scala b/tests/neg/i14966a.scala new file mode 100644 index 000000000000..f2643798f1e5 --- /dev/null +++ b/tests/neg/i14966a.scala @@ -0,0 +1,4 @@ +class Test: + def f[X <: String](x: List[X]): String = ??? + def f(x: List[Int]): String = ??? // error + From 806386ecd42858c564e7a646c4fa9b842ab27d2d Mon Sep 17 00:00:00 2001 From: odersky Date: Thu, 21 Apr 2022 12:36:12 +0200 Subject: [PATCH 5/5] Pick correct forwarder to avoid clashes We should pick the one which is better, but we mistakenly chose the worst one instead, since I misinterpreted the ordering. --- compiler/src/dotty/tools/dotc/typer/Namer.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index 8a147d388ad0..ac85798cb128 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -1266,7 +1266,7 @@ class Namer { typer: Typer => |and ${forwarder1.rhs.symbol}: ${alt2.widen} |have the same signature after erasure and overloading resolution could not disambiguate.""", exp.srcPos) - avoidClashWith(if cmp < 0 then forwarder else forwarder1, forwarders1) + avoidClashWith(if cmp < 0 then forwarder1 else forwarder, forwarders1) else val (forwarder2, forwarders2) = avoidClashWith(forwarder, forwarders1) (forwarder2, forwarders.derivedCons(forwarder1, forwarders2))