Skip to content

Fix #9408: Warn on implicit view resolved with -source:3.0-migration #9998

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,27 @@ trait Implicits:
SelectionProto(name, memberProto, compat, privateOK = false)
case tp => tp
}
try inferImplicit(adjust(to), from, from.span)

def isOldStyleFunctionConversion(tpe: Type): Boolean =
tpe match {
case PolyType(_, resType) => isOldStyleFunctionConversion(resType)
case _ => tpe.derivesFrom(defn.FunctionClass(1)) && !tpe.derivesFrom(defn.ConversionClass) && !tpe.derivesFrom(defn.SubTypeClass)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the code does not handle the following cases:

implicit def implicitLength[A](implicit a: A): String => Int = _.length
implicit def implicitLength(implicit a: Dummy): String => Int = _.length

The following snippet might be useful:

tpe match {
  case tp: MethodOrPoly => isOldStyleFunctionConversion(tp.finalResultType)
}

It would be good to add a test case:

def foo()(implicit x: String => Int) = ???
implicit val f: String => Int = _.size
foo()  // no warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed quite surprising but in both cases below

implicit def implicitLength[A](implicit a: A): String => Int = _.length
implicit def implicitLength(implicit a: Dummy): String => Int = _.length

implicitLength does NOT work as an implicit conversion but at least this behaviour is consistent between dotty and scala 2.13

Regarding

def foo()(implicit x: String => Int) = ???
implicit val f: String => Int = _.size
foo()  // no warning

which use case exactly is it going to check? If it's just to verify that no waring is raised if a function is passed as an implicit parameter but it's not used as an implicit conversion then this is already covered here

object Test8 {
  implicit def a2int[A](a: A)(implicit ev: A => Int): Int = ev(a) // ok
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implicitLength does NOT work as an implicit conversion but at least this behaviour is consistent between dotty and scala 2.13

Thanks, that is good to know.

which use case exactly is it going to check?

It's intended to check that an implicit function is used as an implicit argument instead of implicit conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so this case is already covered as stated above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor issue with the coverage: in Test8, ev is used explicitly. The test I suggested uses it implicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say in your example ev is finally not used at all. But I added that case anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that case tp: MethodOrPoly => isOldStyleFunctionConversion(tp.finalResultType) is wrong and it caused bootstrapped tests to fail. I reverted this change and added a new test case for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to figure out the cause. What about the following test case:

  implicit def mySeq2seq[A]: Seq[A] = ???
  def foo(implicit ev: Seq[Int]): Unit = ???
  foo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like tp.finalResultType simply goes too far. In the test case

object Tes14 {
  case class MySeq[A](underlying: Seq[A])

  implicit def mySeq2seq[A](mySeq: MySeq[A]): Seq[A] = mySeq.underlying
  val s: Seq[Int] = MySeq(Seq(1, 2, 3)) // ok
}

mySeq2seq is a method so it shouldn't be reported as deprecated conversion and in this case case PolyType(_, resType), resType is a MethodType while tp.finalResultType would return the final return type of that method, which is Seq[A] and which happens to be a subtype of Function1 so isOldStyleFunctionConversion finally returns true.

The test case you're suggesting now succeeds for both implementation variants.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks good to me now.

}

try
val inferred = inferImplicit(adjust(to), from, from.span)

inferred match {
case SearchSuccess(_, ref, _) =>
if isOldStyleFunctionConversion(ref.underlying) then
report.migrationWarning(
i"The conversion ${ref} will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.",
from
)
case _ =>
}

inferred
catch {
case ex: AssertionError =>
implicits.println(s"view $from ==> $to")
Expand Down
24 changes: 24 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i9408a.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
-- Error: tests/neg-custom-args/fatal-warnings/i9408a.scala:16:20 ------------------------------------------------------
16 | val length: Int = "qwerty" // error
| ^^^^^^^^
|The conversion (Test3.implicitLength : String => Int) will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.
-- Error: tests/neg-custom-args/fatal-warnings/i9408a.scala:21:20 ------------------------------------------------------
21 | val length: Int = "qwerty" // error
| ^^^^^^^^
|The conversion (Test4.implicitLength : => String => Int) will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.
-- Error: tests/neg-custom-args/fatal-warnings/i9408a.scala:26:20 ------------------------------------------------------
26 | val length: Int = "qwerty" // error
| ^^^^^^^^
|The conversion (Test5.implicitLength : [A] => String => Int) will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.
-- Error: tests/neg-custom-args/fatal-warnings/i9408a.scala:31:20 ------------------------------------------------------
31 | val length: Int = "qwerty" // error
| ^^^^^^^^
|The conversion (Test6.implicitLength : Map[String, Int]) will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.
-- Error: tests/neg-custom-args/fatal-warnings/i9408a.scala:35:60 ------------------------------------------------------
35 | implicit def a2int[A](a: A)(implicit ev: A => Int): Int = a // error
| ^
|The conversion (ev : A => Int) will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.
-- Error: tests/neg-custom-args/fatal-warnings/i9408a.scala:59:2 -------------------------------------------------------
59 | 123.foo // error
| ^^^
|The conversion (Test11.a2foo : [A] => A => Test11.Foo) will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove the check file, it will incur efforts to maintain if we change the error message.

Copy link
Contributor Author

@prolativ prolativ Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I was quite interested in checking that

  1. the proper error position is marked
  2. the conversion is displayed in an understandable way in the context of the line marked with an error
    because I had some problems with these in the previous implementation.

Should I then split this file to test displaying the message separately and keep a much smaller check file just for testing this part of functionality or should I delete the check file entirely anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's intended, then it's fine to keep the check file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the check file then

86 changes: 86 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i9408a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import language.`3.0-migration`
import scala.language.implicitConversions

object Test1 {
implicit def implicitLength(s: String): Int = s.length
val length: Int = "qwerty" // ok
}

object Test2 {
implicit val implicitLength: Conversion[String, Int] = _.length
val length: Int = "qwerty" // ok
}

object Test3 {
implicit val implicitLength: String => Int = _.length
val length: Int = "qwerty" // error
}

object Test4 {
implicit def implicitLength: String => Int = _.length
val length: Int = "qwerty" // error
}

object Test5 {
implicit def implicitLength[A]: String => Int = _.length
val length: Int = "qwerty" // error
}

object Test6 {
implicit val implicitLength: Map[String, Int] = Map("qwerty" -> 6)
val length: Int = "qwerty" // error
}

object Test7 {
implicit def a2int[A](a: A)(implicit ev: A => Int): Int = a // error
}

object Test8 {
implicit def a2int[A](a: A)(implicit ev: A => Int): Int = ev(a) // ok
}

object Test9 {
implicit def a2int[A](a: A)(implicit ev: A <:< Int): Int = a // ok
}

object Test10 {
trait Foo {
def foo = "foo"
}
implicit def a2foo[A](a: A): Foo = new Foo {}
123.foo // ok
}

object Test11 {
trait Foo {
def foo = "foo"
}
implicit def a2foo[A]: A => Foo = _ => new Foo {}
123.foo // error
}

object Test12 {
implicit class FooOps(a: Any) {
def foo = "foo"
}
123.foo // ok
}

object Test13 {
def foo()(implicit x: String => Int) = ???
implicit val f: String => Int = _.size
foo() // ok
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here f is used as a normal implicit argument, therefore no warnings are expected.

}

object Test14 {
case class MySeq[A](underlying: Seq[A])

implicit def mySeq2seq[A](mySeq: MySeq[A]): Seq[A] = mySeq.underlying
val s: Seq[Int] = MySeq(Seq(1, 2, 3)) // ok
}

object Test15 {
implicit def implicitSeq[A]: Seq[A] = ???
def foo(implicit ev: Seq[Int]): Unit = ???
foo
}
5 changes: 5 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i9408b.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@

-- Error: tests/neg-custom-args/fatal-warnings/i9408b/Test_2.scala:6:20 ------------------------------------------------
6 | val length: Int = "abc" // error
| ^^^^^
|The conversion (test.conversions.Conv.implicitLength : String => Int) will not be applied implicitly here in Scala 3 because only implicit methods and instances of Conversion class will continue to work as implicit views.
5 changes: 5 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i9408b/Conv_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package test.conversions

object Conv {
implicit val implicitLength: String => Int = _.length
}
7 changes: 7 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i9408b/Test_2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import language.`3.0-migration`
import scala.language.implicitConversions

object Test {
import test.conversions.Conv._
val length: Int = "abc" // error
}