About the "assert" bytecode

[vthenoathena]

BPO	34880
Nosy	@nascheme, @ericvsmith, @stevendaprano, @serhiy-storchaka, @ZackerySpytz, @thautwarm
PRs	bpo-34880: assert statement misbehavior if AssertionError is shadowed #15073

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-08-25.13:11:49.473>
created_at = <Date 2018-10-03.06:58:30.947>
labels = ['interpreter-core', 'type-bug', '3.9']
title = 'About the "assert" bytecode'
updated_at = <Date 2019-08-25.13:11:49.471>
user = 'https://bugs.python.org/vthenoathena'

bugs.python.org fields:

activity = <Date 2019-08-25.13:11:49.471>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2019-08-25.13:11:49.473>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2018-10-03.06:58:30.947>
creator = 'vtheno athena'
dependencies = []
files = []
hgrepos = []
issue_num = 34880
keywords = ['patch']
message_count = 15.0
messages = ['326942', '326944', '326954', '326955', '326957', '326959', '326969', '326983', '326987', '326988', '327119', '327361', '327427', '350444', '350452']
nosy_count = 7.0
nosy_names = ['nascheme', 'eric.smith', 'steven.daprano', 'serhiy.storchaka', 'ZackerySpytz', 'thautwarm', 'vtheno athena']
pr_nums = ['15073']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue34880'
versions = ['Python 3.9']

[vthenoathena]

When I was involved in the YaPyPy project, I found a problem, An source "assert 0" will compile to an bytecode, On that bytecode sequence, it always raise a "AssertionError" from global, when we rewrite global "AssertionError" like here:

class AssertionError(Exception):
    def __init__(self,msg):
        self.msg = f"User AssertionError: {msg}"
        # other code

so, "assert" is meta-programming?

[stevendaprano]

If I have understood you correctly, you are asking whether it is expected behaviour for assert to use the global AssertionError instead of the built-in AssertionError.

I don't see why it shouldn't. In any case, that behaviour goes back to Python 1.5 and perhaps older.

I don't think this is a bug, so I'm going to close this. If you disagree, please re-open it with more detail explaining why you think it is a bug.

[thautwarm]

Steven, this problem is quite interesting because it just allows users to cause fatal errors without any invalid operations.

AssertionError = 1
assert 1 == 2, 2
---------------------------------------------------------------------------
TypeError: 'int' object is not callable

It's better to deal with it seriously.

[thautwarm]

If we could generate LOAD_CONST instructions for assert statements, any prospective dangers could be avoided.

from dis import dis
@dis
def f():
    assert x
    
  3           0 LOAD_GLOBAL              0 (x)
              2 POP_JUMP_IF_TRUE         8
              4 LOAD_GLOBAL              1 (AssertionError)
              6 RAISE_VARARGS            1
        >>    8 LOAD_CONST               0 (None)
             10 RETURN_VALUE

[stevendaprano]

On Wed, Oct 03, 2018 at 09:49:06AM +0000, thautwarm wrote:

Steven, this problem is quite interesting because it just allows users
to cause fatal errors without any invalid operations.

How is that different from every other case of shadowing a builtin?

len = 45
print(len("hello world"))

The ability to shadow builtins is a feature, not a bug.

I have no specific objection to changing assert so that it raises the
actual honest-to-goodness AssertionError, but that would be an
enhancement, not a bug-fix.

(To be honest, that's what I assumed it would do, until I tried it.)

[thautwarm]

Reply to this:

How is that different from every other case of shadowing a builtin?

len = 45
print(len("hello world"))

AssertionError = 42
assert 1 != 2

assert implicitly invokes AssertionError, while len does that explicitly. That is to say, simply changing a global variable breaks the work of a keyword.

Another difference is that shadow builtins could be resumed in the
nested functions without something like globals() or exec(..., {}), while you cannot perform this to the breakage of assert:

len = 1
def g():
   from builtins import len

   return len([1, 2, 3])

g() # => 3

AssertionError = +1

def f():
   from builtins import AssertionError
   assert False

f() # boooom

[ericvsmith]

I think this is a bug that should be fixed. This is similar to how f-strings used to work: the generated byte code would call format(something-or-other), and if you'd defined a name "format" in your code it would fail.

Now admittedly "format" is more common than "AssertionError", but in any event I don't think assert should fail because of a name you happen to be using. That's a surprising action-at-a-distance, in my mind.

And I especially think that's true in the case of assert: when an assert fires, the last thing I want is something that I normally wouldn't have tested for causing me to not see what the assertion is.

And, I think a broader discussion on python-dev might be useful, too, in order to get more opinions.

[stevendaprano]

On Wed, Oct 03, 2018 at 01:56:00PM +0000, Eric V. Smith wrote:

I think this is a bug that should be fixed.

Supporting this position, shadowing other exceptions doesn't change the
exception generated by the interpreter:

py> TypeError = None
py> 1 + "a"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'int' and 'str'

On the other hand, this has been the behaviour going back to Python 1.5,
it is hard to see why this is worse than any other example of shadowing.
I don't think there's anything in the documentation that says that
assert *shouldn't* do a LOAD_GLOBAL on AssertionError.

Hence this would be an enhancement rather than a bug fix.

And, I think a broader discussion on python-dev might be useful, too,
in order to get more opinions.

I agree. I think we need to clarify the intent here, and then decide
that if it is a bug, should we bother fixing it in pre-3.8 versions?

[stevendaprano]

And, I think a broader discussion on python-dev might be useful, too, in order to get more opinions.

Done:

https://mail.python.org/pipermail/python-dev/2018-October/155410.html

Changing the status to Pending until we have more clarity on whether this is a bug, feature, accident or whatnot.

[nascheme]

Assuming it is not crazy complicated to fix, I would like to to be changed. It should work like the TypeError example. That fact that it has worked the current way since Python 1.5 isn't a strong argument. I think no one should be surprised if it changes. Also, expecting other Python implementations to match the LOAD_GLOBAL behavior seems to put unnecessary burden on them. If someone writes code that relies on that behavior, they should not be surprised if it gets broken, IMHO.

[arigo]

A middle ground might be to copy the behavior of __import__: it is loaded from the builtins module where specific hacks can change its definition, but it is not loaded from the globals.

[serhiy-storchaka]

This can't be changed in 3.6. There are two ways of changing it:

1. Make the marshal module supporting AssertionError as it supports booleans, None, Ellipsis and StopIteration. This will require adding the new marshal format version.

2. Add a new opcode in bytecode.

The latter way is easier. Bytecode is changed in every feature release, and it was already changed in 3.8. New marshal versions are added less often.

There was similar issue with StopAsyncIteration. See bpo-33041.

[thautwarm]

Hi, Serhiy, there could be an another way to fix all this sort of problems IMO.

We can figure out all the cases that implicitly require shadow builtins, and then change symtable visitor to add corresponding free variables with name mangling, for instance:

1. implicitly add free variable ".AssertionError"

def f():
   assert 1

where ".AssertionError" is a name-mangled free variable and is assigned once the module is executed.

The same to StopAsyncIteration, TypeError, __build_class__ and so on.

[serhiy-storchaka]

New changeset ce6a070 by Serhiy Storchaka (Zackery Spytz) in branch 'master':
bpo-34880: Add the LOAD_ASSERTION_ERROR opcode. (GH-15073)
ce6a070

[serhiy-storchaka]

Thank you Zackery for your contribution.

where ".AssertionError" is a name-mangled free variable and is assigned once the module is executed.

But this still allows to overriding builtin AssertionError before importing a module. And this solution would be much more complex that adding a new opcode.