8000 Fix crashes in class scoped imports by hauntsaninja · Pull Request #12023 · python/mypy · GitHub
[go: up one dir, main page]

Skip to content

Fix crashes in class scoped imports #12023

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

Merged
merged 33 commits into from
Feb 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@
reduce memory use).
"""

import copy
from contextlib import contextmanager

from typing import (
List, Dict, Set, Tuple, cast, TypeVar, Union, Optional, Callable, Iterator, Iterable
Any, List, Dict, Set, Tuple, cast, TypeVar, Union, Optional, Callable, Iterator, Iterable
)
from typing_extensions import Final, TypeAlias as _TypeAlias

Expand All @@ -78,7 +79,7 @@
typing_extensions_aliases,
EnumCallExpr, RUNTIME_PROTOCOL_DECOS, FakeExpression, Statement, AssignmentExpr,
ParamSpecExpr, EllipsisExpr, TypeVarLikeExpr, implicit_module_attrs,
MatchStmt,
MatchStmt, FuncBase
)
from mypy.patterns import (
AsPattern, OrPattern, ValuePattern, SequencePattern,
Expand Down Expand Up @@ -4802,7 +4803,38 @@ def add_imported_symbol(self,
module_hidden: bool) -> None:
"""Add an alias to an existing symbol through import."""
assert not module_hidden or not module_public
symbol = SymbolTableNode(node.kind, node.node,

symbol_node: Optional[SymbolNode] = node.node

if self.is_class_scope():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Collaborator Author
@hauntsaninja hauntsaninja Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

# I promise this type checks; I'm just making mypyc issues go away.
# mypyc is absolutely convinced that `symbol_node` narrows to a Var in the following,
# when it can also be a FuncBase. Once fixed, `f` in the following can be removed.
# See also https://github.com/mypyc/mypyc/issues/892
f = cast(Any, lambda x: x)
if isinstance(f(symbol_node), (FuncBase, Var)):
# For imports in class scope, we construct a new node to represent the symbol and
# set its `info` attribute to `self.type`.
existing = self.current_symbol_table().get(name)
if (
# The redefinition checks in `add_symbol_table_node` don't work for our
# constructed Var / FuncBase, so check for possible redefinitions here.
existing is not None
and isinstance(f(existing.node), (FuncBase, Var))
and f(existing.type) == f(symbol_node).type
):
symbol_node = existing.node
else:
# Construct the new node
constructed_node = copy.copy(f(symbol_node))
assert self.type is not None # guaranteed by is_class_scope
constructed_node.line = context.line
constructed_node.column = context.column
constructed_node.info = self.type
constructed_node._fullname = self.qualified_name(name)
symbol_node = constructed_node

symbol = SymbolTableNode(node.kind, symbol_node,
module_public=module_public,
module_hidden=module_hidden)
self.add_symbol_table_node(name, symbol, context)
Expand Down
109 changes: 109 additions & 0 deletions test-data/unit/check-classes.test
Original file line number Diff line number Diff line change
Expand Up @@ -7134,3 +7134,112 @@ class B(A): # E: Final class __main__.B has abstract attributes "foo"
[case testUndefinedBaseclassInNestedClass]
class C:
class C1(XX): pass # E: Name "XX" is not defined

[case testClassScopeImportFunction]
class Foo:
from mod import foo

reveal_type(Foo.foo) # N: Revealed type is "def (x: builtins.int, y: builtins.int) -> builtins.int"
reveal_type(Foo().foo) # E: Invalid self argument "Foo" to attribute function "foo" with type "Callable[[int, int], int]" \
# N: Revealed type is "def (y: builtins.int) -> builtins.int"
[file mod.py]
def foo(x: int, y: int) -> int: ...

[case testClassScopeImportVariable]
class Foo:
from mod import foo

reveal_type(Foo.foo) # N: Revealed type is "builtins.int"
reveal_type(Foo().foo) # N: Revealed type is "builtins.int"
[file mod.py]
foo: int

[case testClassScopeImportModule]
class Foo:
import mod

reveal_type(Foo.mod) # N: Revealed type is "builtins.object"
reveal_type(Foo.mod.foo) # N: Revealed type is "builtins.int"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think reveal_type(Foo.mod) will also be helpful here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, can add!

[file mod.py]
foo: int

[case testClassScopeImportFunctionAlias]
class Foo:
from mod import foo
bar = foo

from mod import const_foo
const_bar = const_foo

reveal_type(Foo.foo) # N: Revealed type is "def (x: builtins.int, y: builtins.int) -> builtins.int"
reveal_type(Foo.bar) # N: Revealed type is "def (x: builtins.int, y: builtins.int) -> builtins.int"
reveal_type(Foo.const_foo) # N: Revealed type is "builtins.int"
reveal_type(Foo.const_bar) # N: Revealed type is "builtins.int"
[file mod.py]
def foo(x: int, y: int) -> int: ...
const_foo: int

[case testClassScopeImportModuleStar]
class Foo:
from mod import *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a syntax error, you can only do import * in the global scope (at least in Pyt 67E6 hon 3).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, TIL. Tried on Python 2 and it works but gives me a SyntaxWarning. Thanks Python 3!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think tests should tell us about syntax errors. Why do they pass? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another example is nonlocal x at module level


reveal_type(Foo.foo) # N: Revealed type is "builtins.int"
reveal_type(Foo.bar) # N: Revealed type is "def (x: builtins.int) -> builtins.int"
reveal_type(Foo.baz) # E: "Type[Foo]" has no attribute "baz" \
# N: Revealed type is "Any"
[file mod.py]
foo: int
def bar(x: int) -> int: ...

[case testClassScopeImportFunctionNested]
class Foo:
class Bar:
from mod import baz

reveal_type(Foo.Bar.baz) # N: Revealed type is "def (x: builtins.int) -> builtins.int"
reveal_type(Foo.Bar().baz) # E: Invalid self argument "Bar" to attribute function "baz" with type "Callable[[int], int]" \
# N: Revealed type is "def () -> builtins.int"
[file mod.py]
def baz(x: int) -> int: ...

[case testClassScopeImportUndefined]
class Foo:
from unknown import foo # E: Cannot find implementation or library stub for module named "unknown" \
# N: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

reveal_type(Foo.foo) # N: Revealed type is "Any"
reveal_type(Foo().foo) # N: Revealed type is "Any"

[case testClassScopeImportWithFollowImports]
# flags: --follow-imports=skip
class Foo:
from mod import foo

reveal_type(Foo().foo) # N: Revealed type is "Any"
[file mod.py]
def foo(x: int, y: int) -> int: ...

[case testClassScopeImportVarious]
class Foo:
from mod1 import foo
from mod2 import foo # E: Name "foo" already defined on line 2

from mod1 import meth1
def meth1(self, a: str) -> str: ... # E: Name "meth1" already defined on line 5

def meth2(self, a: str) -> str: ...
from mod1 import meth2 # E: Name "meth2" already defined on line 8

class Bar:
from mod1 import foo

import mod1
reveal_type(Foo.foo) # N: Revealed type is "def (x: builtins.int, y: builtins.int) -> builtins.int"
reveal_type(Bar.foo) # N: Revealed type is "def (x: builtins.int, y: builtins.int) -> builtins.int"
reveal_type(mod1.foo) # N: Revealed type is "def (x: builtins.int, y: builtins.int) -> builtins.int"
[file mod1.py]
def foo(x: int, y: int) -> int: ...
def meth1(x: int) -> int: ...
def meth2(x: int) -> int: ...
[file mod2.py]
def foo(z: str) -> int: ...
4 changes: 2 additions & 2 deletions test-data/unit/check-modules.test
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be placed on the wrong line.

Copy link
Collaborator Author
@hauntsaninja hauntsaninja Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see! 👍

f()
f(C) # E: Too many arguments for "f"
f(C) # E: Too many arguments for "f" of "C"
[file m.py]
def f() -> None: pass
[out]
Expand Down
4 changes: 2 additions & 2 deletions test-data/unit/check-newsemanal.test
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here 🤔

@dec(f)
def m(self): pass

Expand All @@ -2742,7 +2742,7 @@ import m

[file m/__init__.py]
class C:
from m.m import f
from m.m import f # E: Method must have at least one argument
@dec(f)
def m(self): pass

Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/semanal-modules.test
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ MypyFile:1(
ImportFrom:2(_x, [y])
AssignmentStmt:3(
NameExpr(z* [m])
NameExpr(y [_x.y]))))
NameExpr(y [__main__.A.y]))))

[case testImportInClassBody2]
class A:
Expand Down
0