-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Support type aliases in fine-grained incremental mode #4525
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
Changes from 7 commits
3eed2a5
3e895d6
8d42ca7
c0a6ed7
54289f0
0e365e8
8d88890
e43821f
2b25e91
d5306f2
fa6d426
d9b397e
d7adb15
3a4b12b
afa4967
d19455a
3984464
224d02d
108a40d
b3a0488
ee1fdc9
811be9c
f31e6af
edabb05
b3db8e0
7b2a13c
8b666bb
d1e0d07
2803764
9ad014c
6e1cc7a
3b9b19e
e5ab3eb
4832e37
b08aef8
35f4380
a027399
8f3cb32
e8e01c9
b259da4
1fb320c
6817ba5
b8893f0
ef7f3b9
86b1bfd
0a4a23e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,15 @@ | |
|
||
import os | ||
from abc import abstractmethod | ||
from collections import OrderedDict | ||
from collections import OrderedDict, defaultdict | ||
from typing import ( | ||
Any, TypeVar, List, Tuple, cast, Set, Dict, Union, Optional, Callable, Sequence, | ||
Any, TypeVar, List, Tuple, cast, Set, Dict, Union, Optional, Callable, Sequence | ||
) | ||
|
||
MYPY = False | ||
if MYPY: | ||
from typing import DefaultDict | ||
|
||
import mypy.strconv | ||
from mypy.util import short_type | ||
from mypy.visitor import NodeVisitor, StatementVisitor, ExpressionVisitor | ||
|
@@ -194,6 +198,8 @@ class MypyFile(SymbolNode): | |
path = '' | ||
# Top-level definitions and statements | ||
defs = None # type: List[Statement] | ||
# Type alias dependencies as mapping from node to set of alias full names | ||
alias_deps = None # type: DefaultDict[Union[MypyFile, FuncItem, ClassDef], Set[str]] | ||
# Is there a UTF-8 BOM at the start? | ||
is_bom = False | ||
names = None # type: SymbolTable | ||
|
@@ -213,6 +219,7 @@ def __init__(self, | |
self.line = 1 # Dummy line number | ||
self.imports = imports | ||
self.is_bom = is_bom | ||
self.alias_deps = defaultdict(set) | ||
if ignored_lines: | ||
self.ignored_lines = ignored_lines | ||
else: | ||
|
@@ -2331,6 +2338,10 @@ class SymbolTableNode: | |
normalized = False # type: bool | ||
# Was this defined by assignment to self attribute? | ||
implicit = False # type: bool | ||
# Is this node refers to other node via node aliasing? | ||
# (This is currently used for simple aliases like `A = int` instead of .type_override) | ||
is_aliasing = False # type: bool | ||
alias_depends_on = None # type: Set[str] | ||
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 wonder if this actually is necessary. Even without this, the target that contains the alias definition should depend on these aliases, right? And change in the alias should be detected by 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. Ideally this shouldn't be necessary. But IIRC correctly some tests failed without this. I added a TODO about this in |
||
|
||
def __init__(self, | ||
kind: int, | ||
|
@@ -2349,6 +2360,7 @@ def __init__(self, | |
self.alias_tvars = alias_tvars | ||
self.implicit = implicit | ||
self.module_hidden = module_hidden | ||
self.alias_depends_on = set() | ||
|
||
@property | ||
def fullname(self) -> Optional[str]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -597,9 +597,13 @@ def analyze_function(self, defn: FuncItem) -> None: | |
assert isinstance(defn.type, CallableType) | ||
# Signature must be analyzed in the surrounding scope so that | ||
# class-level imported names and type variables are in scope. | ||
defn.type = self.type_analyzer().visit_callable_type(defn.type, nested=False) | ||
analyzer = self.type_analyzer() | ||
defn.type = analyzer.visit_callable_type(defn.type, nested=False) | ||
if analyzer.aliases_used: | ||
self.cur_mod_node.alias_deps[defn].update(analyzer.aliases_used) | ||
self.check_function_signature(defn) | ||
if isinstance(defn, FuncDef): | ||
assert isinstance(defn.type, CallableType) | ||
defn.type = set_callable_name(defn.type, defn) | ||
for arg in defn.arguments: | ||
if arg.initializer: | ||
|
@@ -1066,7 +1070,7 @@ def analyze_base_classes(self, defn: ClassDef) -> None: | |
|
||
for base_expr in defn.base_type_exprs: | ||
try: | ||
base = self.expr_to_analyzed_type(base_expr) | ||
base = self.expr_to_analyzed_type(base_expr, from_bases=defn) | ||
except TypeTranslationError: | ||
self.fail('Invalid base class', base_expr) | ||
info.fallback_to_any = True | ||
|
@@ -1178,7 +1182,8 @@ def update_metaclass(self, defn: ClassDef) -> None: | |
return | ||
defn.metaclass = metas.pop() | ||
|
||
def expr_to_analyzed_type(self, expr: Expression) -> Type: | ||
def expr_to_analyzed_type(self, expr: Expression, *, | ||
from_bases: Optional[ClassDef] = None) -> Type: | ||
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. The name of argument |
||
if isinstance(expr, CallExpr): | ||
expr.accept(self) | ||
info = self.check_namedtuple(expr) | ||
|
@@ -1190,7 +1195,7 @@ def expr_to_analyzed_type(self, expr: Expression) -> Type: | |
fallback = Instance(info, []) | ||
return TupleType(info.tuple_type.items, fallback=fallback) | ||
typ = expr_to_unanalyzed_type(expr) | ||
return self.anal_type(typ) | ||
return self.anal_type(typ, from_bases=from_bases) | ||
|
||
def verify_base_classes(self, defn: ClassDef) -> bool: | ||
info = defn.info | ||
|
@@ -1674,12 +1679,32 @@ def anal_type(self, t: Type, *, | |
tvar_scope: Optional[TypeVarScope] = None, | ||
allow_tuple_literal: bool = False, | ||
aliasing: bool = False, | ||
third_pass: bool = False) -> Type: | ||
third_pass: bool = False, | ||
from_bases: Optional[ClassDef] = None) -> Type: | ||
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. Another instance of the |
||
a = self.type_analyzer(tvar_scope=tvar_scope, | ||
aliasing=aliasing, | ||
allow_tuple_literal=allow_tuple_literal, | ||
third_pass=third_pass) | ||
return t.accept(a) | ||
tp = t.accept(a) | ||
if a.aliases_used: | ||
if from_bases is not None: | ||
self.cur_mod_node.alias_deps[from_bases].update(a.aliases_used) | ||
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. Code similar to this line of code ( |
||
return tp | ||
self.add_alias_deps(a.aliases_used) | ||
return tp | ||
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. The logic is a bit convoluted. What about rewriting this like this: if a.aliases_used:
if from_bases is not None:
self.cur_mod_node.alias_deps[from_bases].update(a.aliases_used)
else:
self.add_alias_deps(a.aliases_used)
return t.accept(a) |
||
|
||
def add_alias_deps(self, aliases_used: Set[str]) -> None: | ||
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. Consider renaming to |
||
"""Add full names of type aliases on which the current node depends. | ||
|
||
This is used by fine-grained incremental mode to re-chek the corresponding nodes. | ||
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. Typo: re-chek |
||
""" | ||
if self.is_class_scope(): | ||
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. Why can't we use 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. There is no 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. Do you test that classes nested within function work as expected? The fine-grained incremental target will be the enclosing function. If you follow my idea above where we track the current fine-grained incremental target, this could perhaps be simplified significantly. |
||
assert self.type is not None, "Type not set at class scope" | ||
self.cur_mod_node.alias_deps[self.type.defn].update(aliases_used) | ||
elif self.is_func_scope(): | ||
self.cur_mod_node.alias_deps[self.function_stack[-1]].update(aliases_used) | ||
else: | ||
self.cur_mod_node.alias_deps[self.cur_mod_node].update(aliases_used) | ||
|
||
def visit_assignment_stmt(self, s: AssignmentStmt) -> None: | ||
for lval in s.lvalues: | ||
|
@@ -1755,7 +1780,7 @@ def alias_fallback(self, tp: Type) -> Instance: | |
return Instance(fb_info, []) | ||
|
||
def analyze_alias(self, rvalue: Expression, | ||
warn_bound_tvar: bool = False) -> Tuple[Optional[Type], List[str]]: | ||
warn_bound_tvar: bool = False) -> Tuple[Optional[Type], List[str], Set[str]]: | ||
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. Update docstring to reflect the new return type. |
||
"""Check if 'rvalue' represents a valid type allowed for aliasing | ||
(e.g. not a type variable). If yes, return the corresponding type and a list of | ||
qualified type variable names for generic aliases. | ||
|
@@ -1775,12 +1800,15 @@ def analyze_alias(self, rvalue: Expression, | |
in_dynamic_func=dynamic, | ||
global_scope=global_scope, | ||
warn_bound_tvar=warn_bound_tvar) | ||
tp = None # type: Optional[Type] | ||
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. Style nit: we generally use |
||
if res: | ||
tp, depends_on = res | ||
alias_tvars = [name for (name, _) in | ||
res.accept(TypeVariableQuery(self.lookup_qualified, self.tvar_scope))] | ||
tp.accept(TypeVariableQuery(self.lookup_qualified, self.tvar_scope))] | ||
else: | ||
alias_tvars = [] | ||
return res, alias_tvars | ||
depends_on = set() | ||
return tp, alias_tvars, depends_on | ||
|
||
def check_and_set_up_type_alias(self, s: AssignmentStmt) -> None: | ||
"""Check if assignment creates a type alias and set it up as needed. | ||
|
@@ -1809,11 +1837,18 @@ def check_and_set_up_type_alias(self, s: AssignmentStmt) -> None: | |
# annotations (see the second rule). | ||
return | ||
rvalue = s.rvalue | ||
res, alias_tvars = self.analyze_alias(rvalue, warn_bound_tvar=True) | ||
res, alias_tvars, depends_on = self.analyze_alias(rvalue, warn_bound_tvar=True) | ||
if not res: | ||
return | ||
node = self.lookup(lvalue.name, lvalue) | ||
assert node is not None | ||
node.alias_depends_on = depends_on.copy() | ||
if lvalue.fullname is not None: | ||
# To avoid extra attributes on SymbolTableNode we add the fullname | ||
# of alias to what it depends on. | ||
node.alias_depends_on.add(lvalue.fullname) | ||
if depends_on: | ||
self.add_alias_deps(depends_on) | ||
if not lvalue.is_inferred_def: | ||
# Type aliases can't be re-defined. | ||
if node and (node.kind == TYPE_ALIAS or isinstance(node.node, TypeInfo)): | ||
|
@@ -1830,6 +1865,7 @@ def check_and_set_up_type_alias(self, s: AssignmentStmt) -> None: | |
# For simple (on-generic) aliases we use aliasing TypeInfo's | ||
# to allow using them in runtime context where it makes sense. | ||
node.node = res.type | ||
node.is_aliasing = True | ||
if isinstance(rvalue, RefExpr): | ||
sym = self.lookup_type_node(rvalue) | ||
if sym: | ||
|
@@ -3439,12 +3475,14 @@ def visit_index_expr(self, expr: IndexExpr) -> None: | |
elif isinstance(expr.base, RefExpr) and expr.base.kind == TYPE_ALIAS: | ||
# Special form -- subscripting a generic type alias. | ||
# Perform the type substitution and create a new alias. | ||
res, alias_tvars = self.analyze_alias(expr) | ||
res, alias_tvars, depends_on = self.analyze_alias(expr) | ||
assert res is not None, "Failed analyzing already defined alias" | ||
expr.analyzed = TypeAliasExpr(res, alias_tvars, fallback=self.alias_fallback(res), | ||
in_runtime=True) | ||
expr.analyzed.line = expr.line | ||
expr.analyzed.column = expr.column | ||
if depends_on: # for situations like `L = LongGeneric; x = L[int]()` | ||
self.add_alias_deps(depends_on) | ||
elif refers_to_class_or_function(expr.base): | ||
# Special form -- type application. | ||
# Translate index to an unanalyzed type. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,8 @@ def visit_file(self, file_node: MypyFile, fnam: str, options: Options, | |
self.patches = patches | ||
self.is_typeshed_file = self.errors.is_typeshed_file(fnam) | ||
self.sem.cur_mod_id = file_node.fullname() | ||
self.cur_mod_node = file_node | ||
self.cur_node = file_node # type: Union[MypyFile, FuncItem, ClassDef] | ||
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. Add comment about the purpose of this. |
||
self.sem.globals = file_node.names | ||
with experiments.strict_optional_set(options.strict_optional): | ||
self.accept(file_node) | ||
|
@@ -92,8 +94,11 @@ def visit_func_def(self, fdef: FuncDef) -> None: | |
if not self.recurse_into_functions: | ||
return | ||
self.errors.push_function(fdef.name()) | ||
old_node = self.cur_node | ||
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. What about having a context manager that temporarily sets |
||
self.cur_node = fdef | ||
self.analyze(fdef.type, fdef) | ||
super().visit_func_def(fdef) | ||
self.cur_node = old_node | ||
self.errors.pop_function() | ||
|
||
def visit_overloaded_func_def(self, fdef: OverloadedFuncDef) -> None: | ||
|
@@ -134,7 +139,10 @@ def visit_class_def(self, tdef: ClassDef) -> None: | |
elif isinstance(tdef.analyzed, NamedTupleExpr): | ||
self.analyze(tdef.analyzed.info.tuple_type, tdef.analyzed, warn=True) | ||
self.analyze_info(tdef.analyzed.info) | ||
old_node = self.cur_node | ||
self.cur_node = tdef | ||
super().visit_class_def(tdef) | ||
self.cur_node = old_node | ||
|
||
def visit_decorator(self, dec: Decorator) -> None: | ||
"""Try to infer the type of the decorated function. | ||
|
@@ -353,6 +361,8 @@ def analyze(self, type: Optional[Type], node: Union[Node, SymbolTableNode], | |
type.accept(analyzer) | ||
self.check_for_omitted_generics(type) | ||
self.generate_type_patches(node, indicator, warn) | ||
if analyzer.aliases_used: | ||
self.cur_mod_node.alias_deps[self.cur_node].update(analyzer.aliases_used) | ||
|
||
def analyze_types(self, types: List[Type], node: Node) -> None: | ||
# Similar to above but for nodes with multiple types. | ||
|
@@ -361,6 +371,8 @@ def analyze_types(self, types: List[Type], node: Node) -> None: | |
analyzer = self.make_type_analyzer(indicator) | ||
type.accept(analyzer) | ||
self.check_for_omitted_generics(type) | ||
if analyzer.aliases_used: | ||
self.cur_mod_node.alias_deps[self.cur_node].update(analyzer.aliases_used) | ||
self.generate_type_patches(node, indicator, warn=False) | ||
|
||
def generate_type_patches(self, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,14 +81,18 @@ class 'mod.Cls'. This can also refer to an attribute inherited from a | |
|
||
from typing import Dict, List, Set, Optional, Tuple, Union | ||
|
||
MYPY = False | ||
if MYPY: | ||
from typing import DefaultDict | ||
|
||
from mypy.checkmember import bind_self | ||
from mypy.nodes import ( | ||
Node, Expression, MypyFile, FuncDef, ClassDef, AssignmentStmt, NameExpr, MemberExpr, Import, | ||
ImportFrom, CallExpr, CastExpr, TypeVarExpr, TypeApplication, IndexExpr, UnaryExpr, OpExpr, | ||
ComparisonExpr, GeneratorExpr, DictionaryComprehension, StarExpr, PrintStmt, ForStmt, WithStmt, | ||
TupleExpr, ListExpr, OperatorAssignmentStmt, DelStmt, YieldFromExpr, Decorator, Block, | ||
TypeInfo, FuncBase, OverloadedFuncDef, RefExpr, SuperExpr, Var, NamedTupleExpr, TypedDictExpr, | ||
LDEF, MDEF, GDEF, | ||
LDEF, MDEF, GDEF, FuncItem, | ||
op_methods, reverse_op_methods, ops_with_inplace_method, unary_op_methods | ||
) | ||
from mypy.traverser import TraverserVisitor | ||
|
@@ -106,17 +110,20 @@ def get_dependencies(target: MypyFile, | |
python_version: Tuple[int, int]) -> Dict[str, Set[str]]: | ||
"""Get all dependencies of a node, recursively.""" | ||
visitor = DependencyVisitor(type_map, python_version) | ||
visitor.alias_deps = target.alias_deps | ||
target.accept(visitor) | ||
return visitor.map | ||
|
||
|
||
def get_dependencies_of_target(module_id: str, | ||
module_tree: MypyFile, | ||
target: Node, | ||
type_map: Dict[Expression, Type], | ||
python_version: Tuple[int, int]) -> Dict[str, Set[str]]: | ||
"""Get dependencies of a target -- don't recursive into nested targets.""" | ||
# TODO: Add tests for this function. | ||
visitor = DependencyVisitor(type_map, python_version) | ||
visitor.alias_deps = module_tree.alias_deps | ||
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. Instead of directly modifying an attribute of |
||
visitor.scope.enter_file(module_id) | ||
if isinstance(target, MypyFile): | ||
# Only get dependencies of the top-level of the module. Don't recurse into | ||
|
@@ -138,6 +145,8 @@ def get_dependencies_of_target(module_id: str, | |
|
||
|
||
class DependencyVisitor(TraverserVisitor): | ||
alias_deps = None # type: DefaultDict[Union[MypyFile, FuncItem, ClassDef], Set[str]] | ||
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. Add comment describing the purpose of this. |
||
|
||
def __init__(self, | ||
type_map: Dict[Expression, Type], | ||
python_version: Tuple[int, int]) -> None: | ||
|
@@ -153,13 +162,15 @@ def __init__(self, | |
# await | ||
# protocols | ||
# metaclasses | ||
# type aliases | ||
# functional enum | ||
# type variable with value restriction | ||
|
||
def visit_mypy_file(self, o: MypyFile) -> None: | ||
self.scope.enter_file(o.fullname()) | ||
self.is_package_init_file = o.is_package_init_file() | ||
if o in self.alias_deps: | ||
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. These three lines are repeated multiple times so it would be better to put them in a helper method. |
||
for alias in self.alias_deps[o]: | ||
self.add_dependency(make_trigger(alias)) | ||
super().visit_mypy_file(o) | ||
self.scope.leave() | ||
|
||
|
@@ -177,6 +188,9 @@ def visit_func_def(self, o: FuncDef) -> None: | |
if o.info: | ||
for base in non_trivial_bases(o.info): | ||
self.add_dependency(make_trigger(base.fullname() + '.' + o.name())) | ||
if o in self.alias_deps: | ||
for alias in self.alias_deps[o]: | ||
self.add_dependency(make_trigger(alias)) | ||
super().visit_func_def(o) | ||
self.scope.leave() | ||
|
||
|
@@ -202,6 +216,9 @@ def visit_class_def(self, o: ClassDef) -> None: | |
self.add_type_dependencies(o.info.typeddict_type, target=make_trigger(target)) | ||
# TODO: Add dependencies based on remaining TypeInfo attributes. | ||
super().visit_class_def(o) | ||
if o in self.alias_deps: | ||
for alias in self.alias_deps[o]: | ||
self.add_dependency(make_trigger(alias)) | ||
self.is_class = old_is_class | ||
info = o.info | ||
for name, node in info.names.items(): | ||
|
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.
Add docstring for
alias_depends_on
.There are starting to be quite a few alias-related attributes in symbol table nodes. Maybe it's time to add a new AST node for type aliases soon after this PR has been merged so that we can remove several of these attributes?
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.
Yes, I will take care of it as part of #4082