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

Differing exception between builtins and importlib when importing beyond top-level package #81625

Closed
brettcannon opened this issue Jun 28, 2019 · 13 comments
Labels
3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir

Comments

@brettcannon
Copy link
Member

BPO 37444
Nosy @brettcannon, @rhettinger, @ncoghlan, @ericsnowcurrently, @serhiy-storchaka, @hroncok, @miss-islington, @benjimin
PRs
  • bpo-37444: Update differing exception between builtins and importlib #14869
  • 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-06.21:05:52.653>
    created_at = <Date 2019-06-28.19:22:13.296>
    labels = ['interpreter-core', '3.8', 'library', '3.9']
    title = 'Differing exception between builtins and importlib when importing beyond top-level package'
    updated_at = <Date 2020-01-23.17:51:31.624>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2020-01-23.17:51:31.624>
    actor = 'brett.cannon'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-08-06.21:05:52.653>
    closer = 'brett.cannon'
    components = ['Interpreter Core', 'Library (Lib)']
    creation = <Date 2019-06-28.19:22:13.296>
    creator = 'brett.cannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37444
    keywords = ['patch']
    message_count = 13.0
    messages = ['346855', '346858', '346859', '346860', '347021', '348215', '348943', '360394', '360433', '360434', '360496', '360523', '360570']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'rhettinger', 'ncoghlan', 'eric.snow', 'serhiy.storchaka', 'hroncok', 'miss-islington', 'Ben Lewis2']
    pr_nums = ['14869']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue37444'
    versions = ['Python 3.8', 'Python 3.9']

    @brettcannon
    Copy link
    Member Author

    builtins.__import__() raises ValueError (as it did in Python 2.7) while importlib.__import__() raises ImportError (which makes more sense to me).

    Found by Ben Lewis.

    @brettcannon brettcannon added 3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir labels Jun 28, 2019
    @brettcannon
    Copy link
    Member Author

    Importllib code raising the exception is

    bits = package.rsplit('.', level - 1)
    if len(bits) < level:
    raise ValueError('attempted relative import beyond top-level package')
    .

    @brettcannon
    Copy link
    Member Author

    I think this should be an ImportError and so that means builtins.__import__() is wrong. Anyone want to argue for the other side?

    @brettcannon
    Copy link
    Member Author

    import.c code raising the exception:

    cpython/Python/import.c

    Lines 1675 to 1677 in f9f8e3c

    _PyErr_SetString(tstate, PyExc_ValueError,
    "attempted relative import beyond top-level "
    "package");

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 1, 2019

    ImportError sounds right to me.

    We already raise that just above this for the "no dots at all" case, so also raising it for the "not enough dots" case makes more sense than leaving them different.

    @rhettinger
    Copy link
    Contributor

    I think this should be an ImportError

    I also think ImportError is what people would expect when an import fails.

    @miss-islington
    Copy link
    Contributor

    New changeset c5fa449 by Miss Islington (bot) (Ngalim Siregar) in branch 'master':
    bpo-37444: Update differing exception between builtins and importlib (GH-14869)
    c5fa449

    @hroncok
    Copy link
    Mannequin

    hroncok mannequin commented Jan 21, 2020

    While raising ImportError certainly makes much more sense in this case, this change is backwards incompatible and there was no DeprecationWarning. I don't consider that fair.

    As a certain compromise, we could maybe raise a custom ImportError+ValueError offspring (e.g. BeyondToplevelImportError)?

    If feeling fancy, we could rise a DeprecationWarning when issublcass(BeyondToplevelImportError, ValueError) is used?

    @brettcannon
    Copy link
    Member Author

    There's no deprecation warning because how do you warn about that? You raise DeprecationWarning about an exception you have yet to raise?

    And I personally think calling this unfair is a bit harsh.

    @hroncok
    Copy link
    Mannequin

    hroncok mannequin commented Jan 21, 2020

    I know that raising a DeprecationWarning here is most likely not possible or too black-magical to do. My intention was not to be harsh, sorry about that. I just wanted to point out that a backwards incompatible behavior like this without a (possible) deprecation warning might not counterweigh the gain of raising the logical exception type. As a compromise, I proposed to raise a hybrid that inherits from both.

    @brettcannon
    Copy link
    Member Author

    The problem with the hybrid exception is that importlib would then still raise a different exception from import itself, so the discrepancy would persist. And if you updated import you could then break people in another way where they were catching ValueError and ImportError separately for different cases and now they would potentially catch the wrong exception for that ValueError case.

    Basically changing what exceptions get raised sucks as someone is almost inevitably broken. But since import and importlib strive to mirror each other as much as possible and be drop-in replacements, the consistency for the next 30 years of Python's life is more important I think than the case of:

    1. Someone using importlib.import_module()
    2. Who is specifically worried about relative imports going too far and no other import-related import and thus only catching ValueError and not ImportError
    3. And are supporting older versions of Python
    4. And will have a difficult time updating their except clause to now also capture ImportError

    @hroncok
    Copy link
    Mannequin

    hroncok mannequin commented Jan 22, 2020

    For the record, looking at https://docs.python.org/3.9/whatsnew/3.9.html I had an impression that this also affects regular imports, as described in https://bugzilla.redhat.com/show_bug.cgi?id=1791769#c2 -- if that's not that case, I'm sorry.

    Anyway, I get your point of "next 30 years of Python".

    @brettcannon
    Copy link
    Member Author

    Sorry, you're right; I forgot importlib got it "right" while import got it "wrong". :)

    @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.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants