-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Select local implicits over name-imported over wildcard imported #18203
Conversation
a563208
to
650f854
Compare
I haven't studied this issue, but intuitively, it's too bad to add special rules which are reminiscent of name-binding precedence, though implicit search was freed from naming and relied only on a notion of context depth. I see the test is about implicits and not givens. I don't even remember if they are treated differently, aside from import syntax. |
There's some name shadowing still in play, for instance from tests/pos/Monoid.scala Other than that, "all" I'm trying to do is deal with the unfortunate inversion that exists between the context for a method block scope being the "outer" context to the context that comes from encountering an import in the block. Rather than try to change that setup and everything that entails, I went with the fix that akin to
🙄 We need a new tools pragma |
Note that new style givens already work correctly. It's just old-style implicits that differ. In that light, we should not spend too much complexity for something that so far did not seem to cause a problem in practice. |
That's true, in as much as |
3c07ab5
to
b701344
Compare
b701344
to
463ee8c
Compare
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.
Otherwise LGTM
def preferDefinitions = isImport && !outer.isImport | ||
def preferNamedImport = isWildcardImport && !isWildcardImport(using outer.irefCtx) | ||
|
||
if !migrateTo3(using irefCtx) && level == outer.level && (preferDefinitions || preferNamedImport) then |
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.
Why the !migrateTo3 condition? I thought Scala 2 has the same preferences of definitions over imports?
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.
To counter the fact that we're flattening it in "level". Otherwise we end up preferring the candidates in an outer scope, which should have a smaller level, over the ones in an inner scope.
I encountered it in CI, and minimised the problem to tests/pos/i18183.migration.scala
. Because it runs under -source:3.0-migration
then outer implicit F: Async[F]
"shadows" inner implicit F: Async[M]
, making it impossible to call semiflatMap
for lack of a Async[M]
in scope.
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 see. Yes that makes sense.
No description provided.