Skip to content

Conversation

@som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jan 25, 2019

Add standard option -Werror with -Xfatal-warnings alias.
As with javac, it appears on the -help output.

Rename -Ywarn-foo-bar as -Wfoo-bar, with the previous
name as a abbreviation or alias.

-W shows warnings, including all the -Xlint and -Wunused.

Use underscore to mean all phases

Move option note to bottom of message

-V for verbose nee debug options

Former flags future and experimental are more former.

Move output options to -V. Options which modify output
remain -Y. -Yprint-trees:text,compact,format,text+format
replaces -Yshow-trees-stringified, et al.

As rashly proposed.

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Jan 25, 2019
@adriaanm
Copy link
Contributor

Sorry about the conflict -- checksensible needs an update.


// Warning semantics.
val fatalWarnings = BooleanSetting("-Xfatal-warnings", "Fail the compilation if there are any warnings.")
val fatalWarnings = BooleanSetting("-Werror", "Fail the compilation if there are any warnings.") withAbbreviation "-Xfatal-warnings"
Copy link
Contributor

Choose a reason for hiding this comment

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

✅ on maintaining -Xfatal-warnings as an alias. I haven't checked throughout, but it would be good to list the settings that have been renamed (if Any) without leaving an alias in place, for future reference.

@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 25, 2019

  • Rebase on top of Improve the tuple adaptation warning message #7683
  • List of changes, including anything not aliased, for release notes
  • TIghten vertical space by shortening descriptions & redundant info
  • More -Y could be -V
  • REPL options could be listed only on scala -help
  • Update compiler options page
  • Update task parser
  • Enhance -Werror:value-discard to select warnings to promote to error
  • Review this list

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jan 28, 2019
@adriaanm adriaanm self-assigned this Feb 4, 2019
@SethTisue
Copy link
Member

SethTisue commented Feb 16, 2019

this looks fantastic. I would ❤️❤️❤️ to see this make RC1

collecting like flags under -V and -W is 💯

@SethTisue
Copy link
Member

SethTisue commented Feb 16, 2019

@som-snytt src/compilerOptionsExporter/scala/tools/nsc/ScalaCompilerOptionsExporter.scala needs concomitant changes

and project/ScalaOptionParser.scala

@SethTisue
Copy link
Member

@smarter might like to look this over?

@som-snytt
Copy link
Contributor Author

som-snytt commented Feb 16, 2019

Thanks for the encouragement. I realize I could blow all the good will by suggesting we drop -help in favor of -U for -Usage. Then -U -opt would explain how to enable the optimiszer.

It's usually spelled "optimiser" because it is so parsimonious with its optimal code, that is, a real miser.

@smarter
Copy link
Member

smarter commented Feb 16, 2019

I'm all for -Wfoo, as a bonus I'd like to see -Wno-foo to negate a previous -Wfoo (this is what gcc/clang already do, so it's expected behavior for -W flags). I'm not convinced about -V, most of those are still internal debug options that users shouldn't touch, if we want to be able to distinguish them from other -Y options I'd rather use -Ylog- as a prefix.
In any case when a flag is renamed, using the old name should print a warning letting the user know it's been renamed.

@som-snytt
Copy link
Contributor Author

som-snytt commented Feb 16, 2019

I may be too lazy to do -no negation, but I did just alias "-YdisableFlatCpCaching" as "-Yno-flat-classpath-cache". (Scala flags default false, and you can -Yno-flat-classpath-cache:false for explicit negation.)

I also had reservations about -V as a "public" option, even though frankly the distinction between -X and -Y is not terribly coherent. Is it meaningful to deprecate -Xexperimental and -Xfuture when you really just changed the semantics (or voided the semantics)?

As a semi-casual user, I really don't like sifting through the -Ys for all the switches to turn on some debug. Just give me the debug! So I came down in favor of -V even if the deprecation policy is totally meaningless, because of course it is.

(Edit: "semi-casual user" has a type inference problem and has to boost -Ytyper-debug aka -Vtyper.)

Apparently people don't do this anymore, but it's nice to scalac -V and see a list of debug output flags that maybe fits on one screen.

Also: "An extraterrestrial race arrives on Earth with seemingly good intentions, only to slowly reveal their true machinations the more ingrained into society they become. "

@dwijnand
Copy link
Member

What are the stability guarantees around regular options, -X options, and -Y options? Is that documented somewhere? And what's the stability intended for introducing the -W and -V options?

@som-snytt
Copy link
Contributor Author

-W is no more stable than -Xlint, which has seen evolving semantics.

I'm sorting what is a "user-facing" verbose option, -Xprint and -Xshow vs -Ylog and -Ydebug.

There will be an update on the options page. Maybe sort the options by Odersky level? With additional levels for "macro nerd", "I report issues on scala/bug", "casual contributer", "I am LARS (lrytz, adriaanm, retronym, smarter)". That would be C1 - C4.

@som-snytt som-snytt force-pushed the issue/werror branch 2 times, most recently from dc330ea to b147b74 Compare February 18, 2019 04:05
@som-snytt
Copy link
Contributor Author

/rebuild

@adriaanm
Copy link
Contributor

It’s a good point that we should document the stability somewhere. I don’t think we ever did. -X goes through deprecation, -Y is why are you even using this it could change at any time. The new -W should be more stable than -X

@som-snytt
Copy link
Contributor Author

They've really spiffied up the docs for javac. I was looking for battle-hardened terminology, but they are still a bit imprecise about "extra" or "non-standard" options, not all of which begin with -X. They "are subject to change in the future", which is tautological.

Maybe I'll submit a PR to promote -Yimports to -X so folks feel like they're allowed to use it.

I had already made -Werror a "front page" option because javac grants it that privilege.

I would be hard-pressed to say what is a "standard option." I was going to suggest: "an option useful for ordinary compilation." But -uniqid? I need -help to remind me how to spell it. And -no-specialization? That strikes me as extraordinary compilation, unless one considers specialization so fragile that it deserves a standard option to disable it. The -opt- options feel somewhat like that portion of sidewalk where some local kids inscribed their initials while the concrete was wet and are now enshrined for all passers-by.

@som-snytt som-snytt added the WIP label Feb 19, 2019
@SethTisue
Copy link
Member

SethTisue commented Feb 19, 2019

@tpolecat @DavidGregory084 as the chiselers of the compiler-options tablet which is handed down from the mountain, any feedback on this?

@tpolecat
Copy link
Contributor

This all seems good to me. I will bring in @som-snytt as a consultant for the 2.13 edition.

@som-snytt
Copy link
Contributor Author

The current options page puts -opt with "private" options because they are liable to change on a whim. I will follow scalac -help (because it's simpler and more correct). Just because no one knows how to use -opt:l:weirdness, doesn't mean you shouldn't let the concrete set on your build that uses it. Also, today I figured out that the ell stands for Lukas, the only person who knows how to use it. Maybe I'll add a section, "Lukas's Private Options".

@DavidGregory084
Copy link
Contributor

I think in general it would be nice if the options had clearer deprecation cycles as they are the public API of scalac, but it's not a particularly onerous task to support several Scala versions at the moment and it doesn't look like this would make things significantly harder 🙂

This looks like a good change to me in any case 💯

@adriaanm
Copy link
Contributor

Rebased over at an other PR. @som-snytt, assuming that LGTY, could you update your branch to the HEAD of my pr7686-rebased branch?

Add standard option `-Werror` with `-Xfatal-warnings` alias.
As with javac, it appears on the `-help` output.

Rename `-Ywarn-foo-bar` as `-Wfoo-bar`, with the previous
name as a abbreviation or alias.

`-W` shows warnings, including all the `-Xlint` and `-Wunused`.

Use underscore to mean all phases

Move option note to bottom of message

-V for verbose nee debug options

Former flags future and experimental are more former.

Move output options to -V. Options which modify output
remain -Y. -Yprint-trees:text,compact,format,text+format
replaces -Yshow-trees-stringified, et al.

Reporter says -Werror

Briefer choices list

Briefer multichoices list

Tweak expressions

More -V
Also remove dependency on jackson for yaml output.
@som-snytt
Copy link
Contributor Author

Rebased and squashed. I'll try to give the task list some attention. That is mostly sanity-checking and documentation.

@eed3si9n
Copy link
Member

As 2.13.0-RC1 is getting closer, I am concerned about the state of this PR.
Is it awaiting one more rebase?

@eed3si9n
Copy link
Member

Here's a rebase in another PR - #7908

@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 23, 2019

@eed3si9n Thanks! That gives me renewed confidence that I, too, may one day achieve the rebase!

"All your rebase are belong to us."

@adriaanm
Copy link
Contributor

Merged the Rebased.

@adriaanm adriaanm closed this Mar 24, 2019
@SethTisue SethTisue removed this from the 2.13.0-RC1 milestone Mar 24, 2019
@som-snytt
Copy link
Contributor Author

@adriaanm kind of annoying. It makes it much harder to manage branches, because this will appear forever in my list of unmerged. I don't think github has a way to make them disappear.

@adriaanm
Copy link
Contributor

I deciphered your comment as gratitude for the other branch. Had I understood better, I'd have gladly closed the other PR and waited for you to achieve rebase.

@SethTisue
Copy link
Member

@som-snytt submitting a PR so desirable you nerd-sniped Eugene and Adriaan into finishing it for you is an achievement no one can ever take away from you

@eed3si9n
Copy link
Member

Sorry about the confusion. I tend to send alternative PRs as a form of code review, so feel free to let me know if it becomes too much.
As per branch management issue, should we consider using merge as opposed to rebase? That will properly retain the original PRs history, so likely GitHub will know.

@som-snytt
Copy link
Contributor Author

Sometimes I underestimate the power of sarcasm. I'll reopen this ASAP, rebased, with the other follow-up from the checklist. Git rebase offers that nothing changed, try --skip, so maybe it knows what it is doing; it's a little bit of work to get there.

@SethTisue SethTisue removed the WIP label Jun 13, 2019
hrhino added a commit to hrhino/scala that referenced this pull request Jan 1, 2020
I nearly resolved to clear the scala/bug backlog this year.

Fixes scala/bug#3899
Fixes scala/bug#7686
Fixes scala/bug#9668
@som-snytt som-snytt deleted the issue/werror branch December 20, 2020 01:23
@som-snytt
Copy link
Contributor Author

this will appear forever in my list of unmerged

in fact, I drove by today while revivifying PRs in the old neighborhood.

The detour was worth the bon-mots.

@som-snytt som-snytt changed the title Replace -warn-option with -Woption [SUPERSEDED] Replace -warn-option with -Woption Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes worth highlighting in next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants