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

[KacperFKorban]

Is this PR close to being merged? It contains a fix for interpreting boolean settings with explicit values e.g. -deprecation:false.
If it still requires some work then maybe we can extract those changes to a separate PR? (I think that this is quite an important fix)

[som-snytt]

Rebased.

There are a couple of technical issues with turning -configuration into -Xlint:configuration. (I try to introduce -Xlint in a separate PR.) The warning mechanism wants a Setting that must be enabled. In that other PR, I've tried to "link" settings as in Scala 2, so -Xlint:configuration could "trigger" the internal Setting, but even that was challenging. Also, Scala 3 doesn't yet have a notion of "internal" setting for this purpose.

I'll attempt to follow up, before -configuration is released.

As a footnote, to bang the drum, it would have been happier to share code and develop uninteresting infra only once.

[som-snytt]

Rebase on the refactor of settings, to which this is a follow-up.

Still to address "uniformity". -option:a,b,c was a way to -option a b c not -option a,b,c. Perhaps it doesn't matter in practice. Not sure if there are edge cases to nail down. The unified behavior means that -Vprint cannot have an omitted default, that is, there is no way to require the arg to use colon syntax. That was predicated on no one writing -Vprint typer.

The other footnote is that unconsumed args are collected by processArguments: scalac -d junk file.scala collects junk file.scala, which is OK if the error state (due to -d) is always handled. It would be nicer if -d discarded the junk arg.

The "ignore" bit is of course dubious.

This PR no longer introduces a flag for warning, analogous to -deprecation. Instead, the option to silence is

-Wconf:cat=configuration:s

(It might be an improvement if -deprecation -feature were not opt-in, but opt-out via -Wconf.)

The heuristic for the ticket is only to warn for conflicting settings: -Werror -Werror is fine, -Werror -Werror:false is not.

[som-snytt]

The "extra" layer of arg processing in MainGeneric classes was incorrectly consuming elements:

-Vprint:typer -cp -d /tmp

was reordered to

-cp -d -Vprint:typer /tmp

That was a test of missing arg.

Still need to consider if "empty" args are handled correctly. There is something about choices can't be empty; but a string setting can take an empty string. The important use case is -cp "" to remove "." from the value. I considered requiring -cp:, where the colon enforces using the empty arg (and not the next arg). That is somewhat like requiring -Werror:false for a flag.

The tooling interface in Setting is distinguished from "command line handling" in MainGeneric and from whatever scala-cli may do in pre-processing args (from cli and directives).

[som-snytt]

Considered warning for -Vprint:typer,parser,typer (as mentioned 4 yrs ago) but decided instead to remove the warning.

The purpose of warning "set redundantly" was to show that different parts of a build are contributing the same option; but that is probably benign; and other tools can be used for "optimizing" or "cleaning up" a build configuration. Possibly those tools are specific to the build tool.

For single-valued options, it's helpful to warn if the value changes (with respect to equals). The property preferPrevious was introduced to avoid mutation; however, instead of "last value wins" (as mentioned above), all single-value settings should be set at most once lazily, since there is no general structured scoping: in a build, options may be aggregated any old way, or collected by the test rig in an arbitrary order. The default behavior "should be" preferPrevious with an opt-in for mutable.

[som-snytt]

I joked privately that "warn redundantly" didn't even have a test, but actually the test was in dotc.config.ScalaSettingsTests not in dotc.SettingsTests; one should test ScalaSettings, the other should test SettingGroup for testing purposes.

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