-
Notifications
You must be signed in to change notification settings - Fork 1.1k
ParamForwarding: do not require param accessors to be private[this] #617
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
Conversation
Note that we do less parameter forwarding than |
3189fd9
to
4407c13
Compare
Also mark the forwarder as Stable otherwise we get a RefChecks error. This fixes scala#608. Note that we do less parameter forwarding than scalac. See for example D and Y in tests/run/paramForwarding.scala which don't get their own local fields in scalac but do in dotty.
4407c13
to
beba970
Compare
LGTM, thanks! |
ParamForwarding: do not require param accessors to be private[this]
I had a quick play with this and couldn't find any big holes. Suggested test variations would include varying repeated / by-name modifiers in the sub/super class, and access modifiers. I noticed that dotty's tree printer seems not to print the qualifier in qualified protected/private, I was testing that to see if the flags of the constructor param were correctly propagated to the accessor. I managed to crash the compiler with: class Outer {
class A(protected[Outer] val theValue: Any*) {
val theValueInA = theValue
def getTheValue = theValue
}
class C(override protected[Outer] val theValue: Nothing) extends A(theValue)
} I also noticed that the cat sandbox/test.scala && ./bin/dotc sandbox/test.scala
class C(val a: Int*)
class Test {
def test(c: C): Nothing = c.a
}
sandbox/test.scala:4: error: type mismatch:
found : scala.collection.Seq[Int] @Repeated(c.a)
required: Nothing
def test(c: C): Nothing = c.a
^
one error found |
@retronym : Thanks for looking at this! Would you mind opening issues for the problems you found? |
@smarter done |
Qualified private is currently not a high priority, because we will most likely get rid of it. In that case it will be a task for the migration tool to find a rewrite. |
So you would rewrite |
Also mark the forwarder as Stable otherwise we get a RefChecks error.
This fixes #608.
Review by @odersky, @retronym .