Skip to content

Commit 1380c90

Browse files
oderskyodersky
committed
Check for implausible patterns as a linter option
Use -Wimplausible-patterns to do the checks formerly done in checkEqualityEvidence. Create a new source file typer/Linter.scala for all linting checks done at Typer.
1 parent 01d68c6 commit 1380c90

File tree

14 files changed

+161
-116
lines changed

14 files changed

+161
-116
lines changed

‎compiler/src/dotty/tools/dotc/config/ScalaSettings.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ private sealed trait WarningSettings:
161161
val XfatalWarnings: Setting[Boolean] = BooleanSetting("-Werror", "Fail the compilation if there are any warnings.", aliases = List("-Xfatal-warnings"))
162162
val WvalueDiscard: Setting[Boolean] = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.")
163163
val WNonUnitStatement = BooleanSetting("-Wnonunit-statement", "Warn when block statements are non-Unit expressions.")
164-
164+
val WimplausiblePatterns = BooleanSetting("-Wimplausible-patterns", "Warn if comparison with a pattern value looks like it might always fail.")
165165
val Wunused: Setting[List[ChoiceWithHelp[String]]] = MultiChoiceHelpSetting(
166166
name = "-Wunused",
167167
helpArg = "warning",

‎compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
199199
case ClosureCannotHaveInternalParameterDependenciesID // errorNumber: 183
200200
case MatchTypeNoCasesID // errorNumber: 184
201201
case UnimportedAndImportedID // errorNumber: 185
202+
case ImplausiblePatternWarningID // erorNumber: 186
202203

203204
def errorNumber = ordinal - 1
204205

‎compiler/src/dotty/tools/dotc/reporting/messages.scala‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2938,3 +2938,11 @@ class ClosureCannotHaveInternalParameterDependencies(mt: Type)(using Context)
29382938
i"""cannot turn method type $mt into closure
29392939
|because it has internal parameter dependencies"""
29402940
def explain(using Context) = ""
2941+
2942+
class ImplausiblePatternWarning(pat: tpd.Tree, selType: Type)(using Context)
2943+
extends TypeMsg(ImplausiblePatternWarningID):
2944+
def msg(using Context) =
2945+
i"""|Implausible pattern:
2946+
|$pat could match selector of type $selType
2947+
|only if there is an `equals` method identifying elements of the two types."""
2948+
def explain(using Context) = ""

‎compiler/src/dotty/tools/dotc/typer/Linter.scala‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
package dotty.tools
2+
package dotc
3+
package typer
4+
5+
import core.*
6+
import Types.*, Contexts.*, Symbols.*, Flags.*, Constants.*
7+
import reporting.*
8+
import Decorators.i
9+
10+
object Linter:
11+
import ast.tpd.*
12+
13+
/** If -Wnonunit-statement is set, warn about statements in blocks that are non-unit expressions.
14+
* @return true if a warning was issued, false otherwise
15+
*/
16+
private[typer] def warnOnInterestingResultInStatement(t: Tree)(using Context): Boolean =
17+
18+
def isUninterestingSymbol(sym: Symbol): Boolean =
19+
sym == NoSymbol ||
20+
sym.isConstructor ||
21+
sym.is(Package) ||
22+
sym.isPackageObject ||
23+
sym == defn.BoxedUnitClass ||
24+
sym == defn.AnyClass ||
25+
sym == defn.AnyRefAlias ||
26+
sym == defn.AnyValClass
27+
28+
def isUninterestingType(tpe: Type): Boolean =
29+
tpe == NoType ||
30+
tpe.typeSymbol == defn.UnitClass ||
31+
defn.isBottomClass(tpe.typeSymbol) ||
32+
tpe =:= defn.UnitType ||
33+
tpe.typeSymbol == defn.BoxedUnitClass ||
34+
tpe =:= defn.AnyValType ||
35+
tpe =:= defn.AnyType ||
36+
tpe =:= defn.AnyRefType
37+
38+
def isJavaApplication(t: Tree): Boolean = t match
39+
case Apply(f, _) => f.symbol.is(JavaDefined) && !defn.ObjectClass.isSubClass(f.symbol.owner)
40+
case _ => false
41+
42+
def checkInterestingShapes(t: Tree): Boolean = t match
43+
case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart)
44+
case Block(_, res) => checkInterestingShapes(res)
45+
case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body))
46+
case _ => checksForInterestingResult(t)
47+
48+
def checksForInterestingResult(t: Tree): Boolean =
49+
!t.isDef // ignore defs
50+
&& !isUninterestingSymbol(t.symbol) // ctors, package, Unit, Any
51+
&& !isUninterestingType(t.tpe) // bottom types, Unit, Any
52+
&& !isThisTypeResult(t) // buf += x
53+
&& !isSuperConstrCall(t) // just a thing
54+
&& !isJavaApplication(t) // Java methods are inherently side-effecting
55+
// && !treeInfo.hasExplicitUnit(t) // suppressed by explicit expr: Unit // TODO Should explicit `: Unit` be added as warning suppression?
56+
57+
if ctx.settings.WNonUnitStatement.value && !ctx.isAfterTyper && checkInterestingShapes(t) then
58+
val where = t match
59+
case Block(_, res) => res
60+
case If(_, thenpart, Literal(Constant(()))) =>
61+
thenpart match {
62+
case Block(_, res) => res
63+
case _ => thenpart
64+
}
65+
case _ => t
66+
report.warning(UnusedNonUnitValue(where.tpe), t.srcPos)
67+
true
68+
else false
69+
end warnOnInterestingResultInStatement
70+
71+
def warnOnImplausiblePattern(pat: Tree, selType: Type)(using Context): Unit =
72+
// approximate type params with bounds
73+
def approx = new ApproximatingTypeMap {
74+
var alreadyExpanding: List[TypeRef] = Nil
75+
def apply(tp: Type) = tp.dealias match
76+
case tp: TypeRef if !tp.symbol.isClass =>
77+
if alreadyExpanding contains tp then tp else
78+
val saved = alreadyExpanding
79+
alreadyExpanding ::= tp
80+
val res = expandBounds(tp.info.bounds)
81+
alreadyExpanding = saved
82+
res
83+
case _ =>
84+
mapOver(tp)
85+
}
86+
87+
// Is it possible that a value of `clsA` is equal to a value of `clsB`?
88+
// This ignores user-defined equals methods, but makes an exception
89+
// for numeric classes.
90+
def canOverlap(clsA: ClassSymbol, clsB: ClassSymbol): Boolean =
91+
clsA.mayHaveCommonChild(clsB)
92+
|| clsA.isNumericValueClass // this is quite coarse, but matches to what was done before
93+
|| clsB.isNumericValueClass
94+
95+
// Can type `a` possiblly have a common instance with type `b`?
96+
def canEqual(a: Type, b: Type): Boolean = trace(i"canEqual $a $b"):
97+
b match
98+
case _: TypeRef | _: AppliedType if b.typeSymbol.isClass =>
99+
a match
100+
case a: TermRef if a.symbol.isOneOf(Module | Enum) =>
101+
(a frozen_<:< b) // fast track
102+
|| (a frozen_<:< approx(b))
103+
case _: TypeRef | _: AppliedType if a.typeSymbol.isClass =>
104+
if a.isNullType then !b.isNotNull
105+
else canOverlap(a.typeSymbol.asClass, b.typeSymbol.asClass)
106+
case a: TypeProxy =>
107+
canEqual(a.superType, b)
108+
case a: AndOrType =>
109+
canEqual(a.tp1, b) || canEqual(a.tp2, b)
110+
case b: TypeProxy =>
111+
canEqual(a, b.superType)
112+
case b: AndOrType =>
113+
// we lose precision with and/or types, but it's hard to do better and
114+
// still compute `canEqual(A & B, B & A) = true`.
115+
canEqual(a, b.tp1) || canEqual(a, b.tp2)
116+
117+
if ctx.settings.WimplausiblePatterns.value && !canEqual(pat.tpe, selType) then
118+
report.warning(ImplausiblePatternWarning(pat, selType), pat.srcPos)
119+
end warnOnImplausiblePattern
120+
121+
end Linter

‎compiler/src/dotty/tools/dotc/typer/Typer.scala‎

Lines changed: 8 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -3303,7 +3303,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
33033303
traverse(xtree :: rest)
33043304
case stat :: rest =>
33053305
val stat1 = typed(stat)(using ctx.exprContext(stat, exprOwner))
3306-
if !checkInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner)
3306+
if !Linter.warnOnInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner)
33073307
buf += stat1
33083308
traverse(rest)(using stat1.nullableContext)
33093309
case nil =>
@@ -4361,123 +4361,18 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
43614361
tree match
43624362
case _: RefTree | _: Literal
43634363
if !isVarPattern(tree) && !(pt <:< tree.tpe) =>
4364-
withMode(Mode.GadtConstraintInference) {
4364+
withMode(Mode.GadtConstraintInference):
43654365
TypeComparer.constrainPatternType(tree.tpe, pt)
4366-
}
43674366

4368-
// approximate type params with bounds
4369-
def approx = new ApproximatingTypeMap {
4370-
var alreadyExpanding: List[TypeRef] = Nil
4371-
def apply(tp: Type) = tp.dealias match
4372-
case tp: TypeRef if !tp.symbol.isClass =>
4373-
if alreadyExpanding contains tp then tp else
4374-
val saved = alreadyExpanding
4375-
alreadyExpanding ::= tp
4376-
val res = expandBounds(tp.info.bounds)
4377-
alreadyExpanding = saved
4378-
res
4379-
case _ =>
4380-
mapOver(tp)
4381-
}
4367+
Linter.warnOnImplausiblePattern(tree, pt)
43824368

4383-
// Is it possible that a value of `clsA` is equal to a value of `clsB`?
4384-
// This ignores user-defined equals methods, but makes an exception
4385-
// for numeric classes.
4386-
def canOverlap(clsA: ClassSymbol, clsB: ClassSymbol): Boolean =
4387-
clsA.mayHaveCommonChild(clsB)
4388-
|| clsA.isNumericValueClass // this is quite coarse, but matches to what was done before
4389-
|| clsB.isNumericValueClass
4390-
4391-
// Can type `a` possiblly have a common instance with type `b`?
4392-
def canEqual(a: Type, b: Type): Boolean = trace(i"canEqual $a $b"):
4393-
b match
4394-
case _: TypeRef | _: AppliedType if b.typeSymbol.isClass =>
4395-
a match
4396-
case a: TermRef if a.symbol.isOneOf(Module | Enum) =>
4397-
(a frozen_<:< b) // fast track
4398-
|| (a frozen_<:< approx(b))
4399-
case _: TypeRef | _: AppliedType if a.typeSymbol.isClass =>
4400-
if a.isNullType then !b.isNotNull
4401-
else canOverlap(a.typeSymbol.asClass, b.typeSymbol.asClass)
4402-
case a: TypeProxy =>
4403-
canEqual(a.superType, b)
4404-
case a: AndOrType =>
4405-
canEqual(a.tp1, b) || canEqual(a.tp2, b)
4406-
case b: TypeProxy =>
4407-
canEqual(a, b.superType)
4408-
case b: AndOrType =>
4409-
// we lose precision with and/or types, but it's hard to do better and
4410-
// still compute `canEqual(A & B, B & A) = true`.
4411-
canEqual(a, b.tp1) || canEqual(a, b.tp2)
4412-
4413-
if !canEqual(tree.tpe, pt) then
4414-
// We could check whether `equals` is overridden.
4415-
// Reasons for not doing so:
4416-
// - it complicates the protocol
4417-
// - such code patterns usually implies hidden errors in the code
4418-
// - it's safe/sound to reject the code
4419-
report.error(TypeMismatch(tree.tpe, pt, Some(tree), "\npattern type is incompatible with expected type"), tree.srcPos)
4420-
else
4421-
val cmp =
4422-
untpd.Apply(
4423-
untpd.Select(untpd.TypedSplice(tree), nme.EQ),
4424-
untpd.TypedSplice(dummyTreeOfType(pt)))
4425-
typedExpr(cmp, defn.BooleanType)
4369+
val cmp =
4370+
untpd.Apply(
4371+
untpd.Select(untpd.TypedSplice(tree), nme.EQ),
4372+
untpd.TypedSplice(dummyTreeOfType(pt)))
4373+
typedExpr(cmp, defn.BooleanType)
44264374
case _ =>
44274375

4428-
private def checkInterestingResultInStatement(t: Tree)(using Context): Boolean = {
4429-
def isUninterestingSymbol(sym: Symbol): Boolean =
4430-
sym == NoSymbol ||
4431-
sym.isConstructor ||
4432-
sym.is(Package) ||
4433-
sym.isPackageObject ||
4434-
sym == defn.BoxedUnitClass ||
4435-
sym == defn.AnyClass ||
4436-
sym == defn.AnyRefAlias ||
4437-
sym == defn.AnyValClass
4438-
def isUninterestingType(tpe: Type): Boolean =
4439-
tpe == NoType ||
4440-
tpe.typeSymbol == defn.UnitClass ||
4441-
defn.isBottomClass(tpe.typeSymbol) ||
4442-
tpe =:= defn.UnitType ||
4443-
tpe.typeSymbol == defn.BoxedUnitClass ||
4444-
tpe =:= defn.AnyValType ||
4445-
tpe =:= defn.AnyType ||
4446-
tpe =:= defn.AnyRefType
4447-
def isJavaApplication(t: Tree): Boolean = t match {
4448-
case Apply(f, _) => f.symbol.is(JavaDefined) && !defn.ObjectClass.isSubClass(f.symbol.owner)
4449-
case _ => false
4450-
}
4451-
def checkInterestingShapes(t: Tree): Boolean = t match {
4452-
case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart)
4453-
case Block(_, res) => checkInterestingShapes(res)
4454-
case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body))
4455-
case _ => checksForInterestingResult(t)
4456-
}
4457-
def checksForInterestingResult(t: Tree): Boolean = (
4458-
!t.isDef // ignore defs
4459-
&& !isUninterestingSymbol(t.symbol) // ctors, package, Unit, Any
4460-
&& !isUninterestingType(t.tpe) // bottom types, Unit, Any
4461-
&& !isThisTypeResult(t) // buf += x
4462-
&& !isSuperConstrCall(t) // just a thing
4463-
&& !isJavaApplication(t) // Java methods are inherently side-effecting
4464-
// && !treeInfo.hasExplicitUnit(t) // suppressed by explicit expr: Unit // TODO Should explicit `: Unit` be added as warning suppression?
4465-
)
4466-
if ctx.settings.WNonUnitStatement.value && !ctx.isAfterTyper && checkInterestingShapes(t) then
4467-
val where = t match {
4468-
case Block(_, res) => res
4469-
case If(_, thenpart, Literal(Constant(()))) =>
4470-
thenpart match {
4471-
case Block(_, res) => res
4472-
case _ => thenpart
4473-
}
4474-
case _ => t
4475-
}
4476-
report.warning(UnusedNonUnitValue(where.tpe), t.srcPos)
4477-
true
4478-
else false
4479-
}
4480-
44814376
private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, exprOwner: Symbol)(using Context): Unit =
44824377
if !tree.tpe.isErroneous
44834378
&& !ctx.isAfterTyper

‎tests/neg/gadt-contradictory-pattern.scala‎ renamed to ‎tests/neg-custom-args/fatal-warnings/gadt-contradictory-pattern.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// scalac: -Wimplausible-patterns
12
object Test {
23
sealed abstract class Foo[T]
34
case object Bar1 extends Foo[Int]

‎tests/neg/i5077.scala‎ renamed to ‎tests/neg-custom-args/fatal-warnings/i5077.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// scalac: -Wimplausible-patterns
12
trait Is[A]
23
case object IsInt extends Is[Int]
34
case object IsString extends Is[String]

‎tests/neg/i9166.scala‎ renamed to ‎tests/neg-custom-args/fatal-warnings/i9166.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// scalac: -Wimplausible-patterns
12
object UnitTest extends App {
23
def foo(m: Unit) = m match {
34
case runtime.BoxedUnit.UNIT => println("ok") // error

‎tests/neg-custom-args/fatal-warnings/i9740.check‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
-- [E186] Type Error: tests/neg-custom-args/fatal-warnings/i9740.scala:10:9 --------------------------------------------
2+
10 | case RecoveryCompleted => println("Recovery completed") // error
3+
| ^^^^^^^^^^^^^^^^^
4+
| Implausible pattern:
5+
| RecoveryCompleted could match selector of type object TypedRecoveryCompleted
6+
| only if there is an `equals` method identifying elements of the two types.
7+
-- [E186] Type Error: tests/neg-custom-args/fatal-warnings/i9740.scala:15:9 --------------------------------------------
8+
15 | case RecoveryCompleted => // error
9+
| ^^^^^^^^^^^^^^^^^
10+
| Implausible pattern:
11+
| RecoveryCompleted could match selector of type TypedRecoveryCompleted
12+
| only if there is an `equals` method identifying elements of the two types.

‎tests/neg/i9740.scala‎ renamed to ‎tests/neg-custom-args/fatal-warnings/i9740.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// scalac: -Wimplausible-patterns
12
abstract class RecoveryCompleted
23
object RecoveryCompleted extends RecoveryCompleted
34

0 commit comments

Comments
 (0)