Skip to content

Improve our ability to override default parameters #11704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 14, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
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.
  • Loading branch information
smarter committed Mar 12, 2021
commit 5b46ac2f8ab6618055e895adc39e4d82af48f87d
11 changes: 2 additions & 9 deletions compiler/src/dotty/tools/dotc/ast/Desugar.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 3 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Namer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Expand Down
45 changes: 45 additions & 0 deletions tests/pos/default-getter-variance.scala
Original file line number Diff line number Diff line change
@@ -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`