Skip to content

[clean strict optional] Fix more strict optional errors #3292

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 7 commits into from
May 2, 2017

Conversation

ilevkivskyi
Copy link
Member

This PR makes --strict-optional clean for:

  • join.py
  • meet.py
  • solve.py
  • infer.py
  • constraints.py
  • exprtotype.py
  • checkmember.py
  • nodes.py

Fixing annotations in nodes.py lead to few dozens new errors (reducing the net progress), mostly in fastparse.py, fastparse2.py, semanal.py, checker.py, checkexpr.py. Actually those files seem to be most "dirty" (in general, the type layer is much "cleaner" than the expression layer).

There are few observations about what features would be useful with -strict-optional:

  • Always represent Union[X, None] as Optioal[X] in errors, not something like some element of union doesn't have attribute x or unsupported left type for + (some union)
  • Allow x = None # type: X in functions (not only in classes). This could be fixed by PEP 526 syntax, but only in Python 3.6+
  • It will be useful to recognize more complex checks like x[i] is not None, x.attr is not None, and None not in x (for collections).

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Basically LGTM -- all I have is small nits. The biggest one being about avoiding a cast().

@@ -518,7 +520,7 @@ def type_object_type_from_function(init_or_new: FuncBase, info: TypeInfo,

if init_or_new.info.fullname() == 'builtins.dict':
# Special signature!
special_sig = 'dict'
special_sig = 'dict' # type: Optional[str]
Copy link
Member

Choose a reason for hiding this comment

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

Honestly this would look better rewritten as

special_sig = None  # type: Optional[str]
if <blah>:
    special_sig = 'dict'

@@ -25,7 +25,7 @@ class Constraint:
It can be either T <: type or T :> type (T is a type variable).
"""

type_var = None # Type variable id
type_var = None # type: TypeVarId # Type variable id
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the comment seems pretty redundant now. :-)

@@ -52,6 +52,7 @@ def expr_to_unanalyzed_type(expr: Expression) -> Type:
result = parse_type_comment(expr.value, expr.line, None)
except SyntaxError:
raise TypeTranslationError()
assert result 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.

Could be moved closer to the assignment (inside the try block)?

mypy/meet.py Outdated
(item_name, s_item_type or t_item_type)
for (item_name, s_item_type, t_item_type) in self.s.zipall(t)
])
item_list = cast(List[Tuple[str, Type]],
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like casts, and this one looks particularly scary given the complexity of what it's casting from. Suggestion:

item_list = [(item_name, s_item_type if s_item_type is not None else t_item_type)
             for (item_name, s_item_type, t_item_type) in self.s.zipall(t)]]  # type: List[str, Optional[Type]]
# Actually none of the items are None now,
# since at least one of s_item_type and t_item_type is not None
items = OrderedDict(cast(List[str, Type], items_list))

That's still a cast, but the comprehension gets type-checked.

Another alternative would built item_list is a good old-fashioned for-loop, with a declared type of List[str, Type], and an assert in the loop body that the item is not None.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would choose the second option (for-loop), the comprehension is a bit hard to read here.

@ilevkivskyi
Copy link
Member Author

ilevkivskyi commented May 1, 2017

Thanks for review! I implemented all comments in the new commits.

@gvanrossum gvanrossum merged commit 058a8a6 into python:master May 2, 2017
gvanrossum pushed a commit that referenced this pull request May 2, 2017
This reverts commit 058a8a6.

The reason is that it crashes when run with Python 3.5.

Fixes #3301 (with a sledgehammer :-).
@ilevkivskyi ilevkivskyi deleted the fix-more-strict-optional branch October 7, 2017 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants