New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SI-8597 Issue an elusive unchecked warning #3764
Conversation
My intent is to turn on the new unchecked warning under 2.11.2, but leave the logic used by |
PLS REBUILD ALL |
@adriaanm I've pushed another version of this that leaves |
@@ -1156,6 +1156,7 @@ trait Infer extends Checkable { | |||
val pt = abstractTypesToBounds(pt0) | |||
val ptparams = freeTypeParamsOfTerms(pt) | |||
val tpparams = freeTypeParamsOfTerms(pattp) | |||
val pattpwide = pattp.widen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed.
In the new method, I'm using wildcards to model pattern type vars, and leaving everything else in place. The old method uses existentialAbstraction to erase all type args. I don't think one size fits all here. We could refactor the handling of Array better. Actually we should add test variations for arrays with pat type vars as args. |
I have to run (well, drive), but I'll do my homework on where else matchesPattern is used. It still (also) feels wrong to use existentialAbstraction for the reason we saw while debugging (it closes unrelated existentials when faced with skolems of method type params). |
The spec says that `case _: List[Int]` should be always issue an unchecked warning: > Types which are not of one of the forms described above are > also accepted as type patterns. However, such type patterns > will be translated to their erasure (§3.7). The Scala compiler > will issue an “unchecked” warning for these patterns to flag > the possible loss of type-safety. But the implementation goes a little further to omit warnings based on the static type of the scrutinee. As a trivial example: def foo(s: Seq[Int]) = s match { case _: List[Int] => } need not issue this warning. These discriminating unchecked warnings are domain of `CheckabilityChecker`. It was written in terms of `X matchesPattern P`, where `X` is the scrutinee type and `P` is the pattern type. `matchesPattern` internally approximates erased type tests by existentially abstracting over abstract types in the pattern type. `X <:< existentialAbstraction(P.typeArgs, P)`. (Arrays are special cased). However, this answers the wrong question when it comes to unchecked warnings. Let's deconstruct the reported bug: def nowarn[T] = (null: Any) match { case _: Some[T] => } We used to determine that if the first case matched, the scrutinee type would be `Some[Any]` (`Some` is covariant). If this statically matches `Some[T]` in a pattern context, we don't need to issue an unchecked warning. But, our blanket use of `existentialAbstraction` loosened the pattern type to `Some[Any]`, and the scrutinee type was deemed compatible. I believe this tells us that we shouldn't be using `matchesPattern` for unchecked warnings. I've added a new method, `scrutConformsToPatternType` which replaces pattern type variables by wildcards, but leaves other abstract types intact in the pattern type. I have introduced a new symbol test to (try to) identify pattern type variables introduced by `typedBind`. Its not pretty, and it might be cleaner to reserve a new flag for these. I've also included a test variation exercising with nested matches. The pattern type of the inner case can't, syntactically, refer to the pattern type variable of the enclosing case. If it could, we would have to be more selective in our wildcarding in `ptMatchesPatternType` by restricting ourselves to type variables associated with the closest enclosing `CaseDef`. As some further validation of the correctness of this patch, four stray warnings have been teased out of neg/unchecked-abstract.scala
I've just pushed a new version. I think this gets much closer to answering the question of checkability. I'm going to close the PR as the commit message and internal documentation need more work. But please feel free to preview, poke and prod. |
For:
Review by @adriaanm. I'll be out your way on Wedsneday, let's
run through this together then.
See also a followup issue that I lodged as SI-8604.