Skip to content

Conversation

@tek
Copy link
Contributor

@tek tek commented Jun 24, 2017

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.

@tek
Copy link
Contributor Author

tek commented Jun 24, 2017

signed CLA

@retronym
Copy link
Member

A few tests have failed.

ok 120 - jvm/throws-annot-from-java              

# Failed test paths (this command will update checkfiles)
partest --update-check \
  /home/jenkins/workspace/scala-2.11.x-validate-test/test/files/pos/t2421c.scala \
  /home/jenkins/workspace/scala-2.11.x-validate-test/test/files/neg/quasiquotes-unliftable-not-found.scala \
  /home/jenkins/workspace/scala-2.11.x-validate-test/test/files/neg/t7289.scala \
  /home/jenkins/workspace/scala-2.11.x-validate-test/test/files/neg/t7289_status_quo.scala

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)
Copy link
Member

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.

@retronym
Copy link
Member

While we're instrumenting implicit search, let's consider if we can have a useful metric of how much work was involved (number of typedImplicit calls).

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)

@jvican
Copy link
Member

jvican commented Jun 26, 2017

While we're instrumenting implicit search, let's consider if we can have a useful metric of how much work was involved (number of typedImplicit calls).

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.

Reference: https://github.com/scalacenter/advisoryboard/blob/master/proposals/010-compiler-profiling.md

Copy link
Member

@lrytz lrytz left a 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)
Copy link
Member

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
Copy link
Member

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

@tek
Copy link
Contributor Author

tek commented Jun 27, 2017

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

@SethTisue
Copy link
Member

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

@SethTisue SethTisue added this to the WIP milestone Jun 27, 2017
@SethTisue SethTisue added the WIP label Jun 27, 2017
@tek
Copy link
Contributor Author

tek commented Jun 27, 2017

ok, I'll get going with 2.12.

@tek
Copy link
Contributor Author

tek commented Jun 27, 2017

@SethTisue applied the changes successfully to 2.12.x. Do you want me to rebase or create a new PR?

@SethTisue
Copy link
Member

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.

tek added 5 commits June 27, 2017 22:31
* 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
@tek tek changed the base branch from 2.11.x to 2.12.x June 27, 2017 21:06
@tek
Copy link
Contributor Author

tek commented Jun 27, 2017

alright, I used an old version of 2.12.x at first, and there was an actual conflict with the current HEAD, but it was easily resolved. Hope I haven't screwed anything up, but it should be fine now.

@milessabin
Copy link
Contributor

I would love to see splain's functionality rolled into the compiler directly.

@tek
Copy link
Contributor Author

tek commented Jul 4, 2017

right, then we have to reach an agreement on what parts of it should be included.
The ability to add changes more frequently than new compiler releases is also a significant aspect in my opinion.
And it would probably be sensible to have other people auditing the correctness of the generated implicit trees and type formatting.
There are already some specific cases in there, like shapeless.Record, would we want that to be included as well?

@SethTisue
Copy link
Member

closing for inactivity. we can reopen it if activity resumes

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.

6 participants