Rename -Xlint:named-booleans to -Wunnamed-boolean-literal (and no longer include it in -Xlint)

[som-snytt]

The existing lint to check that boolean literal arguments in method calls use named arg syntax (f(flag = true)) is moved to a warning option -Wunnamed-boolean-literal.

The warning is issued if the unnamed arg may be inadvertently swapped with another: there is another unnamed boolean arg or another boolean parameter that is supplied using a default argument.

A stricter warning is available under -Wunnamed-boolean-literal-strict, which warns about any unnamed boolean arg, except for the pattern of an assert method that takes exactly one boolean parameter in first position.

The exclusion for assert assumes that such a method will have a name that conveys how the boolean arg is used, so that naming the arg is superfluous.

This warning is on a par with -Wvalue-discard, which is also an ordinary language feature, but may be deemed warnable for some projects.

Follow-up to #10612

Removes the exclusion Lukas doubted at #10612 (comment)

[SethTisue]

I've posted a call for feedback on this in various places (including the Known issues section of the 2.13.13 release notes), with a link to this PR.

[He-Pin]

In pekko we just turn it off for now, otherwise, there are many code that need to be changed, but overall I think this is a good feature.
-Wconf:cat=unused-nowarn:s,cat=lint-named-booleans:s,cat=other-shadowing:s,any:e

[lrytz]

Where were the complaints? I don't have the context.

Is demoting to -W still necessary with the adjustments in this PR?

[milessabin]

Scala 2 appears to accept unnamed arguments after named, which means that a quick fix simply inserting the argument name is possible. I was surprised to find this was accepted, because, IMO, this is a bug in Scala 2, long standing presumably, which has just been fixed in Scala 3: scala/scala3#18363.

This means that the simple quick fix of inserting the name will risk breaking cross builds with Scala 3. A more comprehensive fix which moves named boolean argument to the end of arglists might be possible (evaluation order effects?), but a lot more complicated.

Ultimately I think an implication of this lint is that library authors should move all booleans to the end of parameter lists to allow downstream users to enable the lint, or not, with a minimum of fuss. But this impact on APIs seems wildly excessive to me.

[lrytz]

Scala 2 appears to accept unnamed arguments after named

Only if all preceding named arguments are on the correct position. I beleive Scala 3 behaves the same, also after scala/scala3#18363. IIRC Scala 3 used to allow more unnamed after named than Scala 2, and that PR brought the two in line.

[milessabin]

I beleive Scala 3 behaves the same, also after scala/scala3#18363

It does. I take it back.

[som-snytt]

The misleading text in the tour has been updated at https://docs.scala-lang.org/tour/named-arguments.html

[som-snytt]

As lrytz mentioned on the PR, the two aspects are safety (guard against swapping) and style (readability).

This commit puts safety under -Wnamed-literal and style under -Ylint (name is arbitrary for private option).

The -W option suggests that in future, other literals may wish to be linted too.

I think trying to find a "sweet spot" for all use cases is a fool's errand. Personally, I don't want to see assert(false) but fail(). All the more so with f(true). I would compile with -W:11, at least in CI.

[som-snytt]

Rebased and took lrytz's two suggestions. Copying my inline comment:

The paradigm is to consider "effectively unnamed boolean args", which are unnamed literals and default args. The relaxed check warns if there are at least 2 effectively unnamed args, at least one of which is a literal.

There is an old paulp comment to the effect that a boolean arg with a default is peak evil. I did not add that to the warning text, however.

Edit: also realized (again) that I can't enable -Ylinternal for its intended internal purpose until restarr.

[som-snytt]

More random scripted test errors.

[info] [error] ## Exception when compiling 1 sources to /tmp/sbt_3bd3d6f3/target/classes
[info] [error] java.lang.NoClassDefFoundError: scala/unchecked
[info] [error] scala.reflect.internal.Definitions$DefinitionsClass.UncheckedClass$lzycompute(Definitions.scala:1357)
[info] [error] scala.reflect.internal.Definitions$DefinitionsClass.UncheckedClass(Definitions.scala:1357)
[info] [error] scala.tools.nsc.typechecker.Checkable.scala$tools$nsc$typechecker$Checkable$$uncheckedOk(Checkable.scala:131)
[info] [error] scala.tools.nsc.typechecker.Checkable$InferCheckable.checkCheckable(Checkable.scala:309)
[info] [error] scala.tools.nsc.typechecker.Checkable$InferCheckable.checkCheckable$(Checkable.scala:309)
[info] [error] scala.tools.nsc.typechecker.Infer$Inferencer.checkCheckable(Infer.scala:210)
[info] [error] scala.tools.nsc.typechecker.Typers$Typer.typedTypeApply(Typers.scala:4521)

[snip]

[info] [info] welcome to sbt 1.9.9 (Oracle Corporation Java 1.8.0_332)
[info] [info] loading project definition from /tmp/sbt_8b7edbef/project
[info] [info] loading settings for project sbt_8b7edbef from common.sbt ...
[info] [info] set current project to sbt_8b7edbef (in build file:/tmp/sbt_8b7edbef/)
[info] [info] compiling 3 Scala sources to /tmp/sbt_8b7edbef/target/classes ...
[info] [error] ## Exception when compiling 3 sources to /tmp/sbt_8b7edbef/target/classes
[info] [error] java.lang.NoClassDefFoundError: scala/collection/ArrayOps$ArrayView
[info] [error] scala.tools.nsc.Reporting$WConf$.$anonfun$parse$1(Reporting.scala:861)
[info] [error] scala.tools.nsc.Reporting$WConf$.parse(Reporting.scala:859)
[info] [error] scala.tools.nsc.Reporting$PerRunReporting.wconf$lzycompute(Reporting.scala:51)
[info] [error] scala.tools.nsc.Reporting$PerRunReporting.wconf(Reporting.scala:51)
[info] [error] scala.tools.nsc.Reporting$PerRunReporting.scala$tools$nsc$Reporting$PerRunReporting$$issueWarning(Reporting.scala:189)
[info] [error] scala.tools.nsc.Reporting$PerRunReporting.issueIfNotSuppressed(Reporting.scala:234)
[info] [error] scala.tools.nsc.Reporting$PerRunReporting.deprecationWarning(Reporting.scala:300)
[info] [error] scala.tools.nsc.Reporting$PerRunReporting.deprecationWarning(Reporting.scala:306)
[info] [error] scala.tools.nsc.Reporting$PerRunReporting.deprecationWarning(Reporting.scala:312)
[info] [error] scala.tools.nsc.typechecker.RefChecks$RefCheckTransformer.scala$tools$nsc$typechecker$RefChecks$RefCheckTransformer$$checkUndesiredProperties(RefChecks.scala:1330)
[info] [error] scala.tools.nsc.typechecker.RefChecks$RefCheckTransformer.transformSelect(RefChecks.scala:1771)

etc. /cc @lrytz

[lrytz]

Looks good in general, but I'm still not in favor of the -Ylinterator flag. Who is it intended for?

[lrytz]

random scripted test errors.

🤷‍♂️ no idea... clicked rebuild.

[som-snytt]

I'll rename to -Ylinterator. I should have asked you for a good name first. That audience is internal us. I think the single boolean literal arg is odoriferous enough to warrant a private option. If people try it out and demand it, it could be promoted.

The broad lesson, as well, is to introduce noise under a flag and then promote to lint under public pressure, rather than the other way around.

[som-snytt]

Why did applications with named args stop working?

They were out of order.

[som-snytt]

NamedApplyInfo now carries the original Apply and besides the Block for the rewrite, attaches to the result Apply.

Args in the result Apply retain attachments (Unnamed) from original args. (Note: don't copy all attachments, as MacroExpansion is used by linting.)

[SethTisue]

I think I would expect the name to be -Wunnamed-boolean rather than the reverse? wdyt?

[som-snytt]

That would be a warning about mononyms, as it's pronounced "one-named boolean".

I considered changing it, but ran out of options.

On reconsideration, I see the issue is that the word is not "unnamed" but "positional". -Wpositional-boolean-literal-arg.

I'll keep the phone lines open right up until squashing, for any further suggestions.

[SethTisue]

(I'm pretty swamped this week and next and will probably be slow with the more in-depth feedback, I'm afraid.)

[lrytz]

Looks good!

[lrytz]

Re-reviewed. The implementation LGTM, I'd change the flag names. My suggestion is rather verbose but I think easier to understand.

Leaving to @SethTisue for 👓

[som-snytt]

/rebuild

[som-snytt]

Someday, instead of coding, we'll just ask the A.I. to "rebuild" and maybe "tweak". The second commit fixes detecting "a default arg was used for a boolean param in a named arg block". $default$7 means param 7. We just need to count them, as a threshold for warning (for the non-strict option). Added the test drawn from "real code", if reflect is real code.

A third commit adds a check for f$default$7$extension, which is not seen at refchecks but the utility method was decommissioned in 4ed31f4 for "value classes in REPL". I'm not sure why unused check was running late, but the previous usage was to peel off the method name; here, it is to peel off the parameter index.

Updated the PR OP comment @SethTisue

Sorry @lrytz for the re-review re-request.

[SethTisue]

I agree on the new naming here.

@lrytz you can merge after taking the last look it seems Som wants you to take

[lrytz]

All good, thanks for fixing the defaults!