Change ConditionalTypeBinder to mainly use with statements #1731
Conversation
Rather than use `binder.push_frame()` and `binder.pop_frame(*args)`, you now do `with binder.frame_context(*args)`. There was a previous context manager used as `with binder`, but it was only used in a few places because it didn't support options. In the process, I discovered several bugs in the old control flow analysis; new test cases have been added to catch them.
This allows the context manager to deal with having multiple parallel blocks that can break out separately. It will clear the breaking_out parameter and propagate variables to an enclosing frame only if breaking_out wasn't True after falling off the frame.
Also make it share more code with while, and add a test case.
Previously if statements with else: and without else: had two different code paths, and one had a bug.
It was only used once, so simpler to just do the logic there.
Having fallthrough and clear_breaking was confusing.
|
Thanks for cleaning things up! I won't be able to review this until next week, as I'll be traveling the rest of the week. Perhaps @rwbarton can have a look? |
|
Yes, I've just been taking a look at the conditional type binder this past weekend, and I'd be happy to review this. |
I realized that fall_through and clear_breaking really served the same purpose, just one is for the parent and one is for the grandparent; now there's just fall_through, which is an integer.
ecprice
force-pushed
the
typebinder
branch
2 times, most recently
from
bd2b757
to
427f3ea
Compare
| @@ -81,13 +106,13 @@ def __init__(self) -> None: | |||
| self.frames = [] # type: List[Frame] | |||
| # The first frame is special: it's the declared types of variables. | |||
| self.frames.append(Frame()) | |||
| # We want a second frame that we can update. | |||
| self.frames.append(Frame()) | |||
There was a problem hiding this comment.
I notice this second frame's parent is not the first frame. Is the first frame so special that it should just be a separate field of the binder object?
There was a problem hiding this comment.
Yeah, that's probably right. Changed.
|
Apparently when I commented from the individual commit view it shows up here as "commented on an outdated diff" here, so please make sure to expand all of those! |
ecprice
force-pushed
the
typebinder
branch
2 times, most recently
from
ba7af85
to
8bafc26
Compare
|
Thanks a bunch for working on this! This already looks much better than the original version. I have some further clean-up/restructuring changes I'd like to make in the near future so it'd be great if we can land this one quickly. The major points are
|
- make declarations a separate variable, not frames[0] - get rid of add_on_return
It's a bit simpler than doing so if fall_through, and we don't rely on the distinction.
|
OK, I've dropped the questionable list comprehension change, and made mypy give a note rather than crash in the exposed test. I've also dropped the move to another file until this lands. Note that runtests.py currently has a few errors (Cannot determine type of 'frame_context') but they will disappear once this is in its own file. (Not sure if there's a github issue for that) |
Oh, just to make the PR easier to review? Or what? Not sure I follow. |
|
Yeah, just until you think it's ready to commit. It's annoying to make changes with the move on top, since git isn't smart enough to rebase it. |
| x = B() | ||
| except: | ||
| pass | ||
| finally: |
There was a problem hiding this comment.
Can you add a failing access x.b here so we can see that mypy doesn't just think the type of x is B?
|
Okay, this looks good to me aside from a few minor test improvements. Please add the move commit back on top and I'll merge this. |
|
Done. |
Previously it cleared binder.breaking_out, but this had an issue with return: we want to set binder.breaking_out before the complicated logic there with multiple exit points, but evaluating boolean expressions would clear the flag.
|
(I've added another commit fixing a bug.) |
Rather than use
binder.push_frame()andbinder.pop_frame(*args),you now do
with binder.frame_context(*args). There was a previouscontext manager used as
with binder:, but it was only used in a fewplaces because it didn't support versatile enough options.
The rewrite exposed a number of bugs in the old implementation, which I have fixed and added tests for. It also exposed a bug with redefining functions, which I have not fixed; testRedefinedFunctionInTryWithElse is skipped for now.
For reviewing this, note that: