Skip to content

dotty doesn't seem to take the full context into account when resolving implicits #13974

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
bartosz-witkowski opened this issue Nov 18, 2021 · 3 comments · Fixed by #14072
Closed
Milestone

Comments

@bartosz-witkowski
Copy link

Compiler version

3.1.0 (also tested on latest master 3.1.2-RC1-bin-SNAPSHOT-nonbootstrapped-git-2b37257)

Minimized code

object Test {
  sealed abstract class Use[A]
  case class UseArray[A]() extends Use[Array[A]]

  def go[X](u: Use[X], x: X): Unit = {
    u match {
      case UseArray() =>
        x.size
    }
  }
}

Output

-- [E008] Not Found Error: /home/bartosz/work/programming/scala/scalac/test/src/main/scala/Main.scala:8:10 
8 |        x.size
  |        ^^^^^^
  |        value size is not a member of X
  |
  |        where:    X is a type in method go which is an alias of Array[A$1]

Expectation

Code compiles without errors.

@SethTisue
Copy link
Member

SethTisue commented Nov 18, 2021

alternate minimization that's more self-contained (no Array):

object Test {
  class C
  class Use[A]
  case class UseC() extends Use[C]
  class ConversionTarget
  implicit def convert(c: C): ConversionTarget = ???
  def go[X](u: Use[X], x: X) =
    u match {
      case UseC() =>
        x: ConversionTarget
    }
}

Scala 2.13.7, 2.12.15, 2.11.12, and 2.10.7 all compile this

@bartosz-witkowski
Copy link
Author

bartosz-witkowski commented Nov 30, 2021

I've tried digging deeper into what's happening and working out a solution.

Now given my limited understanding of dotty, I think the problem arises, due to the fact that when dotty is searching for eligible implicits it uses ContextualImplicits of the current Context (via Context#implicits in Implicits.searchImplcit). But the Context (ContextualImplicits#initctx) of those ContextualImplicits is only related to the place where the implicit references were introduced.

Hence, any new information (e.g ones gained due to unification in pattern matching) won't be "seen" by ContextualImplicits and won't be available for the purposes of implicit search.

I tried to test my intuition and introduced a small change that passed the current Context to ContextualImplicits and that made the minimal test case written by @SethTisue to compile.

Unfortunately, that made some illegal programs to compile on the regression suite - particularly tests pertaining to ambiguous implicits - but that seemed to be salvageable by first testing for ambiguous implicits and only then trying to look for implicitis using the full current Context. I have pushed my changes to branch on a forked repository: https://github.com/bartosz-witkowski/dotty/tree/wip/use-full-context-to-resolve-implicits

Right now, only tests/neg/gadt-approximation-interaction.scala fails to compile - but maybe that doesn't seem to be too problematic seeing that the test was written assuming that it might break in that particular place one day.

What worries me, is that using the full Context the way I did might introduce some other type checking errors, or introduce resource leaks (I am not sure that Contexts should be passed around so liberally as with my changes). Advice on how - and if - to proceed would be appreciated. At the very least I hope that this investigation might at least point somebody more knowledgeable to a better solution.

odersky added a commit to dotty-staging/dotty that referenced this issue Dec 8, 2021
Under a non-empty GADT constraint, bypass caches when computing eligible implicits.
The GADT constraint might influence what implicits are available locally, so cached
results would be unreliable.

Fixes scala#13974
@odersky
Copy link
Contributor

odersky commented Dec 8, 2021

@bartosz-witkowski Thanks for the analysis., which helped to come up with the fix in #14072.

odersky added a commit to dotty-staging/dotty that referenced this issue Dec 8, 2021
Under a non-empty GADT constraint, bypass caches when computing eligible implicits.
The GADT constraint might influence what implicits are available locally, so cached
results would be unreliable.

Fixes scala#13974
olsdavis pushed a commit to olsdavis/dotty that referenced this issue Apr 4, 2022
Under a non-empty GADT constraint, bypass caches when computing eligible implicits.
The GADT constraint might influence what implicits are available locally, so cached
results would be unreliable.

Fixes scala#13974
@Kordyjan Kordyjan added this to the 3.1.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants