Skip to content

Commit 54d6fcd

Browse files
oderskyodersky
authored
Improve logic when to emit pattern type error (#18093)
Improve logic when to emit "pattern type is incompatible with expected type" error. Fixes #18083 The whole thing is not very satisfactory. We have an approximation which has both false positives and false negatives. False positives: We are too lenient for numeric types and and/or types False negatives: We ignore user-generated equals methods The new approximation seems to be somewhat better than the old, but it's still an approximation. It begs the question why we attempt to do this at all.
2 parents afc8ec7 + a70ac54 commit 54d6fcd

18 files changed

+193
-117
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/core/Definitions.scala

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2009,20 +2009,21 @@ class Definitions {
20092009
vcls
20102010
}
20112011

2012+
def boxedClass(cls: Symbol): ClassSymbol =
2013+
if cls eq ByteClass then BoxedByteClass
2014+
else if cls eq ShortClass then BoxedShortClass
2015+
else if cls eq CharClass then BoxedCharClass
2016+
else if cls eq IntClass then BoxedIntClass
2017+
else if cls eq LongClass then BoxedLongClass
2018+
else if cls eq FloatClass then BoxedFloatClass
2019+
else if cls eq DoubleClass then BoxedDoubleClass
2020+
else if cls eq UnitClass then BoxedUnitClass
2021+
else if cls eq BooleanClass then BoxedBooleanClass
2022+
else sys.error(s"Not a primitive value type: $cls")
2023+
20122024
/** The type of the boxed class corresponding to primitive value type `tp`. */
2013-
def boxedType(tp: Type)(using Context): TypeRef = {
2014-
val cls = tp.classSymbol
2015-
if (cls eq ByteClass) BoxedByteClass
2016-
else if (cls eq ShortClass) BoxedShortClass
2017-
else if (cls eq CharClass) BoxedCharClass
2018-
else if (cls eq IntClass) BoxedIntClass
2019-
else if (cls eq LongClass) BoxedLongClass
2020-
else if (cls eq FloatClass) BoxedFloatClass
2021-
else if (cls eq DoubleClass) BoxedDoubleClass
2022-
else if (cls eq UnitClass) BoxedUnitClass
2023-
else if (cls eq BooleanClass) BoxedBooleanClass
2024-
else sys.error(s"Not a primitive value type: $tp")
2025-
}.typeRef
2025+
def boxedType(tp: Type)(using Context): TypeRef =
2026+
boxedClass(tp.classSymbol).typeRef
20262027

20272028
def unboxedType(tp: Type)(using Context): TypeRef = {
20282029
val cls = tp.classSymbol

‎compiler/src/dotty/tools/dotc/core/SymDenotations.scala

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2023,8 +2023,10 @@ object SymDenotations {
20232023
* @return The result may contain false positives, but never false negatives.
20242024
*/
20252025
final def mayHaveCommonChild(that: ClassSymbol)(using Context): Boolean =
2026-
!this.is(Final) && !that.is(Final) && (this.is(Trait) || that.is(Trait)) ||
2027-
this.derivesFrom(that) || that.derivesFrom(this.symbol)
2026+
this.is(Trait) && !that.isEffectivelyFinal
2027+
|| that.is(Trait) && !this.isEffectivelyFinal
2028+
|| this.derivesFrom(that)
2029+
|| that.derivesFrom(this.symbol)
20282030

20292031
final override def typeParamCreationFlags: FlagSet = ClassTypeParamCreationFlags
20302032

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

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ class Synthesizer(typer: Typer)(using @constructorOnly c: Context):
166166

167167
def cmpWithBoxed(cls1: ClassSymbol, cls2: ClassSymbol) =
168168
cls2 == defn.NothingClass
169-
|| cls2 == defn.boxedType(cls1.typeRef).symbol
169+
|| cls2 == defn.boxedClass(cls1)
170170
|| cls1.isNumericValueClass && cls2.derivesFrom(defn.BoxedNumberClass)
171171

172172
if cls1.isPrimitiveValueClass then

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

Lines changed: 8 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -3306,7 +3306,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
33063306
traverse(xtree :: rest)
33073307
case stat :: rest =>
33083308
val stat1 = typed(stat)(using ctx.exprContext(stat, exprOwner))
3309-
if !checkInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner)
3309+
if !Linter.warnOnInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner)
33103310
buf += stat1
33113311
traverse(rest)(using stat1.nullableContext)
33123312
case nil =>
@@ -4372,109 +4372,18 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
43724372
tree match
43734373
case _: RefTree | _: Literal
43744374
if !isVarPattern(tree) && !(pt <:< tree.tpe) =>
4375-
withMode(Mode.GadtConstraintInference) {
4375+
withMode(Mode.GadtConstraintInference):
43764376
TypeComparer.constrainPatternType(tree.tpe, pt)
4377-
}
4378-
4379-
// approximate type params with bounds
4380-
def approx = new ApproximatingTypeMap {
4381-
var alreadyExpanding: List[TypeRef] = Nil
4382-
def apply(tp: Type) = tp.dealias match
4383-
case tp: TypeRef if !tp.symbol.isClass =>
4384-
if alreadyExpanding contains tp then tp else
4385-
val saved = alreadyExpanding
4386-
alreadyExpanding ::= tp
4387-
val res = expandBounds(tp.info.bounds)
4388-
alreadyExpanding = saved
4389-
res
4390-
case _ =>
4391-
mapOver(tp)
4392-
}
43934377

4394-
// Is it certain that a value of `tree.tpe` is never a subtype of `pt`?
4395-
// It is true if either
4396-
// - the class of `tree.tpe` and class of `pt` cannot have common subclass, or
4397-
// - `tree` is an object or enum value, which cannot possibly be a subtype of `pt`
4398-
val isDefiniteNotSubtype = {
4399-
val clsA = tree.tpe.widenDealias.classSymbol
4400-
val clsB = pt.dealias.classSymbol
4401-
clsA.exists && clsB.exists
4402-
&& clsA != defn.NullClass
4403-
&& (!clsA.isNumericValueClass && !clsB.isNumericValueClass) // approximation for numeric conversion and boxing
4404-
&& !clsA.asClass.mayHaveCommonChild(clsB.asClass)
4405-
|| tree.symbol.isOneOf(Module | Enum)
4406-
&& !(tree.tpe frozen_<:< pt) // fast track
4407-
&& !(tree.tpe frozen_<:< approx(pt))
4408-
}
4378+
Linter.warnOnImplausiblePattern(tree, pt)
44094379

4410-
if isDefiniteNotSubtype then
4411-
// We could check whether `equals` is overridden.
4412-
// Reasons for not doing so:
4413-
// - it complicates the protocol
4414-
// - such code patterns usually implies hidden errors in the code
4415-
// - it's safe/sound to reject the code
4416-
report.error(TypeMismatch(tree.tpe, pt, Some(tree), "\npattern type is incompatible with expected type"), tree.srcPos)
4417-
else
4418-
val cmp =
4419-
untpd.Apply(
4420-
untpd.Select(untpd.TypedSplice(tree), nme.EQ),
4421-
untpd.TypedSplice(dummyTreeOfType(pt)))
4422-
typedExpr(cmp, defn.BooleanType)
4380+
val cmp =
4381+
untpd.Apply(
4382+
untpd.Select(untpd.TypedSplice(tree), nme.EQ),
4383+
untpd.TypedSplice(dummyTreeOfType(pt)))
4384+
typedExpr(cmp, defn.BooleanType)
44234385
case _ =>
44244386

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

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// scalac: -Wimplausible-patterns
12
enum Recovery:
23
case RecoveryCompleted
34

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
// scalac: -Wimplausible-patterns
12
sealed trait Exp[T]
23
case class IntExp(x: Int) extends Exp[Int]
34
case class StrExp(x: String) extends Exp[String]

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
// scalac: -Wimplausible-patterns
2+
13
sealed trait Exp[T]
24
case class IntExp(x: Int) extends Exp[Int]
35
case class StrExp(x: String) extends Exp[String]

‎tests/neg/i5004.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
object i0 {
22
1 match {
33
def this(): Int // error
4-
def this() // error
4+
def this() // error
55
}
66
}

0 commit comments

Comments
 (0)