Change ConditionalTypeBinder to mainly use with statements by ecprice · Pull Request #1731 · python/mypy

ecprice · 2016-06-22T10:11:31Z

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 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:

The last commit just moves the type binder code into its own file
Many of the changed lines only have changed indentation.

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.

JukkaL · 2016-06-22T10:33:33Z

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?

rwbarton · 2016-06-22T13:20:08Z

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.

rwbarton · 2016-06-23T14:53:59Z

        # 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())


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?

Yeah, that's probably right. Changed.

rwbarton · 2016-06-23T17:01:46Z

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!

rwbarton · 2016-06-23T22:54:59Z

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

testRedefinedFunctionInTryWithElse needs to not crash mypy, at least. It's probably an extraction from a real program that uses this pattern with imports that may fail. If it's easy to make mypy do the right thing here, then great, otherwise it may be tolerable if we need # type: ignore comments for now.
The fix for the list comprehension scoping bug seems questionable to me. I'd prefer it if you removed that commit from this PR (since the bug is pre-existing anyways) and address the issue in a separate PR so it doesn't block this one.
The Frame.changed stuff is pretty confusing, which it looks like you've just addressed; I'll take a look at that now.

- 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.

ecprice · 2016-06-23T23:52:22Z

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)

rwbarton · 2016-06-24T01:58:22Z

I've also dropped the move to another file until this lands.

Oh, just to make the PR easier to review? Or what? Not sure I follow.

ecprice · 2016-06-24T02:07:03Z

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.

rwbarton · 2016-06-24T14:29:24Z

+        x = B()
+    except:
+        pass
+    finally:


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?

rwbarton · 2016-06-24T14:48:39Z

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.

ecprice · 2016-06-24T16:12:24Z

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.

ecprice · 2016-06-24T18:57:27Z

(I've added another commit fixing a bug.)

ecprice added 7 commits June 21, 2016 23:48

Fix bug with type binder and for/else

d539e6d

Also make it share more code with while, and add a test case.

Fix bug with ConditionalTypeBinder and if/else.

cfc3f2b

Previously if statements with else: and without else: had two different code paths, and one had a bug.

Remove canskip option from ConditionalTypeBinder

09dc8f5

It was only used once, so simpler to just do the logic there.

Rename fallthrough to fall_through for consistency.

5eab5ed

Having fallthrough and clear_breaking was confusing.

Comment explaining the frame_context() function.

9d14b23

Keep track of whether a try/except block can fall off its end.

a9f8ced

ecprice force-pushed the typebinder branch from 25c698e to a8c1a9a Compare June 22, 2016 10:35

ecprice added 2 commits June 22, 2016 13:26

Cleaner frame_context() interface.

9c21fe3

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.

Remove unused functions

88650b7

ecprice force-pushed the typebinder branch 2 times, most recently from bd2b757 to 427f3ea Compare June 23, 2016 05:25

rwbarton reviewed Jun 23, 2016
View reviewed changes

ecprice force-pushed the typebinder branch 2 times, most recently from ba7af85 to 8bafc26 Compare June 23, 2016 22:42

ecprice added 13 commits June 23, 2016 16:40

Explanatory comments

01d98a9

Remove collapse_ancestry option, and always do it

296746b

Fixes for try/finally binder

2dc37f3

Updates to binder

23e2444

- make declarations a separate variable, not frames[0] - get rid of add_on_return

Add another test for dependencies

b910c49

Cleaner binder dependencies

dcae0e9

Remove unused binder function update_expand

0d830dd

Remove broken/changed from Frame, place in binder.last_pop_*

fc40c75

Move options_on_return into binder, making Frame empty

0d9bcef

Formatting improvements

a396173

Always clear breaking_out in pop_frame()

edba00f

It's a bit simpler than doing so if fall_through, and we don't rely on the distinction.

cleanups

c6f1a29

More comments

737ccb3

ecprice force-pushed the typebinder branch from f3c08f3 to 737ccb3 Compare June 23, 2016 23:41

Don't crash on the function redefinition example

6a67b83

ecprice force-pushed the typebinder branch from b51856d to 6a67b83 Compare June 23, 2016 23:55

rwbarton reviewed Jun 24, 2016
View reviewed changes

ecprice added 2 commits June 24, 2016 09:03

Test changes

26d4ce5

Move binder into its own file.

151dae8

rwbarton merged commit a702c40 into python:master Jun 26, 2016

rwbarton mentioned this pull request Jun 26, 2016

Error when checking function conditionally defined in try/except/else clause #1289

Closed

Navigation Menu

Repository navigation

Change ConditionalTypeBinder to mainly use with statements - #1731

Change ConditionalTypeBinder to mainly use with statements#1731
rwbarton merged 28 commits into
python:masterfrom
ecprice:typebinder

ecprice commented Jun 22, 2016

Uh oh!

JukkaL commented Jun 22, 2016

Uh oh!

rwbarton commented Jun 22, 2016

Uh oh!

rwbarton Jun 23, 2016

Uh oh!

ecprice Jun 23, 2016

Uh oh!

rwbarton commented Jun 23, 2016

Uh oh!

rwbarton commented Jun 23, 2016

Uh oh!

ecprice commented Jun 23, 2016 •

edited

Loading

Uh oh!

rwbarton commented Jun 24, 2016

Uh oh!

ecprice commented Jun 24, 2016

Uh oh!

rwbarton Jun 24, 2016

Uh oh!

rwbarton commented Jun 24, 2016

Uh oh!

ecprice commented Jun 24, 2016

Uh oh!

ecprice commented Jun 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ecprice commented Jun 22, 2016

Uh oh!

JukkaL commented Jun 22, 2016

Uh oh!

rwbarton commented Jun 22, 2016

Uh oh!

rwbarton Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

ecprice Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

rwbarton commented Jun 23, 2016

Uh oh!

rwbarton commented Jun 23, 2016

Uh oh!

ecprice commented Jun 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwbarton commented Jun 24, 2016

Uh oh!

ecprice commented Jun 24, 2016

Uh oh!

rwbarton Jun 24, 2016

Choose a reason for hiding this comment

Uh oh!

rwbarton commented Jun 24, 2016

Uh oh!

ecprice commented Jun 24, 2016

Uh oh!

ecprice commented Jun 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ecprice commented Jun 23, 2016 •

edited

Loading