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

Make suggestions of missing implicits imports on type errors #7862

Merged
merged 27 commits into from Jan 5, 2020

Conversation

@odersky
Copy link
Contributor

odersky commented Dec 28, 2019

Add an addendum to an error message where the error might be fixed
by some implicit argument or conversion or some extension method
that is however not found. The addendum suggests implicit
imports that might fix the problem.

odersky added 3 commits Dec 28, 2019
Don't issue a "did you mean" hint if a short name gets as a hint
a completely unrelated short name.
Add an addendum to an error message where the error might be fixed
be some implicit argument or conversion or some extension method
that is however not found. The addendum suggests suggests implicit
imports that might fix the problem.
This is needed to get stability of test outputs. But we should
try to find more useful sorting criteria.
@hepin1989

This comment has been minimized.

Copy link

hepin1989 commented Dec 30, 2019

Finally see some Unison style error msg:)

odersky added 6 commits Dec 30, 2019
 - Do search in completed packages (this did not work before for nested packages)
 - Don't search in Java-defined objects
 - Don't suggest any of the root imports that are imported by default: There
   might be a deeper problem, or the import was disabled intentionally. In either
   case it makes no sense to suggest the import again.
nor for conversions to Any, AnyRef, or AnyVal.
Add an `isCancelled` variable to `Run`. If this variable is true,
`typed` and `typedImplicit` calls will return immediately.
Currently:

 Max 1 sec to test a single implicit
 Max 10 secs to make suggestions for a missing implicit import
Copy link

julienrf left a comment

I’m extremely happy to see work in this direction! ❤️

I believe we should also provide suggestions for dynamic givens: given members that can be provided by extending or instantiating a class, as opposed to importing a static value. This could be done in a separate PR, though. Should I open an issue to keep track of such a feature?

compiler/src/dotty/tools/dotc/Run.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/Implicits.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/Implicits.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/typer/Implicits.scala Outdated Show resolved Hide resolved
* be some implicit value of type `pt` that is however not found.
* The addendum suggests suggests implicit imports that might fix the problem.
*/
override def implicitSuggestionsFor(pt: Type)(given ctx: Context): String =

This comment has been minimized.

Copy link
@julienrf

julienrf Jan 1, 2020

This signature looks error-prone to me as there is no way to distinguish between the presence and the absence of a suggestion (in any case, a String is returned). What do you think of returning an Option[String] instead? Returning an empty String to model the absence of suggestion does work here by chance because this method is the latest to be called in a chain of if/else if, but we should not have to look at how this method is called to be able to reason on its correctness (this is not the concern of this method, we should be able to locally reason about it).

This comment has been minimized.

Copy link
@odersky

odersky Jan 1, 2020

Author Contributor

In fact an empty string is preferable, since it's easier to consume. Generally, the compiler always prefers sentinels over optional values, as long as feasible.

compiler/src/dotty/tools/dotc/util/Scheduler.scala Outdated Show resolved Hide resolved
tests/neg/missing-implicit.scala Show resolved Hide resolved
Also, implement other review suggestions
@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Jan 1, 2020

I believe we should also provide suggestions for dynamic givens: given members that can be provided by extending or instantiating a class, as opposed to importing a static value. This could be done in a separate PR, though. Should I open an issue to keep track of such a feature?

It would be good to see some examples of this, and brainstorm about possible scenarios how to do this.

odersky added 2 commits Jan 2, 2020
 - Also search in nested objects
 - Avoid calling `fields` since this can cause cyclic
   reference exceptions (e.g. in neg/i2006.scala).
@odersky odersky force-pushed the dotty-staging:add-suggest-givens branch from 9d67f9b to 9c67ee4 Jan 2, 2020
@odersky odersky force-pushed the dotty-staging:add-suggest-givens branch from 5153062 to b3a4966 Jan 2, 2020
Copy link

julienrf left a comment

Thanks for adding the test!

May I ask to also add the following tests?

trait X
trait Y
def f(given x: X) = ???
object instances {
  given y: Y = ???
}
locally {
  given xFromY(given y: Y): X = ???
  f // error
}
locally {
  object instances2 {
    given xFromY(given y: Y): X = ???
  }
  f // error
}
trait X[A, B]
trait Y
trait Z
given xyz: X[Y, Z] = ???
object instance {
  given z: Z = ???
}
def f[A](given xya: X[Y, A], a: A) = ???
f // error
trait X

object instance1 {
  given x: X = ???
}
object instance2 {
  given x: X = ???
}

implicitly[X] // error

I believe we should also provide suggestions for dynamic givens: given members that can be provided by extending or instantiating a class, as opposed to importing a static value.

It would be good to see some examples of this, and brainstorm about possible scenarios how to do this.

Here are some examples:

So, there seem to be two cases: either the class is the very type that the compiler is looking for (ActorSystem, Toolbox, etc.), or the class provides implicit members that the compiler is looking for (FailFastCirceSupport, etc.).

Maybe the first case is relatively easy to manage without suggestions from the compiler? If a user already knows how to instantiate that class, then he only has to make that instance a given instance, which should be easy to figure out just by getting the “no implicit argument of type T …” message.

For the second case, it is worth noting that it is a common practice for library authors to also provide a static object that extends the class providing the given members (for instance, akka-http-json also provides an object FailFastCirceSupport extends FailFastCirceSupport), which means that that object should already be mentioned in the suggestions.

So, maybe suggesting static imports is enough, after all!

-- [E008] Member Not Found Error: tests/neg/missing-implicit1.scala:18:16 ----------------------------------------------
18 | List(1, 2, 3).traverse(x => Option(x)) // error
| ^^^^^^^^^^^^^^^^^^^^^^
| value traverse is not a member of List[Int] - did you mean List[Int].reverse?

This comment has been minimized.

Copy link
@julienrf

julienrf Jan 2, 2020

This part of the error message is confusing. It should not say that traverse is not a member, but that no argument of type Zip[Option] was inferred.

This comment has been minimized.

Copy link
@odersky

odersky Jan 5, 2020

Author Contributor

I improved the first line, it now says:

16 |  List(1, 2, 3).traverse(x => Option(x)) // error
   |  ^^^^^^^^^^^^^^^^^^^^^^
   |  value traverse is not a member of List[Int], but could be made available as an extension method.
   |
   |  The following import might make progress towards fixing the problem:
   |
   |    import testObjectInstance.instances.traverseList
tests/neg/missing-implicit1.check Show resolved Hide resolved
|
| The following import might fix the problem:
|
| import concurrent.duration.pairIntToDuration

This comment has been minimized.

Copy link
@julienrf

julienrf Jan 2, 2020

I believe that eventually, we should not provide suggestions for implicit conversions (but this is currently necessary to support the implicit classes of the scala-library 2.13), only for given instances of type Conversion.

tests/neg/missing-implicit.scala Show resolved Hide resolved
odersky added 3 commits Jan 4, 2020
Distinguish between confirmed matches for which dependent implicits can be found,
and partial matches, for which these dependent implicits might require additional imports.

Show partial matches only if there are no confirmed matches.

Also, if there are multiple suggestions prioritize according to specificity.
@odersky odersky force-pushed the dotty-staging:add-suggest-givens branch from 608526b to 848dd0a Jan 4, 2020
@odersky odersky force-pushed the dotty-staging:add-suggest-givens branch from d025751 to 78f72f2 Jan 4, 2020
@odersky odersky force-pushed the dotty-staging:add-suggest-givens branch from 78f72f2 to 1124b35 Jan 4, 2020
@soronpo

This comment has been minimized.

Copy link
Contributor

soronpo commented Jan 5, 2020

Maybe a dumb question, because I don't know enough about LSP and editors, but is it possible to have this information available as a service by the front-end compiler aside from an error message, to make it easier for the editors to internally use hints instead of parsing the error messages?

odersky added 6 commits Jan 5, 2020
Also suggest direct imports of extension methods that are not
members of a given instance.
This used to work only for ViewProtos before.
@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Jan 5, 2020

@soronpo I don't know enough about LSP to answer this question.

@odersky odersky merged commit 255a538 into lampepfl:master Jan 5, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@odersky odersky deleted the dotty-staging:add-suggest-givens branch Jan 5, 2020
@gabro

This comment has been minimized.

Copy link
Contributor

gabro commented Jan 6, 2020

@odersky the LSP idiom I would use here is a Code Action: code actions can be used to provide fixes for existing errors (in which case they’re called “quickfixes” in LSP lingo) and this seems to be a perfect match.

I don’t know the status of dotty’s own LSP implementation but Metals already provides a similar action (import missing symbol) and it would make sense to implement this there, once we support Scala 3.

Is it possibile to surface this “hint” API to the presentation compiler so that external tools such as Metals can use it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.