Skip to content

Name clash causes wrong implicit to be selected #18183

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

Closed
dwijnand opened this issue Jul 11, 2023 · 6 comments · Fixed by #18203
Closed

Name clash causes wrong implicit to be selected #18183

dwijnand opened this issue Jul 11, 2023 · 6 comments · Fixed by #18203
Labels
area:implicits related to implicits itype:bug
Milestone

Comments

@dwijnand
Copy link
Member

dwijnand commented Jul 11, 2023

Compiler version

3.3.0

Minimized code

class Foo

class Bar {
  implicit val foo: Foo = new Foo
}

object Test {
  def main(args: Array[String]): Unit = {
    val bar = new Bar
    import bar._
    implicit val foo: Foo = new Foo
    assert(foo == implicitly[Foo])
  }
}

Output

java.lang.AssertionError: assertion failed
	at scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:11)
	at Test$.main(Test.scala:14)
	at Test.main(Test.scala)

Expectation

I don't expect the wrong implicit to be selected.

The name clash is part of the cause here. With a different name, I expected the more locally defined implicit to win over the wildcard imported one, but that in situation they're actually ambiguous - which is not ideal but it is a little better.

Unimporting, i.e. import bar.{ foo => _, _ } is a workaround.
Also, it works correctly in Scala 2.

@dwijnand dwijnand added itype:bug area:implicits related to implicits labels Jul 11, 2023
@SethTisue
Copy link
Member

SethTisue commented Jul 12, 2023

https://docs.scala-lang.org/scala3/reference/changed-features/implicit-resolution.html says:

This section describes changes to the implicit resolution that apply both to the new givens and to the old-style implicits in Scala 3.

so it's potentially fair game for the code to behave differently than in 2.

It then goes on to say:

Nesting is now taken into account for selecting an implicit

and there is no further language about what exactly this means. There is some language describing an example consequence of the change:

The previous possibility of an implicit search failure due to shadowing (where an implicit is hidden by a nested definition) no longer applies

but this is only illustrative. So we're left with only "Nesting is now taken into account", which is realllllllllllly vague.

The language here is so vague that I don't think we can say for sure what outcome was intended here.

There are three possible outcomes:

  1. find Bar.foo
  2. find the local foo
  3. complain of ambiguity

I agree that 1 is surprising and seems wrong. I can see arguing for either 2 (the Scala 2 behavior) or 3, but I can't see what the design justification for 1 (the current behavior) could be.

@SethTisue
Copy link
Member

SethTisue commented Jul 12, 2023

Dale has found #8092, which seems to be the same issue, and where Bjorn's expectations matched mine.

Now reading through that, and #17809, to try to understand the justification. (There, we see that Michał also found the current behavior puzzling.)

cc @som-snytt

@SethTisue
Copy link
Member

SethTisue commented Jul 12, 2023

I see that #17809 remains unresolved, and that none of us seem to fully understand Martin's remark on the subject. 🤷

@som-snytt
Copy link
Contributor

per the old odersky remark, imports introduce a context, so that's why the nesting rule lends them priority.

I agree that it's a bit mind bending. I was just looking at a Scala 2 issue about imports and realized that the mechanics are quite subtle. (I mean the internal dance between namer and typer.) I think it's OK to ask the user to have a "mental model" about contexts, which are loosely associated with braces.

@odersky
Copy link
Contributor

odersky commented Jul 12, 2023

This looks indeed like a bug.

@som-snytt
Copy link
Contributor

Instead of Marshall McLuhan in the Woody Allen movie, it's odersky.

https://www.youtube.com/watch?v=9wWUc8BZgWE

image

@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
area:implicits related to implicits itype:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants