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

About the "assert" bytecode #79061

Closed
vthenoathena mannequin opened this issue Oct 3, 2018 · 15 comments
Closed

About the "assert" bytecode #79061

vthenoathena mannequin opened this issue Oct 3, 2018 · 15 comments
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@vthenoathena
Copy link
Mannequin

vthenoathena mannequin commented Oct 3, 2018

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
    Copy link
    Mannequin Author

    vthenoathena mannequin commented Oct 3, 2018

    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?

    @vthenoathena vthenoathena mannequin added the type-bug An unexpected behavior, bug, or error label Oct 3, 2018
    @stevendaprano
    Copy link
    Member

    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
    Copy link
    Mannequin

    thautwarm mannequin commented Oct 3, 2018

    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
    Copy link
    Mannequin

    thautwarm mannequin commented Oct 3, 2018

    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
    Copy link
    Member

    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
    Copy link
    Mannequin

    thautwarm mannequin commented Oct 3, 2018

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

    nascheme commented Oct 3, 2018

    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
    Copy link
    Mannequin

    arigo mannequin commented Oct 5, 2018

    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
    Copy link
    Member

    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.

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Oct 8, 2018
    @thautwarm
    Copy link
    Mannequin

    thautwarm mannequin commented Oct 9, 2018

    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.

    @ZackerySpytz ZackerySpytz mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.9 only security fixes and removed 3.8 only security fixes labels Aug 1, 2019
    @serhiy-storchaka
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants