8000 Tighter types for strconv, report and check* by elazarg · Pull Request #2223 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

Tighter types for strconv, report and check* #2223

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.

Sign up for GitHub

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 2 commits into from
Oct 10, 2016
Merged

Tighter types for strconv, report and check* #2223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
534378e
bring Expression for strconv, report and check*
elazarg Oct 6, 2016
afc3dff
minor
elazarg Oct 10, 2016
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
44 changes: 23 additions & 21 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from mypy.errors import Errors, report_internal_error
from mypy.nodes import (
SymbolTable, Node, MypyFile, Var, Expression, Lvalue,
SymbolTable, Statement, MypyFile, Var, Expression, Lvalue,
OverloadedFuncDef, FuncDef, FuncItem, FuncBase, TypeInfo,
ClassDef, GDEF, Block, AssignmentStmt, NameExpr, MemberExpr, IndexExpr,
TupleExpr, ListExpr, ExpressionStmt, ReturnStmt, IfStmt,
Expand Down Expand Up @@ -64,7 +64,7 @@
DeferredNode = NamedTuple(
'DeferredNode',
[
('node', Node),
('node', Union[Expression, Statement, FuncItem]),
Copy link
Member
@gvanrossum gvanrossum Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I think it can only be FuncItem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :|
Fixed

('context_type_name', Optional[str]), # Name of the surrounding class (for error messages)
])

Expand All @@ -82,9 +82,9 @@ class TypeChecker(NodeVisitor[Type]):
# Utility for generating messages
msg = None # type: MessageBuilder
# Types of type checked nodes
type_map = None # type: Dict[Node, Type]
type_map = None # type: Dict[Expression, Type]
# Types of type checked nodes within this specific module
module_type_map = None # type: Dict[Node, Type]
module_type_map = None # type: Dict[Expression, Type]

# Helper for managing conditional types
binder = None # type: ConditionalTypeBinder
Expand Down Expand Up @@ -209,15 +209,18 @@ def handle_cannot_determine_type(self, name: str, context: Context) -> None:
else:
self.msg.cannot_determine_type(name, context)

def accept(self, node: Node, type_context: Type = None) -> Type:
def accept(self, node: Union[Expression, Statement, FuncItem],
Copy link
Member

Choose a reason for hiding this comment

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

Not your fault (except for pointing it out :-), but it is really terrible that this takes either a syntax node or a SymbolNode. Those really aren't like each other. :-(

type_context: Type = None) -> Type:
"""Type check a node in the given type context."""
self.type_context.append(type_context)
try:
typ = node.accept(self)
except Exception as err:
report_internal_error(err, self.errors.file, node.line, self.errors, self.options)
self.type_context.pop()
self.store_type(node, typ)
if typ is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Explain why this check is needed? store_type() doesn't mind if typ is None, and the assert appears to be unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

store_type() expects an expression, and node might be a statement, in which case node.accept() returns None (answering your question below regarding del_stmt). So the assertion holds only if typ is None. And I wanted the assertion to actually catch mistakes if there are any, so if isinstance is not good enough.
This (and other issues) will be better handled by a separate ExpressionVisitor and a StatementVisitor, but I did try to keep the change small :)

assert isinstance(node, Expression)
self.store_type(node, typ)
if not self.in_checked_function():
return AnyType()
else:
Expand Down Expand Up @@ -1788,7 +1791,8 @@ def visit_del_stmt(self, s: DelStmt) -> Type:
m.line = s.line
c = CallExpr(m, [e.index], [nodes.ARG_POS], [None])
c.line = s.line
return c.accept(self)
c.accept(self)
return None
else:
def flatten(t: Expression) -> List[Expression]:
"""Flatten a nested sequence of tuples/lists into one list of nodes."""
Expand All @@ -1812,7 +1816,7 @@ def visit_decorator(self, e: Decorator) -> Type:
if d.fullname == 'typing.no_type_check':
e.var.type = AnyType()
e.var.is_ready = True
return NoneTyp()
return None
Copy link
Member
@gvanrossum gvanrossum Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Explain? This changes the return type to Optional[Type], and that will become a problem once we turn in --strict-optional. I sort of see that this function doesn't return a useful type in the other branch either (it just falls off the end) and it seems the return value isn't used, but if this really is just being called for its side effect maybe we should just admit that in its signature? (Or if that's beyond hope let's at least add a clarifying comment.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment above. The solution has to be a separate visitor, or at least it makes sense to me.

8000

e.func.accept(self)
sig = self.function_type(e.func) # type: Type
Expand Down Expand Up @@ -2203,7 +2207,7 @@ def check_type_equivalency(self, t1: Type, t2: Type, node: Context,
if not is_equivalent(t1, t2):
self.fail(msg, node)

def store_type(self, node: Node, typ: Type) -> None:
def store_type(self, node: Expression, typ: Type) -> None:
"""Store the type of a node in the type map."""
self.type_map[node] = typ
if typ is not None:
Expand Down Expand Up @@ -2338,7 +2342,7 @@ def method_type(self, func: FuncBase) -> FunctionLike:
# probably be better to have the dict keyed by the nodes' literal_hash
# field instead.

TypeMap = Optional[Dict[Node, Type]]
TypeMap = Optional[Dict[Expression, Type]]


def conditional_type_map(expr: Expression,
Expand Down Expand Up @@ -2417,7 +2421,7 @@ def or_conditional_maps(m1: TypeMap, m2: TypeMap) -> TypeMap:


def find_isinstance_check(node: Expression,
type_map: Dict[Node, Type],
type_map: Dict[Expression, Type],
) -> Tuple[TypeMap, TypeMap]:
"""Find any isinstance checks (within a chain of ands). Includes
implicit and explicit checks for None.
Expand All @@ -2442,8 +2446,8 @@ def find_isinstance_check(node: Expression,
# Check for `x is None` and `x is not None`.
is_not = node.operators == ['is not']
if is_not or node.operators == ['is']:
if_vars = {} # type: Dict[Node, Type]
else_vars = {} # type: Dict[Node, Type]
if_vars = {} # type: Dict[Expression, Type]
else_vars = {} # type: Dict[Expression, Type]
for expr in node.operands:
if expr.literal == LITERAL_TYPE and not is_literal_none(expr) and expr in type_map:
# This should only be true at most once: there should be
Expand All @@ -2462,7 +2466,7 @@ def find_isinstance_check(node: Expression,
vartype = type_map[node]
if_type = true_only(vartype)
else_type = false_only(vartype)
ref = node # type: Node
ref = node # type: Expression
if_map = {ref: if_type} if not isinstance(if_type, UninhabitedType) else None
else_map = {ref: else_type} if not isinstance(else_type, UninhabitedType) else None
return if_map, else_map
Expand Down Expand Up @@ -2490,7 +2494,7 @@ def find_isinstance_check(node: Expression,
return {}, {}


def get_isinstance_type(expr: Expression, type_map: Dict[Node, Type]) -> Type:
def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> Type:
type = type_map[expr]

if isinstance(type, TupleType):
Expand All @@ -2517,13 +2521,11 @@ def get_isinstance_type(expr: Expression, type_map: Dict[Node, Type]) -> Type:
return UnionType(types)


def expand_node(defn: FuncItem, map: Dict[TypeVarId, Type]) -> Node:
visitor = TypeTransformVisitor(map)
A3E2 return defn.accept(visitor)


def expand_func(defn: FuncItem, map: Dict[TypeVarId, Type]) -> FuncItem:
return cast(FuncItem, expand_node(defn, map))
visitor = TypeTransformVisitor(map)
ret = defn.accept(visitor)
assert isinstance(ret, FuncItem)
return ret


class TypeTransformVisitor(TransformVisitor):
Expand Down
8 changes: 4 additions & 4 deletions mypy/checkexpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
)
from mypy.nodes import (
NameExpr, RefExpr, Var, FuncDef, OverloadedFuncDef, TypeInfo, CallExpr,
Node, MemberExpr, IntExpr, StrExpr, BytesExpr, UnicodeExpr, FloatExpr,
MemberExpr, IntExpr, StrExpr, BytesExpr, UnicodeExpr, FloatExpr,
OpExpr, UnaryExpr, IndexExpr, CastExpr, RevealTypeExpr, TypeApplication, ListExpr,
TupleExpr, DictExpr, FuncExpr, SuperExpr, SliceExpr, Context, Expression,
ListComprehension, GeneratorExpr, SetExpr, MypyFile, Decorator,
Expand Down Expand Up @@ -1734,8 +1734,8 @@ def visit_conditional_expr(self, e: ConditionalExpr) -> Type:

return res

def analyze_cond_branch(self, map: Optional[Dict[Node, Type]],
node: Node, context: Optional[Type]) -> Type:
def analyze_cond_branch(self, map: Optional[Dict[Expression, Type]],
node: Expression, context: Optional[Type]) -> Type:
with self.chk.binder.frame_context():
if map:
for var, type in map.items():
Expand All @@ -1750,7 +1750,7 @@ def visit_backquote_expr(self, e: BackquoteExpr) -> Type:
# Helpers
#

def accept(self, node: Node, context: Type = None) -> Type:
def accept(self, node: Expression, context: Type = None) -> Type:
"""Type check a node. Alias for TypeChecker.accept."""
return self.chk.accept(node, context)

Expand Down
50 changes: 26 additions & 24 deletions mypy/checkstrformat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
Type, AnyType, TupleType, Instance, UnionType
)
from mypy.nodes import (
Node, StrExpr, BytesExpr, UnicodeExpr, TupleExpr, DictExpr, Context
StrExpr, BytesExpr, UnicodeExpr, TupleExpr, DictExpr, Context, Expression
)
if False:
# break import cycle only needed for mypy
Expand Down Expand Up @@ -57,9 +57,10 @@ def __init__(self,

# TODO: In Python 3, the bytes formatting has a more restricted set of options
# compared to string formatting.
# TODO: Bytes formatting in Python 3 is only supported in 3.5 and up.
def check_str_interpolation(self,
str: Union[StrExpr, BytesExpr, UnicodeExpr],
replacements: Node) -> Type:
replacements: Expression) -> Type:
"""Check the types of the 'replacements' in a string interpolation
expression: str % replacements
"""
Expand Down Expand Up @@ -114,7 +115,7 @@ def analyze_conversion_specifiers(self, specifiers: List[ConversionSpecifier],
return has_key

def check_simple_str_interpolation(self, specifiers: List[ConversionSpecifier],
replacements: Node) -> None:
replacements: Expression) -> None:
checkers = self.build_replacement_checkers(specifiers, replacements)
if checkers is None:
return
Expand Down Expand Up @@ -149,7 +150,7 @@ def check_simple_str_interpolation(self, specifiers: List[ConversionSpecifier],
check_type(rep_type)

def check_mapping_str_interpolation(self, specifiers: List[ConversionSpecifier],
replacements: Node) -> None:
replacements: Expression) -> None:
dict_with_only_str_literal_keys = (isinstance(replacements, DictExpr) and
all(isinstance(k, (StrExpr, BytesExpr))
for k, v in replacements.items))
Expand Down Expand Up @@ -183,9 +184,9 @@ def check_mapping_str_interpolation(self, specifiers: List[ConversionSpecifier],
'expression has type', 'expected type for mapping is')

def build_replacement_checkers(self, specifiers: List[ConversionSpecifier],
context: Context) -> List[Tuple[Callable[[Node], None],
context: Context) -> List[Tuple[Callable[[Expression], None],
Callable[[Type], None]]]:
checkers = [] # type: List[Tuple[Callable[[Node], None], Callable[[Type], None]]]
checkers = [] # type: List[Tuple[Callable[[Expression], None], Callable[[Type], None]]]
for specifier in specifiers:
checker = self.replacement_checkers(specifier, context)
if checker is None:
Expand All @@ -194,13 +195,13 @@ def build_replacement_checkers(self, specifiers: List[ConversionSpecifier],
return checkers

def replacement_checkers(self, specifier: ConversionSpecifier,
context: Context) -> List[Tuple[Callable[[Node], None],
context: Context) -> List[Tuple[Callable[[Expression], None],
Callable[[Type], None]]]:
"""Returns a list of tuples of two functions that check whether a replacement is
of the right type for the specifier. The first functions take a node and checks
its type in the right type context. The second function just checks a type.
"""
checkers = [] # type: List[ Tuple[ Callable[[Node], None], Callable[[Type], None] ] ]
checkers = [] # type: List[Tuple[Callable[[Expression], None], Callable[[Type], None]]]

if specifier.width == '*':
checkers.append(self.checkers_for_star(context))
Expand All @@ -218,7 +219,7 @@ def replacement_checkers(self, specifier: ConversionSpecifier,
checkers.append(c)
return checkers

def checkers_for_star(self, context: Context) -> Tuple[Callable[[Node], None],
def checkers_for_star(self, context: Context) -> Tuple[Callable[[Expression], None],
Callable[[Type], None]]:
"""Returns a tuple of check functions that check whether, respectively,
a node or a type is compatible with a star in a conversion specifier
Expand All @@ -229,14 +230,14 @@ def check_type(type: Type = None) -> None:
expected = self.named_type('builtins.int')
self.chk.check_subtype(type, expected, context, '* wants int')

def check_node(node: Node) -> None:
type = self.accept(node, expected)
def check_expr(expr: Expression) -> None:
type = self.accept(expr, expected)
check_type(type)

return check_node, check_type
return check_expr, check_type

def checkers_for_regular_type(self, type: str,
context: Context) -> Tuple[Callable[[Node], None],
context: Context) -> Tuple[Callable[[Expression], None],
Callable[[Type], None]]:
"""Returns a tuple of check functions that check whether, respectively,
a node or a type is compatible with 'type'. Return None in case of an
Expand All @@ -250,14 +251,15 @@ def check_type(type: Type = None) -> None:
messages.INCOMPATIBLE_TYPES_IN_STR_INTERPOLATION,
'expression has type', 'placeholder has type')

def check_node(node: Node) -> None:
type = self.accept(node, expected_type)
def check_expr(expr: Expression) -> None:
type = self.accept(expr, expected_type)
check_type(type)

return check_node, check_type
return check_expr, check_type

def checkers_for_c_type(self, type: str, context: Context) -> Tuple[Callable[[Node], None],
Callable[[Type], None]]:
def checkers_for_c_type(self, type: str,
context: Context) -> Tuple[Callable[[Expression], None],
Callable[[Type], None]]:
"""Returns a tuple of check functions that check whether, respectively,
a node or a type is compatible with 'type' that is a character type
"""
Expand All @@ -270,14 +272,14 @@ def check_type(type: Type = None) -> None:
messages.INCOMPATIBLE_TYPES_IN_STR_INTERPOLATION,
'expression has type', 'placeholder has type')

def check_node(node: Node) -> None:
def check_expr(expr: Expression) -> None:
"""int, or str with length 1"""
type = self.accept(node, expected_type)
if isinstance(node, (StrExpr, BytesExpr)) and len(cast(StrExpr, node).value) != 1:
type = self.accept(expr, expected_type)
if isinstance(expr, (StrExpr, BytesExpr)) and len(cast(StrExpr, expr).value) != 1:
self.msg.requires_int_or_char(context)
check_type(type)

return check_node, check_type
return check_expr, check_type

def conversion_type(self, p: str, context: Context) -> Type:
"""Return the type that is accepted for a string interpolation
Expand Down Expand Up @@ -310,6 +312,6 @@ def named_type(self, name: str) -> Instance:
"""
return self.chk.named_type(name)

def accept(self, node: Node, context: Type = None) -> Type:
def accept(self, expr: Expression, context: Type = None) -> Type:
"""Type check a node. Alias for TypeChecker.accept."""
return self.chk.accept(node, context)
return self.chk.accept(expr, context)
Loading
0