Skip to content

bpo-34880: assert statement misbehavior if AssertionError is shadowed#15073

Merged
serhiy-storchaka merged 7 commits into
python:masterfrom
ZackerySpytz:bpo-34880-AssertionError-shadowing-assert
Aug 25, 2019
Merged

bpo-34880: assert statement misbehavior if AssertionError is shadowed#15073
serhiy-storchaka merged 7 commits into
python:masterfrom
ZackerySpytz:bpo-34880-AssertionError-shadowing-assert

Conversation

@ZackerySpytz

@ZackerySpytz ZackerySpytz commented Aug 1, 2019

Copy link
Copy Markdown
Contributor

The assert statement was not working properly if the AssertionError
exception was being shadowed.

https://bugs.python.org/issue34880

Comment thread Lib/importlib/_bootstrap_external.py Outdated
Comment thread Lib/test/test_exceptions.py Outdated
Comment thread Lib/test/test_exceptions.py
Comment thread Lib/test/test_exceptions.py
Comment thread Python/ceval.c Outdated
@markshannon

Copy link
Copy Markdown
Member

I'd like to keep any new bytecodes as simple as possible. The simplest I can think of is a LOAD_ASSERTION_ERROR which would be equivalent to LOAD_CONSTANT AssertionError.
It would also keep the changes to the compiler to a minimum.

@serhiy-storchaka

Copy link
Copy Markdown
Member

I concur with @markshannon. There is no benefit from implementing it as a single bytecode: it is neither performance critical nor should be atomical. Using LOAD_ASSERTION_ERROR would simplify the compiler and the bytecode interpreter a bit.

@ZackerySpytz

Copy link
Copy Markdown
Contributor Author

Thanks for all of the comments. I have implemented LOAD_ASSERTION_ERROR. Please take a look.

Comment thread Python/compile.c Outdated

@fusioncid fusioncid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better for "{" at the end of the previous line.

@ZackerySpytz

Copy link
Copy Markdown
Contributor Author

@qigangxu All of the code in this PR follows the PEP 7 and PEP 8 style guides, so I'm not sure what the purpose of your comment is.

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but it needs to wait until #15320 be merged.

Comment thread Lib/test/test_exceptions.py
@fusioncid

Copy link
Copy Markdown
Contributor

@qigangxu All of the code in this PR follows the PEP 7 and PEP 8 style guides, so I'm not sure what the purpose of your comment is.

Sorry, no code location information was provided last comment. The issue is in line 3327 of Python/compile.c.

@serhiy-storchaka serhiy-storchaka merged commit ce6a070 into python:master Aug 25, 2019
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Aug 25, 2019
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
Fix assert statement misbehavior if AssertionError is shadowed.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
Fix assert statement misbehavior if AssertionError is shadowed.
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
Fix assert statement misbehavior if AssertionError is shadowed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants