classification
Title: Raise an ImportWarning when __spec__.parent/__package__ isn't defined for a relative import
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, eric.snow, ncoghlan, pitrou, python-dev, scoder, superluser
Priority: low Keywords: patch

Created on 2015-12-03 20:48 by brett.cannon, last changed 2017-06-26 21:50 by scoder. This issue is now closed.

Files
File name Uploaded Description Edit
issue25791.patch superluser, 2016-01-13 21:32 review
issue25791_2.patch superluser, 2016-01-13 23:00 review
Messages (19)
msg255838 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-12-03 20:48
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().
msg258171 - (view) Author: Rose Ames (superluser) * Date: 2016-01-13 21:34
Patch with tests.  Not sure if importlib.h should be included?
msg258172 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-01-13 21:36
Including importlib.h doesn't hurt.
msg258173 - (view) Author: Rose Ames (superluser) * Date: 2016-01-13 21:39
that's what I figured.
msg258178 - (view) Author: Rose Ames (superluser) * Date: 2016-01-13 23:00
Thanks for the quick review, new patch uploaded.
msg258329 - (view) Author: Roundup Robot (python-dev) Date: 2016-01-15 21:33
New changeset 6908b2c9a404 by Brett Cannon in branch 'default':
Issue #25791: Raise an ImportWarning when __spec__ or __package__ are
https://hg.python.org/cpython/rev/6908b2c9a404
msg258330 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-01-15 21:33
Thanks for the patch, Rose! I did notice your review comments about the missing goto and the stacklevel and I simply added them myself.
msg258354 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-01-16 04:43
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.
msg258400 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-01-16 18:18
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.
msg258842 - (view) Author: Roundup Robot (python-dev) Date: 2016-01-22 23:27
New changeset 219c44fe8968 by Brett Cannon in branch 'default':
Issue #25791: Warn when __package__ != __spec__.parent.
https://hg.python.org/cpython/rev/219c44fe8968
msg294632 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-05-28 08:32
This new warning is introducing difficulties for some Cython-compiled modules: https://github.com/cython/cython/issues/1720
msg294652 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2017-05-28 18:03
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 (https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Objects/moduleobject.c#L80).
msg294653 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-05-28 18:10
AFAICT, Cython simply calls PyModule_Create() on Python 3.
msg294656 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2017-05-28 19:18
OK, so the warning is triggered if __package is None or __spec__ is None (https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Lib/importlib/_bootstrap.py#L1038). That's defined in _calc___package__() which is only called if index != 0 when calling __import__() (https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Lib/importlib/_bootstrap.py#L1062 or https://github.com/python/cpython/blob/ac5bbd43bc7b769c13ae0412cb28a3521f4d4ff1/Python/import.c#L1520).

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.
msg294657 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-05-28 19:25
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__)
msg294668 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-05-29 04:10
Is it possible Cython is still supporting pre-PEP-328 style implicit relative imports, even in Python 2.7+?
msg294677 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-05-29 07:10
> 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?
msg294678 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-05-29 07:14
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.
msg296951 - (view) Author: Stefan Behnel (scoder) * Date: 2017-06-26 21:50
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.
History
Date User Action Args
2017-06-26 21:50:10scodersetmessages: + msg296951
2017-05-29 07:14:33pitrousetmessages: + msg294678
2017-05-29 07:10:17pitrousetmessages: + msg294677
2017-05-29 04:10:22ncoghlansetmessages: + msg294668
2017-05-28 19:25:15pitrousetmessages: + msg294657
2017-05-28 19:18:17brett.cannonsetmessages: + msg294656
2017-05-28 18:10:24pitrousetnosy: + scoder
messages: + msg294653
2017-05-28 18:03:30brett.cannonsetmessages: + msg294652
2017-05-28 08:32:06pitrousetnosy: + pitrou
messages: + msg294632
2016-01-22 23:28:43brett.cannonlinkissue21762 dependencies
2016-01-22 23:27:30brett.cannonsetstatus: open -> closed
2016-01-22 23:27:05python-devsetmessages: + msg258842
2016-01-16 18:18:45brett.cannonsetstatus: closed -> open

messages: + msg258400
2016-01-16 04:43:41ncoghlansetmessages: + msg258354
2016-01-15 21:33:44brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg258330

stage: test needed -> resolved
2016-01-15 21:33:11python-devsetnosy: + python-dev
messages: + msg258329
2016-01-14 00:20:29brett.cannonsetassignee: brett.cannon
2016-01-13 23:00:29superlusersetfiles: + issue25791_2.patch

messages: + msg258178
2016-01-13 21:39:28superlusersetmessages: + msg258173
2016-01-13 21:36:08brett.cannonsetmessages: + msg258172
2016-01-13 21:34:49superlusersetmessages: + msg258171
2016-01-13 21:32:36superlusersetfiles: + issue25791.patch
keywords: + patch
2015-12-05 22:27:56superlusersetnosy: + superluser
2015-12-03 20:48:12brett.cannoncreate