Integrate splain – implicit resolution chains and type diffs in error messages

[tek]

This integrates https://github.com/tek/splain into the compiler

• -Vimplicits makes the compiler print implicit resolution chains when no implicit value can be found
• -Vtype-diffs turns type error messages (found: X, required: Y) into colored diffs between the two types

I copied most of splain's code into the directory src/compiler/scala/tools/nsc/typechecker/splain and changed its integration to be less intrusive. There are now compiler options like -Ysplain to control its behaviour. I kept the analyzer plugin method that allows consumers to override or amend output formatting, e.g. for providing more detailed messages about specific typeclasses with access to the typechecker and involved types.

(previous PR at #5958).

[som-snytt]

You might consider grouping the compiler flags, -Ysplain:no-color,trunc-refined etc.

[tek]

@som-snytt could you elaborate how that would work? The only tool I'm seeing that allows grouping like this is MultiChoiceSetting, but it doesn't seem very suitable for this specific case

[som-snytt]

That is, like -Xlint, except any flag would also enable -Ysplain; only the int-valued settings can't be grouped this way (although one could imagine supporting -Ysplain:myint=3 syntax).

The other possibility for avoiding a huge -Y help page would be to only show prefixes under -Y and then -Ysplain:help would show options with that prefix.

[som-snytt]

Note that you could also provide a man page for: man splain.

[tek]

ok, got it!
I hoped that specifying only -Ysplain would make setByUser true, but it's gonna have to be -Ysplain:enable.

[tek]

any other suggestions?

[julienrf]

I find this message a bit hard to interpret. I have the impression that Dotty provides a more helpful error message (see an example here). Am I right, or are there limitations in what Dotty does that are addressed by splain?

[tek]

the motivation for this notation is to be terse, concise and parsable in the face of implicit chains that are tens of levels deep. I haven't gotten any feedback on this by splain users so far, so I can't tell whether this is as useful to others as it is to me.
The test output with dummy names is certainly less readable than one where the involved names and types are recognizable, like circe.Decoder and shapeless.Generic, which is the intended use case.

One solution would certainly be to offer both verbose and terse messages, to be configured by the user.

Are you suggesting that Dotty's messages would make splain obsolete? Would that imply that it wouldn't be needed in current Scala?
I haven't looked at Dotty's implicit messages yet, does it display whole chains?

[julienrf]

Are you suggesting that Dotty's messages would make splain obsolete?
Would that imply that it wouldn't be needed in current Scala?

This is not clear yet. In any case, it’s definitely useful to work on improving error messages!

I haven't looked at Dotty's implicit messages yet, does it display whole chains?

I have posted a comparison of messages produced by Scala and Dotty here

[tek]

I will have to investigate some test cases myself.

In any case, implicit conversions are not a feature that is handled by splain explicitly, just FYI.

My question now is: do you want to make this PR about how error messages should look? So far the mission was to make splain available as-is – if this should be different, we should make this an explicit statement.

[julienrf]

My question now is: do you want to make this PR about how error messages should look? So far the mission was to make splain available as-is – if this should be different, we should make this an explicit statement.

That’s a good point. I agree that working on how error messages should look could be done in another PR.

[SethTisue]

marking as "resolved" because although further improvement would be welcome, it's not a blocker

[kubukoz]

Hi, what's the state of this PR? It's been some time since the last update :)

[tek]

I am waiting for activity!

[som-snytt]

With the new github inbox for notifications, I'll never miss another review request.

I'll get up to speed. First, I'll get up.

[SethTisue]

@lrytz we're open to eventually merging this, right? I think you and I discussed it a few weeks ago but I admit that I forgot what we said. did we have any medium-sized-or-larger reservations about it?

[julienrf]

FWIW, I haven’t had a look at the PR implementation but I think the feature itself is extremely valuable.

[lrytz]

@lrytz we're open to eventually merging this, right? I think you and I discussed it a few weeks ago but I admit that I forgot what we said. did we have any medium-sized-or-larger reservations about it?

I also don't remember :-)

My only reservation here is that the improvements live behind new compiler flags, and that the implementation is separate from the ordinary error messages and type printing. Can we integrate this deeper and make more people benefit from the helpful messages?

• for infix type printing, we have @showAsInfix - how does it relate?
• type diffs would be really good to have by default, is there a reason against it? maybe not to break IDEs?
• can the implementation of type printing be combined with what already exists in the compiler?
• is there a drawback in adding compact implicit chains by default? compiler perfromance?

[SethTisue]

My only reservation here is that the improvements live behind new compiler flags

Even if we decide that this style of output should remain under a flag, perhaps one or more normal error messages could mention the flag, to improve discoverability.

Regardless, I agree that deeper integration is better. Because passing flags is cumbersome; people tend to either turn a flag on all the time, or never.

[kubukoz]

Frankly I'd rather have it soon as a flag than wait months and get new behavior as a default in a 2.13.x version :)

[lrytz]

@kubukoz that's understandable of course :-)

However, features we add to the compiler are shipped to the entire Scala community, so changing things (even compiler flags or the style of error messages) comes with a cost, that's why things often move a bit slower.

Also, from our perspective, it's a piece of code that we'll have to maintain for a long time, so it makes sense to integrate well into our codebase. For example, showType is re-implementing functionality that exists (to some degree) in the compiler.

[tek]

I would suggest starting out with the flag, since the plugin has so far only been used by people intentionally looking for the provided functionality, and some of the edge cases in, e.g. the found/req type diffs can be slightly buggy – not to the point of significant impediment, but it could probably need a bit of bug reporting.

There's nothing keeping us from making it the default in the future, and it does feel like a lot of commitment to do it out of the gate.

[lrytz]

35% of the respondents in https://scalacenter.github.io/scala-developer-survey-2019/ say that "handling missing implicit errors" is a "main pain point in daily workflow".

[SethTisue]

(final review/merge by @lrytz?)

[lrytz]

LGTM too!

The message formatting / wording can still be improved, that would be helpful. Basically what's discussed in these comments: https://github.com/scala/scala/pull/7785/files#diff-0d00a887e46a40735249285fb63ea04a8aa3de82943535fdfdbf86106cdcb88d

[tek]

wooooo 🥳 🎉

[kubukoz]

WOOOOOOOOOOO!

[bl-ue]

So does this close tek/splain#4?

[tek]

fuck yeah!

[SethTisue]

There is some (trigger warning: somewhat grouchy) negative feedback at typelevel/sbt-tpolecat#38 and typelevel/sbt-tpolecat#45. Not sure if there's anything actionable in the sense that the feature could be improved, or if it's only actionable by telling people "this isn't intended to be enabled by default".

[tek]

well, it isn't enabled by default, so not sure what else could be said 🙂 And the issue 38 talks about -explain-types, which isn't related to splain

[tribbloid]

At this moment (Sept 2021), who is the maintainer/reviewer of this feature?

There are a few hotfixes in splain 0.5.9 that wasn't included.

Unfortunately the first patch to scala compiler takes a lot of copy and paste. IMHO, we can't afford to do this repeatedly for hotfixes.

After consulting with @tek. I may have to rebase them on this repo

[SethTisue]

who is the maintainer/reviewer of this feature?

any of us, all of us. PRs welcome

[tribbloid]

@SethTisue thanks a lot. I've talked to @tek and discovered that many config options are also omitted:

tek/splain#58

So I'll have to introduce them first. The following 2 PR should be ideally submitted in succession:

1. add the following configurations that are ingested though -P:splain:<param>[:<value>] options:

  val keyAll = "all"
  val keyImplicits = "implicits"
  val keyFoundReq = "foundreq"
  val keyInfix = "infix"
  val keyBounds = "bounds"
  val keyColor = "color"
  val keyBreakInfix = "breakinfix"
  val keyCompact = "compact"
  val keyTree = "tree"
  val keyBoundsImplicits = "boundsimplicits"
  val keyTruncRefined = "truncrefined"
  val keyRewrite = "rewrite"
  val keyKeepModules = "keepmodules"

  val analyzer =
    new { val global = SplainPluginCompat.this.global } with Analyzer {
      def featureImplicits = boolean(keyImplicits)
      def featureFoundReq = boolean(keyFoundReq)
      def featureInfix = boolean(keyInfix)
      def featureBounds = boolean(keyBounds)
      def featureColor = boolean(keyColor)
      def featureBreakInfix = int(keyBreakInfix).filterNot(_ == 0)
      def featureCompact = boolean(keyCompact)
      def featureTree = boolean(keyTree)
      def featureBoundsImplicits = boolean(keyBoundsImplicits)
      def featureTruncRefined = int(keyTruncRefined).filterNot(_ == 0)
      def featureRewrite = opt(keyRewrite, "")
      def featureKeepModules = int(keyKeepModules)
    }

(feel free to suggest alternative names)

2. backport the patch

Wonder if I can catch the 2.13.7 release deadline?