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

Bug in IMPORT_NAME #71539

Closed
serhiy-storchaka opened this issue Jun 19, 2016 · 11 comments
Closed

Bug in IMPORT_NAME #71539

serhiy-storchaka opened this issue Jun 19, 2016 · 11 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 27352
Nosy @brettcannon, @birkenfeld, @ncoghlan, @vstinner, @benjaminp, @ericsnowcurrently, @serhiy-storchaka, @1st1
Files
  • import_name_level.patch
  • import_name_level_2.patch
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2016-06-27.20:54:17.536>
    created_at = <Date 2016-06-19.11:53:12.714>
    labels = ['interpreter-core', 'type-feature']
    title = 'Bug in IMPORT_NAME'
    updated_at = <Date 2016-06-27.20:54:17.511>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-06-27.20:54:17.511>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2016-06-27.20:54:17.536>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2016-06-19.11:53:12.714>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['43470', '43545']
    hgrepos = []
    issue_num = 27352
    keywords = ['patch']
    message_count = 11.0
    messages = ['268849', '268931', '268939', '268940', '269306', '269385', '269394', '269395', '269401', '269402', '269403']
    nosy_count = 9.0
    nosy_names = ['brett.cannon', 'georg.brandl', 'ncoghlan', 'vstinner', 'benjamin.peterson', 'python-dev', 'eric.snow', 'serhiy.storchaka', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27352'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    Seems there is a bug in the implementation of the IMPORT_NAME opcode in ceval.c.

    If the level argument is -1, it is not passed to __import__ (should never happen, but looks correct). If it is an integer != -1 in C long range, it is passed to __import__ (this is correct). But if it is not integer (e.g. None) or can't be converted to C long, an exception is set and __import__ is called with level and not cleared error (this is wrong).

    In correct bytecode the level argument can be either integer or None. Default __import__ accepts only integers as the level argument and checks the range. Proposed patch makes the code always passing the level argument to __import__ unless it is None.

    @serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jun 19, 2016
    @brettcannon
    Copy link
    Member

    My suspicion is the -1 aspect is a hold-over from that being the default level value in Python 2.7 that no one removed.

    The patch LGTM. Do we need a test for this for some reason?

    @serhiy-storchaka
    Copy link
    Member Author

    Thak you for the explanation Brett.

    Since current "import" without "from" is not broken, I suppose that the raised exception is cleared somewhere later. The patch doesn't have visible effect (unless may be using nonstandard loaders or __import__, don't know). It just cleans up the code, avoid raising unneeded exception (small performance boost) and may be fix some hidden non-reproducible bug.

    @brettcannon
    Copy link
    Member

    Then I say go ahead and commit it.

    @serhiy-storchaka
    Copy link
    Member Author

    Sorry, I was wrong. The level argument is always non-negative integer (if a bytecode is not corrupted). The second branch can be just removed.

    Following patch also fixes the validation of the ImportFrom AST node.

    @serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jun 26, 2016
    @brettcannon
    Copy link
    Member

    LGTM

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 27, 2016

    New changeset e3164c9edb0b by Serhiy Storchaka in branch 'default':
    Issue bpo-27352: Correct the validation of the ImportFrom AST node and simplify
    https://hg.python.org/cpython/rev/e3164c9edb0b

    @serhiy-storchaka
    Copy link
    Member Author

    Thanks Brett.

    @vstinner
    Copy link
    Member

    You broke Python! Example:

    http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%203.x/builds/4853/steps/test/logs/stdio

    ======================================================================
    FAIL: test_importfrom (test.test_ast.ASTValidatorTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 757, in test_importfrom
        self.stmt(imp, "level less than -1")
      File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 579, in stmt
        self.mod(mod, msg)
      File "/Users/buildbot/buildarea/3.x.murray-snowleopard/build/Lib/test/test_ast.py", line 571, in mod
        self.assertIn(msg, str(cm.exception))
    AssertionError: 'level less than -1' not found in 'Negative ImportFrom level'

    @vstinner vstinner reopened this Jun 27, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 27, 2016

    New changeset e5063a82f490 by Serhiy Storchaka in branch 'default':
    Issue bpo-27352: Fixed an error message in a test.
    https://hg.python.org/cpython/rev/e5063a82f490

    @serhiy-storchaka
    Copy link
    Member Author

    Thanks Victor!

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants