diff --git a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala index b650a0088de4..7c987b7dc887 100644 --- a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala +++ b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala @@ -110,6 +110,24 @@ trait TreeInfo[T <: Untyped] { self: Trees.Instance[T] => case _ => 0 } + /** The type arguments of a possibly curried call */ + def typeArgss(tree: Tree): List[List[Tree]] = + @tailrec + def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match + case TypeApply(fn, args) => loop(fn, args :: argss) + case Apply(fn, args) => loop(fn, argss) + case _ => argss + loop(tree, Nil) + + /** The term arguments of a possibly curried call */ + def termArgss(tree: Tree): List[List[Tree]] = + @tailrec + def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match + case Apply(fn, args) => loop(fn, args :: argss) + case TypeApply(fn, args) => loop(fn, argss) + case _ => argss + loop(tree, Nil) + /** All term arguments of an application in a single flattened list */ def allArguments(tree: Tree): List[Tree] = unsplice(tree) match { case Apply(fn, args) => allArguments(fn) ::: args @@ -308,6 +326,132 @@ trait TreeInfo[T <: Untyped] { self: Trees.Instance[T] => case Block(_, expr) => forallResults(expr, p) case _ => p(tree) } + + /** Applications in Scala can have one of the following shapes: + * + * 1) naked core: Ident(_) or Select(_, _) or basically anything else + * 2) naked core with targs: TypeApply(core, targs) or AppliedTypeTree(core, targs) + * 3) apply or several applies wrapping a core: Apply(core, _), or Apply(Apply(core, _), _), etc + * + * This class provides different ways to decompose applications and simplifies their analysis. + * + * ***Examples*** + * (TypeApply in the examples can be replaced with AppliedTypeTree) + * + * Ident(foo): + * * callee = Ident(foo) + * * core = Ident(foo) + * * targs = Nil + * * argss = Nil + * + * TypeApply(foo, List(targ1, targ2...)) + * * callee = TypeApply(foo, List(targ1, targ2...)) + * * core = foo + * * targs = List(targ1, targ2...) + * * argss = Nil + * + * Apply(foo, List(arg1, arg2...)) + * * callee = foo + * * core = foo + * * targs = Nil + * * argss = List(List(arg1, arg2...)) + * + * Apply(Apply(foo, List(arg21, arg22, ...)), List(arg11, arg12...)) + * * callee = foo + * * core = foo + * * targs = Nil + * * argss = List(List(arg21, arg22, ...), List(arg11, arg12, ...)) + * + * Apply(Apply(TypeApply(foo, List(targs1, targs2, ...)), List(arg21, arg22, ...)), List(arg11, arg12...)) + * * callee = TypeApply(foo, List(targs1, targs2, ...)) + * * core = foo + * * targs = Nil + * * argss = List(List(arg21, arg22, ...), List(arg11, arg12, ...)) + */ + final class Applied(val tree: Tree) { + /** The tree stripped of the possibly nested applications. + * The original tree if it's not an application. + */ + def callee: Tree = stripApply(tree) + + /** The `callee` unwrapped from type applications. + * The original `callee` if it's not a type application. + */ + def core: Tree = callee match { + case TypeApply(fn, _) => fn + case AppliedTypeTree(fn, _) => fn + case tree => tree + } + + /** The type arguments of the `callee`. + * `Nil` if the `callee` is not a type application. + */ + def targs: List[Tree] = callee match { + case TypeApply(_, args) => args + case AppliedTypeTree(_, args) => args + case _ => Nil + } + + /** (Possibly multiple lists of) value arguments of an application. + * `Nil` if the `callee` is not an application. + */ + def argss: List[List[Tree]] = termArgss(tree) + } + + /** Destructures applications into important subparts described in `Applied` class, + * namely into: core, targs and argss (in the specified order). + * + * Trees which are not applications are also accepted. Their callee and core will + * be equal to the input, while targs and argss will be Nil. + * + * The provided extractors don't expose all the API of the `Applied` class. + * For advanced use, call `dissectApplied` explicitly and use its methods instead of pattern matching. + */ + object Applied { + def apply(tree: Tree): Applied = new Applied(tree) + + def unapply(applied: Applied): Some[(Tree, List[Tree], List[List[Tree]])] = + Some((applied.core, applied.targs, applied.argss)) + + def unapply(tree: Tree): Some[(Tree, List[Tree], List[List[Tree]])] = + unapply(new Applied(tree)) + } + + /** Is tree an application with result `this.type`? + * Accept `b.addOne(x)` and also `xs(i) += x` + * where the op is an assignment operator. + */ + def isThisTypeResult(tree: Tree)(using Context): Boolean = tree match { + case Applied(fun @ Select(receiver, op), _, argss) => + tree.tpe match { + case ThisType(tref) => + tref.symbol == receiver.symbol + case tref: TermRef => + tref.symbol == receiver.symbol || argss.exists(_.exists(tref.symbol == _.symbol)) + case _ => + def checkSingle(sym: Symbol): Boolean = + (sym == receiver.symbol) || { + receiver match { + case Apply(_, _) => op.isOpAssignmentName // xs(i) += x + case _ => receiver.symbol != NoSymbol && + (receiver.symbol.isGetter || receiver.symbol.isField) // xs.addOne(x) for var xs + } + } + @tailrec def loop(mt: Type): Boolean = mt match { + case m: MethodType => + m.resType match { + case ThisType(tref) => checkSingle(tref.symbol) + case tref: TermRef => checkSingle(tref.symbol) + case restpe => loop(restpe) + } + case PolyType(_, restpe) => loop(restpe) + case _ => false + } + fun.symbol != NoSymbol && loop(fun.symbol.info) + } + case _ => + tree.tpe.isInstanceOf[ThisType] + } } trait UntypedTreeInfo extends TreeInfo[Untyped] { self: Trees.Instance[Untyped] => @@ -683,24 +827,6 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => } } - /** The type arguments of a possibly curried call */ - def typeArgss(tree: Tree): List[List[Tree]] = - @tailrec - def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match - case TypeApply(fn, args) => loop(fn, args :: argss) - case Apply(fn, args) => loop(fn, argss) - case _ => argss - loop(tree, Nil) - - /** The term arguments of a possibly curried call */ - def termArgss(tree: Tree): List[List[Tree]] = - @tailrec - def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match - case Apply(fn, args) => loop(fn, args :: argss) - case TypeApply(fn, args) => loop(fn, argss) - case _ => argss - loop(tree, Nil) - /** The type and term arguments of a possibly curried call, in the order they are given */ def allArgss(tree: Tree): List[List[Tree]] = @tailrec diff --git a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala index 64b8a91e9096..131199b30754 100644 --- a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala +++ b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala @@ -157,6 +157,7 @@ private sealed trait WarningSettings: self: SettingGroup => val Whelp: Setting[Boolean] = BooleanSetting("-W", "Print a synopsis of warning options.") val XfatalWarnings: Setting[Boolean] = BooleanSetting("-Werror", "Fail the compilation if there are any warnings.", aliases = List("-Xfatal-warnings")) + val WvalueDiscard: Setting[Boolean] = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.") val Wunused: Setting[List[String]] = MultiChoiceSetting( name = "-Wunused", diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala index 926533b01f1b..9f0d71645833 100644 --- a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala +++ b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala @@ -188,6 +188,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe case MissingImplicitArgumentID // errorNumber 172 case CannotBeAccessedID // errorNumber 173 case InlineGivenShouldNotBeFunctionID // errorNumber 174 + case ValueDiscardingID // errorNumber 175 def errorNumber = ordinal - 1 diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index 1300afc9413c..e8029d790d0a 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -2790,3 +2790,8 @@ extends SyntaxMsg(InlineGivenShouldNotBeFunctionID): | inline def apply(x: A) = x.toB """ +class ValueDiscarding(tp: Type)(using Context) + extends Message(ValueDiscardingID): + def kind = MessageKind.PotentialIssue + def msg(using Context) = i"discarded non-Unit value of type $tp" + def explain(using Context) = "" diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index c01868da27ed..5a7daadc774a 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -3944,6 +3944,9 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer // so will take the code path that decides on inlining val tree1 = adapt(tree, WildcardType, locked) checkStatementPurity(tree1)(tree, ctx.owner) + if (!ctx.isAfterTyper && !tree.isInstanceOf[Inlined] && ctx.settings.WvalueDiscard.value && !isThisTypeResult(tree)) { + report.warning(ValueDiscarding(tree.tpe), tree.srcPos) + } return tpd.Block(tree1 :: Nil, Literal(Constant(()))) } diff --git a/tests/neg/warn-value-discard.check b/tests/neg/warn-value-discard.check new file mode 100644 index 000000000000..ab6539dd5cd8 --- /dev/null +++ b/tests/neg/warn-value-discard.check @@ -0,0 +1,20 @@ +-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:15:35 ---------------------------------------------- +15 | firstThing().map(_ => secondThing()) // error + | ^^^^^^^^^^^^^ + | discarded non-Unit value of type Either[Failed, Unit] +-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:18:35 ---------------------------------------------- +18 | firstThing().map(_ => secondThing()) // error + | ^^^^^^^^^^^^^ + | discarded non-Unit value of type Either[Failed, Unit] +-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:27:36 ---------------------------------------------- +27 | mutable.Set.empty[String].remove("") // error + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | discarded non-Unit value of type Boolean +-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:39:41 ---------------------------------------------- +39 | mutable.Set.empty[String].subtractOne("") // error + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | discarded non-Unit value of type scala.collection.mutable.Set[String] +-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:59:4 ----------------------------------------------- +59 | mutable.Set.empty[String] += "" // error + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | discarded non-Unit value of type scala.collection.mutable.Set[String] diff --git a/tests/neg/warn-value-discard.scala b/tests/neg/warn-value-discard.scala new file mode 100644 index 000000000000..149433395cc5 --- /dev/null +++ b/tests/neg/warn-value-discard.scala @@ -0,0 +1,66 @@ +// scalac: -Wvalue-discard -Werror + +import scala.util.{Either, Right, Left} +import scala.collection.mutable + +case class Failed(msg: String) + +def firstThing(): Either[Failed, Unit] = + Right(()) + +def secondThing(): Either[Failed, Unit] = + Left(Failed("whoops you should have flatMapped me")) + +def singleExpr(): Either[Failed, Unit] = + firstThing().map(_ => secondThing()) // error + +def block(): Either[Failed, Unit] = { + firstThing().map(_ => secondThing()) // error +} + +class ValueDiscardTest: + val field = mutable.Set.empty[String] + + def remove(): Unit = + // Set#remove returns a Boolean, not this.type + // --> Warning + mutable.Set.empty[String].remove("") // error + + // TODO IMHO we don't need to support this, + // as it's just as easy to add a @nowarn annotation as a Unit ascription + //def removeAscribed(): Unit = { + // mutable.Set.empty[String].remove(""): Unit // nowarn + //} + + def subtract(): Unit = + // - Set#subtractOne returns this.type + // - receiver is not a field or a local variable (not quite sure what you'd call it) + // --> Warning + mutable.Set.empty[String].subtractOne("") // error + + def mutateLocalVariable(): Unit = { + // - Set#subtractOne returns this.type + // - receiver is a local variable + // --> No warning + val s: mutable.Set[String] = mutable.Set.empty[String] + s.subtractOne("") + } + + def mutateField(): Unit = + // - Set#subtractOne returns this.type + // - receiver is a local variable + // --> No warning + field.subtractOne("") + + def assignmentOperator(): Unit = + // - += returns this.type + // - receiver is not a field or a local variable + // --> Warning + mutable.Set.empty[String] += "" // error + + def assignmentOperatorLocalVariable(): Unit = + // - += returns this.type + // - receiver is a local variable + // --> No warning + val s: mutable.Set[String] = mutable.Set.empty[String] + s += ""