-
Notifications
You must be signed in to change notification settings - Fork 3.1k
implicit error formatter analyzer plugin #5958
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
implicit error formatter analyzer plugin #5958
Conversation
|
signed CLA |
|
A few tests have failed. |
| fun.tpe match { | ||
| case PolyType(tparams, restpe) if tparams.nonEmpty && sameLength(tparams, args) => | ||
| val targs = mapList(args)(treeTpe) | ||
| val ncb = NonConfBounds(pt, itree3, implicitNesting, targs, tparams) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it would be useful to capture err in this data structure, too.
|
While we're instrumenting implicit search, let's consider if we can have a useful metric of how much work was involved (number of I'm also interested to see whether we have enough extra contextual info to improve diverging implicit error messages (scala/bug#8454 / 2.12.x...retronym:ticket/8460-3) |
I'll be working on this shortly as part of the compiler performance / profiler Scala Center proposal. I'm planning to do something similar as you describe. I suggest to leave that to a future PR. |
lrytz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should discuss how much of splain we can port into the compiler, that would make these improvements available by default for everyone. Have you thought about this before?
| .headOption | ||
| .map(ImpError(_, tree, implicitNesting, param)) | ||
| .foreach(err => implicitErrors = err :: implicitErrors) | ||
| issueNormalTypeError(tree, defaultErrMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation is off
|
|
||
| def shortName(ident: String) = ident.split('.').toList.lastOption.getOrElse(ident) | ||
|
|
||
| trait ImpFailReason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd prefer non-abbreviated names here and in the classes below
|
@lrytz I moved the data classes and helpers into the companion, is that satisfactory? @retronym I hadn't taken care of diverging implicits before because the routines were too inaccessible from the perspective of a plugin, but we should definitely incorporate them now. Anyhow, I would suggest making this a separate story, unless it's a simple addition. @lrytz The analyzer plugin has precedence over the default implementation, so we could move parts of splain into the compiler as a fallback. The more noncanonical features, however, should probably not be integrated, like colors and abbreviated names. Do you have suggestions on what should be made the default? as for the test failures, those magically disappeared after having implemented the issues from your comments. |
|
I see this targets the 2.11.x branch. No further 2.11.x release are planned, so we're not accepting PRs to the 2.11.x branch (except under very unusual circumstances). |
|
ok, I'll get going with 2.12. |
|
@SethTisue applied the changes successfully to 2.12.x. Do you want me to rebase or create a new PR? |
Rebasing works. GitHub now lets you retarget a PR to a new branch, but you still have to do the rebasing part of it yourself. Feel free to give it a try if you like, or make a new PR instead if it's easier for you. |
* trait `ImpFailReason` stores implicit candidate info for regular implicit errors and nonconformant bounds errors * when `NoImplicitFoundError` is invoked, the current implicit is added to the stack as `ImpError`, or, if the current implicit is not nested, the analyzer plugins are queried for a custom error message given the implicit stack * when nonconformant bounds are detected, a `NonConfBounds` is added to the stack
|
alright, I used an old version of |
|
I would love to see splain's functionality rolled into the compiler directly. |
|
right, then we have to reach an agreement on what parts of it should be included. |
|
closing for inactivity. we can reopen it if activity resumes |
This is a port of the implicit chain caching functionality of https://github.com/tek/splain and adds two analyzer plugin hooks with which splain can inject the detailed errors into the compiler.
While resolving implicits, these changes push the involved types onto a stack each time a nested implicit resolution begins, removing successful ones afterwards and sending the complete list to the analyzer plugin in the case of an error.
Both regular implicit errors and nonconformant bounds errors are mixed in the stack.
As a second feature, found/required errors are filtered through the analyzer plugin.
See the examples in the splain readme for a visualization.
Please advise on making this conformant to the project standards.
The PR can be tested with splain by publishing locally and checking out the branch at https://github.com/tek/splain/tree/analyzer_plugin . Either run the tests or publish locally to try it with a real project.