-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Defer all types whos metaclass is not ready #13579
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
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
8fbd00a
Test that illustrates `sympy` metaclass problem
sobolevn 3b51864
Progress!
sobolevn 276786f
Fix builtins cycle import
sobolevn 4d24290
Fix ci
sobolevn b502458
Fix ci
sobolevn 914fc70
Fix ci
sobolevn d2b5bea
Fix CI
sobolevn 9e2800e
Try this setup
sobolevn 9dc5b1c
Address review
sobolevn 50b5d49
Fix ci
sobolevn 7248fe1
Tests pass locally
sobolevn 57c5bfe
Tests pass locally
sobolevn 47fa22a
Try `defer` on incomplete implicit attrs
sobolevn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -581,7 +581,11 @@ def refresh_partial( | |
def refresh_top_level(self, file_node: MypyFile) -> None: | ||
"""Reanalyze a stale module top-level in fine-grained incremental mode.""" | ||
self.recurse_into_functions = False | ||
self.add_implicit_module_attrs(file_node) | ||
# We do it in the very last order, because of | ||
# `builtins.dict <-> typing.Mapping <-> abc.ABCMeta` | ||
# cyclic imports. | ||
if file_node.fullname not in ("typing", "abc", "builtins"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would instead re-use |
||
self.add_implicit_module_attrs(file_node) | ||
for d in file_node.defs: | ||
self.accept(d) | ||
if file_node.fullname == "typing": | ||
|
@@ -1374,7 +1378,7 @@ def analyze_class(self, defn: ClassDef) -> None: | |
defn.base_type_exprs.extend(defn.removed_base_type_exprs) | ||
defn.removed_base_type_exprs.clear() | ||
|
||
self.update_metaclass(defn) | ||
self.infer_metaclass_and_bases_from_compat_helpers(defn) | ||
|
||
bases = defn.base_type_exprs | ||
bases, tvar_defs, is_protocol = self.clean_up_bases_and_infer_type_variables( | ||
|
@@ -1390,20 +1394,25 @@ def analyze_class(self, defn: ClassDef) -> None: | |
self.defer() | ||
|
||
self.analyze_class_keywords(defn) | ||
result = self.analyze_base_classes(bases) | ||
|
||
if result is None or self.found_incomplete_ref(tag): | ||
bases_result = self.analyze_base_classes(bases) | ||
if bases_result is None or self.found_incomplete_ref(tag): | ||
# Something was incomplete. Defer current target. | ||
self.mark_incomplete(defn.name, defn) | ||
return | ||
|
||
base_types, base_error = result | ||
base_types, base_error = bases_result | ||
if any(isinstance(base, PlaceholderType) for base, _ in base_types): | ||
# We need to know the TypeInfo of each base to construct the MRO. Placeholder types | ||
# are okay in nested positions, since they can't affect the MRO. | ||
self.mark_incomplete(defn.name, defn) | ||
return | ||
|
||
declared_metaclass, should_defer = self.get_declared_metaclass(defn.name, defn.metaclass) | ||
if should_defer or self.found_incomplete_ref(tag): | ||
# Metaclass was not ready. Defer current target. | ||
self.mark_incomplete(defn.name, defn) | ||
return | ||
|
||
if self.analyze_typeddict_classdef(defn): | ||
if defn.info: | ||
self.setup_type_vars(defn, tvar_defs) | ||
|
@@ -1422,7 +1431,7 @@ def analyze_class(self, defn: ClassDef) -> None: | |
with self.scope.class_scope(defn.info): | ||
self.configure_base_classes(defn, base_types) | ||
defn.info.is_protocol = is_protocol | ||
self.analyze_metaclass(defn) | ||
self.recalculate_metaclass(defn, declared_metaclass) | ||
defn.info.runtime_protocol = False | ||
for decorator in defn.decorators: | ||
self.analyze_class_decorator(defn, decorator) | ||
|
@@ -1968,7 +1977,7 @@ def calculate_class_mro( | |
if hook: | ||
hook(ClassDefContext(defn, FakeExpression(), self)) | ||
|
||
def update_metaclass(self, defn: ClassDef) -> None: | ||
def infer_metaclass_and_bases_from_compat_helpers(self, defn: ClassDef) -> None: | ||
"""Lookup for special metaclass declarations, and update defn fields accordingly. | ||
|
||
* six.with_metaclass(M, B1, B2, ...) | ||
|
@@ -2046,30 +2055,33 @@ def is_base_class(self, t: TypeInfo, s: TypeInfo) -> bool: | |
visited.add(base.type) | ||
return False | ||
|
||
def analyze_metaclass(self, defn: ClassDef) -> None: | ||
if defn.metaclass: | ||
def get_declared_metaclass( | ||
self, name: str, metaclass_expr: Expression | None | ||
) -> tuple[Instance | None, bool]: | ||
"""Returns either metaclass instance or boolean whether we should defer.""" | ||
declared_metaclass = None | ||
if metaclass_expr: | ||
metaclass_name = None | ||
if isinstance(defn.metaclass, NameExpr): | ||
metaclass_name = defn.metaclass.name | ||
elif isinstance(defn.metaclass, MemberExpr): | ||
metaclass_name = get_member_expr_fullname(defn.metaclass) | ||
if isinstance(metaclass_expr, NameExpr): | ||
metaclass_name = metaclass_expr.name | ||
elif isinstance(metaclass_expr, MemberExpr): | ||
metaclass_name = get_member_expr_fullname(metaclass_expr) | ||
if metaclass_name is None: | ||
self.fail(f'Dynamic metaclass not supported for "{defn.name}"', defn.metaclass) | ||
return | ||
sym = self.lookup_qualified(metaclass_name, defn.metaclass) | ||
self.fail(f'Dynamic metaclass not supported for "{name}"', metaclass_expr) | ||
return None, False | ||
sym = self.lookup_qualified(metaclass_name, metaclass_expr) | ||
if sym is None: | ||
# Probably a name error - it is already handled elsewhere | ||
return | ||
return None, False | ||
if isinstance(sym.node, Var) and isinstance(get_proper_type(sym.node.type), AnyType): | ||
# 'Any' metaclass -- just ignore it. | ||
# | ||
# TODO: A better approach would be to record this information | ||
# and assume that the type object supports arbitrary | ||
# attributes, similar to an 'Any' base class. | ||
return | ||
return None, False | ||
if isinstance(sym.node, PlaceholderNode): | ||
self.defer(defn) | ||
return | ||
return None, True # defer later in the caller | ||
|
||
# Support type aliases, like `_Meta: TypeAlias = type` | ||
if ( | ||
|
@@ -2083,16 +2095,20 @@ def analyze_metaclass(self, defn: ClassDef) -> None: | |
metaclass_info = sym.node | ||
|
||
if not isinstance(metaclass_info, TypeInfo) or metaclass_info.tuple_type is not None: | ||
self.fail(f'Invalid metaclass "{metaclass_name}"', defn.metaclass) | ||
return | ||
self.fail(f'Invalid metaclass "{metaclass_name}"', metaclass_expr) | ||
return None, False | ||
if not metaclass_info.is_metaclass(): | ||
self.fail( | ||
'Metaclasses not inheriting from "type" are not supported', defn.metaclass | ||
'Metaclasses not inheriting from "type" are not supported', metaclass_expr | ||
) | ||
return | ||
return None, False | ||
inst = fill_typevars(metaclass_info) | ||
assert isinstance(inst, Instance) | ||
defn.info.declared_metaclass = inst | ||
declared_metaclass = inst | ||
return declared_metaclass, False | ||
|
||
def recalculate_metaclass(self, defn: ClassDef, declared_metaclass: Instance | None) -> None: | ||
defn.info.declared_metaclass = declared_metaclass | ||
defn.info.metaclass_type = defn.info.calculate_metaclass_type() | ||
if any(info.is_protocol for info in defn.info.mro): | ||
if ( | ||
|
@@ -2104,13 +2120,15 @@ def analyze_metaclass(self, defn: ClassDef) -> None: | |
abc_meta = self.named_type_or_none("abc.ABCMeta", []) | ||
if abc_meta is not None: # May be None in tests with incomplete lib-stub. | ||
defn.info.metaclass_type = abc_meta | ||
if defn.info.metaclass_type is None: | ||
if declared_metaclass is not None and defn.info.metaclass_type is None: | ||
# Inconsistency may happen due to multiple baseclasses even in classes that | ||
# do not declare explicit metaclass, but it's harder to catch at this stage | ||
if defn.metaclass is not None: | ||
self.fail(f'Inconsistent metaclass structure for "{defn.name}"', defn) | ||
else: | ||
if defn.info.metaclass_type.type.has_base("enum.EnumMeta"): | ||
if defn.info.metaclass_type and defn.info.metaclass_type.type.has_base( | ||
"enum.EnumMeta" | ||
): | ||
defn.info.is_enum = True | ||
if defn.type_vars: | ||
self.fail("Enum class cannot be generic", defn) | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is not the cyclic import itself, but that the method assumes that e.g.
dict
(for__annotations__
) would be never marked incomplete (i.e. it may be not defined yet, but as soon as it is, it will beTypeInfo
, rather thanPlaceholderNode
). This makes me think maybe a cleaner solution (if it works), would be just to replaceassert
with anif
(and simply continue if it is a placeholder, like we do if the symbol isNone
). Can you try this?Btw curiously this seem to only affect tests for now, it looks in real typeshed currently
dict
is not an instance ofABCMeta
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, looks like it works! Let's wait to see what tests will show us :)