[red-knot] Fix control flow for assert statements
#17702
Conversation
AlexWaygood
force-pushed
the
alex/assert-narrowing
branch
from
72af944 to
b28723b
Compare
|
AlexWaygood
force-pushed
the
alex/assert-narrowing
branch
from
b28723b to
22ffa02
Compare
|
Just spot-checking the very first error (from parso), it looks to me like this PR is now wrongly suggesting that there is a possible control flow path from the assertion message to the following code, where in fact, the assertion message control flow path is always terminal. |
|
Should not have pushed just before leaving. I'll fix that, of course. |
sharkdp
force-pushed
the
alex/assert-narrowing
branch
from
112b9d6 to
2b8cbbd
Compare
|
Ok, this looks much better. The three new diagnostics on |
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks great to me (pending investigation of the new attrs diagnostics.)
The new psycopg2 diagnostics look related at least in part to not understanding cyclic control flow, though it's not clear to me why this PR surfaces them.
I think it's related to #17723. I will look into that tomorrow. |
sharkdp
force-pushed
the
alex/assert-narrowing
branch
from
2b8cbbd to
08db457
Compare
|
@AlexWaygood The new fuzz check claims it found a new panic on this branch. The following snippet panics indeed: class name_3[name_2: name_0]:
pass
name_3.name_4
assert name_3[name_0]but it also panics on If it's just the fuzzer test that's being flaky, maybe that's something to be aware of / to look into. |
Turns out this was also related to #17723. After fixing this, these new diagnostics are gone again.
It's true that we don't completely understand that code. But in the absence of cyclic control flow, we should not emit an error, because the assert … # some assertion that affected reachability constraints below (without the fix for #17723)
first = True
for params in params_seq:
if first: # always true, in the absence of cyclic control flow
pgq = …
first = False
else:
use(pgq) # unreachable, in the absence of cyclic control flowAfter fixing #17723, we now "correctly" identify that |
It's weird to me that it's highlighting this snippet specifically. It looks like something that could well be affected by this branch, since it includes an
Yes, this seems to be flakiness in the fuzzer test. Passing the |
|
@sharkdp there is a bug in the fuzzer script here... but it's not quite what it seemed. There is indeed code that involves What happened here is the following:
I'll update the minimization logic in the fuzzer script to be more sophisticated about this in the future, so that it doesn't produce such confusing results. I don't know if it's worth looking into the panic from https://gist.github.com/AlexWaygood/0925516f7b39516cf02f5dac6fca541e right now or not. As you say, we have plenty of pre-existing cases involving |
Ahh — that makes sense. Thank you for the analysis. I think I'm going to skip analyzing this further for now. |
* main: [red-knot] Use 'full' salsa backtrace output that includes durability and revisions (#17735) [red-knot] Initial support for protocol types (#17682) [red-knot] Computing a type ordering for two non-normalized types is meaningless (#17734) [red-knot] Include salsa backtrace in check and mdtest panic messages (#17732) [red-knot] Fix control flow for `assert` statements (#17702) [red-knot] Fix recording of negative visibility constraints (#17731) [red-knot] Update salsa (#17730) [red-knot] Support overloads for callable equivalence (#17698) [red-knot] Run py-fuzzer in CI to check for new panics (#17719) Upload red-knot binaries in CI on completion of linux tests (#17720) [`flake8-use-pathlib`] Fix `PTH123` false positive when `open` is passed a file descriptor from a function call (#17705)
|
Here is a minimized repro of something that did not panic prior to this PR, but does now (courtesy of a fixed version of the fuzzer script that I have locally): class name_3[name_2: name_0]:
pass
name_3.name_4
assert name_3[name_0]
import name_0 |
|
I think it is probably still the case that this PR just exposed a pre-existing issue rather than actually introducing a new issue. So I think it's still okay to just leave this for now. |
|
I also think so. The following snippet also panics (introduces the same reachability constraints than the class name_3[name_2: name_0]:
pass
name_3.name_4
if name_3[name_0]:
import name_0 |
|
And here's a fix for the fuzzer issue: #17739 |
3 participants
Summary
@sharkdp and I realised in our 1:1 this morning that our control flow for
assertstatements isn't quite accurate at the moment. Namely, for something like this:we currently reveal
Noneforxhere, but this is incorrect. In actual fact, themsgexpression of anassertstatement (the expression after the comma) will only be evaluated if the test (x is None) evaluates toFalse. As such, we should be adding a constraint of~Nonetoxin themsgexpression, which should simplify the inferred type ofxtointin that context ((int | None) & ~None->int).Test Plan
Mdtests added.