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

Given leaked in outer scope #20071

Closed
nicolasstucki opened this issue Apr 2, 2024 · 7 comments · Fixed by #20077
Closed

Given leaked in outer scope #20071

nicolasstucki opened this issue Apr 2, 2024 · 7 comments · Fixed by #20077
Assignees
Labels
area:implicits related to implicits itype:bug
Milestone

Comments

@nicolasstucki
Copy link
Contributor

Compiler version

3.5.0-RC1-bin-SNAPSHOT

Minimized code

trait Decoder
object Decoder:
  given foo: Int = ???

type DecoderToInt[Why] >: Int <: Int

def bar[T](using d: DecoderToInt[T]): Any = ???
def test: Unit = bar[Decoder]

Output

Compiles (but should not)

Expectation

Should not compile. foo should not be available in test witouth an import.

@nicolasstucki nicolasstucki added itype:bug area:implicits related to implicits labels Apr 2, 2024
@nicolasstucki
Copy link
Contributor Author

Annother test case

//  decoder_1.scala
trait Decoder
object Decoder:
  given foo: Int = ???

def bar(using d: M[Decoder]): Any = ???

type M[Y] = Y match
  case Decoder => Int
// test_2.scala
// should fail, does when compiling at the same time as decoder_2.scala
def test: Unit = bar

@nicolasstucki
Copy link
Contributor Author

This is a minimization from tests/pos/i15183 when compiling the two files at the same time.

@odersky
Copy link
Contributor

odersky commented Apr 3, 2024

Bar[Decoder] has type

(using d: DecoderToInt[Decoder]): Any

Decoder appears in that type, so it is in the implicit scope.

I think the issue has rather to do with match types. Sometimes they are resolved sometimes not, and the implicit scope is different for the two versions.

@odersky
Copy link
Contributor

odersky commented Apr 3, 2024

I can confirm, that's excactly what happens. decoder and test are run together, M[Decoder] is simplified to Int, so Decoder does not reduce to the implicit scope. But if we compile test_2 separately, we still see an M[Decoder].

EugeneFlesselle added a commit to dotty-staging/dotty that referenced this issue Apr 3, 2024
This is necessary to ensure the implicit scope is consistent when involving match types, since they may or may not have been reduced before implicit search.
We can for example get different results when loading from tasty than when in the same run.

Fixes scala#20071
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this issue Apr 3, 2024
This is necessary to ensure the implicit scope is consistent when involving match types, since they may or may not have been reduced before implicit search.
We can for example get different results when loading from tasty than when in the same run.

Fixes scala#20071
odersky added a commit to dotty-staging/dotty that referenced this issue Apr 3, 2024
We now try to reduce match types before computing their contributions to an implicit scope.
This avoids problems where joint and separate compilations gave different results, as in
scala#20071 and scala#15183.

Background: If a match type is reducible to a type R the compiler is free to replace the
match type with R. That should not affect the implicit scope computation. Consequently,
we have to try to reduce match types before computing their contributions to an implicit scope.

scala#20071 and scala#15183 are really the same problem. Both used to compile in sequence and both
gave an implicit not found error when two files were compiled together.

In scala#15183 a weird match type was constructed intentionally, in order to avoid an otherwise
necessary given import. That exploited a bug in the compiler which this PR fixes.
odersky added a commit to dotty-staging/dotty that referenced this issue Apr 3, 2024
We now try to reduce match types before computing their contributions to an implicit scope.
This avoids problems where joint and separate compilations gave different results, as in
scala#20071 and scala#15183.

Background: If a match type is reducible to a type R the compiler is free to replace the
match type with R. That should not affect the implicit scope computation. Consequently,
we have to try to reduce match types before computing their contributions to an implicit scope.

scala#20071 and scala#15183 are really the same problem. Both used to compile in sequence and both
gave an implicit not found error when two files were compiled together.

In scala#15183 a weird match type was constructed intentionally, in order to avoid an otherwise
necessary given import. That exploited a bug in the compiler which this PR fixes.
@EugeneFlesselle
Copy link
Contributor

EugeneFlesselle commented Apr 3, 2024

A related minimisation, where inference rather than implicit search, is impacted by whether or not the match type is reduced.

type MyMap[X] <: Any = X match
  case Double => Int

def myMap[T]: MyMap[T] = ???
def derived(d: MyMap[Double]): Any = ???

val der = derived(myMap)
-- [E007] Type Mismatch Error: tests/pos/i15183b.scala:10:18 -----------------------------------------------------------
10 |val der = derived(myMap)
   |                  ^^^^^
   |                  Found:    MyMap[T]
   |                  Required: Int
   |
   |                  where:    T is a type variable
   |
   |
   |                  Note: a match type could not be fully reduced:
   |
   |                    trying to reduce  MyMap[T]
   |                    failed since selector T
   |                    does not match  case Double => Int
   |                    and cannot be shown to be disjoint from it either.
   |
   | longer explanation available when compiling with `-explain`

But the same code is accepted when moving der into a separate file and compiling with a classpath dependency.

@odersky
Copy link
Contributor

odersky commented Apr 7, 2024

Not sure what to do about this second example.

@odersky odersky assigned EugeneFlesselle and unassigned odersky Apr 7, 2024
@EugeneFlesselle
Copy link
Contributor

EugeneFlesselle commented Apr 7, 2024

Not sure what to do about this second example.

Yes indeed, contrarily to the implicit scope situation where making both cases consistently fail was good enough.
The same canot be said here, doing compareAppliedTypes only on normalised types not only makes the TypeComparer much slower, but also breaks many use cases relying on the inference of match type scrutinees.

This also might be related to #18261

@odersky odersky closed this as completed in 90c3fbd Apr 8, 2024
EugeneFlesselle added a commit that referenced this issue Apr 9, 2024
Both observe different behaviours match type reductions
depending on whether they are compiled together or separately.
They both compile only with separate compilation.
EugeneFlesselle added a commit that referenced this issue Apr 9, 2024
This already wasn't the case for unpickled match types,
which caused varying results for `ImplicitRunInfo#isAnchor`,
by not reaching the `isMatchAlias` condition.

Ensures both #20071 and #20136 each have the same result,
when compiled with a classpath dependency as when merged.
Note that they both still fail (20071 compiles but shouldn't),
but at least do so consistently.

Also update TreeUnpickler MATCHtpt doc to align with changes from #19871

Co-authored-by: Guillaume Martres <smarter@ubuntu.com>
odersky added a commit that referenced this issue Apr 10, 2024
Match types are already not flagged as `Deferred` when unpickled.
This caused varying results for `ImplicitRunInfo#isAnchor`, by not
reaching the `isMatchAlias` condition when created from the Namer.

Ensures both #20071 and #20136 each have the same result, when compiled
with a classpath dependency as when merged into a single file.

Fixes #20136
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment