Fix crashes in class scoped imports #12023
Conversation
Fixes python#11045, fixes huggingface/transformers#13390 Fixes python#10488 Fixes python#7045 Fixes python#7806 Fixes python#11641 Fixes python#11351 Fixes python#10488 Co-authored-by: @A5rocks
hauntsaninja
changed the title
This comment has been minimized.
This comment has been minimized.
|
Does anyone know what mypyc wants from us? where L4805 is: and (and type checking proper is happy) |
|
@JelleZijlstra thanks for the feedback! I fixed the self binding behaviour and changed things so that we get line numbers for the "already defined" errors |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hauntsaninja
force-pushed
the
class-scope-import
branch
from
073ed86
to
7bd60db
Compare
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| class Foo: | ||
| import mod | ||
|
|
||
| reveal_type(Foo.mod.foo) # N: Revealed type is "builtins.int" |
There was a problem hiding this comment.
I think reveal_type(Foo.mod) will also be helpful here.
There was a problem hiding this comment.
Okay, can add!
There was a problem hiding this comment.
It's revealed as builtins.object, not types.ModuleType, but pretty sure that's just because of the stub fixtures
| @@ -131,9 +131,9 @@ def f() -> None: pass | |||
| [case testImportWithinClassBody2] | |||
| import typing | |||
| class C: | |||
| from m import f | |||
| from m import f # E: Method must have at least one argument | |||
There was a problem hiding this comment.
This seems to be placed on the wrong line.
There was a problem hiding this comment.
This is actually on the correct line. In this kind of situation, C.f is capable of working like a method would. However, because this f takes no arguments, if you try to do C().f() you'd get a TypeError at runtime. See also: #7045 (comment)
I agree that for the code in this test, where there is no attempt to use C.f as a method, the error is a little surprising. But I think the code in this test is not particularly idiomatic. Maybe we could make it only error on the caller side if they try to call it as a method, but implementing that is not easy. So I'd like to keep that out of scope for this PR, which fixes crashes and whose behaviour is technically correct.
|
|
||
| symbol_node: Optional[SymbolNode] = node.node | ||
|
|
||
| if self.is_class_scope(): |
There was a problem hiding this comment.
Maybe we can somehow create a new function here to make the inference better? Right now - this is sooo complex, but I understand the problem you have with mypyc 🙂
There was a problem hiding this comment.
mypy is actually able to narrow the types just fine. If you remove f, everything type checks perfectly. It's only mypyc that's the issue :'( I've tried a lot of variants, if you have a suggestion that you think is better and that mypyc will understand, I'm happy to try!
| @@ -2722,7 +2722,7 @@ import m | |||
|
|
|||
| [file m.py] | |||
| class C: | |||
| from mm import f | |||
| from mm import f # E: Method must have at least one argument | |||
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
|
||
| [case testClassScopeImportModuleStar] | ||
| class Foo: | ||
| from mod import * |
There was a problem hiding this comment.
This is actually a syntax error, you can only do import * in the global scope (at least in Python 3).
There was a problem hiding this comment.
Huh, TIL. Tried on Python 2 and it works but gives me a SyntaxWarning. Thanks Python 3!
There was a problem hiding this comment.
I think tests should tell us about syntax errors. Why do they pass? 🤔
There was a problem hiding this comment.
Most syntax errors happen at parse time, but some, like this one, do not. The ones that happen later often cause problems for mypy, e.g. this: #11499 or from __future__ import braces
There was a problem hiding this comment.
Another example is nonlocal x at module level
More context in python#12197 This essentially walks back changes in python#12023 that apply to function-like things that are imported in class-scope. We just issue a (non-blocking) error and mark the type as Any. Inference for variables still works fine; I'm yet to think of any problems that could cause. A full fix seems quite difficult, but this is better than both a crash and a blocking error.
Fixes #12197 This essentially walks back changes in #12023 that apply to function-like things that are imported in class-scope. We just issue a (non-blocking) error and mark the type as Any. Inference for variables still works fine; I'm yet to think of any problems that could cause. A full fix seems quite difficult, but this is better than both a crash and a blocking error.
Builds on #11344 and addresses feedback provided in that PR.
Fixes #11045, fixes huggingface/transformers#13390
Fixes #10488
Fixes #7045
Fixes #7806
Fixes #11641
Fixes #11351
Fixes #10488
Co-authored-by: @A5rocks