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

New analyser: Fix crash in plugins when star import blocks adding a symbol #7136

Merged
merged 3 commits into from
Jul 3, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 4 additions & 0 deletions mypy/newsemanal/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,10 @@ class NewSemanticAnalyzer(NodeVisitor[None],
# unbound names due to cyclic definitions and should not defer)?
_final_iteration = False
# These names couldn't be added to the symbol table due to incomplete deps.
# Note that missing names are per module, _not_ per namespace. This means that e.g.
# a missing name at global scope will block adding same name at a class scope.
# This should not affect correctness and is purely a performance issue,
# since it can cause unnecessary deferrals.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also document * in missing_names. Actually, I'm not sure if anything other than * needs to be supported here, since otherwise we should just be able to add a placeholder. If that's the case, this could be replaced with a boolean flag. (If this is feasible, this change should be made in a separate PR.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is hard to tell how easy it is. I can try it after landing this.

missing_names = None # type: Set[str]
# Callbacks that will be called after semantic analysis to tweak things.
patches = None # type: List[Tuple[int, Callable[[], None]]]
Expand Down
5 changes: 4 additions & 1 deletion mypy/plugins/attrs.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,10 @@ def attr_class_maker_callback(ctx: 'mypy.plugin.ClassDefContext',
if ctx.api.options.new_semantic_analyzer:
# Check if attribute types are ready.
for attr in attributes:
if info[attr.name].type is None and not ctx.api.final_iteration:
node = info.get(attr.name)
if node is None:
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment. Discuss why we don't defer here.

if node.type is None and not ctx.api.final_iteration:
ctx.api.defer()
return

Expand Down
8 changes: 7 additions & 1 deletion mypy/plugins/dataclasses.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,13 @@ def collect_attributes(self) -> List[DataclassAttribute]:
if not isinstance(lhs, NameExpr):
continue

node = cls.info.names[lhs.name].node
sym = cls.info.names.get(lhs.name)
if sym is None:
# This name is likely blocked by a star import.
assert ctx.api.options.new_semantic_analyzer
continue

node = sym.node
if isinstance(node, PlaceholderNode):
# This node is not ready yet.
continue
Expand Down
16 changes: 16 additions & 0 deletions test-data/unit/check-attr.test
Original file line number Diff line number Diff line change
Expand Up @@ -1151,3 +1151,19 @@ class C:

C(0).total = 1 # E: Property "total" defined in "C" is read-only
[builtins fixtures/bool.pyi]

[case testTypeInAttrDeferredStar]
import lib
[file lib.py]
import attr
from other import *

@attr.s
class C:
total = attr.ib(type=int)

C() # E: Too few arguments for "C"
C('no') # E: Argument 1 to "C" has incompatible type "str"; expected "int"
[file other.py]
import lib
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like in this case it should be possible to process the star import without a deferral. Maybe create an issue about this if you agree with that, and add a comment here.

[builtins fixtures/bool.pyi]
16 changes: 16 additions & 0 deletions test-data/unit/check-dataclasses.test
Original file line number Diff line number Diff line change
Expand Up @@ -665,3 +665,19 @@ def func() -> int: ...
c: C
c.x = 1 # E: Property "x" defined in "C" is read-only
[builtins fixtures/bool.pyi]

[case testTypeInDataclassDeferredStar]
import lib
[file lib.py]
from dataclasses import dataclass
from other import *
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to above, maybe we should be able to process this import without adding anything to missing_names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #7141 for these two.


@dataclass
class C:
total: int

C() # E: Too few arguments for "C"
C('no') # E: Argument 1 to "C" has incompatible type "str"; expected "int"
[file other.py]
import lib
[builtins fixtures/bool.pyi]