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]

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