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

prolativ · 2020-10-13T16:22:34Z

No description provided.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

liufengyun

Thanks @prolativ . I left a small comment to add more tests.

liufengyun · 2020-10-16T17:10:34Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+                  s"Functions will no longer work as implicit conversions in Scala 3. Use an implicit parameter of Conversion type instead.",
+                  ctx.source.atSpan(span)
+                )(using irefCtx)
+


A potential issue: how can we know that the symbol is defined in the current source file?

Could you create a test that involves two separately compiled files? Here is an example on how to organize the tests: https://github.com/lampepfl/dotty/tree/master/tests/neg/inlineAccess

Actually we can't know that and I realised that when one of the community build tests failed. I simply found this line working in some other place and used it because I was not aware that the source position could be taken directly from a symbol.

Now I'm trying to figure out how the message should actually be displayed so that it's understandable for a user what should be changed in the code to fix the issue. If the conversion is defined within the user's code that should be quite straightforward - we warn on the line with the definition (which is either in the file where the conversion is applied or another). But if the conversion was defined in an external library we might get no source position at all. And even if we got the position from an external library, one normally has no possibility to modify it so to remove the warning message one would have to apply the conversion explicitly in their code (which might be quite problematic when e.g. the code heavily relies on some DSL) and in such a case it would make sense to show the warning at the point of conversion application. But this would introduce a not very nice asymmetry with the case when we get the source position of the definition.

Currently I'm investigating into the case when conversions are derived by chaining implicits because it looks like the source position might not be available then even if everything is defined within the same codebase.

According to #9935 , the policy has changed:

Do not warn if an old-style conversion is used

So, maybe it's fine to not issue a warning here?

After reading that PR I'm a bit confused what is understood as "old-style conversion".
There seem to be 3 ways to implicitly convert a value of type A to a value of type B:

/* 1 */ implicit def a2b(a: A): B = ??? /* 2 */ implicit val a2b: A => B = ??? /* 3 */ implicit val a2b: Conversion[A, B] = ???

According to the latest documentation https://dotty.epfl.ch/docs/reference/changed-features/implicit-conversions-spec.html in Scala 3.0 the case 2 will not work anymore unless compiled with -source:3.0-migration flag. #9935 refers to the case 1 as old-style implicit def conversion. Does this mean it has been already decided that the case 1 will be deprecated soon? I've seen the discussion here https://contributors.scala-lang.org/t/can-we-wean-scala-off-implicit-conversions/4388 but it doesn't seems to reach any final conclusions. Anyway my changes are related specifically to -source:3.0-migration flag. #9935 limits an overwhelming number of warnings on a feature which is potentially dangerous but in most cases will work as expected. My changes are supposed to warn on things that would normally stop compiling in Scala 3 to smoothen the migration process. So I assume users using the migration flag would like to get these warnings.

To make the warnings consistent no matter which of the cases mentioned in the previous comment occurs, I might try to consistently use the source position of implicit conversion application and include the fully qualified name of the val/def working as the conversion. Then the user will have a choice to either update the definition of the conversion or make the conversion explicit.

Thanks for the detailed analysis, @prolativ , it makes perfect sense 👍

BTW, the method Symbol.isDefinedInCurrentRun might be useful.

Warnings are now consistently reported in the place of implicit conversions applications.
Proper test cases were added.

liufengyun · 2020-10-21T11:55:28Z

tests/neg-custom-args/fatal-warnings/i9408a.check

+-- 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.


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

Actually I was quite interested in checking that

the proper error position is marked

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?

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

Leaving the check file then

liufengyun · 2020-10-21T12:08:18Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+        tpe match {
+          case PolyType(_, resType) =>
+            isOldStyleFunctionConversion(resType)
+          case _ => tpe.derivesFrom(defn.FunctionClass(1)) && !tpe.derivesFrom(defn.ConversionClass) && !tpe.derivesFrom(defn.SubTypeClass)


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

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 }

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.

ok, so this case is already covered as stated above

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

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

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.

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

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.

Thanks, it looks good to me now.

liufengyun · 2020-10-21T12:08:51Z

tests/neg-custom-args/fatal-warnings/i9408b/Conv.scala

+package test.conversions
+
+import language.`3.0-migration`
+import scala.language.implicitConversions


Suggested change

import scala.language.implicitConversions

liufengyun · 2020-10-21T12:09:07Z

tests/neg-custom-args/fatal-warnings/i9408b/Conv.scala

@@ -0,0 +1,8 @@
+package test.conversions
+
+import language.`3.0-migration`


Suggested change

import language.`3.0-migration`

liufengyun · 2020-10-21T12:10:13Z

tests/neg-custom-args/fatal-warnings/i9408b/Conv.scala

+
+object Conv {
+  implicit val implicitLength: String => Int = _.length
+}


BTW, if the intent is to test separate compilation, we need the suffix Conv_1.scala and Test_2.scala.

I mainly wanted to check how a reference to a definition in a different package will be displayed in a warning message but checking that splitting compilation into multiple phases doesn't break anything might be a good idea as well.

liufengyun · 2020-10-21T14:32:22Z

tests/neg-custom-args/fatal-warnings/i9408a.scala

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


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

…tion

liufengyun

LGTM

dottybot reviewed Oct 13, 2020

View reviewed changes

liufengyun reviewed Oct 16, 2020

View reviewed changes

prolativ force-pushed the warn-on-implicit-conversion-with-migration-flag branch 4 times, most recently from e020713 to 9c95bfb Compare October 21, 2020 09:22

liufengyun reviewed Oct 21, 2020

View reviewed changes

prolativ force-pushed the warn-on-implicit-conversion-with-migration-flag branch 2 times, most recently from d451105 to db1e4c6 Compare October 21, 2020 14:06

liufengyun reviewed Oct 21, 2020

View reviewed changes

prolativ force-pushed the warn-on-implicit-conversion-with-migration-flag branch from db1e4c6 to 0620dc1 Compare October 21, 2020 14:53

Fix scala#9408: Warn on implicit view resolved with -source:3.0-migra…

d52332e

…tion

prolativ force-pushed the warn-on-implicit-conversion-with-migration-flag branch from 0620dc1 to d52332e Compare October 21, 2020 15:54

liufengyun approved these changes Oct 21, 2020

View reviewed changes

liufengyun merged commit 97da3cb into scala:master Oct 21, 2020

rjolly mentioned this pull request Nov 11, 2020

Over-zealous warning about unintended implicit conversion #10259

Closed

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

prolativ commented Oct 13, 2020

dottybot left a comment

liufengyun left a comment

liufengyun Oct 16, 2020

prolativ Oct 19, 2020 •

edited

Loading

liufengyun Oct 19, 2020

prolativ Oct 19, 2020

liufengyun Oct 20, 2020

prolativ Oct 21, 2020

liufengyun Oct 21, 2020

prolativ Oct 21, 2020 •

edited

Loading

liufengyun Oct 21, 2020

prolativ Oct 21, 2020

liufengyun Oct 21, 2020

prolativ Oct 21, 2020

liufengyun Oct 21, 2020

prolativ Oct 21, 2020

liufengyun Oct 21, 2020

prolativ Oct 21, 2020

prolativ Oct 21, 2020

liufengyun Oct 21, 2020

prolativ Oct 21, 2020

liufengyun Oct 21, 2020

liufengyun Oct 21, 2020

liufengyun Oct 21, 2020

liufengyun Oct 21, 2020

prolativ Oct 21, 2020

liufengyun Oct 21, 2020

liufengyun left a comment

		@@ -0,0 +1,8 @@
		package test.conversions

		import language.`3.0-migration`

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

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

Conversation

prolativ commented Oct 13, 2020

dottybot left a comment

Choose a reason for hiding this comment

liufengyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolativ Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolativ Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun left a comment

Choose a reason for hiding this comment

prolativ Oct 19, 2020 •

edited

Loading

prolativ Oct 21, 2020 •

edited

Loading