-
Notifications
You must be signed in to change notification settings - Fork 337
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
fix: don't look for overshadow conflicts for symbols not in the scope #7001
Conversation
|
||
def validateSymbol(symbol: Symbol) = | ||
!symbol.isSynthetic && !symbol.isImplicit && | ||
(!symbol.safeOwner.isType || symbol.safeOwner.isModuleClass || classParents |
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.
What about:
class O {
val t = new T()
import t._
val bb = a + a
class Inner {
val a = 123
val cc = <<bb>>
}
}
both a's have a type as parent. We should differentiate Select and Ident. I think the logic is correct on Select, but not on Ident
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.
I found another example, where this logic fails but in a different way.
object O {
def ==(o: O) = false
}
object P {
def test() = {
val isOne = O == O
<<i>>sOne //claims it cannot inline
}
}
Being concerned only with the first qualifier for select (this
might need some special logic) should be a better approach. Ident was for sake of (infix is also Select)infix
methods, I guess we should detect them and handle appropriately.
However, this will fail to cover overshadow connected to extension methods and implicit classes. But lets maybe make it into a followup issue.
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.
However, this will fail to cover overshadow connected to extension methods and implicit classes. But lets maybe make it into a followup issue.
Yeah, we should be able to check that separately.
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.
Also in your example reference to a
gets turned into a Select
in the typed tree.
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.
That's not great then 🤔 Maybe we can check the ident span in Select(ident: Ident, sel) to see if it actually exist in the code ?
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.
I changed the approach and I check if symbols are in scope before inline and look for conflicts on these symbols. Seems like the simplest solution.
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.
LGTM!
fixes: #6924
Before:
We search for overshadowed symbols before performing inline - names, which would refer to different symbol after inline, by looking at symbols available in the new scope.
Problem:
For symbol
scala/Int#==()
, conflictingscala/Any#==()
was found.Now:
Since symbols for
methods
andvalues
on a class/type instance that is not an owner of the inlined method are not in the scope, we don't check them for conflicts.