Incremental bug involving aliases and/or nested modules or classes and/or import cycles #2133
Comments
|
The issue appears elusive. I wonder if it's related to #2126 (the second bullet)? I may have run mypy with strict optional and that may have corrupted the cache. |
|
(Issue #2126 looking less likely now that I've seen the exact same set of errors from two different repos.) |
|
This is almost certainly related to import cycles; the error "Invalid type" almost exclusively stems from those. |
|
I've got a repro! When run with (which is correct, since that file was touched). This is a clue that something not quite right has ended up in the cache. I'll investigate... |
|
Shorter repro, involving a package with just three minimal files: Then run This produces one error message on the second run: |
|
Some more of the story:
|
This introduces None entries in modules for those modules known to exist in the graph. This fixes the issue at the cost of making the type of modules Dict[str, Optional[MypyFile]]. I have to think about whether I like this enough; there may be another solution. Also, this still needs tests.
|
I've got a possible fix at https://github.com/python/mypy/tree/fix2133. Not sure whether I like it that much. The solution inserts None entries into the modules dict just so that SemanticAnalyzer.visit_import_from() can tell whether a module is supposed to exist or not. An alternative fix would be to add the parent package to the submodule's dependencies with a low priority, rather than the current approach of maintaining a separate list of ancestors which are taken into account sometimes but not other times. I haven't tried to code that up yet to see how it would go. |
|
@Michael0x2a Any thoughts about this? It's taken me all day Friday to get this far... |
|
@gvanrossum --ah, sorry. I meant to respond sooner, but got side-tracked by grading :( I'll try and take a look tonight/tomorrow morning. |
This takes the case "from P import M" (where P is a package and M is a submodule) and adds a dependency on P to the current module with a new, super-low priority. The effect on the example from #2133 is that the requests package gets forced into an SCC with requests.sessions. This then makes SemanticAnalyzer.visit_import_from() take the intended path because modules['requests'] is populated -- this happens because within an SCC, if any member is stale, all are processed as stale, and this causes requests to be parsed before the semantic analysis of requests.sessions happens. The cost is that packages are more likely to be treated as cycles. Then again the cause of the problem was in a sense that the requests package is one big cycle (the parent package imports many of its submodules) but we had artificially excluded the dependency from the children on the parent (see the old version of the long comment updated by this commit).
|
The alt fix also misses tests; I am still meditating on which fix I like better. Note that there would also be a cop-out way to fix this, by making some subtle changes to requests/sessions.pyi. But that would just keep the bug in incremental until the next time someone has a package with a similar structure. |
|
The cost of increased circularity is real. The alt fix puts requests and 7 submodules in a single SCC (requests.cookies requests.utils requests.auth requests.models requests.api requests.adapters requests.sessions requests) where there were previously 8 independent singletons. |
|
There's yet another fix I'd like to try. It's more similar to the first version, making subtle changes to SemanticAnalyzer.visit_import_from() in case a module is deemed to exist but hasn't been processed yet. But instead of pre-populating the modules dict with None values, it should use a separate data structure to indicate whether the module exists or not (e.g. derived from graph.keys()). |
This takes the case "from P import M" (where P is a package and M is a submodule) and adds a dependency on P to the current module with a new, super-low priority. The effect on the example from #2133 is that the requests package gets forced into an SCC with requests.sessions. This then makes SemanticAnalyzer.visit_import_from() take the intended path because modules['requests'] is populated -- this happens because within an SCC, if any member is stale, all are processed as stale, and this causes requests to be parsed before the semantic analysis of requests.sessions happens. The cost is that packages are more likely to be treated as cycles. Then again the cause of the problem was in a sense that the requests package is one big cycle (the parent package imports many of its submodules) but we had artificially excluded the dependency from the children on the parent (see the old version of the long comment updated by this commit).
Fixes #2133. This is the third fix for this issue. I think I've finally nailed it. The solution is not to bail out early when the parent module is unavailable -- if all the imports are submodules of a package that's acceptable (the bug was that in incremental mode it's possible that the parent is fresh and has not been entered into the modules dict yet, but the bail-out caused the import to be marked as missing).
|
I like this fix best! https://github.com/python/mypy/tree/fix2133alt2 (It still needs a test.) |
|
Now with test. I think I'll just merge this. |
Fixes #2133. The solution is not to bail out early when the parent module is unavailable -- if all the imports are submodules of a package that's acceptable (the bug was that in incremental mode it's possible that the parent is fresh and has not been entered into the modules dict yet, but the bail-out caused the import to be marked as missing).
Uh oh!
There was an error while loading. Please reload this page.
I'm encountering a complex bug in incremental. One of the ways it appears is spurious errors complaining about some types in the requests stubs, e.g.
There's no need to panic but I'd like to have an issue I can point to.
The text was updated successfully, but these errors were encountered: