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

@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

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.

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

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

any other suggestions?

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

I am waiting for activity!

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.

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

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

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

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.

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

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

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.

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

fair point 😸

regarding your concern about integrating it deeper: One benefit of having it separated, especially with Analyzer Plugin machinery involved, is that it would make it very simple to provide highly customized error message plugins for specific libraries with access to the typer. One example of this is the builtin shapeless Record and Lazy support.

Of course there is some compromise in between. My only concern is that this PR and its predecessor have been going on for years and it would be nice to close the plugin repo and develop this in the compiler with some version already released, because it would make development significantly simpler. A flag seems a good approach, treating it as an experimental feature.

So my proposal would be 2.13.4 with -Vimplicits, 2.13.5 as default

My suspicion is that both parameter messages and parent messages weren't around (or not at that location) when I initially wrote this five years ago.
The obvious solution is to copy the missing code over to splain, but I could also refactor it slightly so the annotation message is shared with the default printer.

Caveat: I noticed that the parameter message's assembly is a bit more complex.

implemented the latter, please take a look. It's not complete though, this causes annotations to be ignored entirely for all but the first parameter in the chain.

Edit: fixed that as well.

@dwijnand I think that addresses all of the examples of information loss you quoted, do you have any others?

Sorry, I missed your question here, @tek! No, what I meant was why it was creating its own hook rather than reusing the existing hooks and having splain be an AnalysisPlugin. And, yeah, I don't think we should provide that plugin addition - at least not until it provides necessary.

The existing hooks didn't include the necessary information, iirc. If they had, the plugin would have been a lot simpler 😄

Necessity wouldn't be an argument for this feature – its intended purpose is to allow people to provide a AnalyzerPlugin for e.g. shapeless, that includes additional formatting, like the Record rules that were part of the splain plugin and have now been removed. If you consider that to be not worth the addition of a new hook, I'll remove it.

Sadly there is that potential, but I'd rather let it crash and fix the root causes that slowly sprinkle these all over.

Removed!

I'm not 100% sure, but I think unless we're convinced the messages are pointless or misleading, I dislike just removing them. I'm also thinking we should keep -Xlog-implicits as an alias ("abbreviation") of -Vimplicits, so that means (1) and (3) clash. I agree with you integrating it shouldn't block, so I'm thinking either just -Vdebug or -Vimplicits -Vdebug.

ok!

@dwijnand I think that addresses all of the examples of information loss you quoted, do you have any others?

Thanks for the quick fix! I'll re-run all the tests with the option enabled again (and actually we should just have that commit run in the PR).

Necessity wouldn't be an argument for this feature – its intended purpose is to allow people to provide a AnalyzerPlugin for e.g. shapeless, that includes additional formatting, like the Record rules that were part of the splain plugin and have now been removed. If you consider that to be not worth the addition of a new hook, I'll remove it.

As it's simple (and already implemented) I'm fine with keeping it, you've convinced me 😄

I have a patch on top of yours which applies a whole bunch of changes, which allowed me to understand the code, and try different things. I'll look to rebasing it onto your branch here and PRing your fork next week so you have a look.

we should just have that commit run in the PR

sorry, don't understand what that means. do you need me to do something there?

As it's simple (and already implemented) I'm fine with keeping it, you've convinced me smile

🙂

I have a patch on top of yours which applies a whole bunch of changes, which allowed me to understand the code, and try different things. I'll look to rebasing it onto your branch here and PRing your fork next week so you have a look.

ok cool. do you want me to squash again now?

we should just have that commit run in the PR

sorry, don't understand what that means. do you need me to do something there?

My bad, that was a quick self/team-reminder. See #9557 for an example: make a change with the flag default on, to get a CI-verified test pass commit, and then default it off in the next commit for the merge.

ok cool. do you want me to squash again now?

No, hopefully we can get my changes in and then we can squash it all up (I've been debating whether it's best to have my changes after or squashed with yours - I don't care about the commit credit, I'm just thinking if any of the two options are preferable.)

My bad, that was a quick self/team-reminder. See #9557 for an example: make a change with the flag default on, to get a CI-verified test pass commit, and then default it off in the next commit for the merge.

ahhh clever!

No, hopefully we can get my changes in and then we can squash it all up (I've been debating whether it's best to have my changes after or squashed with yours - I don't care about the commit credit, I'm just thinking if any of the two options are preferable.)

👍

so, we decided to remove the link to the docs.scala-lang.org link in favor of a better description for the option. what are the requirements for this?

scala/docs.scala-lang#1840 is the docs page, which needs an update with how I've reworked the options. Previously everything was under a single "MultiChoiceSetting", while now is broken up, which gives more lines to give some basic description, and have that docs page provide more context and nice examples.

Personally I welcome suggestions on improved description, but I'm happy if we were to ship what we have currently in the PR.

updated the doc page, please let me know what's bad!

wooooo 🥳 🎉

WOOOOOOOOOOO!

So does this close tek/splain#4?

fuck yeah!

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

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

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

who is the maintainer/reviewer of this feature?

any of us, all of us. PRs welcome

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