From 216640425ca6a5e26883e69a8bd5fa68df26f2db Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Tue, 27 Feb 2024 16:33:32 -0800 Subject: [PATCH 1/3] Add -Wunnamed-boolean-literal 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 --- src/compiler/scala/tools/nsc/Reporting.scala | 4 +- .../scala/tools/nsc/settings/Warnings.scala | 4 +- .../tools/nsc/typechecker/NamesDefaults.scala | 39 ++++----- .../tools/nsc/typechecker/RefChecks.scala | 64 +++++++++++---- .../scala/tools/nsc/typechecker/Typers.scala | 2 +- test/files/neg/named-booleans-relaxed.check | 45 ++++++++++ test/files/neg/named-booleans-relaxed.scala | 82 +++++++++++++++++++ test/files/neg/named-booleans.check | 27 ++++-- test/files/neg/named-booleans.scala | 13 ++- test/files/neg/warn-unused-locals.scala | 5 ++ test/junit/scala/tools/nsc/QuickfixTest.scala | 16 ++++ 11 files changed, 248 insertions(+), 53 deletions(-) create mode 100644 test/files/neg/named-booleans-relaxed.check create mode 100644 test/files/neg/named-booleans-relaxed.scala diff --git a/src/compiler/scala/tools/nsc/Reporting.scala b/src/compiler/scala/tools/nsc/Reporting.scala index 906c6c312f27..76c212f0bb18 100644 --- a/src/compiler/scala/tools/nsc/Reporting.scala +++ b/src/compiler/scala/tools/nsc/Reporting.scala @@ -571,6 +571,7 @@ object Reporting { WFlagExtraImplicit, WFlagNumericWiden, WFlagSelfImplicit, + WFlagNamedLiteral, WFlagValueDiscard = wflag() @@ -623,8 +624,7 @@ object Reporting { LintPerformance, LintIntDivToFloat, LintUniversalMethods, - LintNumericMethods, - LintNamedBooleans + LintNumericMethods = lint() sealed class Feature extends WarningCategory { diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala index 4829c7847f6d..049dd7313078 100644 --- a/src/compiler/scala/tools/nsc/settings/Warnings.scala +++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala @@ -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.") @@ -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.") @@ -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) diff --git a/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala b/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala index b28c136281b9..5266c18ca246 100644 --- a/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala +++ b/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala @@ -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 */ @@ -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 */ @@ -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 @@ -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 @@ -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 } } diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 22bfdc69eb60..8df15a451202 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -1703,6 +1703,7 @@ 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) { @@ -1710,13 +1711,14 @@ abstract class RefChecks extends Transform { 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 = { @@ -1724,26 +1726,52 @@ abstract class RefChecks extends Transform { 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 _ => + } + } } } diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 1504d850f06a..474cf78e672d 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -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 diff --git a/test/files/neg/named-booleans-relaxed.check b/test/files/neg/named-booleans-relaxed.check new file mode 100644 index 000000000000..9134ae922df9 --- /dev/null +++ b/test/files/neg/named-booleans-relaxed.check @@ -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 diff --git a/test/files/neg/named-booleans-relaxed.scala b/test/files/neg/named-booleans-relaxed.scala new file mode 100644 index 000000000000..0f6d92cdfc40 --- /dev/null +++ b/test/files/neg/named-booleans-relaxed.scala @@ -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 +} diff --git a/test/files/neg/named-booleans.check b/test/files/neg/named-booleans.check index ed9391411803..ad1ab31a7888 100644 --- a/test/files/neg/named-booleans.check +++ b/test/files/neg/named-booleans.check @@ -1,21 +1,30 @@ -named-booleans.scala:22: warning: Boolean literals should be passed using named argument syntax for parameter x. +named-booleans.scala:22: warning: Boolean literals should be passed using named argument syntax for parameter x. [quickfixable] val x0 = c.f(17, true, b) // warn ^ -named-booleans.scala:38: warning: Boolean literals should be passed using named argument syntax for parameter b. +named-booleans.scala:38: warning: Boolean literals should be passed using named argument syntax for parameter b. [quickfixable] val ss = c.fs(42)("hello", true) ^ -named-booleans.scala:39: warning: Boolean literals should be passed using named argument syntax for parameter b. +named-booleans.scala:39: warning: Boolean literals should be passed using named argument syntax for parameter b. [quickfixable] val tt = c.gs(42)("hello", true) ^ -named-booleans.scala:44: warning: Boolean literals should be passed using named argument syntax for parameter cond. +named-booleans.scala:44: warning: Boolean literals should be passed using named argument syntax for parameter cond. [quickfixable] c.uncheck(false, "OK", true) ^ -named-booleans.scala:44: warning: Boolean literals should be passed using named argument syntax for parameter flag. +named-booleans.scala:44: warning: Boolean literals should be passed using named argument syntax for parameter flag. [quickfixable] c.uncheck(false, "OK", true) ^ -named-booleans.scala:63: warning: Boolean literals should be passed using named argument syntax for parameter isKlazz. - def test = Klazz(true) // warn case class apply as for ctor - ^ +named-booleans.scala:70: warning: Boolean literals should be passed using named argument syntax for parameter down. [quickfixable] + def g2 = f(42, up=false, true) + ^ +named-booleans.scala:71: warning: Boolean literals should be passed using named argument syntax for parameter up. [quickfixable] + def g3 = f(42, false) + ^ +named-booleans.scala:72: warning: Boolean literals should be passed using named argument syntax for parameter up. [quickfixable] + def g4 = f(42, false, true) + ^ +named-booleans.scala:72: warning: Boolean literals should be passed using named argument syntax for parameter down. [quickfixable] + def g4 = f(42, false, true) + ^ error: No warnings can be incurred under -Werror. -6 warnings +9 warnings 1 error diff --git a/test/files/neg/named-booleans.scala b/test/files/neg/named-booleans.scala index e1ca8c9a10fd..a9ceaf575b21 100644 --- a/test/files/neg/named-booleans.scala +++ b/test/files/neg/named-booleans.scala @@ -1,4 +1,4 @@ -// scalac: -Werror -Xlint:named-booleans +//> using options -Werror -Wunnamed-boolean-literal-strict class C { def f(n: Int = 42, x: Boolean, y: Boolean) = if (x && y) n else 0 @@ -60,5 +60,14 @@ class Functions { case class Klazz(isKlazz: Boolean) class Klazzy { - def test = Klazz(true) // warn case class apply as for ctor + def test = Klazz(true) // nowarn 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) + def g1 = f(42, up=false) + def g2 = f(42, up=false, true) + def g3 = f(42, false) + def g4 = f(42, false, true) } diff --git a/test/files/neg/warn-unused-locals.scala b/test/files/neg/warn-unused-locals.scala index f8a2ea249a62..09c7c4067cd6 100644 --- a/test/files/neg/warn-unused-locals.scala +++ b/test/files/neg/warn-unused-locals.scala @@ -36,3 +36,8 @@ object Types { (new Bippy): Something } } + +// breakage: local val x$1 in method skolemize is never used +case class SymbolKind(accurate: String, sanitized: String, abbreviation: String) { + def skolemize: SymbolKind = copy(accurate = s"$accurate skolem", abbreviation = s"$abbreviation#SKO") +} diff --git a/test/junit/scala/tools/nsc/QuickfixTest.scala b/test/junit/scala/tools/nsc/QuickfixTest.scala index 33bdc6ef577f..e4d0aff3d361 100644 --- a/test/junit/scala/tools/nsc/QuickfixTest.scala +++ b/test/junit/scala/tools/nsc/QuickfixTest.scala @@ -149,4 +149,20 @@ class QuickfixTest extends BytecodeTesting { |""".stripMargin testQuickfixs(a2s, b2, "-Xsource:3 -quickfix:any") } + + @Test def `named boolean literal lint has a fix`: Unit = { + val a = + sm"""|class C { + | def f(hasState: Boolean, isMutable: Boolean): Boolean = hasState && isMutable + | def test = f(true, false) + |} + |""" + val b = + sm"""|class C { + | def f(hasState: Boolean, isMutable: Boolean): Boolean = hasState && isMutable + | def test = f(hasState = true, isMutable = false) + |} + |""" + testQuickfix(a, b, "-Wunnamed-boolean-literal -quickfix:any") + } } From 2bc7faf552c848661576d59b5bdd0330c2d941ae Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Fri, 5 Apr 2024 12:08:13 -0700 Subject: [PATCH 2/3] Correctly count boolean default arg usage --- build.sbt | 1 + .../scala/tools/nsc/settings/Warnings.scala | 4 +- .../tools/nsc/typechecker/RefChecks.scala | 58 +++++++++---------- test/files/neg/named-booleans-relaxed.check | 8 ++- test/files/neg/named-booleans-relaxed.scala | 16 +++++ 5 files changed, 55 insertions(+), 32 deletions(-) diff --git a/build.sbt b/build.sbt index b3ec23697b26..be747758e1da 100644 --- a/build.sbt +++ b/build.sbt @@ -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", diff --git a/src/compiler/scala/tools/nsc/settings/Warnings.scala b/src/compiler/scala/tools/nsc/settings/Warnings.scala index 049dd7313078..1536866a18bd 100644 --- a/src/compiler/scala/tools/nsc/settings/Warnings.scala +++ b/src/compiler/scala/tools/nsc/settings/Warnings.scala @@ -121,8 +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) + 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.") diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 8df15a451202..5d13ee54b1c9 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -1717,7 +1717,7 @@ abstract class RefChecks extends Transform { /** Check that boolean literals are passed as named args. * 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, + * 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)`. */ @@ -1731,40 +1731,40 @@ abstract class RefChecks extends Transform { } loop(fn, 0) } - 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 - } - } - ) + 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) 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 + val checkable = if (strictly) 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 - } + 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) + isNameableBoolean(allParams(i-1)) + case _ => false + } + case _ => 0 + } + case _ => args.count(arg => arg.symbol != null && arg.symbol.isDefaultGetter) } - val warn = !unnamed.isEmpty && (settings.warnNamedBoolean.value || numSuspicious >= 2) + } + val warn = !unnamed.isEmpty && (strictly || numSuspicious >= 2) if (warn) - unnamed.reverse.foreach { + 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) diff --git a/test/files/neg/named-booleans-relaxed.check b/test/files/neg/named-booleans-relaxed.check index 9134ae922df9..e0b021457d35 100644 --- a/test/files/neg/named-booleans-relaxed.check +++ b/test/files/neg/named-booleans-relaxed.check @@ -40,6 +40,12 @@ named-booleans-relaxed.scala:80: warning: Boolean literals should be passed usin 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 + ^ error: No warnings can be incurred under -Werror. -14 warnings +16 warnings 1 error diff --git a/test/files/neg/named-booleans-relaxed.scala b/test/files/neg/named-booleans-relaxed.scala index 0f6d92cdfc40..37ae97d69178 100644 --- a/test/files/neg/named-booleans-relaxed.scala +++ b/test/files/neg/named-booleans-relaxed.scala @@ -80,3 +80,19 @@ class Defaulting { 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 } + +class Printers { + def needsParentheses(parent: String)(insideIf: Boolean = true, insideMatch: Boolean = true, insideTry: Boolean = true, insideAnnotated: Boolean = true, insideBlock: Boolean = true, insideLabelDef: Boolean = true, insideAssign: Boolean = true): Boolean = true + + def f(x: Boolean, y: String = "hi", z: Int = 2, b: Boolean = false) = if (x && b) y+z else y*z +} +object TestPrinters { + val p = new Printers + def ok(s: String) = p.needsParentheses(s)(insideLabelDef = false) + def sus(s: String) = p.needsParentheses(s)(false) // warn + def pick(s: String) = p.needsParentheses(s)(true, insideAssign=false, insideLabelDef=false, insideBlock=false, insideAnnotated=false, insideTry=false, insideMatch=false) + + def f = p.f(true, z=42) // warn + def g = p.f(x=true, b=true) // nowarn, no unnamed + def h = p.f(true, b=true) // nowarn, one unnamed but other boolean is named; defaults are non-boolean +} From bd9de74e6e6e1ff18b32a2b7089e47daf8c5f3e8 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Fri, 5 Apr 2024 19:42:12 -0700 Subject: [PATCH 3/3] Guard against extension suffix --- src/compiler/scala/tools/nsc/typechecker/RefChecks.scala | 2 +- src/reflect/scala/reflect/internal/StdNames.scala | 7 ++++++- test/files/neg/named-booleans-relaxed.check | 5 ++++- test/files/neg/named-booleans-relaxed.scala | 9 +++++++++ 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 5d13ee54b1c9..a10fd7fd7dd9 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -1754,7 +1754,7 @@ abstract class RefChecks extends Transform { h.count { case treeInfo.Applied(getter, _, _) if getter.symbol != null && getter.symbol.isDefaultGetter => val (_, i) = nme.splitDefaultGetterName(getter.symbol.name) - isNameableBoolean(allParams(i-1)) + i > 0 && isNameableBoolean(allParams(i-1)) case _ => false } case _ => 0 diff --git a/src/reflect/scala/reflect/internal/StdNames.scala b/src/reflect/scala/reflect/internal/StdNames.scala index 299889a2bdf8..91aa1512e978 100644 --- a/src/reflect/scala/reflect/internal/StdNames.scala +++ b/src/reflect/scala/reflect/internal/StdNames.scala @@ -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 + ">") diff --git a/test/files/neg/named-booleans-relaxed.check b/test/files/neg/named-booleans-relaxed.check index e0b021457d35..8e0235fc026e 100644 --- a/test/files/neg/named-booleans-relaxed.check +++ b/test/files/neg/named-booleans-relaxed.check @@ -46,6 +46,9 @@ named-booleans-relaxed.scala:92: warning: Boolean literals should be passed usin 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. -16 warnings +17 warnings 1 error diff --git a/test/files/neg/named-booleans-relaxed.scala b/test/files/neg/named-booleans-relaxed.scala index 37ae97d69178..e4b5121a4219 100644 --- a/test/files/neg/named-booleans-relaxed.scala +++ b/test/files/neg/named-booleans-relaxed.scala @@ -96,3 +96,12 @@ object TestPrinters { def g = p.f(x=true, b=true) // nowarn, no unnamed def h = p.f(true, b=true) // nowarn, one unnamed but other boolean is named; defaults are non-boolean } + +object Testy { + class V(val x: Boolean) extends AnyVal { + def combo(y: Boolean = true, z: Boolean = false) = x&&y&&z + } + + def v = new V(true) + def w = new V(true).combo(false) +}