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

Raise an ImportWarning when __spec__.parent/__package__ isn't defined for a relative import #69977

Closed
brettcannon opened this issue Dec 3, 2015 · 19 comments
Assignees
Labels
interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@brettcannon
Copy link
Member

brettcannon commented Dec 3, 2015

BPO 25791
Nosy @brettcannon, @ncoghlan, @pitrou, @scoder, @ericsnowcurrently
Files
  • issue25791.patch
  • issue25791_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/brettcannon'
    closed_at = <Date 2016-01-22.23:27:30.158>
    created_at = <Date 2015-12-03.20:48:12.375>
    labels = ['interpreter-core', 'type-feature']
    title = "Raise an ImportWarning when __spec__.parent/__package__ isn't defined for a relative import"
    updated_at = <Date 2017-06-26.21:50:10.205>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2017-06-26.21:50:10.205>
    actor = 'scoder'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2016-01-22.23:27:30.158>
    closer = 'brett.cannon'
    components = ['Interpreter Core']
    creation = <Date 2015-12-03.20:48:12.375>
    creator = 'brett.cannon'
    dependencies = []
    files = ['41609', '41611']
    hgrepos = []
    issue_num = 25791
    keywords = ['patch']
    message_count = 19.0
    messages = ['255838', '258171', '258172', '258173', '258178', '258329', '258330', '258354', '258400', '258842', '294632', '294652', '294653', '294656', '294657', '294668', '294677', '294678', '296951']
    nosy_count = 7.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'pitrou', 'scoder', 'python-dev', 'eric.snow', 'superluser']
    pr_nums = []
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25791'
    versions = ['Python 3.6']

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 3, 2015

    When you do a relative import, package is used to help resolve it, but if it isn't defined we fall back on using name and path. We should probably consider raising an ImportWarning if package isn't defined so that some day we can consider cleaning up import and its call signature to be a bit more sane and drop the globals argument. It would also help people catch errors where they went overboard deleting attributes off a module.

    We should probably even extend it to start using __spec__.parent and then falling back to __package__, and only after both are found missing do we raise the ImportWarning about needing to use __name__ and __path__. That way __import__ can simply start taking in the spec of the calling module to do all of its work instead of having to pass all of globals().

    @brettcannon brettcannon added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Dec 3, 2015
    @superluser
    Copy link
    Mannequin

    superluser mannequin commented Jan 13, 2016

    Patch with tests. Not sure if importlib.h should be included?

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jan 13, 2016

    Including importlib.h doesn't hurt.

    @superluser
    Copy link
    Mannequin

    superluser mannequin commented Jan 13, 2016

    that's what I figured.

    @superluser
    Copy link
    Mannequin

    superluser mannequin commented Jan 13, 2016

    Thanks for the quick review, new patch uploaded.

    @brettcannon brettcannon self-assigned this Jan 14, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 15, 2016

    New changeset 6908b2c9a404 by Brett Cannon in branch 'default':
    Issue bpo-25791: Raise an ImportWarning when __spec__ or __package__ are
    https://hg.python.org/cpython/rev/6908b2c9a404

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jan 15, 2016

    Thanks for the patch, Rose! I did notice your review comments about the missing goto and the stacklevel and I simply added them myself.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jan 16, 2016

    Favouring __spec__.parent over __package__ will break the documented workaround in PEP-366 for enabling explicit relative imports from __main__ even when a module is run directly instead of via -m:

        if __name__ == "__main__" and __package__ is None:
            __package__ = "expected.package.name"

    It may be that workaround is something we *want* to break (as per http://bugs.python.org/issue21762#msg258332 ), but if so, it's at least worthy of a porting note in the What's New document.

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jan 16, 2016

    As I commented on another issue, I think I'm going to tweak the change to continue to prefer __package__, but raise ImportWarning when it doesn't match __spec__.parent. Once we do that for all attributes we can wait until Python 2.7 is done and then swap priority to __spec__ and raise a DeprecationWarning if __spec__ is missing. That way post-Python 2.7 we can move entirely to a spec-based import system.

    @brettcannon brettcannon reopened this Jan 16, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 22, 2016

    New changeset 219c44fe8968 by Brett Cannon in branch 'default':
    Issue bpo-25791: Warn when __package__ != __spec__.parent.
    https://hg.python.org/cpython/rev/219c44fe8968

    @pitrou
    Copy link
    Member

    pitrou commented May 28, 2017

    This new warning is introducing difficulties for some Cython-compiled modules: cython/cython#1720

    @brettcannon
    Copy link
    Member Author

    brettcannon commented May 28, 2017

    Is Cython not defining actual module objects or working around types.ModuleType? I'm just trying to figure out how a Cython module is ending up in a place where the attributes that are set by PyModule_NewObject() aren't there (

    PyModule_NewObject(PyObject *name)
    ).

    @pitrou
    Copy link
    Member

    pitrou commented May 28, 2017

    AFAICT, Cython simply calls PyModule_Create() on Python 3.

    @brettcannon
    Copy link
    Member Author

    brettcannon commented May 28, 2017

    OK, so the warning is triggered if __package is None or __spec__ is None (

    _warnings.warn("can't resolve package from __spec__ or __package__, "
    ). That's defined in _calc___package__() which is only called if index != 0 when calling __import__() (
    package = _calc___package__(globals_)
    or
    if (level > 0) {
    ).

    So the question becomes how is Cython importing modules? This warning should only be triggered if you're attempting a relative import from within a package (and thus have a level > 0). Based on Antoine's Cython issue, since something like multidict isn't a package it would suggest either Cython is setting the level > 0 when it doesn't mean to or there's a bug somewhere with level == 0 and yet Python is still trying to calculate the parent package for no reason.

    @pitrou
    Copy link
    Member

    pitrou commented May 28, 2017

    multidict is a package, the Cython module is multidict._multidict.

    Cython translated "import sys" into this:

      __pyx_t_1 = __Pyx_Import(__pyx_n_s_sys, 0, -1); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 5, __pyx_L1_error)

    And __Pyx_Import is this:
    https://gist.github.com/pitrou/9a0d94aba3495ce3eb8630d4797d67bb

    Note that __Pyx_MODULE_NAME is defined thusly:
    #define __Pyx_MODULE_NAME "multidict._multidict"

    So perhaps Cython is indeed attempting a PyImport_ImportModuleLevelObject()
    call with a level of 1 for "import sys" before falling back on a level of -1...

    Why I don't know.

    (also since __Pyx_MODULE_NAME seems known, Cython could actually define __package__)

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented May 29, 2017

    Is it possible Cython is still supporting pre-PEP-328 style implicit relative imports, even in Python 2.7+?

    @pitrou
    Copy link
    Member

    pitrou commented May 29, 2017

    Is it possible Cython is still supporting pre-PEP-328 style implicit relative imports, even in Python 2.7+?

    That might be the case. I can't find anything in the Cython docs. Stefan, could you shed a light?

    @pitrou
    Copy link
    Member

    pitrou commented May 29, 2017

    Ok, I did an experiment. I added "from __future__ import absolute_import" at the top of _multidict.c, and after a recompile the warning went away.

    What changed was that the following line:

      __pyx_t_1 = __Pyx_Import(__pyx_n_s_sys, 0, -1); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 5, __pyx_L1_error)

    was replaced with:

      __pyx_t_1 = __Pyx_Import(__pyx_n_s_sys, 0, 0); if (unlikely(!__pyx_t_1)) __PYX_ERR(0, 3, __pyx_L1_error)

    Ignore the change from "3" to "5", which is the line number for the traceback. The relevant change is the "level" argument to __Pyx_Import() which went from -1 to 0.

    @scoder
    Copy link
    Contributor

    scoder commented Jun 26, 2017

    Sorry for not responding, missed the message, it seems.

    Cython has to support old-style relative imports also in Py3 because that's how the user code was originally written, using Py2-style syntax and semantics. Most Cython code has not been converted to Py3 syntax/semantics, but still needs to work in Py3.

    From a user perspective, it's best to switch on "__future__.absolute_import" and use explicit relative imports (or write Py3 code), because it reduces the overhead at import time.

    I'm hoping to look into multi-step module initialisation this summer. As Nick noted, this might also help with this issue.

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

    No branches or pull requests

    4 participants