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

False positive warning for new implicit resolution rules in 3.5.0-RC3 #21036

Closed
lenguyenthanh opened this issue Jul 5, 2024 · 11 comments · Fixed by #21045
Closed

False positive warning for new implicit resolution rules in 3.5.0-RC3 #21036

lenguyenthanh opened this issue Jul 5, 2024 · 11 comments · Fixed by #21045
Assignees
Labels
area:implicits related to implicits area:reporting Error reporting including formatting, implicit suggestions, etc area:typer itype:bug prio:high regression This worked in a previous version but doesn't anymore

Comments

@lenguyenthanh
Copy link

lenguyenthanh commented Jul 5, 2024

Compiler version

3.5.0-RC3

Minimized code

//> using scala 3.5.0-RC3
// //> using options -source:3.6-migration

trait SameRuntime[A, B]
trait BSONWriter[T]
trait BSONHandler[T] extends BSONWriter[T]

opaque type Id = String
object Id:
  given SameRuntime[Id, String] = ???

given BSONHandler[String]                    = ???
given [T: BSONHandler]: BSONHandler[List[T]] = ???

given opaqueWriter[T, A](using
    rs: SameRuntime[T, A],
    writer: BSONWriter[A]
): BSONWriter[T] = ???

def write[A](a: A)(using writer: BSONWriter[A]): Unit = ???

val ids: List[Id] = ???

val x = write(ids)

Output

[warn] ./ImplicitResolution2.scala:24:19
[warn] Given search preference for BSONWriter[List[Id]] between alternatives (given_BSONHandler_List :
[warn]   [T](implicit evidence$1: BSONHandler[T]): BSONHandler[List[T]]) and (opaqueWriter :
[warn]   [T, A](using rs: SameRuntime[T, A], writer: BSONWriter[A]): BSONWriter[T]) will change
[warn] Current choice           : the first alternative
[warn] New choice from Scala 3.6: none - it's ambiguous
[warn] val x = write(ids)
[warn]                   ^
Compiled project (Scala 3.5.0-RC3, JVM (21))

Expectation

It shouldn't emit any warnings, as there is no implicit evidence for the second case. Also if we enabled //> using options -source:3.6-migration it works perfectly.

Related discussions: #19300 and #20484

@lenguyenthanh lenguyenthanh added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Jul 5, 2024
@lenguyenthanh lenguyenthanh changed the title False positive warning in 3.5.0-RC3 False positive warning for new implicit resolution rules in 3.5.0-RC3 Jul 5, 2024
@lenguyenthanh
Copy link
Author

lenguyenthanh commented Jul 5, 2024

trying to minimize a bit further and found out this desn't emit false warning:
oops, it actually does emit warning I summoned the wrong type 🤦

//> using scala 3.5.0-RC3

trait SameRuntime[A, B]
trait BSONWriter[T]
trait BSONHandler[T] extends BSONWriter[T]

opaque type Id = String
object Id:
  given SameRuntime[Id, String] = ???

given BSONHandler[String]                    = ???
given [T: BSONHandler]: BSONHandler[List[T]] = ???

given opaqueWriter[T, A](using rs: SameRuntime[T, A], writer: BSONWriter[A]): BSONWriter[T] = ???

val x = summon[BSONHandler[List[Id]]] // this doesn't emit warning
val y = summon[BSONWriter[List[Id]]] // this does emit warning

@Gedochao
Copy link
Contributor

Gedochao commented Jul 5, 2024

The reproduction from the first post emits the warning on current main (f2829c3), 3.5.0-RC3 and 3.5.0-RC2.
3.5.0-RC1 and earlier versions are unaffected (no warning is emitted).

@Gedochao Gedochao added area:reporting Error reporting including formatting, implicit suggestions, etc area:implicits related to implicits regression This worked in a previous version but doesn't anymore and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jul 5, 2024
@Gedochao
Copy link
Contributor

Gedochao commented Jul 5, 2024

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Jul 5, 2024

Bisect points to bc26c51 so it's basically since the introduction of #19300 before 3.5.0-RC2 it was hidden, because compiler used -source:3.4 as default one. The issue exists only with -source:3.5 which is default since 3.5.0-RC2. Current main uses -source:3.6

@odersky
Copy link
Contributor

odersky commented Jul 5, 2024

Isn't that a minimization of one of the known failures in reactive mongo. In that case the warning would be correct: implicit resolution will change. I'll verify that this is indeed the case.

@lenguyenthanh
Copy link
Author

Isn't that a minimization of one of the known failures in reactive mongo. In that case the warning would be correct: implicit resolution will change. I'll verify that this is indeed the case.

No, it's not the case, If I compile this with //> using options -source:3.6-migration, it works perfectly fine. And by using //> using options -Xprint:typer, I can verify it resolves exactly the same result with and without //> using options -source:3.6-migration.

@EugeneFlesselle
Copy link
Contributor

EugeneFlesselle commented Jul 5, 2024

Minimization:

//> using options -source:3.5

given Int = 1

trait A[T]
trait B[T] extends A[T]

given b[T](using Int): B[List[T]] = ???
given a[T](using String): A[T] = ???

val x = summon[B[List[String]]] // no warning
val y = summon[A[List[String]]] // warning

Note the order of declarations of a and b is relevant.
If b is removed, then y yields a no given was found error.

@odersky
Copy link
Contributor

odersky commented Jul 5, 2024

I think I found it. We call compareAlternatives in disambiguate, to drop more pending candidates. In that case, we should issue a warning only if the pending candidate is in the final result, not the candidate that was found so far.

@som-snytt
Copy link
Contributor

also #20572

odersky added a commit to dotty-staging/dotty that referenced this issue Jul 5, 2024
odersky added a commit to dotty-staging/dotty that referenced this issue Jul 5, 2024
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this issue Jul 5, 2024
@odersky odersky closed this as completed in c9b9ad4 Jul 8, 2024
odersky added a commit that referenced this issue Jul 8, 2024
WojciechMazur pushed a commit to WojciechMazur/dotty that referenced this issue Jul 10, 2024
@OndrejSpanel
Copy link
Member

For the record: I have experienced this warning with RC3 in my project using borer library. Same as here, the warning disappeared when using -source:3.6-migration, it also disappeared in RC4.

There were many warnings with RC3, some contained implicit evidence in both alternatives, some not:

[warn]     |Given search preference for io.bullet.borer.Encoder[List[XXXXX]] between alternatives (io.bullet.borer.Encoder.forLinearSeq :
[warn]     |  [T, M[X] <: scala.collection.LinearSeq[X]]
[warn]     |    (implicit evidence$5: io.bullet.borer.Encoder[T]):
[warn]     |      io.bullet.borer.Encoder.DefaultValueAware[M[T]]
[warn]     |) and (io.bullet.borer.Encoder.given_Encoder_T :
[warn]     |  [T](using ev: io.bullet.borer.Codec.All[T]): io.bullet.borer.Encoder[T]) will change
[warn]     |Current choice           : the first alternative
[warn]     |New choice from Scala 3.6: none - it's ambiguous
[warn]    |Given search preference for io.bullet.borer.Decoder[Option[Double]] between alternatives (io.bullet.borer.Decoder.forOption :
[warn]    |  [T]
[warn]    |    (implicit evidence$4: io.bullet.borer.Decoder[T]):
[warn]    |      io.bullet.borer.Decoder.DefaultValueAware[Option[T]]
[warn]    |) and (io.bullet.borer.Decoder.fromFactory :
[warn]    |  [T, M[_$11]]
[warn]    |    (implicit evidence$5: io.bullet.borer.Decoder[T], factory:
[warn]    |      scala.collection.Factory[T, M[T]]): io.bullet.borer.Decoder[M[T]]
[warn]    |) will change
[warn]    |Current choice           : the first alternative
[warn]    |New choice from Scala 3.6: none - it's ambiguous

Unless proven otherwise, I assume all those warnings were false, caused by this RC3 issue.

@odersky
Copy link
Contributor

odersky commented Jul 12, 2024

If the warning disappeared in RC4, it definitely was a false positive.

WojciechMazur pushed a commit to WojciechMazur/dotty that referenced this issue Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:implicits related to implicits area:reporting Error reporting including formatting, implicit suggestions, etc area:typer itype:bug prio:high regression This worked in a previous version but doesn't anymore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants