Skip to content

Commit

Permalink
Add -Wunnamed-boolean-literal
Browse files Browse the repository at this point in the history
Also -Wunnamed-boolean-literal-strict
a strict version for booleans.

Remove exclusion of case apply for boolean lint

Quickfix name boolean literal

Warn ambiguous effectively unnamed per review

Examine args only once

Unnamed and original apply survive rewrite
  • Loading branch information
som-snytt committed Apr 4, 2024
1 parent 9681c8c commit 23a15cd
Show file tree
Hide file tree
Showing 11 changed files with 248 additions and 53 deletions.
4 changes: 2 additions & 2 deletions src/compiler/scala/tools/nsc/Reporting.scala
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,7 @@ object Reporting {
WFlagExtraImplicit,
WFlagNumericWiden,
WFlagSelfImplicit,
WFlagNamedLiteral,
WFlagValueDiscard
= wflag()

Expand Down Expand Up @@ -623,8 +624,7 @@ object Reporting {
LintPerformance,
LintIntDivToFloat,
LintUniversalMethods,
LintNumericMethods,
LintNamedBooleans
LintNumericMethods
= lint()

sealed class Feature extends WarningCategory {
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/scala/tools/nsc/settings/Warnings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ trait Warnings {
val warnValueDiscard = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.") withAbbreviation "-Ywarn-value-discard"
val warnNumericWiden = BooleanSetting("-Wnumeric-widen", "Warn when numerics are widened.") withAbbreviation "-Ywarn-numeric-widen"
val warnOctalLiteral = BooleanSetting("-Woctal-literal", "Warn on obsolete octal syntax.") withAbbreviation "-Ywarn-octal-literal"
val warnNamedLiteral = BooleanSetting("-Wunnamed-boolean-literal", "Warn about unnamed boolean literals if there is more than one or defaults are used, unless parameter has @deprecatedName.")
val warnNamedBoolean = BooleanSetting("-Wunnamed-boolean-literal-strict", "Warn about all unnamed boolean literals, unless parameter has @deprecatedName or the method has a single leading boolean parameter.").enabling(warnNamedLiteral :: Nil)

object PerformanceWarnings extends MultiChoiceEnumeration {
val Captured = Choice("captured", "Modification of var in closure causes boxing.")
Expand Down Expand Up @@ -217,7 +219,6 @@ trait Warnings {
val NumericMethods = LintWarning("numeric-methods", "Dubious usages, such as `42.isNaN`.")
val ArgDiscard = LintWarning("arg-discard", "-Wvalue-discard for adapted arguments.")
val IntDivToFloat = LintWarning("int-div-to-float", "Warn when an integer division is converted (widened) to floating point: `(someInt / 2): Double`.")
val NamedBooleans = LintWarning("named-booleans", "Boolean literals should be named args unless param is @deprecatedName.")
val PatternShadow = LintWarning("pattern-shadow", "Pattern variable id is also a term in scope.")
val CloneableObject = LintWarning("cloneable", "Modules (objects) should not be Cloneable.")

Expand Down Expand Up @@ -256,7 +257,6 @@ trait Warnings {
def lintNumericMethods = lint.contains(NumericMethods)
def lintArgDiscard = lint.contains(ArgDiscard)
def lintIntDivToFloat = lint.contains(IntDivToFloat)
def lintNamedBooleans = lint.contains(NamedBooleans)
def warnPatternShadow = lint.contains(PatternShadow)
def warnCloneableObject = lint.contains(CloneableObject)

Expand Down
39 changes: 20 additions & 19 deletions src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala
Original file line number Diff line number Diff line change
Expand Up @@ -55,20 +55,24 @@ trait NamesDefaults { self: Analyzer =>
qual: Option[Tree],
targs: List[Tree],
vargss: List[List[Tree]],
blockTyper: Typer
) { }
blockTyper: Typer,
original: Tree,
)
object NamedApplyBlock {
private[this] val tag = reflect.classTag[NamedApplyInfo]
def namedApplyInfo(t: Tree): Option[NamedApplyInfo] = t.attachments.get[NamedApplyInfo](tag)
def unapply(b: Tree): Option[NamedApplyInfo] = b match {
case _: Block => b.attachments.get[NamedApplyInfo](tag)
case _: Block => namedApplyInfo(b)
case _ => None
}
def apply(stats: List[Tree], expr: Tree)(nai: NamedApplyInfo): Block =
Block(stats, expr.updateAttachment(nai)).updateAttachment(nai)
}

private def nameOfNamedArg(arg: Tree) = Some(arg) collect { case NamedArg(Ident(name), _) => name }
def isNamedArg(arg: Tree) = arg match {
case NamedArg(Ident(_), _) => true
case _ => false
case _ => false
}

/** @param pos maps indices from old to new */
Expand All @@ -81,7 +85,7 @@ trait NamesDefaults { self: Analyzer =>
/** @param pos maps indices from new to old (!) */
private def reorderArgsInv[T: ClassTag](args: List[T], pos: Int => Int): List[T] = {
val argsArray = args.toArray
(argsArray.indices map (i => argsArray(pos(i)))).toList
argsArray.indices.map(i => argsArray(pos(i))).toList
}

/** returns `true` if every element is equal to its index */
Expand Down Expand Up @@ -203,17 +207,14 @@ trait NamesDefaults { self: Analyzer =>
else TypeApply(f, funTargs).setType(baseFun.tpe)
}

val b = Block(List(vd), baseFunTransformed)
.setType(baseFunTransformed.tpe).setPos(baseFun.pos.makeTransparent)
b.updateAttachment(NamedApplyInfo(Some(newQual), defaultTargs, Nil, blockTyper))
b
NamedApplyBlock(List(vd), baseFunTransformed)(NamedApplyInfo(Some(newQual), defaultTargs, Nil, blockTyper, tree))
.setType(baseFunTransformed.tpe)
.setPos(baseFun.pos.makeTransparent)
}

def blockWithoutQualifier(defaultQual: Option[Tree]) = {
val b = atPos(baseFun.pos)(Block(Nil, baseFun).setType(baseFun.tpe))
b.updateAttachment(NamedApplyInfo(defaultQual, defaultTargs, Nil, blockTyper))
b
}
def blockWithoutQualifier(defaultQual: Option[Tree]) =
atPos(baseFun.pos)(NamedApplyBlock(Nil, baseFun)(NamedApplyInfo(defaultQual, defaultTargs, Nil, blockTyper, tree)))
.setType(baseFun.tpe)

def moduleQual(pos: Position, classType: Type) = {
// prefix does 'normalize', which fixes #3384
Expand Down Expand Up @@ -345,7 +346,7 @@ trait NamesDefaults { self: Analyzer =>
val transformedFun = transformNamedApplication(typer, mode, pt)(fun, x => x)
if (transformedFun.isErroneous) setError(tree)
else {
val NamedApplyBlock(NamedApplyInfo(qual, targs, vargss, blockTyper)) = transformedFun: @unchecked
val NamedApplyBlock(NamedApplyInfo(qual, targs, vargss, blockTyper, _)) = transformedFun: @unchecked
val Block(stats, funOnly) = transformedFun: @unchecked

// type the application without names; put the arguments in definition-site order
Expand Down Expand Up @@ -373,16 +374,16 @@ trait NamesDefaults { self: Analyzer =>
tpe.typeSymbol match {
case ByNameParamClass => Apply(ref, Nil)
case RepeatedParamClass => Typed(ref, Ident(tpnme.WILDCARD_STAR))
case _ => ref
case _ => origArg.attachments.get[UnnamedArg.type].foreach(ref.updateAttachment); ref
}
}
})
// cannot call blockTyper.typedBlock here, because the method expr might be partially applied only
val res = blockTyper.doTypedApply(tree, expr, refArgs, mode, pt)
res.setPos(res.pos.makeTransparent)
val block = Block(stats ::: valDefs.flatten, res).setType(res.tpe).setPos(tree.pos.makeTransparent)
block.updateAttachment(NamedApplyInfo(qual, targs, vargss :+ refArgs, blockTyper))
block
NamedApplyBlock(stats ::: valDefs.flatten, res)(NamedApplyInfo(qual, targs, vargss :+ refArgs, blockTyper, tree))
.setType(res.tpe)
.setPos(tree.pos.makeTransparent)
case _ => tree
}
}
Expand Down
64 changes: 46 additions & 18 deletions src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1703,47 +1703,75 @@ abstract class RefChecks extends Transform {

transform(qual)
case Apply(fn, args) =>
currentApplication = tree
// sensicality should be subsumed by the unreachability/exhaustivity/irrefutability
// analyses in the pattern matcher
if (!inPattern) {
checkImplicitViewOptionApply(tree.pos, fn, args)
checkSensible(tree.pos, fn, args) // TODO: this should move to preEraseApply, as reasoning about runtime semantics makes more sense in the JVM type system
checkNamedBooleanArgs(fn, args)
}
currentApplication = tree
tree
}

/** Check that boolean literals are passed as named args.
* The rule is enforced when the type of the parameter is `Boolean`.
* The rule is relaxed when the method has exactly one boolean parameter
* The rule is enforced when the type of the parameter is `Boolean`,
* and there is more than one parameter with an unnamed argument.
* The stricter internal lint warns for any unnamed argument,
* except that the rule is relaxed when the method has exactly one boolean parameter
* and it is the first parameter, such as `assert(false, msg)`.
*/
private def checkNamedBooleanArgs(fn: Tree, args: List[Tree]): Unit = {
val sym = fn.symbol
def applyDepth: Int = {
def loop(t: Tree, d: Int): Int =
t match {
case Apply(f, _) => loop(f, d+1)
case Apply(t, _) => loop(t, d+1)
case _ => d
}
loop(fn, 0)
}
def isAssertParadigm(params: List[Symbol]): Boolean = !sym.isConstructor && !sym.isCaseApplyOrUnapply && {
params match {
case h :: t => h.tpe == BooleanTpe && !t.exists(_.tpe == BooleanTpe)
case _ => false
}
}
if (settings.lintNamedBooleans && !sym.isJavaDefined && !args.isEmpty) {
if (settings.warnNamedLiteral.value && !sym.isJavaDefined && !args.isEmpty) {
def isUnnamedArg(t: Tree) = t.hasAttachment[UnnamedArg.type]
def isDefaultArg(t: Tree) = t.symbol != null && (
t.symbol.isDefaultGetter || t.symbol.name.startsWith(nme.NAMEDARG_PREFIX) && {
analyzer.NamedApplyBlock.namedApplyInfo(currentApplication) match {
case Some(analyzer.NamedApplyInfo(_, _, _, _, original)) =>
val treeInfo.Applied(_, _, argss) = original
val orig = argss.head(t.symbol.name.decoded.stripPrefix(nme.NAMEDARG_PREFIX).toInt - 1)
orig.symbol != null && orig.symbol.isDefaultGetter
case _ => false
}
}
)
val params = sym.paramLists(applyDepth)
if (!isAssertParadigm(params))
foreach2(args, params)((arg, param) => arg match {
case Literal(Constant(_: Boolean))
if arg.hasAttachment[UnnamedArg.type] && param.tpe.typeSymbol == BooleanClass && !param.deprecatedParamName.contains(nme.NO_NAME) =>
runReporting.warning(arg.pos, s"Boolean literals should be passed using named argument syntax for parameter ${param.name}.", WarningCategory.LintNamedBooleans, sym)
case _ =>
})
val numBools = params.count(_.tpe == BooleanTpe)
def onlyLeadingBool = numBools == 1 && params.head.tpe == BooleanTpe
val checkable = if (settings.warnNamedBoolean.value) numBools > 0 && !onlyLeadingBool else numBools >= 2
if (checkable) {
val (unnamed, numSuspicious) = args.lazyZip(params).iterator
.foldLeft((List.empty[(Tree, Symbol)], 0)) { (acc, ap) =>
ap match {
case (arg, param) if param.tpe.typeSymbol == BooleanClass && !param.deprecatedParamName.contains(nme.NO_NAME) =>
arg match {
case Literal(Constant(_: Boolean)) =>
if (isUnnamedArg(arg)) (ap :: acc._1, acc._2 + 1) else acc
case _ =>
if (isDefaultArg(arg)) (acc._1, acc._2 + 1) else acc
}
case _ => acc
}
}
val warn = !unnamed.isEmpty && (settings.warnNamedBoolean.value || numSuspicious >= 2)
if (warn)
unnamed.reverse.foreach {
case (arg, param) =>
val msg = s"Boolean literals should be passed using named argument syntax for parameter ${param.name}."
val action = runReporting.codeAction("name boolean literal", arg.pos.focusStart, s"${param.name} = ", msg)
runReporting.warning(arg.pos, msg, WarningCategory.WFlagNamedLiteral, sym, action)
case _ =>
}
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/Typers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3829,7 +3829,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
val fun1 = transformNamedApplication(Typer.this, mode, pt)(fun, x => x)
if (fun1.isErroneous) duplErrTree
else {
val NamedApplyBlock(NamedApplyInfo(qual, targs, previousArgss, _)) = fun1: @unchecked
val NamedApplyBlock(NamedApplyInfo(qual, targs, previousArgss, _, _)) = fun1: @unchecked
val blockIsEmpty = fun1 match {
case Block(Nil, _) =>
// if the block does not have any ValDef we can remove it. Note that the call to
Expand Down
45 changes: 45 additions & 0 deletions test/files/neg/named-booleans-relaxed.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
named-booleans-relaxed.scala:22: warning: Boolean literals should be passed using named argument syntax for parameter x. [quickfixable]
val x0 = c.f(17, true, false) // warn
^
named-booleans-relaxed.scala:22: warning: Boolean literals should be passed using named argument syntax for parameter y. [quickfixable]
val x0 = c.f(17, true, false) // warn
^
named-booleans-relaxed.scala:44: warning: Boolean literals should be passed using named argument syntax for parameter cond. [quickfixable]
c.uncheck(false, "OK", true)
^
named-booleans-relaxed.scala:44: warning: Boolean literals should be passed using named argument syntax for parameter flag. [quickfixable]
c.uncheck(false, "OK", true)
^
named-booleans-relaxed.scala:63: warning: Boolean literals should be passed using named argument syntax for parameter isKlazz. [quickfixable]
def test = Klazz(true, false) // warn case class apply as for ctor
^
named-booleans-relaxed.scala:63: warning: Boolean literals should be passed using named argument syntax for parameter isWarnable. [quickfixable]
def test = Klazz(true, false) // warn case class apply as for ctor
^
named-booleans-relaxed.scala:71: warning: Boolean literals should be passed using named argument syntax for parameter up. [quickfixable]
def g3 = f(42, false) // warn, unnamed could mean either param with default
^
named-booleans-relaxed.scala:72: warning: Boolean literals should be passed using named argument syntax for parameter up. [quickfixable]
def g4 = f(42, false, true) // warn, swappable
^
named-booleans-relaxed.scala:72: warning: Boolean literals should be passed using named argument syntax for parameter down. [quickfixable]
def g4 = f(42, false, true) // warn, swappable
^
named-booleans-relaxed.scala:79: warning: Boolean literals should be passed using named argument syntax for parameter up. [quickfixable]
def rev3 = rev(42, reverse=true, false) // warn, unnamed could mean either param with default
^
named-booleans-relaxed.scala:80: warning: Boolean literals should be passed using named argument syntax for parameter reverse. [quickfixable]
def rev4 = rev(42, false, true, false) // warn, swappable
^
named-booleans-relaxed.scala:80: warning: Boolean literals should be passed using named argument syntax for parameter up. [quickfixable]
def rev4 = rev(42, false, true, false) // warn, swappable
^
named-booleans-relaxed.scala:80: warning: Boolean literals should be passed using named argument syntax for parameter down. [quickfixable]
def rev4 = rev(42, false, true, false) // warn, swappable
^
named-booleans-relaxed.scala:81: warning: Boolean literals should be passed using named argument syntax for parameter reverse. [quickfixable]
def rev5 = rev(42, true, down=true) // warn, out of order so it's a named block, otherwise same as rev3
^
error: No warnings can be incurred under -Werror.
14 warnings
1 error
82 changes: 82 additions & 0 deletions test/files/neg/named-booleans-relaxed.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
//> using options -Werror -Wunnamed-boolean-literal

class C {
def f(n: Int = 42, x: Boolean, y: Boolean) = if (x && y) n else 0

def g(x: Any) =
x match {
case (true, false) => 0
case _ => 1
}
var b = false
def fs(n: Int)(s: String, b: Boolean) = if (b) s*n else s
def gs[A](n: Int)(s: A, b: Boolean) = if (b) s.toString*n else s.toString

def check(cond: Boolean, msg: => String) = if (cond) println(msg)
def uncheck(cond: Boolean, msg: => String, flag: Boolean) = if (cond && flag) println(msg)
}

object Test extends App {
val c = new C
val b = false
val x0 = c.f(17, true, false) // warn
val x1 = c.f(17, true, b) // nowarn
val x2 = c.f(y = b, n = 17, x = true) // nowarn
c.b = true
val y = Some(false)
val z = Option(false)
val w = (true, false)
val v = c g true // nowarn infix

val s = collection.mutable.Set.empty[String]
def mutateS(): Unit = s("updater") = true
//def updateS(): Unit = s.update("updater", true)

val m = collection.mutable.Map.empty[String, true]
def mutateM(): Unit = m("updater") = true

val ss = c.fs(42)("hello", true)
val tt = c.gs(42)("hello", true)

def f(g: Boolean => Option[Boolean]) = g(true).getOrElse(false)

c.check(true, "OK")
c.uncheck(false, "OK", true)
}

class Arrays {
def test = Array(true, false, true)
}

class Tuples {
def test = (true, false, true)
}

class Functions {
val f: Boolean => Boolean = identity
def test = f(true)
}

case class Klazz(isKlazz: Boolean, isWarnable: Boolean)

class Klazzy {
def test = Klazz(true, false) // warn case class apply as for ctor
}

class Defaulting {
def f(n: Int, up: Boolean = true, down: Boolean = false) = if (up) n+1 else if (down) n-1 else n
def g0 = f(42) // nowarn, all defaults
def g1 = f(42, up=false) // nowarn, named or defaults
def g2 = f(42, up=false, true) // nowarn, in param order so not a named block, unnamed is last remaining param
def g3 = f(42, false) // warn, unnamed could mean either param with default
def g4 = f(42, false, true) // warn, swappable

def rev(n: Int, reverse: Boolean = false, up: Boolean = true, down: Boolean = false) =
if (!reverse) f(n, up, down) else if (down) n+1 else if (up) n-1 else n
def rev0 = rev(42) // nowarn, all defaults
def rev1 = rev(42, up=false) // nowarn, named or defaults
def rev2 = rev(42, true, up=false, down=true) // nowarn, in param order so not a named block, unnamed is last remaining param
def rev3 = rev(42, reverse=true, false) // warn, unnamed could mean either param with default
def rev4 = rev(42, false, true, false) // warn, swappable
def rev5 = rev(42, true, down=true) // warn, out of order so it's a named block, otherwise same as rev3
}

0 comments on commit 23a15cd

Please sign in to comment.