diff --git a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala index e685d8664037..bd3048a8518d 100644 --- a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala +++ b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala @@ -161,7 +161,7 @@ private sealed trait WarningSettings: 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 WNonUnitStatement = BooleanSetting("-Wnonunit-statement", "Warn when block statements are non-Unit expressions.") - + val WimplausiblePatterns = BooleanSetting("-Wimplausible-patterns", "Warn if comparison with a pattern value looks like it might always fail.") val Wunused: Setting[List[ChoiceWithHelp[String]]] = MultiChoiceHelpSetting( name = "-Wunused", helpArg = "warning", diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index 3e839730c42c..ae96ad9c2171 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -2009,20 +2009,21 @@ class Definitions { vcls } + def boxedClass(cls: Symbol): ClassSymbol = + if cls eq ByteClass then BoxedByteClass + else if cls eq ShortClass then BoxedShortClass + else if cls eq CharClass then BoxedCharClass + else if cls eq IntClass then BoxedIntClass + else if cls eq LongClass then BoxedLongClass + else if cls eq FloatClass then BoxedFloatClass + else if cls eq DoubleClass then BoxedDoubleClass + else if cls eq UnitClass then BoxedUnitClass + else if cls eq BooleanClass then BoxedBooleanClass + else sys.error(s"Not a primitive value type: $cls") + /** The type of the boxed class corresponding to primitive value type `tp`. */ - def boxedType(tp: Type)(using Context): TypeRef = { - val cls = tp.classSymbol - if (cls eq ByteClass) BoxedByteClass - else if (cls eq ShortClass) BoxedShortClass - else if (cls eq CharClass) BoxedCharClass - else if (cls eq IntClass) BoxedIntClass - else if (cls eq LongClass) BoxedLongClass - else if (cls eq FloatClass) BoxedFloatClass - else if (cls eq DoubleClass) BoxedDoubleClass - else if (cls eq UnitClass) BoxedUnitClass - else if (cls eq BooleanClass) BoxedBooleanClass - else sys.error(s"Not a primitive value type: $tp") - }.typeRef + def boxedType(tp: Type)(using Context): TypeRef = + boxedClass(tp.classSymbol).typeRef def unboxedType(tp: Type)(using Context): TypeRef = { val cls = tp.classSymbol diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index 304840396641..1b3a89100317 100644 --- a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala @@ -2020,8 +2020,10 @@ object SymDenotations { * @return The result may contain false positives, but never false negatives. */ final def mayHaveCommonChild(that: ClassSymbol)(using Context): Boolean = - !this.is(Final) && !that.is(Final) && (this.is(Trait) || that.is(Trait)) || - this.derivesFrom(that) || that.derivesFrom(this.symbol) + this.is(Trait) && !that.isEffectivelyFinal + || that.is(Trait) && !this.isEffectivelyFinal + || this.derivesFrom(that) + || that.derivesFrom(this.symbol) final override def typeParamCreationFlags: FlagSet = ClassTypeParamCreationFlags diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala index 726cdc7131c4..8d16e5c7ceb2 100644 --- a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala +++ b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala @@ -199,6 +199,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe case ClosureCannotHaveInternalParameterDependenciesID // errorNumber: 183 case MatchTypeNoCasesID // errorNumber: 184 case UnimportedAndImportedID // errorNumber: 185 + case ImplausiblePatternWarningID // erorNumber: 186 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 970c44d54cbc..7cbce57d53ef 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -2938,3 +2938,11 @@ class ClosureCannotHaveInternalParameterDependencies(mt: Type)(using Context) i"""cannot turn method type $mt into closure |because it has internal parameter dependencies""" def explain(using Context) = "" + +class ImplausiblePatternWarning(pat: tpd.Tree, selType: Type)(using Context) + extends TypeMsg(ImplausiblePatternWarningID): + def msg(using Context) = + i"""|Implausible pattern: + |$pat could match selector of type $selType + |only if there is an `equals` method identifying elements of the two types.""" + def explain(using Context) = "" \ No newline at end of file diff --git a/compiler/src/dotty/tools/dotc/typer/Linter.scala b/compiler/src/dotty/tools/dotc/typer/Linter.scala new file mode 100644 index 000000000000..c0ba581b3732 --- /dev/null +++ b/compiler/src/dotty/tools/dotc/typer/Linter.scala @@ -0,0 +1,126 @@ +package dotty.tools +package dotc +package typer + +import core.* +import Types.*, Contexts.*, Symbols.*, Flags.*, Constants.* +import reporting.* +import Decorators.i + +/** A module for linter checks done at typer */ +object Linter: + import ast.tpd.* + + /** If -Wnonunit-statement is set, warn about statements in blocks that are non-unit expressions. + * @return true if a warning was issued, false otherwise + */ + def warnOnInterestingResultInStatement(t: Tree)(using Context): Boolean = + + def isUninterestingSymbol(sym: Symbol): Boolean = + sym == NoSymbol || + sym.isConstructor || + sym.is(Package) || + sym.isPackageObject || + sym == defn.BoxedUnitClass || + sym == defn.AnyClass || + sym == defn.AnyRefAlias || + sym == defn.AnyValClass + + def isUninterestingType(tpe: Type): Boolean = + tpe == NoType || + tpe.typeSymbol == defn.UnitClass || + defn.isBottomClass(tpe.typeSymbol) || + tpe =:= defn.UnitType || + tpe.typeSymbol == defn.BoxedUnitClass || + tpe =:= defn.AnyValType || + tpe =:= defn.AnyType || + tpe =:= defn.AnyRefType + + def isJavaApplication(t: Tree): Boolean = t match + case Apply(f, _) => f.symbol.is(JavaDefined) && !defn.ObjectClass.isSubClass(f.symbol.owner) + case _ => false + + def checkInterestingShapes(t: Tree): Boolean = t match + case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart) + case Block(_, res) => checkInterestingShapes(res) + case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body)) + case _ => checksForInterestingResult(t) + + def checksForInterestingResult(t: Tree): Boolean = + !t.isDef // ignore defs + && !isUninterestingSymbol(t.symbol) // ctors, package, Unit, Any + && !isUninterestingType(t.tpe) // bottom types, Unit, Any + && !isThisTypeResult(t) // buf += x + && !isSuperConstrCall(t) // just a thing + && !isJavaApplication(t) // Java methods are inherently side-effecting + // && !treeInfo.hasExplicitUnit(t) // suppressed by explicit expr: Unit // TODO Should explicit `: Unit` be added as warning suppression? + + if ctx.settings.WNonUnitStatement.value && !ctx.isAfterTyper && checkInterestingShapes(t) then + val where = t match + case Block(_, res) => res + case If(_, thenpart, Literal(Constant(()))) => + thenpart match { + case Block(_, res) => res + case _ => thenpart + } + case _ => t + report.warning(UnusedNonUnitValue(where.tpe), t.srcPos) + true + else false + end warnOnInterestingResultInStatement + + /** If -Wimplausible-patterns is set, warn about pattern values that can match the scrutinee + * type only if there would be some user-defined equality method that equates values of the + * two types. + */ + def warnOnImplausiblePattern(pat: Tree, selType: Type)(using Context): Unit = + // approximate type params with bounds + def approx = new ApproximatingTypeMap { + var alreadyExpanding: List[TypeRef] = Nil + def apply(tp: Type) = tp.dealias match + case tp: TypeRef if !tp.symbol.isClass => + if alreadyExpanding contains tp then tp else + val saved = alreadyExpanding + alreadyExpanding ::= tp + val res = expandBounds(tp.info.bounds) + alreadyExpanding = saved + res + case _ => + mapOver(tp) + } + + // Is it possible that a value of `clsA` is equal to a value of `clsB`? + // This ignores user-defined equals methods, but makes an exception + // for numeric classes. + def canOverlap(clsA: ClassSymbol, clsB: ClassSymbol): Boolean = + clsA.mayHaveCommonChild(clsB) + || clsA.isNumericValueClass // this is quite coarse, but matches to what was done before + || clsB.isNumericValueClass + + // Can type `a` possiblly have a common instance with type `b`? + def canEqual(a: Type, b: Type): Boolean = trace(i"canEqual $a $b"): + b match + case _: TypeRef | _: AppliedType if b.typeSymbol.isClass => + a match + case a: TermRef if a.symbol.isOneOf(Module | Enum) => + (a frozen_<:< b) // fast track + || (a frozen_<:< approx(b)) + case _: TypeRef | _: AppliedType if a.typeSymbol.isClass => + if a.isNullType then !b.isNotNull + else canOverlap(a.typeSymbol.asClass, b.typeSymbol.asClass) + case a: TypeProxy => + canEqual(a.superType, b) + case a: AndOrType => + canEqual(a.tp1, b) || canEqual(a.tp2, b) + case b: TypeProxy => + canEqual(a, b.superType) + case b: AndOrType => + // we lose precision with and/or types, but it's hard to do better and + // still compute `canEqual(A & B, B & A) = true`. + canEqual(a, b.tp1) || canEqual(a, b.tp2) + + if ctx.settings.WimplausiblePatterns.value && !canEqual(pat.tpe, selType) then + report.warning(ImplausiblePatternWarning(pat, selType), pat.srcPos) + end warnOnImplausiblePattern + +end Linter diff --git a/compiler/src/dotty/tools/dotc/typer/Synthesizer.scala b/compiler/src/dotty/tools/dotc/typer/Synthesizer.scala index 103961b68c29..5bb6a25dbb56 100644 --- a/compiler/src/dotty/tools/dotc/typer/Synthesizer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Synthesizer.scala @@ -166,7 +166,7 @@ class Synthesizer(typer: Typer)(using @constructorOnly c: Context): def cmpWithBoxed(cls1: ClassSymbol, cls2: ClassSymbol) = cls2 == defn.NothingClass - || cls2 == defn.boxedType(cls1.typeRef).symbol + || cls2 == defn.boxedClass(cls1) || cls1.isNumericValueClass && cls2.derivesFrom(defn.BoxedNumberClass) if cls1.isPrimitiveValueClass then diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 51787cb5004a..2952bfbba2af 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -3303,7 +3303,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer traverse(xtree :: rest) case stat :: rest => val stat1 = typed(stat)(using ctx.exprContext(stat, exprOwner)) - if !checkInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner) + if !Linter.warnOnInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner) buf += stat1 traverse(rest)(using stat1.nullableContext) case nil => @@ -4361,109 +4361,18 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer tree match case _: RefTree | _: Literal if !isVarPattern(tree) && !(pt <:< tree.tpe) => - withMode(Mode.GadtConstraintInference) { + withMode(Mode.GadtConstraintInference): TypeComparer.constrainPatternType(tree.tpe, pt) - } - - // approximate type params with bounds - def approx = new ApproximatingTypeMap { - var alreadyExpanding: List[TypeRef] = Nil - def apply(tp: Type) = tp.dealias match - case tp: TypeRef if !tp.symbol.isClass => - if alreadyExpanding contains tp then tp else - val saved = alreadyExpanding - alreadyExpanding ::= tp - val res = expandBounds(tp.info.bounds) - alreadyExpanding = saved - res - case _ => - mapOver(tp) - } - // Is it certain that a value of `tree.tpe` is never a subtype of `pt`? - // It is true if either - // - the class of `tree.tpe` and class of `pt` cannot have common subclass, or - // - `tree` is an object or enum value, which cannot possibly be a subtype of `pt` - val isDefiniteNotSubtype = { - val clsA = tree.tpe.widenDealias.classSymbol - val clsB = pt.dealias.classSymbol - clsA.exists && clsB.exists - && clsA != defn.NullClass - && (!clsA.isNumericValueClass && !clsB.isNumericValueClass) // approximation for numeric conversion and boxing - && !clsA.asClass.mayHaveCommonChild(clsB.asClass) - || tree.symbol.isOneOf(Module | Enum) - && !(tree.tpe frozen_<:< pt) // fast track - && !(tree.tpe frozen_<:< approx(pt)) - } + Linter.warnOnImplausiblePattern(tree, pt) - if isDefiniteNotSubtype then - // We could check whether `equals` is overridden. - // Reasons for not doing so: - // - it complicates the protocol - // - such code patterns usually implies hidden errors in the code - // - it's safe/sound to reject the code - report.error(TypeMismatch(tree.tpe, pt, Some(tree), "\npattern type is incompatible with expected type"), tree.srcPos) - else - val cmp = - untpd.Apply( - untpd.Select(untpd.TypedSplice(tree), nme.EQ), - untpd.TypedSplice(dummyTreeOfType(pt))) - typedExpr(cmp, defn.BooleanType) + val cmp = + untpd.Apply( + untpd.Select(untpd.TypedSplice(tree), nme.EQ), + untpd.TypedSplice(dummyTreeOfType(pt))) + typedExpr(cmp, defn.BooleanType) case _ => - private def checkInterestingResultInStatement(t: Tree)(using Context): Boolean = { - def isUninterestingSymbol(sym: Symbol): Boolean = - sym == NoSymbol || - sym.isConstructor || - sym.is(Package) || - sym.isPackageObject || - sym == defn.BoxedUnitClass || - sym == defn.AnyClass || - sym == defn.AnyRefAlias || - sym == defn.AnyValClass - def isUninterestingType(tpe: Type): Boolean = - tpe == NoType || - tpe.typeSymbol == defn.UnitClass || - defn.isBottomClass(tpe.typeSymbol) || - tpe =:= defn.UnitType || - tpe.typeSymbol == defn.BoxedUnitClass || - tpe =:= defn.AnyValType || - tpe =:= defn.AnyType || - tpe =:= defn.AnyRefType - def isJavaApplication(t: Tree): Boolean = t match { - case Apply(f, _) => f.symbol.is(JavaDefined) && !defn.ObjectClass.isSubClass(f.symbol.owner) - case _ => false - } - def checkInterestingShapes(t: Tree): Boolean = t match { - case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart) - case Block(_, res) => checkInterestingShapes(res) - case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body)) - case _ => checksForInterestingResult(t) - } - def checksForInterestingResult(t: Tree): Boolean = ( - !t.isDef // ignore defs - && !isUninterestingSymbol(t.symbol) // ctors, package, Unit, Any - && !isUninterestingType(t.tpe) // bottom types, Unit, Any - && !isThisTypeResult(t) // buf += x - && !isSuperConstrCall(t) // just a thing - && !isJavaApplication(t) // Java methods are inherently side-effecting - // && !treeInfo.hasExplicitUnit(t) // suppressed by explicit expr: Unit // TODO Should explicit `: Unit` be added as warning suppression? - ) - if ctx.settings.WNonUnitStatement.value && !ctx.isAfterTyper && checkInterestingShapes(t) then - val where = t match { - case Block(_, res) => res - case If(_, thenpart, Literal(Constant(()))) => - thenpart match { - case Block(_, res) => res - case _ => thenpart - } - case _ => t - } - report.warning(UnusedNonUnitValue(where.tpe), t.srcPos) - true - else false - } - private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, exprOwner: Symbol)(using Context): Unit = if !tree.tpe.isErroneous && !ctx.isAfterTyper diff --git a/tests/neg/gadt-contradictory-pattern.scala b/tests/neg-custom-args/fatal-warnings/gadt-contradictory-pattern.scala similarity index 90% rename from tests/neg/gadt-contradictory-pattern.scala rename to tests/neg-custom-args/fatal-warnings/gadt-contradictory-pattern.scala index 561c0c23d518..e64a97fb38a6 100644 --- a/tests/neg/gadt-contradictory-pattern.scala +++ b/tests/neg-custom-args/fatal-warnings/gadt-contradictory-pattern.scala @@ -1,3 +1,4 @@ +// scalac: -Wimplausible-patterns object Test { sealed abstract class Foo[T] case object Bar1 extends Foo[Int] diff --git a/tests/neg/i5077.scala b/tests/neg-custom-args/fatal-warnings/i5077.scala similarity index 96% rename from tests/neg/i5077.scala rename to tests/neg-custom-args/fatal-warnings/i5077.scala index bcae42b6a5c5..6053d75cd813 100644 --- a/tests/neg/i5077.scala +++ b/tests/neg-custom-args/fatal-warnings/i5077.scala @@ -1,3 +1,4 @@ +// scalac: -Wimplausible-patterns trait Is[A] case object IsInt extends Is[Int] case object IsString extends Is[String] diff --git a/tests/neg/i9166.scala b/tests/neg-custom-args/fatal-warnings/i9166.scala similarity index 79% rename from tests/neg/i9166.scala rename to tests/neg-custom-args/fatal-warnings/i9166.scala index 7bbf2871eed3..eb03b6282d2f 100644 --- a/tests/neg/i9166.scala +++ b/tests/neg-custom-args/fatal-warnings/i9166.scala @@ -1,3 +1,4 @@ +// scalac: -Wimplausible-patterns object UnitTest extends App { def foo(m: Unit) = m match { case runtime.BoxedUnit.UNIT => println("ok") // error diff --git a/tests/neg-custom-args/fatal-warnings/i9740.check b/tests/neg-custom-args/fatal-warnings/i9740.check new file mode 100644 index 000000000000..446fb574d33a --- /dev/null +++ b/tests/neg-custom-args/fatal-warnings/i9740.check @@ -0,0 +1,12 @@ +-- [E186] Type Error: tests/neg-custom-args/fatal-warnings/i9740.scala:10:9 -------------------------------------------- +10 | case RecoveryCompleted => println("Recovery completed") // error + | ^^^^^^^^^^^^^^^^^ + | Implausible pattern: + | RecoveryCompleted could match selector of type object TypedRecoveryCompleted + | only if there is an `equals` method identifying elements of the two types. +-- [E186] Type Error: tests/neg-custom-args/fatal-warnings/i9740.scala:15:9 -------------------------------------------- +15 | case RecoveryCompleted => // error + | ^^^^^^^^^^^^^^^^^ + | Implausible pattern: + | RecoveryCompleted could match selector of type TypedRecoveryCompleted + | only if there is an `equals` method identifying elements of the two types. diff --git a/tests/neg/i9740.scala b/tests/neg-custom-args/fatal-warnings/i9740.scala similarity index 93% rename from tests/neg/i9740.scala rename to tests/neg-custom-args/fatal-warnings/i9740.scala index 2f342977ef5d..97b46778e455 100644 --- a/tests/neg/i9740.scala +++ b/tests/neg-custom-args/fatal-warnings/i9740.scala @@ -1,3 +1,4 @@ +// scalac: -Wimplausible-patterns abstract class RecoveryCompleted object RecoveryCompleted extends RecoveryCompleted diff --git a/tests/neg/i9740b.scala b/tests/neg-custom-args/fatal-warnings/i9740b.scala similarity index 93% rename from tests/neg/i9740b.scala rename to tests/neg-custom-args/fatal-warnings/i9740b.scala index 8006056684c7..f2b8a554f2bf 100644 --- a/tests/neg/i9740b.scala +++ b/tests/neg-custom-args/fatal-warnings/i9740b.scala @@ -1,3 +1,4 @@ +// scalac: -Wimplausible-patterns enum Recovery: case RecoveryCompleted diff --git a/tests/neg/i9740c.scala b/tests/neg-custom-args/fatal-warnings/i9740c.scala similarity index 91% rename from tests/neg/i9740c.scala rename to tests/neg-custom-args/fatal-warnings/i9740c.scala index 87881c9b20d7..956a5cb3a72c 100644 --- a/tests/neg/i9740c.scala +++ b/tests/neg-custom-args/fatal-warnings/i9740c.scala @@ -1,3 +1,4 @@ +// scalac: -Wimplausible-patterns sealed trait Exp[T] case class IntExp(x: Int) extends Exp[Int] case class StrExp(x: String) extends Exp[String] diff --git a/tests/neg/i9740d.scala b/tests/neg-custom-args/fatal-warnings/i9740d.scala similarity index 89% rename from tests/neg/i9740d.scala rename to tests/neg-custom-args/fatal-warnings/i9740d.scala index 9f2490b697b6..64a330714cb5 100644 --- a/tests/neg/i9740d.scala +++ b/tests/neg-custom-args/fatal-warnings/i9740d.scala @@ -1,3 +1,5 @@ +// scalac: -Wimplausible-patterns + sealed trait Exp[T] case class IntExp(x: Int) extends Exp[Int] case class StrExp(x: String) extends Exp[String] diff --git a/tests/neg/i5004.scala b/tests/neg/i5004.scala index a8acfa231bca..97224d930bbf 100644 --- a/tests/neg/i5004.scala +++ b/tests/neg/i5004.scala @@ -1,6 +1,6 @@ object i0 { 1 match { def this(): Int // error - def this() // error + def this() // error } } diff --git a/tests/pos/i18083.scala b/tests/pos/i18083.scala new file mode 100644 index 000000000000..c7e35a51f4d0 --- /dev/null +++ b/tests/pos/i18083.scala @@ -0,0 +1,9 @@ +sealed trait A +case class Sub1() extends A +case object Sub2 extends A + +def test(x: A | Null): Int = + if x == null then return 0 + x match + case Sub1() => 1 + case Sub2 => 2