Skip to content

Commit

Permalink
Add -Wliteral-args -Wboolean-literal
Browse files Browse the repository at this point in the history
Remove exclusion of case apply for boolean lint

-Wboolean-literal is strict version for booleans.

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 Mar 12, 2024
1 parent 4b124f2 commit e1d6940
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 @@ -578,6 +578,7 @@ object Reporting {
WFlagExtraImplicit,
WFlagNumericWiden,
WFlagSelfImplicit,
WFlagNamedLiteral,
WFlagValueDiscard
= wflag()

Expand Down Expand Up @@ -630,8 +631,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("-Wliteral-args", "Warn when positional literal arguments should be named, unless parameter has @deprecatedName.")
val warnNamedBoolean = BooleanSetting("-Wboolean-literal", "Boolean literal arguments should be named.").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 @@ -3819,7 +3819,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 -Wliteral-args

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 e1d6940

Please sign in to comment.