From 44b1042623248b17f01d0975de7518ef9cf30552 Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Tue, 21 Dec 2021 16:03:03 -0800 Subject: [PATCH 1/4] suggested type improvements --- pyanalyze/name_check_visitor.py | 3 +- pyanalyze/suggested_type.py | 47 ++++++++++++++++++++------------ pyanalyze/test_suggested_type.py | 8 +++++- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/pyanalyze/name_check_visitor.py b/pyanalyze/name_check_visitor.py index 1e6256b3..d60b7f17 100644 --- a/pyanalyze/name_check_visitor.py +++ b/pyanalyze/name_check_visitor.py @@ -109,7 +109,7 @@ ARGS, KWARGS, ) -from .suggested_type import CallArgs, display_suggested_type +from .suggested_type import CallArgs, display_suggested_type, should_suggest_type from .asynq_checker import AsyncFunctionKind, AsynqChecker, FunctionInfo from .yield_checker import YieldChecker from .type_object import TypeObject, get_mro @@ -1393,6 +1393,7 @@ def visit_FunctionDef( decorator == KnownValue(abstractmethod) for _, decorator in info.decorators ) + and should_suggest_type(return_value) ): self._show_error_if_checking( node, diff --git a/pyanalyze/suggested_type.py b/pyanalyze/suggested_type.py index 408bde75..478ee559 100644 --- a/pyanalyze/suggested_type.py +++ b/pyanalyze/suggested_type.py @@ -6,7 +6,7 @@ import ast from collections import defaultdict from dataclasses import dataclass, field -from typing import Dict, Iterator, List, Mapping, Sequence, Union +from typing import Dict, Iterator, List, Mapping, Sequence, Tuple, Union from pyanalyze.safe import safe_isinstance @@ -58,12 +58,14 @@ def check(self) -> Iterator[Failure]: all_values = [v for v in all_values if not isinstance(v, AnyValue)] if not all_values: continue - suggested = display_suggested_type(unite_values(*all_values)) + suggested = unite_values(*all_values) + if not should_suggest_type(suggested): + continue failure = self.ctx.show_error( param, f"Suggested type for parameter {param.arg}", ErrorCode.suggested_parameter_type, - detail=suggested, + detail=display_suggested_type(suggested), # Otherwise we record it twice in tests. We should ultimately # refactor error tracking to make it less hacky for things that # show errors outside of files. @@ -109,6 +111,15 @@ def display_suggested_type(value: Value) -> str: return str(cae) +def should_suggest_type(value: Value) -> bool: + if isinstance(value, AnyValue): + return False + if isinstance(value, MultiValuedValue) and len(value.vals) > 5: + # Big unions probably aren't useful + return False + return True + + def prepare_type(value: Value) -> Value: """Simplify a type to turn it into a suggestion.""" if isinstance(value, AnnotatedValue): @@ -139,22 +150,24 @@ def prepare_type(value: Value) -> Value: return prepare_type(value) elif isinstance(value, MultiValuedValue): vals = [prepare_type(subval) for subval in value.vals] - type_literals = [ - v - for v in vals - if isinstance(v, KnownValue) and safe_isinstance(v.val, type) - ] + type_literals: List[Tuple[Value, type]] = [] + rest: List[Value] = [] + for subval in vals: + if isinstance(subval, KnownValue) and safe_isinstance(subval.val, type): + type_literals.append((subval, subval.val)) + elif ( + isinstance(subval, SubclassValue) + and isinstance(subval.typ, TypedValue) + and safe_isinstance(subval.typ.typ, type) + ): + type_literals.append((subval, subval.typ.typ)) + else: + rest.append(subval) if len(type_literals) > 1: - types = [v.val for v in type_literals if isinstance(v.val, type)] - shared_type = get_shared_type(types) + shared_type = get_shared_type([typ for _, typ in type_literals]) type_val = SubclassValue(TypedValue(shared_type)) - others = [ - v - for v in vals - if not isinstance(v, KnownValue) or not safe_isinstance(v.val, type) - ] - return unite_values(type_val, *others) - return unite_values(*vals) + return unite_values(type_val, *rest) + return unite_values(*[v for v, _ in type_literals], *rest) else: return value diff --git a/pyanalyze/test_suggested_type.py b/pyanalyze/test_suggested_type.py index 1f94d51e..73f24025 100644 --- a/pyanalyze/test_suggested_type.py +++ b/pyanalyze/test_suggested_type.py @@ -45,7 +45,6 @@ class A: class B(A): - pass @@ -58,3 +57,10 @@ def test_prepare_type() -> None: TypedValue(object) ) assert prepare_type(KnownValue(C) | KnownValue(B)) == SubclassValue(TypedValue(A)) + + assert prepare_type(SubclassValue(TypedValue(B)) | KnownValue(C)) == SubclassValue( + TypedValue(A) + ) + assert prepare_type(SubclassValue(TypedValue(B)) | KnownValue(B)) == SubclassValue( + TypedValue(B) + ) From 748ec5264dee8f32fded23e7ac074f71933691c1 Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Tue, 21 Dec 2021 16:15:57 -0800 Subject: [PATCH 2/4] fix return type filter --- pyanalyze/name_check_visitor.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/pyanalyze/name_check_visitor.py b/pyanalyze/name_check_visitor.py index d60b7f17..11c9c313 100644 --- a/pyanalyze/name_check_visitor.py +++ b/pyanalyze/name_check_visitor.py @@ -109,7 +109,12 @@ ARGS, KWARGS, ) -from .suggested_type import CallArgs, display_suggested_type, should_suggest_type +from .suggested_type import ( + CallArgs, + display_suggested_type, + prepare_type, + should_suggest_type, +) from .asynq_checker import AsyncFunctionKind, AsynqChecker, FunctionInfo from .yield_checker import YieldChecker from .type_object import TypeObject, get_mro @@ -1393,13 +1398,14 @@ def visit_FunctionDef( decorator == KnownValue(abstractmethod) for _, decorator in info.decorators ) - and should_suggest_type(return_value) ): - self._show_error_if_checking( - node, - error_code=ErrorCode.suggested_return_type, - detail=display_suggested_type(return_value), - ) + prepared = prepare_type(return_value) + if should_suggest_type(prepared): + self._show_error_if_checking( + node, + error_code=ErrorCode.suggested_return_type, + detail=display_suggested_type(prepared), + ) if evaled_function: return evaled_function From 4600c9075f337f546cd22322d22f97cb8d05831d Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Tue, 21 Dec 2021 16:23:48 -0800 Subject: [PATCH 3/4] add more types --- pyanalyze/suggested_type.py | 5 ++++- pyanalyze/test_suggested_type.py | 8 +++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pyanalyze/suggested_type.py b/pyanalyze/suggested_type.py index 478ee559..f0ef444e 100644 --- a/pyanalyze/suggested_type.py +++ b/pyanalyze/suggested_type.py @@ -165,7 +165,10 @@ def prepare_type(value: Value) -> Value: rest.append(subval) if len(type_literals) > 1: shared_type = get_shared_type([typ for _, typ in type_literals]) - type_val = SubclassValue(TypedValue(shared_type)) + if shared_type is object: + type_val = TypedValue(type) + else: + type_val = SubclassValue(TypedValue(shared_type)) return unite_values(type_val, *rest) return unite_values(*[v for v, _ in type_literals], *rest) else: diff --git a/pyanalyze/test_suggested_type.py b/pyanalyze/test_suggested_type.py index 73f24025..b8ac3a38 100644 --- a/pyanalyze/test_suggested_type.py +++ b/pyanalyze/test_suggested_type.py @@ -53,9 +53,7 @@ class C(A): def test_prepare_type() -> None: - assert prepare_type(KnownValue(int) | KnownValue(str)) == SubclassValue( - TypedValue(object) - ) + assert prepare_type(KnownValue(int) | KnownValue(str)) == TypedValue(type) assert prepare_type(KnownValue(C) | KnownValue(B)) == SubclassValue(TypedValue(A)) assert prepare_type(SubclassValue(TypedValue(B)) | KnownValue(C)) == SubclassValue( @@ -64,3 +62,7 @@ def test_prepare_type() -> None: assert prepare_type(SubclassValue(TypedValue(B)) | KnownValue(B)) == SubclassValue( TypedValue(B) ) + assert prepare_type(KnownValue(None) | TypedValue(str)) == KnownValue( + None + ) | TypedValue(str) + assert prepare_type(KnownValue(True) | KnownValue(False)) == TypedValue(bool) From 06e8cc61dfd0d120f9dd005635aedbc368444116 Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Tue, 21 Dec 2021 16:30:30 -0800 Subject: [PATCH 4/4] more type simplification --- pyanalyze/suggested_type.py | 10 +++++----- pyanalyze/test_suggested_type.py | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pyanalyze/suggested_type.py b/pyanalyze/suggested_type.py index f0ef444e..79a2ca4d 100644 --- a/pyanalyze/suggested_type.py +++ b/pyanalyze/suggested_type.py @@ -139,8 +139,10 @@ def prepare_type(value: Value) -> Value: elif isinstance(value, VariableNameValue): return AnyValue(AnySource.unannotated) elif isinstance(value, KnownValue): - if value.val is None or safe_isinstance(value.val, type): + if value.val is None: return value + elif safe_isinstance(value.val, type): + return SubclassValue(TypedValue(value.val)) elif callable(value.val): return value # TODO get the signature instead and return a CallableValue? value = replace_known_sequence_value(value) @@ -153,9 +155,7 @@ def prepare_type(value: Value) -> Value: type_literals: List[Tuple[Value, type]] = [] rest: List[Value] = [] for subval in vals: - if isinstance(subval, KnownValue) and safe_isinstance(subval.val, type): - type_literals.append((subval, subval.val)) - elif ( + if ( isinstance(subval, SubclassValue) and isinstance(subval.typ, TypedValue) and safe_isinstance(subval.typ.typ, type) @@ -163,7 +163,7 @@ def prepare_type(value: Value) -> Value: type_literals.append((subval, subval.typ.typ)) else: rest.append(subval) - if len(type_literals) > 1: + if type_literals: shared_type = get_shared_type([typ for _, typ in type_literals]) if shared_type is object: type_val = TypedValue(type) diff --git a/pyanalyze/test_suggested_type.py b/pyanalyze/test_suggested_type.py index b8ac3a38..bea3d3ac 100644 --- a/pyanalyze/test_suggested_type.py +++ b/pyanalyze/test_suggested_type.py @@ -55,6 +55,7 @@ class C(A): def test_prepare_type() -> None: assert prepare_type(KnownValue(int) | KnownValue(str)) == TypedValue(type) assert prepare_type(KnownValue(C) | KnownValue(B)) == SubclassValue(TypedValue(A)) + assert prepare_type(KnownValue(int)) == SubclassValue(TypedValue(int)) assert prepare_type(SubclassValue(TypedValue(B)) | KnownValue(C)) == SubclassValue( TypedValue(A)