Improve logic when to emit pattern type error #18093
Conversation
odersky
force-pushed
the
fix-18083
branch
3 times, most recently
from
83bcfd6 to
67ffb4b
Compare
| // still compute `canEqual(A & B, B & A) = true`. | ||
| canEqual(a, b.tp1) || canEqual(a, b.tp2) | ||
|
|
||
| if !canEqual(tree.tpe, pt) then |
There was a problem hiding this comment.
Here is the history of the original code:
@dwijnand has a good point there: it's not worth the effort to fix the bug. Maybe simply remove the code? Otherwise, more bugs will be reported in the future.
There was a problem hiding this comment.
I agree. Let's try to cut it down to
protected def checkEqualityEvidence(tree: tpd.Tree, pt: Type)(using Context) : Unit =
tree match
case _: RefTree | _: Literal
if !isVarPattern(tree) && !(pt <:< tree.tpe) =>
withMode(Mode.GadtConstraintInference) {
TypeComparer.constrainPatternType(tree.tpe, pt)
}
val cmp =
untpd.Apply(
untpd.Select(untpd.TypedSplice(tree), nme.EQ),
untpd.TypedSplice(dummyTreeOfType(pt)))
typedExpr(cmp, defn.BooleanType)
case _ =>
Improve logic when to emit "pattern type is incompatible with expected type" error. Fixes scala#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.
|
That change breaks quite a few tests, which specifically check for the pattern. I agree that making it an error as we did before is excessive since there could be implementations of |
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.
A linting warning will be much more friendly. |
|
With the latest commit we now do it under a linter option. Use -Wimplausible-patterns to do the checks formerly done in checkEqualityEvidence. This also creates a new source file typer/Linter.scala for all linting checks done at Typer. |
| extends TypeMsg(ImplausiblePatternWarningID): | ||
| def msg(using Context) = | ||
| i"""|Implausible pattern: | ||
| |$pat could match selector of type $selType |
There was a problem hiding this comment.
Not sure if the space is intentional:
| |$pat could match selector of type $selType | |
| |$pat could match selector of type $selType |
There was a problem hiding this comment.
I did that intentionally since I wanted to have some visual space between the program code and the actual text. Let's see how it works in practice.
| || 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"): |
There was a problem hiding this comment.
Minor: There is TypeComparer.provablyDisjoint, it seems to be good enough for the purpose of linting --- the semantics is a little different though.
There was a problem hiding this comment.
I looked at that, but it seems too different to be easily adaptable. And I don't want linter code to complicate the other code. So if we could use provably disjoint as is, it would be OK. But it seems the only point where we could use it is to implement the canHaveCommonChild functionality, and that one is simpler by itself.
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.