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

Named tuples in import cycles still fail sometimes #4442

Closed
ilevkivskyi opened this issue Jan 8, 2018 · 10 comments · Fixed by #5635
Closed

Named tuples in import cycles still fail sometimes #4442

ilevkivskyi opened this issue Jan 8, 2018 · 10 comments · Fixed by #5635
Assignees
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-0-high topic-import-cycles topic-named-tuple

Comments

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Jan 8, 2018

It looks like issue #3054 was not completely fixed. Namely, in some scenarios named tuples
in import cycles result in Invalid type. For example, checking this file

import posix

results in

typeshed/stdlib/3/os/__init__.pyi:331: error: Invalid type "posix.uname_result"
typeshed/stdlib/3/os/__init__.pyi:599: error: Invalid type "posix.times_result"
typeshed/stdlib/3/os/__init__.pyi:605: error: Invalid type "posix.waitid_result"
typeshed/stdlib/3/os/__init__.pyi:622: error: Invalid type "posix.sched_param"
typeshed/stdlib/3/os/__init__.pyi:624: error: Invalid type "posix.sched_param"
typeshed/stdlib/3/os/__init__.pyi:625: error: Invalid type "posix.sched_param"

Note that the above "invalid" types are all named tuples and there is an import cycle os <-> posix. Interestingly, import os and from os import * both typecheck cleanly.

I haven't yet tried to find a simple repro (not involving typeshed).

@ilevkivskyi
Copy link
Member Author

Somehow this breaks typshed build since recently, so I raised priority to high.

JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this issue Jan 9, 2018
This removes the circular dependency between the os and posix stub, which
is somehow triggering python/mypy#4442. We should ideally fix the mypy bug,
but since it's easy enough to fix the import cycle, we might as well do that
too.
JelleZijlstra added a commit to JelleZijlstra/typeshed that referenced this issue Jan 9, 2018
This removes the circular dependency between the os and posix stub, which
is somehow triggering python/mypy#4442. We should ideally fix the mypy bug,
but since it's easy enough to fix the import cycle, we might as well do that
too.
matthiaskramm pushed a commit to python/typeshed that referenced this issue Jan 9, 2018
This removes the circular dependency between the os and posix stub, which
is somehow triggering python/mypy#4442. We should ideally fix the mypy bug,
but since it's easy enough to fix the import cycle, we might as well do that
too.
@elliott-beach
Copy link
Contributor

elliott-beach commented Jan 10, 2018

I deleted code from the typestub until I got, in posix.pyi,

from os import stat_result

# This was NamedTuple('etc'), but that doesn't affect the error.
uname_result = "dummy" # E: Invalid type "posix.uname_result" 

and in os/__init__.pyi,

stat_result = "dummy"
from posix import T
def uname() -> uname_result: ...

so there is something fishy with the way circular imports are resolved, but it doesn't seem to me to involve NamedTuple specifically.

@elliott-beach
Copy link
Contributor

elliott-beach commented Jan 10, 2018

Actually, I think this is just a case of the bad circular import discussed in https://stackoverflow.com/a/40094439/5749914 (posix.py is Y.py, os/__init__.py is X.py)

Mypy executes from os import stat_result, but then proceeds to import the rest of os/__init__.pyi, resulting in an error when uname_result is undefined.

@gvanrossum
Copy link
Member

gvanrossum commented Jan 10, 2018 via email

@elliott-beach
Copy link
Contributor

elliott-beach commented Jan 10, 2018

Yes, but if it wouldn't work by python's runtime, I figure there is no reason to expect it to work for mypy, at least in this case IMO.

@elliott-beach
Copy link
Contributor

elliott-beach commented Jan 11, 2018

@ilevkivskyi I think this ought to be closed because the behavior is reasonable, what do you think?

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 8, 2018

@ilevkivskyi Can you come up with a self-contained example that reproduces the problem? I'd like to know if #4695 fixed this issue, and if not, I'll look at fixing this.

@ilevkivskyi
Copy link
Member Author

@JukkaL Unfortunately, I didn't yet find a simple repro, but there is a "dirty" way of testing it by just using the offending typeshed commit 81751306883. I just tried it, and the error is still present, it was not fixed by #4695.

@ilevkivskyi
Copy link
Member Author

@JukkaL

and if not, I'll look at fixing this.

OK, I will spend some more time now to find a simple repro for you.

@ilevkivskyi
Copy link
Member Author

So, here is a simplest repro I can find:

# file a.py
class C:
    pass

from b import tp
x: tp

# file b.py
from a import C
from typing import NamedTuple

tp = NamedTuple('tp', [('x', int)])

Then one needs to run mypy b.py and get:

a.py:5: error: Invalid type "b.tp"

Note that error disappears if tp is a normal class.

Also note that the same error also appears if one or both of a and b are packages and use named tuples in __init__.py.

@ilevkivskyi ilevkivskyi added the false-positive mypy gave an error on correct code label Jun 8, 2018
@ilevkivskyi ilevkivskyi self-assigned this Sep 18, 2018
ilevkivskyi added a commit that referenced this issue Sep 21, 2018
Fixes #5275 
Fixes #4498 
Fixes #4442 

This is a simple _band-aid_ fix for `Invalid type` in import cycles where type aliases, named tuples, or typed dicts appear. Note that this is a partial fix that only fixes the `Invalid type` error when a type alias etc. appears in type context. This doesn't fix errors (e.g. `Cannot determine type of X`) that may appear if the type alias etc. appear in runtime context.

The motivation for partial fix is two-fold:
* The error often appears in stub files (especially for large libraries/frameworks) where we have more import cycles, but no runtime context at all.
* Ideally we should refactor semantic analysis to have deferred nodes, and process them in smaller passes when there is more info (like we do in type checking phase). But this is _much_ harder since this requires a large refactoring.  Also an alternative fix of updating node of every `NameExpr` and `MemberExpr` in third pass is costly from performance point of view, and still nontrivial.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-0-high topic-import-cycles topic-named-tuple
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants