Fix #5758 -- plain import didn't work with namespace package #5762

gvanrossum · 2018-10-09T23:45:37Z

No description provided.

gvanrossum · 2018-10-10T17:33:40Z

So here's a fix for part two of #5758. The way it works is, first, when find_module() encounters an installed non-stub package with a py.typed marker underneath a namespace parent module, it remembers the namespace ancestor(s) in a special cache dict. Then if a later request for one of those ancestors is received and it is not found in the usual way, the cached ancestor's directory is produced.

But that's not all that's needed! @macbeth322 -- you were spot on with your suggestion that this had something to do with the ancestor dependencies in all_imported_modules_in_file()! I ended up moving the "main" dependency to the top so it precedes the ancestors; that way, the first call to find_module() is e.g. find_module('foo.bar.baz'), which squirrels away the namespace ancestor as I described above.

This order dependency is ugly and caused a bunch of tests to fail (since the errors about missing packages come out in a different order) but it's the best I could come up with.

~~(As I write this there are some more failures, I have to look into those later.)~~

chrisphilip322 · 2018-10-10T18:05:34Z

I was able to test ~~0fa27de~~ a569fb2 and it looks like it works for all the edge cases I found.

Edit: I copied the wrong commit

msullivan

Looks good, with one comment request and one test request.
If the test request is super painful then it is probably OK to skip for now if we need to cherry-pick this, but it seems like the sort of thing that is super easy to break later, so.

msullivan · 2018-10-10T21:20:29Z

mypy/build.py

@@ -572,12 +572,12 @@ def correct_rel_imp(imp: Union[ImportFrom, ImportAll]) -> str:
                    pri = import_priority(imp, PRI_MED)
                    ancestor_pri = import_priority(imp, PRI_LOW)
                    for id, _ in imp.ids:
+                        res.append((pri, id, imp.line))


This somewhat subtle order dependency might deserve some comment.

msullivan · 2018-10-10T21:39:31Z

mypy/modulefinder.py

+        # installed package with a py.typed marker that is a
+        # subpackage of a namespace package.  We only fess up to these
+        # if we would otherwise return "not found".
+        return self.ns_ancestors.get(id)


Is it possible to add a test for this case?

gvanrossum · 2018-10-10T22:29:47Z

Added a comment. I believe adding a test for this situation would require adding a big chunk of code to testpep561.py. I'd rather focus on other aspects of the release, but I'll file a bug to remind myself.

ilevkivskyi

Looks good! I think it is fine to add the test later.

Fixes #5758. (Sadly there's no test for the second half of that issue; I've opened #5767 to track adding one.)

Fix python#5758 -- plain import didn't work with namespace package

0fa27de

gvanrossum mentioned this pull request Oct 9, 2018

Importing types from namespace packages emits false positives without from ... import ... #5758

Closed

Guido van Rossum added 4 commits October 9, 2018 18:07

Fix lint

7ae1b53

Possibly a fix for part two of python#5758

8090838

Fix some tests

9b1e1dd

Fix more tests

6326ed3

Fix type error

a569fb2

msullivan approved these changes Oct 10, 2018

View reviewed changes

Add a comment about the subtle order dependecy

9ff6a16

gvanrossum mentioned this pull request Oct 10, 2018

Add a test for #5758 part two #5767

Closed

ilevkivskyi approved these changes Oct 11, 2018

View reviewed changes

gvanrossum merged commit 725247b into python:master Oct 11, 2018

gvanrossum deleted the ns-bug branch October 11, 2018 17:21

gvanrossum added a commit that referenced this pull request Oct 11, 2018

Fix some edge cases of namespace package imports (#5762)

9d62ac8

Fixes #5758. (Sadly there's no test for the second half of that issue; I've opened #5767 to track adding one.)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5758 -- plain import didn't work with namespace package #5762

Fix #5758 -- plain import didn't work with namespace package #5762

Fix #5758 -- plain import didn't work with namespace package #5762

Fix #5758 -- plain import didn't work with namespace package #5762

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment