From 6f729e65c5ccfc5f999c4214e581413e22d6cfcf Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Thu, 11 Mar 2021 15:21:52 +0100 Subject: [PATCH 1/3] Cleanup Namer#valOrDefDefSig#inferredType - Replace rhsType/widenRhs by typedAheadRhs/rhsType to avoid computing rhsProto twice - Rename rhsProto to defaultParamType and move the wildcard handling to rhsType (this will be useful in the next commit) --- .../src/dotty/tools/dotc/typer/Namer.scala | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index 8024ce0857a9..933477e91e2d 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -1386,13 +1386,10 @@ class Namer { typer: Typer => } end inherited - /** The proto-type to be used when inferring the result type from - * the right hand side. This is `WildcardType` except if the definition - * is a default getter. In that case, the proto-type is the type of - * the corresponding parameter where bound parameters are replaced by - * Wildcards. + /** If this is a default getter, the type of the corresponding method parameter, + * otherwise NoType. */ - def rhsProto = sym.asTerm.name collect { + def defaultParamType = sym.name match case DefaultGetterName(original, idx) => val meth: Denotation = if (original.isConstructorName && (sym.owner.is(ModuleClass))) @@ -1401,37 +1398,24 @@ class Namer { typer: Typer => ctx.defContext(sym).denotNamed(original) def paramProto(paramss: List[List[Type]], idx: Int): Type = paramss match { case params :: paramss1 => - if (idx < params.length) wildApprox(params(idx)) + if (idx < params.length) params(idx) else paramProto(paramss1, idx - params.length) case nil => - WildcardType + NoType } val defaultAlts = meth.altsWith(_.hasDefaultParams) if (defaultAlts.length == 1) paramProto(defaultAlts.head.info.widen.paramInfoss, idx) else - WildcardType - } getOrElse WildcardType + NoType + case _ => + NoType // println(s"final inherited for $sym: ${inherited.toString}") !!! // println(s"owner = ${sym.owner}, decls = ${sym.owner.info.decls.show}") // TODO Scala 3.1: only check for inline vals (no final ones) def isInlineVal = sym.isOneOf(FinalOrInline, butNot = Method | Mutable) - // Widen rhs type and eliminate `|' but keep ConstantTypes if - // definition is inline (i.e. final in Scala2) and keep module singleton types - // instead of widening to the underlying module class types. - // We also drop the @Repeated annotation here to avoid leaking it in method result types - // (see run/inferred-repeated-result). - def widenRhs(tp: Type): Type = - tp.widenTermRefExpr.simplified match - case ctp: ConstantType if isInlineVal => ctp - case tp => TypeComparer.widenInferred(tp, rhsProto) - - // Replace aliases to Unit by Unit itself. If we leave the alias in - // it would be erased to BoxedUnit. - def dealiasIfUnit(tp: Type) = if (tp.isRef(defn.UnitClass)) defn.UnitType else tp - var rhsCtx = ctx.fresh.addMode(Mode.InferringReturnType) if sym.isInlineMethod then rhsCtx = rhsCtx.addMode(Mode.InlineableBody) if sym.is(ExtensionMethod) then rhsCtx = rhsCtx.addMode(Mode.InExtensionMethod) @@ -1443,8 +1427,26 @@ class Namer { typer: Typer => rhsCtx.setFreshGADTBounds rhsCtx.gadt.addToConstraint(typeParams) } - def rhsType = PrepareInlineable.dropInlineIfError(sym, - typedAheadExpr(mdef.rhs, (inherited orElse rhsProto).widenExpr)(using rhsCtx)).tpe + + def typedAheadRhs(pt: Type) = + PrepareInlineable.dropInlineIfError(sym, + typedAheadExpr(mdef.rhs, pt)(using rhsCtx)) + + def rhsType = + // For default getters, we use the corresponding parameter type as an + // expected type but we run it through `wildApprox` to allow default + // parameters like in `def mkList[T](value: T = 1): List[T]`. + val defaultTp = defaultParamType + val pt = inherited.orElse(wildApprox(defaultTp)).orElse(WildcardType).widenExpr + val tp = typedAheadRhs(pt).tpe + tp.widenTermRefExpr.simplified match + case ctp: ConstantType if isInlineVal => ctp + case tp => + TypeComparer.widenInferred(tp, pt) + + // Replace aliases to Unit by Unit itself. If we leave the alias in + // it would be erased to BoxedUnit. + def dealiasIfUnit(tp: Type) = if (tp.isRef(defn.UnitClass)) defn.UnitType else tp // Approximate a type `tp` with a type that does not contain skolem types. val deskolemize = new ApproximatingTypeMap { @@ -1455,7 +1457,7 @@ class Namer { typer: Typer => } } - def cookedRhsType = deskolemize(dealiasIfUnit(widenRhs(rhsType))) + def cookedRhsType = deskolemize(dealiasIfUnit(rhsType)) def lhsType = fullyDefinedType(cookedRhsType, "right-hand side", mdef.span) //if (sym.name.toString == "y") println(i"rhs = $rhsType, cooked = $cookedRhsType") if (inherited.exists) From 6fde5c122330ef98481279c0c8baa30f02b232d8 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Thu, 11 Mar 2021 16:15:51 +0100 Subject: [PATCH 2/3] Improve our ability to override default parameters Don't just use the type of the rhs of the default getter as its inferred type, this can be more precise than the type of the corresponding parameter and prevent seemingly valid overrides. --- compiler/src/dotty/tools/dotc/typer/Namer.scala | 6 +++++- tests/pos/default-getter.scala | 10 ++++++++++ tests/{neg/i4659b.scala => pos/i4659c.scala} | 2 +- 3 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 tests/pos/default-getter.scala rename tests/{neg/i4659b.scala => pos/i4659c.scala} (65%) diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index 933477e91e2d..d98bd4661324 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -1439,7 +1439,11 @@ class Namer { typer: Typer => val defaultTp = defaultParamType val pt = inherited.orElse(wildApprox(defaultTp)).orElse(WildcardType).widenExpr val tp = typedAheadRhs(pt).tpe - tp.widenTermRefExpr.simplified match + if (defaultTp eq pt) && (tp frozen_<:< defaultTp) then + // When possible, widen to the default getter parameter type to permit a + // larger choice of overrides (see `default-getter.scala`). + defaultTp + else tp.widenTermRefExpr.simplified match case ctp: ConstantType if isInlineVal => ctp case tp => TypeComparer.widenInferred(tp, pt) diff --git a/tests/pos/default-getter.scala b/tests/pos/default-getter.scala new file mode 100644 index 000000000000..002fa32fef19 --- /dev/null +++ b/tests/pos/default-getter.scala @@ -0,0 +1,10 @@ +class X +class Y extends X + +class A { + def foo(param: X = new Y): X = param +} + +class B extends A { + override def foo(param: X = new X): X = param +} diff --git a/tests/neg/i4659b.scala b/tests/pos/i4659c.scala similarity index 65% rename from tests/neg/i4659b.scala rename to tests/pos/i4659c.scala index fc37ad62f80a..8dc261f1f024 100644 --- a/tests/neg/i4659b.scala +++ b/tests/pos/i4659c.scala @@ -2,4 +2,4 @@ case class SourcePosition(outer: SourcePosition = NoSourcePosition) { assert(outer != null) // crash } -object NoSourcePosition extends SourcePosition() // error \ No newline at end of file +object NoSourcePosition extends SourcePosition() From 5b46ac2f8ab6618055e895adc39e4d82af48f87d Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Fri, 12 Mar 2021 17:56:25 +0100 Subject: [PATCH 3/3] Fix variance checking in default getters This is needed to compile geny in the community build which does: def count(f: A => Boolean = ((_: Any) => true)) This happened to work before the previous commit because the inferred type of the getter was `Any => Boolean`. Crucially, we can only disable variance checking for default getters whose result type matches the parameter type of the method, see default-getter-variance.scala for a detailed justification of why this is safe. This fixes lets us get rid of an unjustified usage of `@uncheckedVariance` when desugaring case classes. --- .../src/dotty/tools/dotc/ast/Desugar.scala | 11 +---- .../src/dotty/tools/dotc/typer/Namer.scala | 4 +- tests/pos/default-getter-variance.scala | 45 +++++++++++++++++++ 3 files changed, 50 insertions(+), 10 deletions(-) create mode 100644 tests/pos/default-getter-variance.scala diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index a537cdeb0f0e..e0053f0f8aff 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -602,13 +602,8 @@ object desugar { // def _1: T1 = this.p1 // ... // def _N: TN = this.pN (unless already given as valdef or parameterless defdef) - // def copy(p1: T1 = p1: @uncheckedVariance, ..., - // pN: TN = pN: @uncheckedVariance)(moreParams) = + // def copy(p1: T1 = p1..., pN: TN = pN)(moreParams) = // new C[...](p1, ..., pN)(moreParams) - // - // Note: copy default parameters need @uncheckedVariance; see - // neg/t1843-variances.scala for a test case. The test would give - // two errors without @uncheckedVariance, one of them spurious. val (caseClassMeths, enumScaffolding) = { def syntheticProperty(name: TermName, tpt: Tree, rhs: Tree) = DefDef(name, Nil, tpt, rhs).withMods(synthetic) @@ -638,10 +633,8 @@ object desugar { } if (mods.is(Abstract) || hasRepeatedParam) Nil // cannot have default arguments for repeated parameters, hence copy method is not issued else { - def copyDefault(vparam: ValDef) = - makeAnnotated("scala.annotation.unchecked.uncheckedVariance", refOfDef(vparam)) val copyFirstParams = derivedVparamss.head.map(vparam => - cpy.ValDef(vparam)(rhs = copyDefault(vparam))) + cpy.ValDef(vparam)(rhs = refOfDef(vparam))) val copyRestParamss = derivedVparamss.tail.nestedMap(vparam => cpy.ValDef(vparam)(rhs = EmptyTree)) DefDef( diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index d98bd4661324..1eb4a37c9a2f 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -1442,7 +1442,9 @@ class Namer { typer: Typer => if (defaultTp eq pt) && (tp frozen_<:< defaultTp) then // When possible, widen to the default getter parameter type to permit a // larger choice of overrides (see `default-getter.scala`). - defaultTp + // For justification on the use of `@uncheckedVariance`, see + // `default-getter-variance.scala`. + AnnotatedType(defaultTp, Annotation(defn.UncheckedVarianceAnnot)) else tp.widenTermRefExpr.simplified match case ctp: ConstantType if isInlineVal => ctp case tp => diff --git a/tests/pos/default-getter-variance.scala b/tests/pos/default-getter-variance.scala new file mode 100644 index 000000000000..c9a7d9618c54 --- /dev/null +++ b/tests/pos/default-getter-variance.scala @@ -0,0 +1,45 @@ +class Foo[+A] { + def count(f: A => Boolean = _ => true): Unit = {} + // The preceding line is valid, even though the generated default getter + // has type `A => Boolean` which wouldn't normally pass variance checks + // because it's equivalent to the following overloads which are valid: + def count2(f: A => Boolean): Unit = {} + def count2(): Unit = count(_ => true) +} + +class Bar1[+A] extends Foo[A] { + override def count(f: A => Boolean): Unit = {} + // This reasoning extends to overrides: + override def count2(f: A => Boolean): Unit = {} +} + +class Bar2[+A] extends Foo[A] { + override def count(f: A => Boolean = _ => true): Unit = {} + // ... including overrides which also override the default getter: + override def count2(f: A => Boolean): Unit = {} + override def count2(): Unit = count(_ => true) +} + +// This can be contrasted with the need for variance checks in +// `protected[this] methods (cf tests/neg/t7093.scala), +// default getters do not have the same problem since they cannot +// appear in arbitrary contexts. + + +// Crucially, this argument does not apply to situations in which the default +// getter result type is not a subtype of the parameter type, for example (from +// tests/neg/variance.scala): +// +// class Foo[+A: ClassTag](x: A) { +// private[this] val elems: Array[A] = Array(x) +// def f[B](x: Array[B] = elems): Array[B] = x +// } +// +// If we tried to rewrite this with an overload, it would fail +// compilation: +// +// def f[B](): Array[B] = f(elems) // error: Found: Array[A], Expected: Array[B] +// +// So we only disable variance checking for default getters whose +// result type is the method parameter type, this is checked by +// `tests/neg/variance.scala`