[red-knot] more type-narrowing in match statements #17302
Conversation
ericmarkmartin
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
ericmarkmartin
changed the title
|
|
Going to go ahead and rebase this, expand the one comment slightly, and land it. |
carljm
force-pushed
the
more-match-narrowing
branch
from
6bb4302 to
3aba946
Compare
|
Oops, never mind, after the rebase there's a recently-added test that fails with this PR, and it looks like the test is correct? With this PR, we seem to no longer realize that in match 42:
case {"something": M}: ...that We should add a test for this that's not part of the star-import tests, and figure out what we need to adjust in this PR in order to avoid regressing it. I pushed my rebase and one slightly expanded comment, but I need to move on to other things for now, so I'll consider this PR back in your court. |
it turns out it was just that I had copied the Stmt::If branch too closely: in the case of If, you must visit the test expression in the "no branch taken" flow because we need to evaluate the test before we can know it's False(y) In the case of match statements, however, visiting the pattern implies undertaking symbol bindings, which we definitely don't want to track on the "no case matched" flow. |
|
Thanks for revisiting this! Looks like we are now seeing a panic in one of the corpus tests, where we enter what should be an unreachable code path. We seem to simultaneously think there is a binding visible for a symbol, while the "unbound" initial state for that symbol is also definitely-visible -- that combination shouldn't be possible. I took a quick glance at the code changes and nothing jumped out at me; let me know if you aren't able to figure out the reason and I can take a closer look. |
ericmarkmartin
force-pushed
the
more-match-narrowing
branch
2 times, most recently
from
4b09066 to
33b34d4
Compare
ericmarkmartin
force-pushed
the
more-match-narrowing
branch
from
33b34d4 to
35dabdc
Compare
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks great! Thank you for persisting with this. I pushed a couple minor simplifications I noticed, will merge once signal is green.
* main: [red-knot] class bases are not affected by __future__.annotations (#17456) [red-knot] Add support for overloaded functions (#17366) [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444) [red-knot] more type-narrowing in match statements (#17302) [red-knot] Add some narrowing for assignment expressions (#17448) [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446) Server: Use `min` instead of `max` to limit the number of threads (#17421)
* main: (123 commits) [red-knot] Handle explicit class specialization in type expressions (#17434) [red-knot] allow assignment expression in call compare narrowing (#17461) [red-knot] fix building unions with literals and AlwaysTruthy/AlwaysFalsy (#17451) [red-knot] Type narrowing for assertions (take 2) (#17345) [red-knot] class bases are not affected by __future__.annotations (#17456) [red-knot] Add support for overloaded functions (#17366) [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444) [red-knot] more type-narrowing in match statements (#17302) [red-knot] Add some narrowing for assignment expressions (#17448) [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446) Server: Use `min` instead of `max` to limit the number of threads (#17421) [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406) [red-knot] Super-basic generic inference at call sites (#17301) ...
3 participants
Summary
Add more narrowing analysis for match statements:
This PR doesn't address that guards can mutate your subject, and so theoretically invalidate some of these narrowing constraints that you've previously accumulated. Some prior art on this issue here.
Test Plan
Add some new tests, and update some existing ones