Exhaustivity warnings on nested case classes #13030
Conversation
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13030/ to see the changes. Benchmarks is based on merging with master (110e5cb) |
| classSym.isAllOf(JavaEnumTrait) | ||
| classSym.isAllOf(JavaEnumTrait) || | ||
| classSym.is(Case) && productSelectorTypes(tpw, sel.srcPos).exists(isCheckable(_)) | ||
| }) |
There was a problem hiding this comment.
I tried to understand why the cache breaks the cycle but couldn't figure it out --- when the recursive call tries to use the cache, the value is not there yet, so it will run into cycles.
But obviously, it works. Could you give a hint? 😄
There was a problem hiding this comment.
I had exactly the same thought after I sent the PR and didn't understand it either.
But I think I do now: because the test case for #12488 is running under -Ycheck-all-patmat so it's short-circuting... Damn -Ycheck-all-patmat. Why does that flag exist? It's misleading test results, as I see it.
There was a problem hiding this comment.
It's created a long time ago for testing whether the algorithm works generally. Now it becomes a "feature".
There was a problem hiding this comment.
I only partially understood your answer there. Do you think it should continue to exist? In Scala 2 the only equivalents we have are: -Xlint:strict-unsealed-patmat to consider unsealed scrutinees checkable and -Xnon-strict-patmat-analysis to disable the pessimistic approximation of guards and custom extractors not matching (the "strict" patmat analysis).
Perhaps looking to transition the tests and the logic out from under the -Ycheck-all-patmat would be a good project to get me pinging around the patmat code, learning parts of it.
There was a problem hiding this comment.
Do you think it should continue to exist?
I don't really have an opinion here. It's not supposed to be used by end-users. So there is room to change.
Fixes scala#13003, by refixing scala#12485 (PR scala#12488). Part of the issue is that isCheckable behaves differently under -Ycheck-all-patmat and our tests only run under that flag. So for starters I added a test variant where that flag isn't used. I'd like to understand why that flag exists to see if we could remove it from guarding the logic and the tests. Also make sure that any patmat test that throws an exception fails the test...
dwijnand
force-pushed
the
exhaust-case-class-in-case-class
branch
from
d8c4ad8
to
9fd4000
Compare
| classSym.isAllOf(JavaEnumTrait) || | ||
| classSym.is(Case) && { | ||
| if seen.add(tpw) then productSelectorTypes(tpw, sel.srcPos).exists(isCheckable(_)) | ||
| else true // recursive case class: return true and other members can still fail the check |
There was a problem hiding this comment.
It seems false aligns better with the protocol that if a component is sealed, then perform the check.
There was a problem hiding this comment.
Given something like case class Foo(parent: Option[Foo], other: Bar) this else branch is hit when we've running isCheckable(Foo) on the Foo in the Option. My thinking is we can optimistically return true here, and then whether or not Bar is checkable will determine if the top Foo is checkable. So I think true is right here, no?
There was a problem hiding this comment.
Given the following type:
case class A(a: A)IIRC, the implementation above will have isCheckable(A) == true. If that's as expected, then true is fine. Otherwise, it seems false should be used here.
There was a problem hiding this comment.
Yeah, I'm happy with that result.
Fixes #13003, by refixing #12485 (PR #12488).
Part of the issue is that isCheckable behaves differently under
-Ycheck-all-patmat and our tests only run under that flag. So for
starters I added a test variant where that flag isn't used. I'd like to
understand why that flag exists to see if we could remove it from
guarding the logic and the tests.