Skip to content
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

Second expression in assert statement should be checked as conditional #7318

Merged
merged 5 commits into from Aug 22, 2019

Conversation

@gantsevdenis
Copy link
Contributor

commented Aug 10, 2019

Fixes #7284

@gantsevdenis gantsevdenis changed the title fix right side assertion fix7284: second expr should be checked as conditional Aug 10, 2019
Copy link
Collaborator

left a comment

Thanks! Generally looks good, I have couple small comments.

test-data/unit/check-expressions.test Outdated Show resolved Hide resolved
if s.msg is not None:
with self.binder.frame_context(can_skip=False, fall_through=1):
self.push_type_map(else_map)
self.expr_checker.accept(s.msg)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Aug 12, 2019

Collaborator

It looks like there is a code duplication with ExpressionChecker.analyze_cond_branch() why can't you re-use it here? (If there is a reason I am missing please add a comment).

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Aug 16, 2019

Author Contributor

I must have missed it! I ll check that

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Aug 21, 2019

Collaborator

It looks like there is either a misunderstanding, or you did something wrong in git. I just asked if it is possible to re-use the existing helper here, but you just removed the context manger instead, which is wrong. Namely, if the else_map is None (i.e. the condition is statically always true), then binder will mark the current frame unreachable.

For example, in this test the missing return statement will be no more caught:

[case testAssertNew]
def f() -> int:  # Correct error here on master, no error on your branch.
    x: int
    assert isinstance(x, int), 'Hm...'
[builtins fixtures/isinstance.pyi]

Btw, since this was not caught by our test suite, could you please add such test? (Maybe also copy few other basic assert tests adding an assert message to them.)

@gantsevdenis gantsevdenis force-pushed the gantsevdenis:fix_assertion branch from 1ed672d to cc1cb35 Aug 21, 2019
@gantsevdenis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Actually I wasn't sure how to use analyze_cond_branch() (I think its 3rd argument, context, is None here), so I went with what I thought, was the safest decision.. turned out to be wrong

So I have added the test that you have suggested, I also added another test with isinstance because with my first attempt that test would fail while it shoudn't
Thank you for your review time

true_map, else_map = self.find_isinstance_check(s.expr)
if s.msg is not None:
with self.binder.frame_context(can_skip=False, fall_through=1):
self.expr_checker.analyze_cond_branch(else_map, s.msg, None)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Aug 22, 2019

Collaborator

Hm, sorry for being annoying, but it looks like something is still wrong, now two frames are being pushed, one manually, and one in the helper. Why can't you just use the helper here?

The None context is indeed right, because we have no expectations about what should be the assertion "message" (it can be arbitrary object, not just a string).

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Aug 22, 2019

Author Contributor

Why can't you just use the helper here?

Mostly because I misunderstood the binder and its fall_through argument
I ve chosen a low-priority issue for a reason xD thanks again for your patience

Copy link
Collaborator

left a comment

Thanks for your patience, LGTM now!

@ilevkivskyi ilevkivskyi changed the title fix7284: second expr should be checked as conditional Second expression in assert statement should be checked as conditional Aug 22, 2019
@ilevkivskyi ilevkivskyi merged commit 95e8c7a into python:master Aug 22, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.