Skip to content

Make suggestions of missing implicits imports on type errors #7862

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
merged 27 commits into from
Jan 5, 2020

Conversation

odersky
Copy link
Contributor

@odersky 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.

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.
@He-Pin
Copy link
Contributor

He-Pin commented Dec 30, 2019

Finally see some Unison style error msg:)

 - 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
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

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?

* 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 =
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Also, implement other review suggestions
@odersky
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.

 - 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 add-suggest-givens branch from 9d67f9b to 9c67ee4 Compare January 2, 2020 12:54
@odersky odersky force-pushed the add-suggest-givens branch from 5153062 to b3a4966 Compare January 2, 2020 14:08
Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

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?
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

|
| The following import might fix the problem:
|
| import concurrent.duration.pairIntToDuration
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 add-suggest-givens branch from 608526b to 848dd0a Compare January 4, 2020 17:38
@odersky odersky force-pushed the add-suggest-givens branch from d025751 to 78f72f2 Compare January 4, 2020 21:43
@odersky odersky force-pushed the add-suggest-givens branch from 78f72f2 to 1124b35 Compare January 4, 2020 21:46
@soronpo
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
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 scala:master Jan 5, 2020
@odersky odersky deleted the add-suggest-givens branch January 5, 2020 17:32
@gabro
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?

odersky added a commit that referenced this pull request Jan 13, 2023
Extends [`b848c57`
(#7862)](b848c57)
to all packages.

Previously when asking for `dotty.tools.dotc.util.<init>` we'd get a
TypeError because both
`LinearMap$package`'s and `LinearSet$package`'s constructors have the
same modification time (in the jar).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants