classification
Title: Import fails when doing a circular import involving an `import *`
Type: behavior Stage: patch review
Components: Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, Steven.Barker, antox, brett.cannon, eric.snow, rbcollins
Priority: normal Keywords: patch

Created on 2015-02-11 18:38 by antox, last changed 2016-09-08 18:02 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
23447_2.diff Steven.Barker, 2015-07-29 00:09 review
23447_3.diff Steven.Barker, 2015-09-09 03:42 patch with updated test review
issue23447.diff brett.cannon, 2015-09-11 21:06 review
23447_4.diff Steven.Barker, 2015-10-01 18:35 review
Messages (15)
msg235761 - (view) Author: Antonio Cota (antox) Date: 2015-02-11 18:38
That's the situation:

a/
  __init__.py
  first.py
  second.py

#init.py
__all__ = ['second', 'first']
print('i\'m starting the directory')

#first.py
print('hi, i\'m the first')
from . import *

#second.py
print('hi, i\'m the second')

From the interactive prompt:
>>> import a.first
i'm starting the directory
hi, i'm the first
hi, i'm the second
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/antox/Scrivania/a/first.py", line 2, in <module>
    from . import *
AttributeError: module 'a' has no attribute 'first'

It's pretty weird.
msg235836 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-02-12 15:30
If you put a print call after your `from . import *` call you will notice it never gets executed. Basically import is still in the middle of finishing imports when that import * is reached, including setting the module attributes on the package. Basically you have a circular import.
msg235847 - (view) Author: Antonio Cota (antox) Date: 2015-02-12 17:24
I tried the following on python 3.5.0a1:

#init.py
__all__ = ['second', 'first']
print('i\'m starting the directory')

#first.py
print('hi, i\'m the first')
from . import second

#second.py
print('hi, i\'m the second')
from . import first

>>> import a.first
i'm starting the directory
hi, i'm the first
hi, i'm the second


it just worked out perfectly, no errors.
But the case I show before still continues to get the AttributeError error.
You told me that basically it doesn't work because it is a circular import, but isn't python already able to manage circular import?
What I expected when running the "from . import *" statament was Python looking up in the __all__ attribute and import everything within it. When it had to import 'first' I expected Python to check in the sys.modules to see if it was already imported so, in this case, it could see that first.py was already imported and no error was raised.
msg235867 - (view) Author: Steven Barker (Steven.Barker) * Date: 2015-02-13 01:03
This issue is a special case of the problem discussed in issue 992389, that modules within packages are not added to the package dictionary until they are fully loaded, which breaks circular imports in the form "from package import module".

The consensus on that issue is that it doesn't need to be fixed completely, given the partial fix from issue 17636. I think the current issue is a corner case that was not covered by the fix, but which probably should be fixed as well, for consistency. The fix so far has made imports that name the module work, even though the module objects are still not placed into the package's namespace yet (this is why Antonio's last example works in the newly released 3.5a1, though not in previous versions). Wildcard imports however still fail.

Can the fix be expanded to cover wildcard imports as well? I know, we're heaping up two kinds of usually-bad code (wildcard imports and circular imports) on top of one another, but still, the failure is very unexpected to anyone who's not read the bug reports.

I don't know my way around the import system at all yet, so I'm not going to be capable of writing up a patch immediately, but if there's any interest at all, and nobody more knowledgeable gets to it first I might see what I can learn and try to put together something.

Here's a more concise example of the issue:


package/__init__.py:

    __all__ = ["module"]


package/module.py:

    from . import module # this works in 3.5a1, thanks to the fix from issue 17636
    from . import *      # this still fails
msg235905 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-02-13 17:16
If you want to take a stab at it, Steven, go for it and I will review the patch, but as you pointed out this is such an edge case that I'm personally not going to worry about fixing it and still don't consider it a bug.
msg247548 - (view) Author: Steven Barker (Steven.Barker) * Date: 2015-07-29 00:09
I've finally gotten around to looking into this issue and it turns out that fixing "from package import *" to work with circular imports is pretty easy, as all that needs to be done is apply the same logic from the patch for issue 17636 to the loop in the next function. I'll attach a patch that does this.

Unfortunately, my Windows build environment seems to be completely messed up at the moment and I'm getting link errors on everything in my python repo, so I've not been able to test the code at all. Since I have no idea when I'll actually have the time to learn what the hell's going wrong with MSVC, I though I'd submit my patch and perhaps somebody on a better OS can make sure it works properly. I've included a new test which should show the issue with circular imports (and verify that the rest of the patch fixes it, hopefully).
msg247610 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-07-29 19:29
Can someone reset the status and open fields, thanks.
msg247615 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-29 20:14
reset.
msg247616 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-07-29 20:16
Languishing? :)
msg247618 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-29 20:18
Yep: "The issue has no clear solution , e.g., no agreement on a technical solution or if it is even a problem worth fixing."

Brett is saying he doesn't consider this a bug.

Steven says he doesn't have time to push it forward.

Oh, I see there is a patch attached though (I wish the attachments were linked at the message that adds them as well as globally).
msg250274 - (view) Author: Steven Barker (Steven.Barker) * Date: 2015-09-09 03:42
I've managed to partially fix my build environment, so I can verify that my patch (attached previously) works as intended. I'm not completely sure that it doesn't break unrelated things as my build still has lots of failing tests (which all appear to be unrelated to this issue). Nothing obviously new breaks with the patch applied.

The test I added in the patch does fail (as expected) on the unpatched interpreter, but passes on a patched version (though I've just noticed that the attempt at catching the exception doesn't work, since the current import code raises an AttributeError rather than the ImportError I had expected). I don't believe any other tests failed anew on the patched code, but like I said, I've got a lot of unrelated test failures so I might be missing some subtle issues.

I'm attaching an updated version of the patch with better exception handling in the test code. The patch applies cleanly to the latest dev branch. I'd appreciate any reviews or other feedback.
msg250495 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-11 21:06
Here is Steven's patch touched up. I still have not decided if the semantic change is worth it. If anyone else has an opinion please feel free to speak up. All tests do pass, though, so it at least doesn't seem to directly break anything.
msg251612 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-25 21:00
Just so Steven knows, I'm still thinking about whether I want to accept the patch (and waiting to see if anyone else has an opinion =). I did realize that the patch as it stands does not add the found module as an attribute on the package which is potentially one way the semantics subtly shift with this patch.
msg252044 - (view) Author: Steven Barker (Steven.Barker) * Date: 2015-10-01 18:35
Thanks for looking at the issue Brett.

I think you're right that your patch has incorrect semantics, since it doesn't save the value to the provided namespace if it had to go through the special path to find the name. I think my patch got that part right (though I definitely missed one PyUnicode_Check).

I've just noticed that our patches may not always do the right thing in all error situations. They may return -1 without setting an exception (or perhaps passing on a strange one from an internal call) if there's something weird going on like `__name__` not existing or not being a string.

I'm attaching a new version of my patch that moves the PyErr_Format call down to the later check for `value == NULL`. This handles both the basic case where nothing strange goes on but the requested name is not found, and the weird cases where unexpected stuff goes wrong. It will replace any errors raised by the earlier code with an ImportError. I think an ImportError is the appropriate exception for most error cases, but perhaps some internal errors should not be overwritten and more complicated error logic is needed.

I also notice that the code in the "import_from" function (from which I borrowed heavily for my patch) was changed in one of the 3.5rc patches to have new error handling (it raises ImportError more often I think). I don't see an obvious way to copy its new error logic to import_all_from, but my C coding skills are rather rusty so I could be missing an easy approach.
msg275087 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-09-08 18:02
I talked the semantic change over with Eric at the core sprint and we are both iffy enough on the semantic change and how it will impact stuff that we don't feel comfortable accepting the patch. Sorry, Steven, but thanks for taking a stab at it.
History
Date User Action Args
2016-09-08 18:02:27brett.cannonsetstatus: open -> closed

nosy: + eric.snow
messages: + msg275087

assignee: brett.cannon ->
resolution: rejected
2015-10-01 18:35:53Steven.Barkersetfiles: + 23447_4.diff

messages: + msg252044
2015-09-25 21:00:34brett.cannonsetmessages: + msg251612
2015-09-11 21:06:43brett.cannonsetfiles: + issue23447.diff

messages: + msg250495
2015-09-09 15:36:26brett.cannonsetassignee: brett.cannon
2015-09-09 03:42:58Steven.Barkersetfiles: + 23447_3.diff

messages: + msg250274
2015-07-29 20:18:49rbcollinssetstatus: languishing -> open

messages: + msg247618
stage: needs patch -> patch review
2015-07-29 20:16:26BreamoreBoysetmessages: + msg247616
2015-07-29 20:14:30rbcollinssetstatus: closed -> languishing

nosy: + rbcollins
messages: + msg247615

resolution: not a bug -> (no value)
stage: needs patch
2015-07-29 19:29:26BreamoreBoysetnosy: + BreamoreBoy

messages: + msg247610
versions: + Python 3.6
2015-07-29 00:09:58Steven.Barkersetfiles: + 23447_2.diff
keywords: + patch
messages: + msg247548
2015-02-13 17:16:22brett.cannonsetmessages: + msg235905
title: Relative imports with __all__ attribute -> Import fails when doing a circular import involving an `import *`
2015-02-13 01:03:11Steven.Barkersetnosy: + Steven.Barker
messages: + msg235867
2015-02-12 17:25:53antoxsetversions: - Python 3.4
2015-02-12 17:24:40antoxsetmessages: + msg235847
2015-02-12 15:30:19brett.cannonsetstatus: open -> closed

nosy: + brett.cannon
messages: + msg235836

resolution: not a bug
2015-02-11 20:43:10antoxsetversions: + Python 3.4
2015-02-11 18:38:45antoxcreate