From cdbee638ecb87885e36be0ffa8aee9ed57ce31fa Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 12 Mar 2019 14:33:11 +0100 Subject: [PATCH 1/3] Add -Xmixin-force-forwarders This controls when mixin forwarders are generated like the homonymous flag in scalac, for now we default to "junit" to keep our existing behavior, next commit will change it to "true" to match scalac. --- .../src/dotty/tools/dotc/config/ScalaSettings.scala | 13 +++++++++++++ .../src/dotty/tools/dotc/transform/MixinOps.scala | 5 +++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala index 1e0644efd00f..ffec33242a71 100644 --- a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala +++ b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala @@ -1,6 +1,7 @@ package dotty.tools.dotc package config +import dotty.tools.dotc.core.Contexts.Context import dotty.tools.io.{ Directory, PlainDirectory, AbstractFile } import PathResolver.Defaults import rewrites.Rewrites @@ -76,6 +77,18 @@ class ScalaSettings extends Settings.SettingGroup { val XfatalWarnings: Setting[Boolean] = BooleanSetting("-Xfatal-warnings", "Fail the compilation if there are any warnings.") val XverifySignatures: Setting[Boolean] = BooleanSetting("-Xverify-signatures", "Verify generic signatures in generated bytecode.") + val XmixinForceForwarders = ChoiceSetting( + name = "-Xmixin-force-forwarders", + helpArg = "mode", + descr = "Generate forwarder methods in classes inhering concrete methods from traits.", + choices = List("true", "junit", "false"), + default = "junit") + + object mixinForwarderChoices { + def isTruthy(implicit ctx: Context) = XmixinForceForwarders.value == "true" + def isAtLeastJunit(implicit ctx: Context) = isTruthy || XmixinForceForwarders.value == "junit" + } + /** -Y "Private" settings */ val YoverrideVars: Setting[Boolean] = BooleanSetting("-Yoverride-vars", "Allow vars to be overridden.") val Yhelp: Setting[Boolean] = BooleanSetting("-Y", "Print a synopsis of private options.") diff --git a/compiler/src/dotty/tools/dotc/transform/MixinOps.scala b/compiler/src/dotty/tools/dotc/transform/MixinOps.scala index 44040d454902..a5da763ba389 100644 --- a/compiler/src/dotty/tools/dotc/transform/MixinOps.scala +++ b/compiler/src/dotty/tools/dotc/transform/MixinOps.scala @@ -59,12 +59,13 @@ class MixinOps(cls: ClassSymbol, thisPhase: DenotTransformer)(implicit ctx: Cont def hasNonInterfaceDefinition = competingMethods.exists(!_.owner.is(Trait)) // there is a definition originating from class !meth.isConstructor && meth.is(Method, butNot = PrivateOrAccessorOrDeferred) && - (meth.owner.is(Scala2x) || needsDisambiguation || hasNonInterfaceDefinition || needsJUnit4Fix(meth) ) && + (ctx.settings.mixinForwarderChoices.isTruthy || meth.owner.is(Scala2x) || needsDisambiguation || hasNonInterfaceDefinition || needsJUnit4Fix(meth)) && isCurrent(meth) } private def needsJUnit4Fix(meth: Symbol): Boolean = { - meth.annotations.nonEmpty && JUnit4Annotations.exists(annot => meth.hasAnnotation(annot)) + meth.annotations.nonEmpty && JUnit4Annotations.exists(annot => meth.hasAnnotation(annot)) && + ctx.settings.mixinForwarderChoices.isAtLeastJunit } final val PrivateOrAccessor: FlagSet = Private | Accessor From 6c6443086071774431dae93709b4102ce3c4d15e Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 12 Mar 2019 14:34:46 +0100 Subject: [PATCH 2/3] Fix #2563: Unconditionally emit mixin forwarders This is what Scala 2.12+ does for cold performance reasons (see #5928 for more details) and we should align ourselves with them when possible. About two years ago in #2563, Dmitry objected that we may not need to do that if we found another way to get performance back, or if newer JDKs improved the performance of default method resolution. It doesn't look like these things have happened so far (but there's some recent glimmer of hope here: https://bugs.openjdk.java.net/browse/JDK-8036580). Dmitry also said "I don't recall triggering bugs by emitting more forwarders rather then less.", but in fact since then I've found one case where the standard library failed to compile with extra forwarders causing name clashes, requiring a non-binary-compatible change: https://github.com/scala/scala/pull/7747/commits/e3ef6573a55cb6e7ecf80ee6ebd312876f7f12df. As of #6079 this is no longer a problem since we now emit mixin forwarders after erasure like scalac, but it still seems prudent to emit as many forwarders as scalac to catch potential name clash issues. --- compiler/src/dotty/tools/dotc/config/ScalaSettings.scala | 2 +- compiler/test/dotty/tools/dotc/CompilationTests.scala | 1 + tests/{run => run-custom-args}/no-useless-forwarders.scala | 0 3 files changed, 2 insertions(+), 1 deletion(-) rename tests/{run => run-custom-args}/no-useless-forwarders.scala (100%) diff --git a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala index ffec33242a71..efee841c3a34 100644 --- a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala +++ b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala @@ -82,7 +82,7 @@ class ScalaSettings extends Settings.SettingGroup { helpArg = "mode", descr = "Generate forwarder methods in classes inhering concrete methods from traits.", choices = List("true", "junit", "false"), - default = "junit") + default = "true") object mixinForwarderChoices { def isTruthy(implicit ctx: Context) = XmixinForceForwarders.value == "true" diff --git a/compiler/test/dotty/tools/dotc/CompilationTests.scala b/compiler/test/dotty/tools/dotc/CompilationTests.scala index 6bdb708e58f6..9eb7f58303c8 100644 --- a/compiler/test/dotty/tools/dotc/CompilationTests.scala +++ b/compiler/test/dotty/tools/dotc/CompilationTests.scala @@ -185,6 +185,7 @@ class CompilationTests extends ParallelTesting { compileFilesInDir("tests/run-custom-args/Yretain-trees", defaultOptions and "-Yretain-trees") + compileFile("tests/run-custom-args/tuple-cons.scala", allowDeepSubtypes) + compileFile("tests/run-custom-args/i5256.scala", allowDeepSubtypes) + + compileFile("tests/run-custom-args/no-useless-forwarders.scala", defaultOptions and "-Xmixin-force-forwarders:false") + compileFilesInDir("tests/run", defaultOptions) }.checkRuns() diff --git a/tests/run/no-useless-forwarders.scala b/tests/run-custom-args/no-useless-forwarders.scala similarity index 100% rename from tests/run/no-useless-forwarders.scala rename to tests/run-custom-args/no-useless-forwarders.scala From 22db0c0490eee337f305a08704240ec20847816a Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 12 Mar 2019 14:12:19 +0100 Subject: [PATCH 3/3] Simplify mixin forwarders tests Now that -Xmixin-force-forwarders defaults to true, tests that showcase some behavior of mixin forwarders can be simplified since we need less indirections to generate them. --- tests/neg/mixin-forwarder-clash1.check | 6 ++--- tests/neg/mixin-forwarder-clash1.scala | 30 ++++++++-------------- tests/neg/mixin-forwarder-clash2.check | 4 +-- tests/neg/mixin-forwarder-clash2/A_1.scala | 16 +++--------- tests/neg/mixin-forwarder-clash2/B_2.scala | 8 +++--- tests/pos/mixin-forwarder-clash1.scala | 22 +++++----------- tests/pos/mixin-forwarder-clash2/A_1.scala | 21 ++++++--------- tests/pos/mixin-forwarder-clash2/B_2.scala | 4 +-- 8 files changed, 38 insertions(+), 73 deletions(-) diff --git a/tests/neg/mixin-forwarder-clash1.check b/tests/neg/mixin-forwarder-clash1.check index fff73b4ef2cc..68eb09ae90e0 100644 --- a/tests/neg/mixin-forwarder-clash1.check +++ b/tests/neg/mixin-forwarder-clash1.check @@ -1,5 +1,5 @@ -<527..527> in mixin-forwarder-clash1.scala +<284..284> in mixin-forwarder-clash1.scala Name clash between inherited members: -override def concat(suffix: Int): X in trait OneB at line 10 and -override def concat: [Dummy](suffix: Int): Y in trait TwoB at line 18 +def concat(suffix: Int): X in trait One at line 4 and +def concat: [Dummy](suffix: Int): Y in trait Two at line 8 have the same type after erasure. diff --git a/tests/neg/mixin-forwarder-clash1.scala b/tests/neg/mixin-forwarder-clash1.scala index 530df1bee1e8..d0671e6b6e45 100644 --- a/tests/neg/mixin-forwarder-clash1.scala +++ b/tests/neg/mixin-forwarder-clash1.scala @@ -1,38 +1,28 @@ class Foo -// Using self-types to force mixin forwarders - -trait OneA[X] { +trait One[X] { def concat(suffix: Int): X = ??? } -trait OneB[X] { self: OneA[X] => - override def concat(suffix: Int): X = ??? -} - -trait TwoA[Y/* <: Foo*/] { +trait Two[Y/* <: Foo*/] { def concat[Dummy](suffix: Int): Y = ??? } -trait TwoB[Y/* <: Foo*/] { self: TwoA[Y] => - override def concat[Dummy](suffix: Int): Y = ??? -} - -class Bar1 extends OneA[Foo] with OneB[Foo] +class Bar1 extends One[Foo] // Because mixin forwarders are generated after erasure, we get: // override def concat(suffix: Int): Object -class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error - // We get a mixin forwarder for TwoB: +class Bar2 extends Bar1 with Two[Foo] // error + // We get a mixin forwarder for Two: // override def concat(suffix: Int): Object // This clashes with the forwarder generated in Bar1, and the compiler detects that: // - // |class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] + // |class Bar2 extends Bar1 with Two[Foo] // | ^ - // | Name clash between inherited members: - // | override def concat(suffix: Int): Object in trait OneB at line 10 and - // | override def concat: [Dummy](suffix: Int): Y in trait TwoB at line 18 - // | have the same type after erasure. + // | Name clash between inherited members: + // | def concat(suffix: Int): X in trait One at line 4 and + // | def concat: [Dummy](suffix: Int): Y in trait Two at line 8 + // | have the same type after erasure. // // This also works with separate compilation as demonstrated by // mixin-forwarder-clash2. diff --git a/tests/neg/mixin-forwarder-clash2.check b/tests/neg/mixin-forwarder-clash2.check index 42653538e72a..66a88e46228d 100644 --- a/tests/neg/mixin-forwarder-clash2.check +++ b/tests/neg/mixin-forwarder-clash2.check @@ -1,5 +1,5 @@ <6..6> in B_2.scala Name clash between inherited members: -override def concat(suffix: Int): X in trait OneB and -override def concat: [Dummy](suffix: Int): Y in trait TwoB +def concat(suffix: Int): X in trait One and +def concat: [Dummy](suffix: Int): Y in trait Two have the same type after erasure. diff --git a/tests/neg/mixin-forwarder-clash2/A_1.scala b/tests/neg/mixin-forwarder-clash2/A_1.scala index 4841df3670b7..73c2250fe8ba 100644 --- a/tests/neg/mixin-forwarder-clash2/A_1.scala +++ b/tests/neg/mixin-forwarder-clash2/A_1.scala @@ -1,23 +1,13 @@ class Foo -// Using self-types to force mixin forwarders - -trait OneA[X] { +trait One[X] { def concat(suffix: Int): X = ??? } -trait OneB[X] { self: OneA[X] => - override def concat(suffix: Int): X = ??? -} - -trait TwoA[Y/* <: Foo*/] { +trait Two[Y/* <: Foo*/] { def concat[Dummy](suffix: Int): Y = ??? } -trait TwoB[Y/* <: Foo*/] { self: TwoA[Y] => - override def concat[Dummy](suffix: Int): Y = ??? -} - -class Bar1 extends OneA[Foo] with OneB[Foo] +class Bar1 extends One[Foo] // Because mixin forwarders are generated after erasure, we get: // override def concat(suffix: Int): Object diff --git a/tests/neg/mixin-forwarder-clash2/B_2.scala b/tests/neg/mixin-forwarder-clash2/B_2.scala index 6723785bdf94..3548bfc8d177 100644 --- a/tests/neg/mixin-forwarder-clash2/B_2.scala +++ b/tests/neg/mixin-forwarder-clash2/B_2.scala @@ -1,5 +1,5 @@ -class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error - // We get a mixin forwarder for TwoB: +class Bar2 extends Bar1 with Two[Foo] // error + // We get a mixin forwarder for Two: // override def concat(suffix: Int): Object // This clashes with the forwarder generated in Bar1, and // we can detect that even with separate compilation, @@ -11,6 +11,6 @@ class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error // |class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // | ^ // | Name clash between inherited members: - // | override def concat(suffix: Int): X in trait OneB and - // | override def concat: [Dummy](suffix: Int): Y in trait TwoB + // | def concat(suffix: Int): X in trait One and + // | def concat: [Dummy](suffix: Int): Y in trait Two // | have the same type after erasure. diff --git a/tests/pos/mixin-forwarder-clash1.scala b/tests/pos/mixin-forwarder-clash1.scala index ef52a17b4e60..cdfaac437099 100644 --- a/tests/pos/mixin-forwarder-clash1.scala +++ b/tests/pos/mixin-forwarder-clash1.scala @@ -4,36 +4,26 @@ class Foo -// Using self-types to force mixin forwarders - -trait OneA[X] { +trait One[X] { def concat(suffix: Int): X = ??? } -trait OneB[X] { self: OneA[X] => - override def concat(suffix: Int): X = ??? -} - -trait TwoA[Y <: Foo] { +trait Two[Y <: Foo] { def concat[Dummy](suffix: Int): Y = ??? } -trait TwoB[Y <: Foo] { self: TwoA[Y] => - override def concat[Dummy](suffix: Int): Y = ??? -} - -class Bar1 extends OneA[Foo] with OneB[Foo] +class Bar1 extends One[Foo] // Because mixin forwarders are generated before erasure, we get: // override def concat(suffix: Int): Foo -class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error - // We get a mixin forwarder for TwoB: +class Bar2 extends Bar1 with Two[Foo] // error + // We get a mixin forwarder for Two: // override def concat[Dummy](suffix: Int): Foo // which gets erased to: // override def concat(suffix: Int): Foo // This clashes with the forwarder generated in Bar1, and the compiler detects that: // - // |class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] + // |class Bar2 extends Bar1 with Two[Foo] // | ^ // | Name clash between defined and inherited member: // | override def concat(suffix: Int): Foo in class Bar1 and diff --git a/tests/pos/mixin-forwarder-clash2/A_1.scala b/tests/pos/mixin-forwarder-clash2/A_1.scala index 1aaabe95eed8..d8f9b27a1ddd 100644 --- a/tests/pos/mixin-forwarder-clash2/A_1.scala +++ b/tests/pos/mixin-forwarder-clash2/A_1.scala @@ -1,23 +1,18 @@ -class Foo +// This test case was supposed to fail when mixin forwarders were generated before erasure, +// but didn't due to separate compilation unlike mixin-forwarder-clash1, +// it's not supposed to fail anymore since the forwarders generated after erasure do not clash, +// the comments are preserved for posterity. -// Using self-types to force mixin forwarders +class Foo -trait OneA[X] { +trait One[X] { def concat(suffix: Int): X = ??? } -trait OneB[X] { self: OneA[X] => - override def concat(suffix: Int): X = ??? -} - -trait TwoA[Y <: Foo] { +trait Two[Y <: Foo] { def concat[Dummy](suffix: Int): Y = ??? } -trait TwoB[Y <: Foo] { self: TwoA[Y] => - override def concat[Dummy](suffix: Int): Y = ??? -} - -class Bar1 extends OneA[Foo] with OneB[Foo] +class Bar1 extends One[Foo] // Because mixin forwarders are generated before erasure, we get: // override def concat(suffix: Int): Foo diff --git a/tests/pos/mixin-forwarder-clash2/B_2.scala b/tests/pos/mixin-forwarder-clash2/B_2.scala index 00c72f665ca3..dd8a7a19da6d 100644 --- a/tests/pos/mixin-forwarder-clash2/B_2.scala +++ b/tests/pos/mixin-forwarder-clash2/B_2.scala @@ -3,8 +3,8 @@ // it's not supposed to fail anymore since the forwarders generated after erasure do not clash, // the comments are preserved for posterity. -class Bar2 extends Bar1 with TwoA[Foo] with TwoB[Foo] // error - // We get a mixin forwarder for TwoB: +class Bar2 extends Bar1 with Two[Foo] // error + // We get a mixin forwarder for Two: // override def concat[Dummy](suffix: Int): Foo // which gets erased to: // override def concat(suffix: Int): Foo