Skip to content
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

Add -Vimplicits and -Vtype-diffs #127

Merged
merged 7 commits into from
Jul 11, 2023

Conversation

sideeffffect
Copy link
Contributor

@sideeffffect sideeffffect commented Oct 28, 2022

@DavidGregory084
Copy link
Member

Hi @sideeffffect, I've previously had feedback that folks don't like -Vimplicits or -Vtype-diffs to be enabled by default (see e.g. #38, #45). Has anything changed about the output recently?

@sideeffffect
Copy link
Contributor Author

Oh, I see.

I don't think these provide much utility in CI. How about we turn these on only in local development mode?

@sideeffffect
Copy link
Contributor Author

It's a pickle... I like them. But I don't think anything, like the output or whatever has changed since then.

According to #45 (comment) it seems like they cause issues with some tools that drop the color of the font. But that seems more like an issue with Metals and/or Bloop (and should be fixed there), not with sbt-tpolecat or -Vimplicits/-Vtype-diffs...

It would be great to get perspectives from other people and perhaps more in depth explanation why it was disliked. So far we only know that

  • it caused long output in Scala 2->3 migration (but that's a once in a lifetime situation and sbt-tpolecat as a whole is not good for that) and that
  • one person an everybody they talked to hate it

@mihaisoloi
Copy link
Contributor

@sideeffffect @DavidGregory084 I like them because when working with ZIO 1 especially when it comes to the layers, they really help in distinguishing what is missing in the type. In ZIO 2 that's not that much of an issue to be honest.

@sideeffffect
Copy link
Contributor Author

sideeffffect commented Nov 21, 2022

Thanks for the feedback @mihaisoloi
Let me stress again that I'm not insistent on having these in sbt-tpolecat. But it would be great to get input from more people, it may turn out that us fans are in the majority.

@DavidGregory084
Copy link
Member

@sideeffffect perhaps we should just add these options, but not enable them by default? That would enable those who want to add these options to easily do so.
I'd also be interested to get some feedback from @tpolecat and @DavidGoodenough as to whether they still feel the same about the output of those options. Perhaps we could use their feedback to improve the compiler output?

@sideeffffect
Copy link
Contributor Author

add these options, but not enable them by default

I'm not sure how much value that has... Anybody can just add their own scalacOptions to deviate from the default.
But let's hear some more feedback 👍

@fdietze
Copy link

fdietze commented Apr 26, 2023

I always enable those in my code-bases. Whenever I come across an implicit error, where these flags are not enabled, the error is often impossible to fix without them. I agree that they make most sense while developing.

@calvinlfer
Copy link
Contributor

My original use case for these two compiler options was when I was trying to debug shapeless derivation for a bunch of codecs. I agree that it’s not a good idea to keep them on all the time due to the problems they cause but we should allow it to be enabled as options and not as default 🙏

@DavidGregory084
Copy link
Member

DavidGregory084 commented Jul 7, 2023

What do you folks think about adding a VerboseMode which has the new verbose options from this PR in addition to the DevMode options?

Then whoever wants these options enabled by default can set ThisBuild / tpolecatDefaultOptionsMode := VerboseMode, set the env var SBT_TPOLECAT_VERBOSE, or run the tpolecatVerboseMode command to enable them interactively.

/** Print dependent missing implicits.
*/
val verboseImplicits =
verboseOption("implicits", _.isBetween(V2_13_0, V3_0_0))
Copy link
Member

Choose a reason for hiding this comment

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

I found that this was added as an alias of -Xlog-implicits for Scala 2.13.0 in #7908.

Copy link
Member

Choose a reason for hiding this comment

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

Just want to check @sideeffffect whether there was a reason you only wanted to enable this above 2.13.6? Did the behaviour change a lot? I seem to remember -Xlog-implicits used to produce a ton of output.


/** Explain errors in more detail.
*/
val explain = ScalacOption("-explain", _ >= V3_0_0)
Copy link
Member

Choose a reason for hiding this comment

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

The dotty source code says that -explain-types is deprecated in favour of -explain, which solves our explaintypes2/explaintypes3 naming problem.

@DavidGregory084
Copy link
Member

What do you folks think about adding a VerboseMode which has the new verbose options from this PR in addition to the DevMode options?

I've made this change on top of @sideeffffect's changes - does this work for everyone? Happy to merge once this goes green if so!

Copy link
Contributor Author

@sideeffffect sideeffffect left a comment

Choose a reason for hiding this comment

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

This will make it easier to have verbose errors across different Scala versions. That alone is indeed helpful.
Let's merge this.

@DavidGregory084 DavidGregory084 merged commit 57e9b17 into typelevel:main Jul 11, 2023
@DavidGregory084
Copy link
Member

Thanks @sideeffffect! Sorry for the long delay on this one

@sideeffffect sideeffffect deleted the Vimplicits branch July 11, 2023 19:13
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.

Useful missing DSL flags
5 participants