Option warnings are conditional

[som-snytt]

Fixes #13887

[som-snytt]

Did not yet investigate how to set up an automated test.

➜  snips ~/projects/dotty/bin/scalac -Wconf:cat=configuration:s -Werror -Werror -d /tmp ok.scala

[dwijnand]

Are all the configuration messages about such trivial matters like setting the flag twice? If so, then I agree this is the right change. But if there are any that are more concerning, then I think it would be wrong to hide them away by default.

[dwijnand]

For instance, setting the output flag twice is by-default warning worthy imo. If I set -d foo but it's last-option-wins and something else is appending after me, I'd like to at least get a warning about it by default. WDYT?

[som-snytt]

Yes, my first thought was to distinguish --deprecation --deprecation from --deprecation --deprecation:false. There is trivial redundancy, conflicting replication, and probably more complex settings that in tandem don't make sense.

In Scala 2, there was discussion around "last setting wins" semantics. But really a warning is warranted, especially because the context is not a command line in a script, where you might want to just make a best effort, but possibly multiple plugins adding options.

[dwijnand]

LGTM. Ready for this to be merged, @som-snytt?

[som-snytt]

Overall, I thought -configuration would not pull its weight, but now I think it may be helpful, with -Wconf:cat=configuration:s escape hatch.

A few observations, it warns on -Vprint:typer -Vprint:parser -Vprint:typer

Setting -Vprint set to typer redundantly

but not -Vprint:typer,parser,typer.

The flag warning sounds flippant. On -deprecation -deprecation:false:

Boolean flag -deprecation flipped

It does not warn on -d /tmp -d /tmp/out.jar which is the use case you were enthusiastic about. Also the Version and Option cases. Worth nailing that down, which I will do.

Let's warn on multiple output dirs

[som-snytt]

Rebased and added warning on multiple outputs. Also implicit Releasable to make Using pleasant.

[som-snytt]

[info] Checking out Scala.js source version 1.7.1
[info] compiling 5 Scala sources to C:\actions-runner2\_work\dotty\dotty\tests\sjs-junit\..\out\bootstrap\sjsJUnitTests\scala-3.1.3-RC1-bin-SNAPSHOT-nonbootstrapped\classes ...
[warn] -- Warning: C:\actions-runner2\_work\dotty\dotty\tests\sjs-junit\..\out\bootstrap\sjsJUnitTests\scala-3.1.3-RC1-bin-SNAPSHOT-nonbootstrapped\src_managed\main\BuildInfo.scala:6:0 
[warn] 6 |val hasSourceMaps = false
[warn]   |^
[warn]   |Line is indented too far to the left, or a `}` is missing

[som-snytt]

Rebased hoping to duck spurious test failures.

[som-snytt]

Accidentally used jdk 11 Path.of in test.

[som-snytt]

Rebased hoping never to have to rebase again. Conflict in settings, of course.

Now that I know about -release and how useful it is, why would I ever suffer the indignity of using Path.of (see last comment).

There is a separate effort for tests to respect javaVersion, it should also use -release and default to 8. That would require -no-exec to be very useful.

I saw in the settings conflict that -release has an alias.

[som-snytt]

Also, may you live in interesting times.

 Error:  -- [E134] Type Error: C:\actions-runner2\_work\dotty\dotty\compiler\test\dotty\tools\dotc\SettingsTests.scala:188:4 
Error:  188 |    assertThrows[AssertionError](_.getMessage.contains("missing argument for option -foo"))(check(List("-foo")))
Error:      |    ^^^^^^^^^^^^
Error:      |None of the overloaded alternatives of method assertThrows in object Assert with types
Error:      | [T <: Throwable]
Error:      |  (x$0: String, x$1: Class[T], x$2: org.junit.function.ThrowingRunnable): T
Error:      | [T <: Throwable](x$0: Class[T], x$1: org.junit.function.ThrowingRunnable): T
Error:      |match type arguments [AssertionError] and arguments (<?> => <?>)
Error:  one error found
Error:  (scala3-compiler / Test / compileIncremental) Compilation failed
Error:  Total time: 10 s, completed Mar 26, 2022 5:46:12 AM

and come to know interesting people.

[smarter]

It looks like this is ready but waiting on a final look by @dwijnand ? I've assigned him and given @som-snytt write privileges in this repo so he can assign people himself.

[som-snytt]

Apologies in advance, Dale. I see a few style nits, so I will take a quick pass over it.

I already checked my white privilege. Oh, it says "write" privilege.

[dwijnand]

LGTM, except the option name...

[som-snytt]

That is a lot of ink spilt over [checks notes] not warning if they set an option twice.

I'll try to simplify the PR. Probably -configuration for configuration warnings is questionable. "Either warn or don't warn, there is no why."

Edit: I wasn't sure that ConfigurationWarning pulls its weight, but it is convenient to ask for

-Wconf:cat=configuration:e

to error if your options are not pristine.

There is no -configuration as for -deprecation. Use -Wconf instead.

It no longer warns for options that are supplied "repeatedly" with the same value, but it warns if the value changes, except that lists of values are simply collected.

It is more precise about -option arg next vs -option:arg next when erroring. That is, it no longer wrongly skips the next arg.

[som-snytt]

@KacperFKorban Maybe you have time to review the refactor of Settings that improves the remaining arguments in the setting state when erroring. There were a couple of fixes in August when I rebased (tweak arg handling in MainGenericRunner/Compiler).

The original feature was to add a category for diagnostics about options, i.e., configuration. That would be useful in a project build, to make sure settings look right (are not set twice, etc). With -Wconf the warnings can be made to error or (if desired) silenced.

[Gedochao]

LGTM

[som-snytt]

@Gedochao Thank-you! I was listening to entertainment media while adding the "deprecated aliases" commit. Would you care to comment? Or I could move it to a separate PR. I also cleaned up the test per review.

[Gedochao]

About the actual alias deprecations.

[Gedochao]

@Gedochao Thank-you! I was listening to entertainment media while adding the "deprecated aliases" commit. Would you care to comment? Or I could move it to a separate PR. I also cleaned up the test per review.

Yup, separate PR would be preferable.
Also, as noted above, let's wait with deprecating -usejavacp until 3.10.0 (so that one in a separate-separate PR 😉)

[Gedochao]

@WojciechMazur we'd need to backport this to 3.8 if we'd backport #24359
Which I suppose we could. 🤷