Skip to content
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

Merged
merged 4 commits into from
Jul 24, 2023

Conversation

dwijnand
Copy link
Member

No description provided.

@dwijnand dwijnand linked an issue Jul 13, 2023 that may be closed by this pull request
@dwijnand dwijnand force-pushed the name-clash-wrong-implicit branch from a563208 to 650f854 Compare July 13, 2023 18:00
@som-snytt
Copy link
Contributor

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.

@dwijnand
Copy link
Member Author

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.

There's some name shadowing still in play, for instance from tests/pos/Monoid.scala import Monoid.ops._; import Ring.ops.* wants Ring.ops.InfixAppend to shadow Monoid.ops.InfixAppend rather than be ambiguous with it.

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 checkNewOrShadowed.

I see the test is about implicits and not givens. I don't even remember if they are treated differently, aside from import syntax.

🙄 We need a new tools pragma // sed: s/implicit/given/ tests/run/i18183.scala.

@odersky
Copy link
Contributor

odersky commented Jul 13, 2023

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.

@dwijnand
Copy link
Member Author

dwijnand commented Jul 14, 2023

Note that new style givens already work correctly. It's just old-style implicits that differ.

That's true, in as much as import bar._ doesn't import givens by default, so there's no chance of accidentally choosing it.
But import bar.{ given, * }, which would be the equivalent new way of doing it, and runs into exactly the same problem, even using givens.

@dwijnand dwijnand force-pushed the name-clash-wrong-implicit branch from 3c07ab5 to b701344 Compare July 14, 2023 17:56
@dwijnand dwijnand force-pushed the name-clash-wrong-implicit branch from b701344 to 463ee8c Compare July 15, 2023 12:07
@dwijnand dwijnand marked this pull request as ready for review July 18, 2023 23:06
@dwijnand dwijnand assigned odersky and unassigned dwijnand Jul 18, 2023
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.

Otherwise LGTM

def preferDefinitions = isImport && !outer.isImport
def preferNamedImport = isWildcardImport && !isWildcardImport(using outer.irefCtx)

if !migrateTo3(using irefCtx) && level == outer.level && (preferDefinitions || preferNamedImport) then
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

@odersky odersky assigned dwijnand and unassigned odersky Jul 23, 2023
@dwijnand dwijnand merged commit ce1ce99 into scala:main Jul 24, 2023
@dwijnand dwijnand deleted the name-clash-wrong-implicit branch July 24, 2023 08:18
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 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.

Name clash causes wrong implicit to be selected
4 participants