Skip to content

Implement per-file strict Optional #3206

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 9 commits into from
Jun 28, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
23 changes: 20 additions & 3 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ def check_first_pass(self) -> None:

Deferred functions will be processed by check_second_pass().
"""
with experiments.strict_optional_set(self.options.strict_optional):
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be better to wrap the call sites in such a context manager? Fewer lines in the diff, and there's only one call site in build.py (and another in server/update.py which is only used by fine-grained incrementalism). Ditto for check_second_pass() and visit_file() -- these are each only called from one place in build.py and perhaps another place in server/.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure how likely it is that we ever add more call sites, but I think that's a bug in the making. The context manager should always wrap this code -- I don't think there's any case where we'd want to call this while ignoring the per-file Strict Optional setting. I understand that you're not a huge fan of code churn, but I don't think it'd be a worthwhile tradeoff in this instance.

Also, FWIW git blame has the -w flag which ignores whitespace changes, so this doesn't have to mess up anyone's blame.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. git might have that flag but GitHub doesn't (as shown in this code review).

Maybe you can make it a decorator containing a context manager?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like that won't quite work, but apparently github has a -w equivalent too! https://github.com/python/mypy/pull/3206/files?w=1

Copy link
Member

Choose a reason for hiding this comment

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

Hm, the -w flag is flawed -- it doesn't show review comments, and it's not sticky.

self.errors.set_file(self.path, self.tree.fullname())
with self.enter_partial_types():
with self.binder.top_frame_context():
Expand Down Expand Up @@ -204,6 +205,7 @@ def check_second_pass(self, todo: List[DeferredNode] = None) -> bool:

This goes through deferred nodes, returning True if there were any.
"""
with experiments.strict_optional_set(self.options.strict_optional):
if not todo and not self.deferred_nodes:
return False
self.errors.set_file(self.path, self.tree.fullname())
Expand Down Expand Up @@ -1498,6 +1500,12 @@ def check_multi_assignment(self, lvalues: List[Lvalue],
rvalue_type = self.expr_checker.accept(rvalue) # TODO maybe elsewhere; redundant
undefined_rvalue = False

if isinstance(rvalue_type, UnionType):
# If this is an Optional type in non-strict Optional code, unwrap it.
relevant_items = rvalue_type.relevant_items()
if len(relevant_items) == 1:
rvalue_type = relevant_items[0]

if isinstance(rvalue_type, AnyType):
for lv in lvalues:
if isinstance(lv, StarExpr):
Expand Down Expand Up @@ -1525,7 +1533,16 @@ def check_multi_assignment_from_tuple(self, lvalues: List[Lvalue], rvalue: Expre
if not undefined_rvalue:
# Infer rvalue again, now in the correct type context.
lvalue_type = self.lvalue_type_for_inference(lvalues, rvalue_type)
rvalue_type = cast(TupleType, self.expr_checker.accept(rvalue, lvalue_type))
reinferred_rvalue_type = self.expr_checker.accept(rvalue, lvalue_type)

if isinstance(reinferred_rvalue_type, UnionType):
# If this is an Optional type in non-strict Optional code, unwrap it.
relevant_items = reinferred_rvalue_type.relevant_items()
if len(relevant_items) == 1:
reinferred_rvalue_type = relevant_items[0]

assert isinstance(reinferred_rvalue_type, TupleType)
rvalue_type = reinferred_rvalue_type

left_rv_types, star_rv_types, right_rv_types = self.split_around_star(
rvalue_type.items, star_index, len(lvalues))
Expand Down Expand Up @@ -2161,7 +2178,7 @@ def get_types_from_except_handler(self, typ: Type, n: Expression) -> List[Type]:
elif isinstance(typ, UnionType):
return [
union_typ
for item in typ.items
for item in typ.relevant_items()
for union_typ in self.get_types_from_except_handler(item, n)
]
elif isinstance(typ, Instance) and is_named_instance(typ, 'builtins.tuple'):
Expand Down Expand Up @@ -2627,7 +2644,7 @@ def partition_by_callable(type: Optional[Type]) -> Tuple[List[Type], List[Type]]
if isinstance(type, UnionType):
callables = []
uncallables = []
for subtype in type.items:
for subtype in type.relevant_items():
subcallables, subuncallables = partition_by_callable(subtype)
callables.extend(subcallables)
uncallables.extend(subuncallables)
Expand Down
12 changes: 6 additions & 6 deletions mypy/checkexpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ def check_call(self, callee: Type, args: List[Expression],
self.msg.disable_type_names += 1
results = [self.check_call(subtype, args, arg_kinds, context, arg_names,
arg_messages=arg_messages)
for subtype in callee.items]
for subtype in callee.relevant_items()]
self.msg.disable_type_names -= 1
return (UnionType.make_simplified_union([res[0] for res in results]),
callee)
Expand Down Expand Up @@ -596,7 +596,7 @@ def analyze_type_type_callee(self, item: Type, context: Context) -> Type:
return res
if isinstance(item, UnionType):
return UnionType([self.analyze_type_type_callee(item, context)
for item in item.items], item.line)
for item in item.relevant_items()], item.line)
if isinstance(item, TypeVarType):
# Pretend we're calling the typevar's upper bound,
# i.e. its constructor (a poor approximation for reality,
Expand Down Expand Up @@ -1982,7 +1982,7 @@ def infer_lambda_type_using_context(self, e: LambdaExpr) -> Tuple[Optional[Calla
ctx = self.type_context[-1]

if isinstance(ctx, UnionType):
callables = [t for t in ctx.items if isinstance(t, CallableType)]
callables = [t for t in ctx.relevant_items() if isinstance(t, CallableType)]
if len(callables) == 1:
ctx = callables[0]

Expand Down Expand Up @@ -2284,7 +2284,7 @@ def has_member(self, typ: Type, member: str) -> bool:
elif isinstance(typ, AnyType):
return True
elif isinstance(typ, UnionType):
result = all(self.has_member(x, member) for x in typ.items)
result = all(self.has_member(x, member) for x in typ.relevant_items())
return result
elif isinstance(typ, TupleType):
return self.has_member(typ.fallback, member)
Expand Down Expand Up @@ -2659,10 +2659,10 @@ def overload_arg_similarity(actual: Type, formal: Type) -> int:
return 2
if isinstance(actual, UnionType):
return max(overload_arg_similarity(item, formal)
for item in actual.items)
for item in actual.relevant_items())
if isinstance(formal, UnionType):
return max(overload_arg_similarity(actual, item)
for item in formal.items)
for item in formal.relevant_items())
if isinstance(formal, TypeType):
if isinstance(actual, TypeType):
# Since Type[T] is covariant, check if actual = Type[A] is
Expand Down
2 changes: 1 addition & 1 deletion mypy/checkmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def analyze_member_access(name: str,
results = [analyze_member_access(name, subtype, node, is_lvalue, is_super,
is_operator, builtin_type, not_ready_callback, msg,
original_type=original_type, chk=chk)
for subtype in typ.items]
for subtype in typ.relevant_items()]
msg.disable_type_names -= 1
return UnionType.make_simplified_union(results)
elif isinstance(typ, TupleType):
Expand Down
3 changes: 0 additions & 3 deletions mypy/constraints.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,9 +254,6 @@ class CompleteTypeVisitor(TypeQuery[bool]):
def __init__(self) -> None:
super().__init__(all)

def visit_none_type(self, t: NoneTyp) -> bool:
return experiments.STRICT_OPTIONAL

def visit_uninhabited_type(self, t: UninhabitedType) -> bool:
return False

Expand Down
12 changes: 11 additions & 1 deletion mypy/experiments.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
from typing import Optional, Tuple
from contextlib import contextmanager
from typing import Optional, Tuple, Iterator
STRICT_OPTIONAL = False
find_occurrences = None # type: Optional[Tuple[str, str]]


@contextmanager
def strict_optional_set(value: bool) -> Iterator[None]:
global STRICT_OPTIONAL
saved = STRICT_OPTIONAL
STRICT_OPTIONAL = value
yield
STRICT_OPTIONAL = saved
Copy link
Member

Choose a reason for hiding this comment

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

I know you said it was hacky, but this is where I realized quite how hacky... :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. :/

8 changes: 4 additions & 4 deletions mypy/meet.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ def narrow_declared_type(declared: Type, narrowed: Type) -> Type:
return declared
if isinstance(declared, UnionType):
return UnionType.make_simplified_union([narrow_declared_type(x, narrowed)
for x in declared.items])
for x in declared.relevant_items()])
elif not is_overlapping_types(declared, narrowed, use_promotions=True):
if experiments.STRICT_OPTIONAL:
return UninhabitedType()
else:
return NoneTyp()
elif isinstance(narrowed, UnionType):
return UnionType.make_simplified_union([narrow_declared_type(declared, x)
for x in narrowed.items])
for x in narrowed.relevant_items()])
elif isinstance(narrowed, AnyType):
return narrowed
elif isinstance(declared, (Instance, TupleType)):
Expand Down Expand Up @@ -99,10 +99,10 @@ class C(A, B): ...
return t.type in s.type.mro or s.type in t.type.mro
if isinstance(t, UnionType):
return any(is_overlapping_types(item, s)
for item in t.items)
for item in t.relevant_items())
if isinstance(s, UnionType):
return any(is_overlapping_types(t, item)
for item in s.items)
for item in s.relevant_items())
if isinstance(t, TypeType) and isinstance(s, TypeType):
# If both types are TypeType, compare their inner types.
return is_overlapping_types(t.item, s.item, use_promotions)
Expand Down
3 changes: 2 additions & 1 deletion mypy/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ class Options:
"ignore_errors",
"strict_boolean",
"no_implicit_optional",
"strict_optional",
}

OPTIONS_AFFECTING_CACHE = PER_MODULE_OPTIONS | {"strict_optional", "quick_and_dirty"}
OPTIONS_AFFECTING_CACHE = PER_MODULE_OPTIONS | {"quick_and_dirty"}

def __init__(self) -> None:
# -- build options --
Expand Down
7 changes: 6 additions & 1 deletion mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
from mypy.exprtotype import expr_to_unanalyzed_type, TypeTranslationError
from mypy.sametypes import is_same_type
from mypy.options import Options
from mypy import experiments
from mypy.plugin import Plugin
from mypy import join

Expand Down Expand Up @@ -276,6 +277,7 @@ def visit_file(self, file_node: MypyFile, fnam: str, options: Options,
self.globals = file_node.names
self.patches = patches

with experiments.strict_optional_set(options.strict_optional):
if 'builtins' in self.modules:
self.globals['__builtins__'] = SymbolTableNode(
MODULE_REF, self.modules['builtins'], self.cur_mod_id)
Expand Down Expand Up @@ -3612,6 +3614,7 @@ def visit_file(self, file: MypyFile, fnam: str, mod_id: str, options: Options) -

defs = file.defs

with experiments.strict_optional_set(options.strict_optional):
# Add implicit definitions of module '__name__' etc.
for name, t in implicit_module_attrs.items():
# unicode docstrings should be accepted in Python 2
Expand All @@ -3636,7 +3639,8 @@ def visit_file(self, file: MypyFile, fnam: str, mod_id: str, options: Options) -
if mod_id == 'builtins':
literal_types = [
('None', NoneTyp()),
# reveal_type is a mypy-only function that gives an error with the type of its arg
# reveal_type is a mypy-only function that gives an error with
# the type of its arg.
('reveal_type', AnyType()),
] # type: List[Tuple[str, Type]]

Expand Down Expand Up @@ -3851,6 +3855,7 @@ def __init__(self, modules: Dict[str, MypyFile], errors: Errors) -> None:
def visit_file(self, file_node: MypyFile, fnam: str, options: Options) -> None:
self.errors.set_file(fnam, file_node.fullname())
self.options = options
with experiments.strict_optional_set(options.strict_optional):
self.accept(file_node)

def refresh_partial(self, node: Union[MypyFile, FuncItem]) -> None:
Expand Down
2 changes: 1 addition & 1 deletion mypy/subtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ def restrict_subtype_away(t: Type, s: Type) -> Type:
if isinstance(t, UnionType):
# Since runtime type checks will ignore type arguments, erase the types.
erased_s = erase_type(s)
new_items = [item for item in t.items
new_items = [item for item in t.relevant_items()
if (not is_proper_subtype(erase_type(item), erased_s)
or isinstance(item, AnyType))]
return UnionType.make_union(new_items)
Expand Down
7 changes: 2 additions & 5 deletions mypy/typeanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,6 @@ def visit_unbound_type(self, t: UnboundType) -> Type:
return self.tuple_type(self.anal_array(t.args))
elif fullname == 'typing.Union':
items = self.anal_array(t.args)
if not experiments.STRICT_OPTIONAL:
items = [item for item in items if not isinstance(item, NoneTyp)]
return UnionType.make_union(items)
elif fullname == 'typing.Optional':
if len(t.args) != 1:
Expand Down Expand Up @@ -780,12 +778,11 @@ def make_optional_type(t: Type) -> Type:
is called during semantic analysis and simplification only works during
type checking.
"""
if not experiments.STRICT_OPTIONAL:
return t
if isinstance(t, NoneTyp):
return t
if isinstance(t, UnionType):
elif isinstance(t, UnionType):
items = [item for item in union_items(t)
if not isinstance(item, NoneTyp)]
return UnionType(items + [NoneTyp()], t.line, t.column)
else:
return UnionType([t, NoneTyp()], t.line, t.column)
10 changes: 9 additions & 1 deletion mypy/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
)
from mypy.sharedparse import argument_elide_name
from mypy.util import IdMapper
from mypy import experiments


T = TypeVar('T')
Expand Down Expand Up @@ -1083,7 +1084,14 @@ def has_readable_member(self, name: str) -> bool:
"""
return all((isinstance(x, UnionType) and x.has_readable_member(name)) or
(isinstance(x, Instance) and x.type.has_readable_member(name))
for x in self.items)
for x in self.relevant_items())

def relevant_items(self) -> List[Type]:
"""Removes NoneTypes from Unions when strict Optional checking is off."""
if experiments.STRICT_OPTIONAL:
return self.items
else:
return [i for i in self.items if not isinstance(i, NoneTyp)]

def serialize(self) -> JsonDict:
return {'.class': 'UnionType',
Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/check-class-namedtuple.test
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ class HasNone(NamedTuple):
x: int
y: Optional[int] = None

reveal_type(HasNone(1)) # E: Revealed type is 'Tuple[builtins.int, builtins.int, fallback=__main__.HasNone]'
reveal_type(HasNone(1)) # E: Revealed type is 'Tuple[builtins.int, Union[builtins.int, builtins.None], fallback=__main__.HasNone]'
Copy link
Member

Choose a reason for hiding this comment

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

Off-topic: I so wish that Union[X, None] were rendered as Optional[X]... (Hm, somewhere below there is a test that shows that: test-data/unit/check-isinstance.test -- what's the difference?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is in most cases, actually. Mypy has 2 different ways of rendering types: an internal representation and an external representation. The external representation is what's used for all errors, and renders Union[X, None] as Optional[X]. The internal representation is used by reveal_type for some reason that I no longer recall, and does not do this translation. I've been wanting to clean this up since forever (and make them all have a bit more PEP 484 style), but haven't ever had the time.


class Parameterized(NamedTuple):
x: int
Expand Down
Loading