Skip to content

Commit

Permalink
Merge pull request #10704 from som-snytt/tweak/nowarn-single-boolean
Browse files Browse the repository at this point in the history
Rename `-Xlint:named-booleans` to `-Wunnamed-boolean-literal` (and no longer include it in `-Xlint`)
  • Loading branch information
lrytz committed Apr 9, 2024
2 parents df5e62c + bd9de74 commit 6e8b4f8
Show file tree
Hide file tree
Showing 13 changed files with 289 additions and 54 deletions.
1 change: 1 addition & 0 deletions build.sbt
Expand Up @@ -169,6 +169,7 @@ lazy val commonSettings = instanceSettings ++ clearSourceAndResourceDirectories
"-Wconf:cat=optimizer:is",
// we use @nowarn for methods that are deprecated in JDK > 8, but CI/release is under JDK 8
"-Wconf:cat=unused-nowarn:s",
//"-Wunnamed-boolean-literal-strict",
),
Compile / doc / scalacOptions ++= Seq(
"-doc-footer", "epfl",
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/scala/tools/nsc/Reporting.scala
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
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 warnUnnamedBoolean = BooleanSetting("-Wunnamed-boolean-literal", "Warn about unnamed boolean literals if there is more than one or defaults are used, unless parameter has @deprecatedName.")
val warnUnnamedStrict = 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(warnUnnamedBoolean :: 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
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
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 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.warnUnnamedBoolean.value && !sym.isJavaDefined && !args.isEmpty) {
val strictly = settings.warnUnnamedStrict.value // warn about any unnamed boolean arg, modulo "assert"
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 (strictly) numBools > 0 && !onlyLeadingBool else numBools >= 2
if (checkable) {
def isUnnamedArg(t: Tree) = t.hasAttachment[UnnamedArg.type]
def isNameableBoolean(param: Symbol) = param.tpe.typeSymbol == BooleanClass && !param.deprecatedParamName.contains(nme.NO_NAME)
val unnamed = args.lazyZip(params).filter {
case (arg @ Literal(Constant(_: Boolean)), param) => isNameableBoolean(param) && isUnnamedArg(arg)
case _ => false
}
def numSuspicious = unnamed.length + {
analyzer.NamedApplyBlock.namedApplyInfo(currentApplication) match {
case Some(analyzer.NamedApplyInfo(_, _, _, _, original)) =>
val treeInfo.Applied(_, _, argss) = original
argss match {
case h :: _ =>
val allParams = sym.paramLists.flatten
h.count {
case treeInfo.Applied(getter, _, _) if getter.symbol != null && getter.symbol.isDefaultGetter =>
val (_, i) = nme.splitDefaultGetterName(getter.symbol.name)
i > 0 && isNameableBoolean(allParams(i-1))
case _ => false
}
case _ => 0
}
case _ => args.count(arg => arg.symbol != null && arg.symbol.isDefaultGetter)
}
}
val warn = !unnamed.isEmpty && (strictly || numSuspicious >= 2)
if (warn)
unnamed.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
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
7 changes: 6 additions & 1 deletion src/reflect/scala/reflect/internal/StdNames.scala
Expand Up @@ -551,7 +551,12 @@ trait StdNames {
case -1 => (name.toTermName, -1)
case idx => (name.toTermName.take(idx), idx + DEFAULT_GETTER_STRING.length)
}
if (i >= 0) (n, name.encoded.substring(i).toInt) else (n, -1)
if (i < 0) (n, -1)
else {
val j = name.indexOf('$', i) // f$default$7$extension
val idx = name.subSequence(i, if (j < 0) name.length else j)
(n, idx.toString.toInt)
}
}

def localDummyName(clazz: Symbol): TermName = newTermName(LOCALDUMMY_PREFIX + clazz.name + ">")
Expand Down
54 changes: 54 additions & 0 deletions test/files/neg/named-booleans-relaxed.check
@@ -0,0 +1,54 @@
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
^
named-booleans-relaxed.scala:92: warning: Boolean literals should be passed using named argument syntax for parameter insideIf. [quickfixable]
def sus(s: String) = p.needsParentheses(s)(false) // warn
^
named-booleans-relaxed.scala:95: warning: Boolean literals should be passed using named argument syntax for parameter x. [quickfixable]
def f = p.f(true, z=42) // warn
^
named-booleans-relaxed.scala:106: warning: Boolean literals should be passed using named argument syntax for parameter y. [quickfixable]
def w = new V(true).combo(false)
^
error: No warnings can be incurred under -Werror.
17 warnings
1 error

0 comments on commit 6e8b4f8

Please sign in to comment.