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

Conversation

smarter
Copy link
Member

@smarter smarter commented Mar 11, 2021

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.

@smarter smarter force-pushed the default-getter-type-2 branch from cdfa4b9 to f0c909c Compare March 11, 2021 15:46
@smarter smarter requested a review from odersky March 11, 2021 15:48
@smarter
Copy link
Member Author

smarter commented Mar 11, 2021

(this is a blocker for #11671)

@odersky
Copy link
Contributor

odersky commented Mar 11, 2021

I think we want to go the other way: make default arguments inline (and hence final). Without that step we won't be able to get any of the other desired improvements of default arguments. So, is this change really necessary?

@smarter
Copy link
Member Author

smarter commented Mar 11, 2021

So, is this change really necessary?

It's necessary to get the scala.js tests to pass with #11671. This change improves compatibility with Scala 2 in general but does not change anything fundamentally, we already allow overrides for default parameters, this change just avoid seemingly arbitrary situations where this fails with a puzzling error message. So getting this change in won't have any impact on our future plans with default parameters.

@smarter
Copy link
Member Author

smarter commented Mar 11, 2021

Also, independent of overrides, this change is good because it avoids breaking unnecessary breaks to binary compatibility, right now:

class A {
  def foo(param: X = new Y): X = param
}

will generate a default getter whose return type is Y, meaning that if I later want to change this to = new X, binary compatibility will be broken.

@smarter

This comment has been minimized.

@smarter smarter force-pushed the default-getter-type-2 branch from f0c909c to d584954 Compare March 11, 2021 16:26
smarter added 2 commits March 12, 2021 17:38
- 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)
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.
@smarter smarter force-pushed the default-getter-type-2 branch from d584954 to fc34c1a Compare March 12, 2021 17:00
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.
@smarter smarter force-pushed the default-getter-type-2 branch from fc34c1a to 5b46ac2 Compare March 12, 2021 17:50
@smarter
Copy link
Member Author

smarter commented Mar 12, 2021

This is now green after fixing some community build issues.

@smarter smarter added this to the 3.0.0-RC2 milestone Mar 14, 2021
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@odersky odersky merged commit a4bee46 into scala:master Mar 14, 2021
@odersky odersky deleted the default-getter-type-2 branch March 14, 2021 16:20
OlivierBlanvillain added a commit to dotty-staging/dotty that referenced this pull request Apr 30, 2021
It looks like this bug got fixed in scala#11704!
michelou pushed a commit to michelou/scala3 that referenced this pull request May 1, 2021
It looks like this bug got fixed in scala#11704!
@Kordyjan Kordyjan modified the milestones: 3.0.0-RC2, 3.0.0 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants