-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #7926: Redo parameter aliasing optimization #8428
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
// if you would use parent param-name `a` to implement param-field `b` | ||
// overriding field `b` will actually override field `a`, that is wrong! | ||
typr.println(i"super alias: ${sym.showLocated}") | ||
sym.setFlag(SuperParamAlias) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This usage of setFlag
is unsafe, the same change applied in b17a836 is needed (this should really be the default behavior of setFlag!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I changed that.
case superCall @ Apply(fn, superArgs) :: _ if superArgs.nonEmpty => | ||
fn.tpe.widen match | ||
case MethodType(superParamNames) => | ||
for case stat: ValDef <- impl.body do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will loop over the whole body looking for param accessors for every super-call. I think it'd be more efficient to collect the param accessors first, then check if any super-argument matches one of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's only the supercall of the first template parent
* This info is used in phase ParamForwarding | ||
*/ | ||
private def forwardParamAccessors(impl: Template)(using Context): Unit = impl.parents match | ||
case superCall @ Apply(fn, superArgs) :: _ if superArgs.nonEmpty => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should exclude super-calls to traits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't happen. First super call is always to a class.
- Move main optimization after unpickling, so that changes to parameters in superclasses do not break Tasty compatibility. - Replace internal Alias annotation by SuperParamAlias flag - Don't generate illegal private overrides - Make the transformation more robust under separate compilation and wrt compilation order
Java requires this. Scala 2 is even stricter: it disallows all provite members to override, no matter whether they are methods or not.
also assert there is no rhs for PARAM tags: since scala#8428 they were no longer pickled. This is a tasty compatible change as the major version has bumped since then.
also assert there is no rhs for PARAM tags: since scala#8428 they were no longer pickled. This is a tasty compatible change as the major version has bumped since then.
to parameters in superclasses do not break Tasty compatibility.
and wrt compilation order
As a separate issue, make sure the user cannot write the sort of illegal "override" of a public with a private method that the compiler generated for this example before.