New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make binder handle unconditional ifs with no else #3567

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
2 participants
@Michael0x2a
Collaborator

Michael0x2a commented Jun 17, 2017

This pull request fixes #2673.

The problem was that in the semantic analysis phase, we can only ever mark blocks as unreachable. So, if we have code of the form:

if typing.TYPE_CHECKING:
    assert isinstance(...)

...nothing happens since there's no "else" block to mark as unreachable, which means the binder sees nothing special about this if statement and consequently ignores the special conditional.

I decided to just add a dummy empty "else" block if one doesn't already exist and mark it as unreachable. Two other solutions I considered were to add another field somewhere to record reachability or to repeat the special conditional checks in the typechecking phase, but these solutions seemed a bit redundant/wasteful.

Make binder handle unconditional ifs with no else
This commit fixes #2673.

The problem was that in the semantic analysis phase, we can only ever
mark blocks as *unreachable*. So, if we have code of the form:

```python
if typing.TYPE_CHECKING:
    assert isinstance(...)
```

...nothing happens since there's no "else" block to mark as unreachable,
which means the binder sees nothing special about this if statement and
ignores the special conditional.

I decided to just add a dummy empty "else" block if one doesn't already
exist and mark it as unreachable. Two other solutions I considered were
to add another field somewhere to record reachability or to repeat the
special conditional checks in the typechecking phase, but these
solutions seemed a bit redundant/wasteful.
@Michael0x2a

This comment has been minimized.

Show comment
Hide comment
@Michael0x2a

Michael0x2a Jun 17, 2017

Collaborator

Also, to clarify, this fix applies to any if statements involving OS platform checks, Python version checks, "are we using mypy" checks, and the like.

I'm not sure if "unconditional ifs" is really the right phrase to use here, but naming things is hard...

Collaborator

Michael0x2a commented Jun 17, 2017

Also, to clarify, this fix applies to any if statements involving OS platform checks, Python version checks, "are we using mypy" checks, and the like.

I'm not sure if "unconditional ifs" is really the right phrase to use here, but naming things is hard...

@JukkaL

JukkaL approved these changes Jun 20, 2017

Thanks for fixing this! Looks good to me.

@JukkaL JukkaL merged commit fc974b1 into python:master Jun 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:binder-ignoring-conditional-ifs-with-no-else branch Jun 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment