`-Xlint` now includes `-Xlint:named-booleans` to lint unnamed boolean literal args #10612

som-snytt · 2023-11-24T09:07:51Z

Summary

Since boolean literals are not expressive of intention when passed as arguments, suggest using a named argument. Instead of throwBalloons(true, false), the intent is clearer with:

throwBalloons(waterFilled = true, now = false)

The lint should warn only for boolean flags which may invert the meaning of the method call. It only warns if the parameter is of type Boolean, so that Some(true) or getOrElse(false) does not warn.

Enabling/disabling

The new lint is part of the package of default lints enabled when you pass -Xlint.

To enable the new lint in isolation, use -Xlint:booleanLiterals.

To disable the new lint while leaving other default lints enabled, use -Xlint:-booleanLiterals.

Details

As a special exemption to reduce false positives, the lint will not warn if there is only one boolean parameter in first position, such as assert(false, msg). Consequently, methods taking a single boolean parameter are expected to have a meaningful name that makes the semantics of the parameter obvious, such as Option.when(true).

Compare:

threatAssessment(launchMissiles = true)  // the parameter name is not enforced
launchMissilesWhen(true) // improved

invoke(arg: A, async = true) // annoying name? try a default arg, overload, or methods with explicit names
invoke(arg) // sync
invokeAsync(arg) // obviously

val expected = Array(true, false, true) // ok as the second parameter is repeating

Since class names tend to be nouns instead of specific verbs, constructors (and also proxies, i.e., case class apply) will warn:

SortedList(reverse = true)

It's terrible to rely on naming for expressiveness or (the illusion of) type safety, as opposed to actual types that ensure you can't accidentally swap start and end values (which one assumes is unlikely only if one reads from left to right or if one does not routinely mix things up), but this lint provides some "support" for the common case. By "support", we understand that it "enables" this bad behavior.

The lint will not warn if the parameter is annotated deprecatedName:

def addIf(@deprecatedName b: Boolean, v: Value, deduplicate: Boolean): this.type // naming `b` is extraneous

Note that it does not warn for tuples, but in future, named tuples should be encouraged for this idiom:

def f = if (cond) (x, true) else (y, false)

Possible future improvements could be to warn for other anonymous literals such as null, primitives such as 42, or special cases of zero or empty such as None, Nil, or Set.empty.

Further details

The corresponding parameter must be of Boolean type. Some(true) does not warn; nor does bs.fold(false).

Exclude methods with exactly one boolean parameter which is in first position, like assert. Such methods are expected to be tastefully named, so that the boolean needn't be.

Parameters marked @deprecatedName are excluded.

Stratagems include a judicious mix of use named arg, sprinkle deprecatedName, or leverage default args (by omitting the literal arg in favor of a default). Also nowarn. These workarounds are mostly applied in test code.

som-snytt · 2023-11-30T05:59:34Z

src/compiler/scala/tools/reflect/ToolBoxFactory.scala

            currentTyper.silent(_.typed(expr, mode, pt), reportAmbiguousErrors = false) match {
              case analyzer.SilentResultValue(result) =>
-                trace("success: ")(showAttributed(result, true, true, settings.Yshowsymkinds.value))
+                trace("success: ")(showAttributed(result, printKinds = settings.Yshowsymkinds.value))


this was a bug. it suggests that requiring names can be broadly useful. (Here, the many booleans are not distinguished by type.)

som-snytt · 2023-11-30T19:58:35Z

Wasn't there a merged retronym PR #2164 Edit: superseded by #2203

I agree with the old paulp comment that it's better to improve API, for example thingWithoutExtra instead of thing(extra = false).

Also there is no lint to make sure I did not accidentally incur an arg block, by writing named args out of order.

There is a ticket about the rewrite not using temporaries of constants, so an application that only switches constants could skip the block and maybe warn. f(p(), y = true, x = false)

The possibly legit use of long tuples of booleans is in tests, but it would be nice to have a cleaner way to create a table and use it, for that use case.

som-snytt · 2023-11-30T23:37:04Z

The festival held once every ten years is called The Gathering of the Names, in memory of all those unnamed program elements which shall go unnamed here.

lrytz

This looks great, thanks!

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

lrytz · 2023-12-01T14:20:03Z

src/compiler/scala/tools/nsc/tasty/bridge/ContextOps.scala

@@ -79,10 +79,10 @@ trait ContextOps { self: TastyUniverse =>
    ???
  }

-  @inline final def assert(assertion: Boolean, msg: => Any): Unit =
+  @inline final def assert(@deprecatedName assertion: Boolean, msg: => Any): Unit =


Ah, a secret handshake!

lrytz · 2023-12-01T14:32:03Z

Can we mention @deprecatedName in the lint option description?

Would a similar lint make sense for null literals?

som-snytt · 2023-12-02T07:47:39Z

I considered whether other literals suffer the same issue (lack of expressiveness): 42 (which may be linted as a "magic number"), and of course null (which is as anonymous as T/F but has the advantage that it merely fails to convey what reference is null, as opposed to in which of two directions the behavior is changed). Arguably null must not be encouraged.

I will try using less deprecatedName for magic names like getOrElse, orElse which take a single boolean param. Instead, target just the name and signature. I didn't like the asymmetry of sprinkling the annotation on just certain API.

I used to exclude constructors; that idea was to exclude mere wrappers. But it's hard to know what semantically merely wraps, as opposed to Map(isThreadSafe: Boolean).

I will mention deprecatedName wherever possible.

Also no special handling of update. xs(k) = true syntax doesn't get the attachment. xs.update(k, included = true) is a nice name, as everyone wants to feel included.

som-snytt · 2023-12-02T08:44:20Z

Worth documenting that refchecks compares SomeClass: when compiling standard lib, SomeModule is different from the synthetic companion; also, the new Some rewrite happens the symbol of the Apply is stale. Flying only by instrumentation, comparing the companion seems to work.

Edit: the comment is stale, except for the bit about what you see at the Apply node after case class rewrite of the Select.

som-snytt · 2023-12-02T21:05:43Z

Also unsatisfactory is variously named fold arg:

linked.fold(none = false)(accessWithin)
c.alias.fold(ifEmpty = false)(a => a == parsedLineWord)
clazz.methods.asScala.foldLeft(z=false)

Also Intelli already lints with action:

som-snytt · 2023-12-02T21:22:50Z

Squashed, in a surge of confidence.

Avoided "boolean default arg" in main code, allowed here/there in tests.

som-snytt · 2023-12-04T16:50:46Z

assert could be specialized.

That is, def assert[@specialized(Boolean) A <: Boolean](assertion: A) or whatever it would take to make assert taking a generic boolean zero-cost (or inline), just to express that the parameter is not a flag but a value. Arguably, it is just flag that says whether to throw, but logically it is a value to apply a test to.

Array still gets special treatment because it is overloaded and takes booleans.

The itf flag everywhere in tests is clearly wrong, there should be two special methods.

lrytz

It occured to me while looking at this, making an exception for the first parameter might be a useful heuristic. It would cover things like assert or literalBool. But it might also be too surprising - what do you think?

src/compiler/scala/tools/nsc/transform/async/TransformUtils.scala

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

test/files/neg/named-booleans.scala

lrytz · 2023-12-05T14:24:46Z

Re-arranged the commits (with a small diff: https://github.com/scala/scala/compare/77320f3bffca6d6e295a137278e481f8d405e371..e7da16c208b25b4919fca0de562a5da730318d96)

making an exception for the first parameter might be a useful heuristic

That could be narrowed down to "if the first parameter is a boolean and there are no other boolean parameters".

som-snytt · 2023-12-05T15:53:25Z

OK I'll just book the education for getting the symbol of an array apply. You may issue me a paper certificate.

Feel free to squash, I was just busy yesterday and tentative. I'm glad for your simplifications.

lrytz · 2023-12-06T13:50:10Z

👍

What do you think about the heuristic for assert or boolConst to skip the warning if there's a single boolean parameter on the first position?

src/library/scala/Array.scala

lrytz

LGTM otherwise!

lrytz · 2023-12-08T08:35:24Z

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

+          }
+        loop(fn, 0)
+      }
+      def isAssertParadigm(params: List[Symbol]): Boolean = !sym.isConstructor && !sym.isCaseApplyOrUnapply && {


I'm not convinced we should make a difference between constructors and other methods here.

classes are less likely to have a name that makes its parameter meaningful, unlike a method which is of narrower semantic scope.

val EmptyInlineInfo = InlineInfo(false, None, SortedMap.empty, None)

I wound up adding names to all four args.

som-snytt · 2023-12-08T15:14:06Z

I thought I had given it one last scan. Now the diff is long and dreary enough that I tune out rather quickly.

This effort is less about a lint than about how to write a method. Whose idea was it to have more than one parameter anyway?

If you accept my theory mitigating the lrytz exclusion for constructors and case apply, you may enable merging with a green review.

SethTisue · 2023-12-11T16:39:17Z

@som-snytt mind updating the PR description to lead with user-facing documentation?

som-snytt · 2023-12-11T19:18:04Z

If Keats were alive today (for some interval), he would pen Ode on the Edit Button. It would be about impermanence and the fragility of existence, but also affirm our place in the flow of time and change.

I should say, he would pun Ode on the Edit Button, because the occasion of the poem would be that his mouse cursor is hovering on the edit button, and he is considering whether to click it and therefore enter our experience of temporality and the passing of all things.

Then critics for hundreds of years following would debate whether edit is a button or a menu item, and whether Keats intended it ironically.

retronym · 2023-12-15T04:11:58Z

I'm seeing this when building 2.13 from sbt without bootstrapping.

[warn] /Users/jz/code/scala/test/junit/scala/reflect/internal/AsSeenFromTest.scala:20:22: Unused import
[warn]   import definitions._
[warn]                      ^
[error] /Users/jz/code/scala/test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala:40:37: deprecated parameter name <none> has to be distinct from any other parameter name (deprecated or not).
[error]                     @deprecatedName safeToInline: Boolean, @deprecatedName atInline: Boolean, @deprecatedName atNoInline: Boolean, argInfos: IntMap[ArgInfo] = IntMap.empty): Unit = {
[error]                                     ^
[error] /Users/jz/code/scala/test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala:40:76: deprecated parameter name <none> has to be distinct from any other parameter name (deprecated or not).
[error]                     @deprecatedName safeToInline: Boolean, @deprecatedName atInline: Boolean, @deprecatedName atNoInline: Boolean, argInfos: IntMap[ArgInfo] = IntMap.empty): Unit = {
[error]                                                                            ^
[error] /Users/jz/code/scala/test/junit/scala/tools/nsc/backend/jvm/opt/CallGraphTest.scala:40:111: deprecated parameter name <none> has to be distinct from any other parameter name (deprecated or not).
[error]                     @deprecatedName safeToInline: Boolean, @deprecatedName atInline: Boolean, @deprecatedName atNoInline: Boolean, argInfos: IntMap[ArgInfo] = IntMap.empty): Unit = {
[error]                                                                                                               ^
[error] /Users/jz/code/scala/test/junit/scala/tools/nsc/backend/jvm/opt/InlineSourceMatcherTest.scala:32:47: deprecated parameter name <none> has to be distinct from any other parameter name (deprecated or not).
[error]   case class E(regex: String, @deprecatedName negated: Boolean = false, @deprecatedName terminal: Boolean = true)
[error]                                               ^
[error] /Users/jz/code/scala/test/junit/scala/tools/nsc/backend/jvm/opt/InlineSourceMatcherTest.scala:32:89: deprecated parameter name <none> has to be distinct from any other parameter name (deprecated or not).
[error]   case class E(regex: String, @deprecatedName negated: Boolean = false, @deprecatedName terminal: Boolean = true)
[error]                                                                                         ^
[warn] 10 warnings found
[error] 5 errors found
[error] (junit / Test / compileIncremental) Compilation failed
[error] Total time: 42 s, completed 15 Dec 2023, 2:06:53 pm

som-snytt · 2023-12-15T05:22:59Z

@retronym I thought I fixed that, but maybe I broke it instead.

Probably I should have silenced warnings the old-fashioned way until re-starr; I wound up not making accommodations in regular code (improving the heuristic instead), but left some in tests.

I'll review this PR (now) for those changes that should wait for re-starr. It's been so long since I had to wait for re-starr, I kind of forgot what it was like, the thrill and the sense of anticipation. Oh wait, that is Christmas morning.

Edit: I remember now, the fix was for specifying deprecatedName more than once in a param list. Cf BooleanSetting.

You wouldn't believe how often I junit/Test/compile that thing.

src/library/scala/concurrent/SyncChannel.scala

SethTisue · 2023-12-15T18:37:03Z

I had a good experience with this in a closed-source codebase. I got two helpful warnings and no annoying ones.

som-snytt · 2023-12-15T19:20:43Z

I need it to also lint

  x.withPartial(false)
                ^
Did you mean true?

som-snytt · 2023-12-22T00:07:48Z

src/compiler/scala/tools/nsc/transform/patmat/MatchTreeMaking.scala

@@ -24,10 +24,10 @@ import scala.tools.nsc.Reporting.WarningCategory
 trait MatchTreeMaking extends MatchCodeGen with Debugging {
  import global._, definitions._, CODE._

-  final case class Suppression(suppressExhaustive: Boolean, suppressUnreachable: Boolean)
+  final case class Suppression private (suppressExhaustive: Boolean, suppressUnreachable: Boolean)


This is charming but makes copy hard to use (under -Xsource:3).

Fixes scala/scala#10612

Fixes scala/scala#10612 (cherry picked from commit a1ca2b3)

SethTisue · 2024-02-29T00:33:07Z

It's likely we'll adjust this for 2.13.14; let's discuss on #10704

Lint unnamed boolean literal args [ci: last-only]

scala-jenkins added this to the 2.13.13 milestone Nov 24, 2023

som-snytt changed the title ~~Lint unnamed boolean literal args~~ Lint unnamed boolean literal args [ci: last-only] Nov 30, 2023

som-snytt force-pushed the lint/booleans branch from 48844ed to 16445b5 Compare November 30, 2023 05:33

som-snytt commented Nov 30, 2023

View reviewed changes

som-snytt force-pushed the lint/booleans branch from 16445b5 to 737b37d Compare November 30, 2023 19:40

som-snytt force-pushed the lint/booleans branch from 737b37d to be92c18 Compare November 30, 2023 21:34

som-snytt marked this pull request as ready for review November 30, 2023 23:23

lrytz reviewed Dec 1, 2023

View reviewed changes

som-snytt force-pushed the lint/booleans branch 3 times, most recently from da5fe5b to 4cc74f3 Compare December 2, 2023 17:29

som-snytt force-pushed the lint/booleans branch from 4cc74f3 to c7281e6 Compare December 2, 2023 21:18

som-snytt force-pushed the lint/booleans branch from c7281e6 to b0cc56e Compare December 3, 2023 06:35

som-snytt force-pushed the lint/booleans branch from ff2addf to 77320f3 Compare December 4, 2023 20:28

som-snytt added 3 commits December 5, 2023 15:13

Lint unnamed boolean literal args

3efd225

Use deprecatedName for some boolean parameters

faf0e05

Use named literal boolean args in project

e7da16c

lrytz reviewed Dec 5, 2023

View reviewed changes

src/compiler/scala/tools/nsc/transform/async/TransformUtils.scala Outdated Show resolved Hide resolved

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala Outdated Show resolved Hide resolved

test/files/neg/named-booleans.scala Outdated Show resolved Hide resolved

lrytz force-pushed the lint/booleans branch from 77320f3 to e7da16c Compare December 5, 2023 14:22

som-snytt commented Dec 7, 2023

View reviewed changes

src/library/scala/Array.scala Outdated Show resolved Hide resolved

som-snytt force-pushed the lint/booleans branch from 74dda37 to b3dacf8 Compare December 7, 2023 18:37

Improvements

2b7e0fe

lrytz force-pushed the lint/booleans branch from b3dacf8 to 2b7e0fe Compare December 8, 2023 08:33

lrytz reviewed Dec 8, 2023

View reviewed changes

lrytz merged commit d561c4e into scala:2.13.x Dec 11, 2023

som-snytt deleted the lint/booleans branch December 11, 2023 10:18

SethTisue added the release-notes worth highlighting in next release notes label Dec 11, 2023

SethTisue changed the title ~~Lint unnamed boolean literal args [ci: last-only]~~ New compiler option -Xlint:named-booleans to lint unnamed boolean literal args [ci: last-only] Dec 11, 2023

som-snytt commented Dec 15, 2023

View reviewed changes

src/library/scala/concurrent/SyncChannel.scala Show resolved Hide resolved

som-snytt mentioned this pull request Dec 15, 2023

Don't use multi-deprecatedName yet, just nowarn #10631

Merged

som-snytt commented Dec 22, 2023

View reviewed changes

SethTisue changed the title ~~New compiler option -Xlint:named-booleans to lint unnamed boolean literal args [ci: last-only]~~ Add -Xlint:named-booleans to lint unnamed boolean literal args [ci: last-only] Feb 10, 2024

mkurz added a commit to mkurz/playframework that referenced this pull request Feb 26, 2024

Pass boolean literal args as named arg in generated routes files

6863ee3

Fixes scala/scala#10612

mkurz added a commit to mkurz/playframework that referenced this pull request Feb 26, 2024

Pass boolean literal args as named arg in generated routes files

fe73187

Fixes scala/scala#10612

mkurz added a commit to mkurz/playframework that referenced this pull request Feb 26, 2024

Pass boolean literal args as named arg in generated routes files

a1ca2b3

Fixes scala/scala#10612

mergify bot pushed a commit to playframework/playframework that referenced this pull request Feb 26, 2024

Pass boolean literal args as named arg in generated routes files

2ae9442

Fixes scala/scala#10612 (cherry picked from commit a1ca2b3)

mkurz added a commit to playframework/playframework that referenced this pull request Feb 26, 2024

Pass boolean literal args as named arg in generated routes files

4cdd012

Fixes scala/scala#10612 (cherry picked from commit a1ca2b3)

SethTisue changed the title ~~Add -Xlint:named-booleans to lint unnamed boolean literal args [ci: last-only]~~ -Xlint now includes -Xlint:named-booleans to lint unnamed boolean literal args Feb 27, 2024

som-snytt mentioned this pull request Feb 28, 2024

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

Merged

hamzaremmal pushed a commit to hamzaremmal/scala3 that referenced this pull request May 2, 2025

Merge pull request scala/scala#10612 from som-snytt/lint/booleans

0b7a017

Lint unnamed boolean literal args [ci: last-only]

hamzaremmal pushed a commit to scala/scala3 that referenced this pull request May 7, 2025

Merge pull request scala/scala#10612 from som-snytt/lint/booleans

3646036

Lint unnamed boolean literal args [ci: last-only]

-Xlint now includes -Xlint:named-booleans to lint unnamed boolean literal args #10612

-Xlint now includes -Xlint:named-booleans to lint unnamed boolean literal args #10612

Uh oh!

Conversation

Uh oh!

Summary

Enabling/disabling

Details

Further details

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

`-Xlint` now includes `-Xlint:named-booleans` to lint unnamed boolean literal args #10612

`-Xlint` now includes `-Xlint:named-booleans` to lint unnamed boolean literal args #10612