Skip to content

Don't interpret arguments with a default of None as Optional #3248

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 3 commits into from
May 26, 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
2 changes: 2 additions & 0 deletions docs/source/config_file.rst
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ overridden by the pattern sections matching the module name.
- ``strict_boolean`` (Boolean, default False) makes using non-boolean
expressions in conditions an error.

- ``no_implicit_optional`` (Boolean, default false) changes the treatment of
arguments with a default value of None by not implicitly making their type Optional

Example
*******
Expand Down
54 changes: 23 additions & 31 deletions mypy/fastparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from mypy import experiments
from mypy import messages
from mypy.errors import Errors
from mypy.options import Options

try:
from typed_ast import ast3
Expand Down Expand Up @@ -58,14 +59,12 @@


def parse(source: Union[str, bytes], fnam: str = None, errors: Errors = None,
pyversion: Tuple[int, int] = defaults.PYTHON3_VERSION,
custom_typing_module: str = None) -> MypyFile:
options: Options = Options()) -> MypyFile:

"""Parse a source file, without doing any semantic analysis.

Return the parse tree. If errors is not provided, raise ParseError
on failure. Otherwise, use the errors object to report parse errors.

The pyversion (major, minor) argument determines the Python syntax variant.
"""
raise_on_error = False
if errors is None:
Expand All @@ -74,14 +73,16 @@ def parse(source: Union[str, bytes], fnam: str = None, errors: Errors = None,
errors.set_file('<input>' if fnam is None else fnam, None)
is_stub_file = bool(fnam) and fnam.endswith('.pyi')
try:
assert pyversion[0] >= 3 or is_stub_file
feature_version = pyversion[1] if not is_stub_file else defaults.PYTHON3_VERSION[1]
if is_stub_file:
feature_version = defaults.PYTHON3_VERSION[1]
else:
assert options.python_version[0] >= 3
feature_version = options.python_version[1]
ast = ast3.parse(source, fnam, 'exec', feature_version=feature_version)

tree = ASTConverter(pyversion=pyversion,
tree = ASTConverter(options=options,
is_stub=is_stub_file,
errors=errors,
custom_typing_module=custom_typing_module,
).visit(ast)
tree.path = fnam
tree.is_stub = is_stub_file
Expand Down Expand Up @@ -136,17 +137,15 @@ def is_no_type_check_decorator(expr: ast3.expr) -> bool:

class ASTConverter(ast3.NodeTransformer): # type: ignore # typeshed PR #931
def __init__(self,
pyversion: Tuple[int, int],
options: Options,
is_stub: bool,
errors: Errors,
custom_typing_module: str = None) -> None:
errors: Errors) -> None:
self.class_nesting = 0
self.imports = [] # type: List[ImportBase]

self.pyversion = pyversion
self.options = options
self.is_stub = is_stub
self.errors = errors
self.custom_typing_module = custom_typing_module

def fail(self, msg: str, line: int, column: int) -> None:
self.errors.report(line, column, msg)
Expand Down Expand Up @@ -260,9 +259,9 @@ def translate_module_id(self, id: str) -> str:

For example, translate '__builtin__' in Python 2 to 'builtins'.
"""
if id == self.custom_typing_module:
if id == self.options.custom_typing_module:
return 'typing'
elif id == '__builtin__' and self.pyversion[0] == 2:
elif id == '__builtin__' and self.options.python_version[0] == 2:
# HACK: __builtin__ in Python 2 is aliases to builtins. However, the implementation
# is named __builtin__.py (there is another layer of translation elsewhere).
return 'builtins'
Expand Down Expand Up @@ -388,7 +387,7 @@ def do_func_def(self, n: Union[ast3.FunctionDef, ast3.AsyncFunctionDef],
return func_def

def set_type_optional(self, type: Type, initializer: Expression) -> None:
if not experiments.STRICT_OPTIONAL:
if self.options.no_implicit_optional or not experiments.STRICT_OPTIONAL:
return
# Indicate that type should be wrapped in an Optional if arg is initialized to None.
optional = isinstance(initializer, NameExpr) and initializer.name == 'None'
Expand Down Expand Up @@ -855,16 +854,13 @@ def visit_Num(self, n: ast3.Num) -> Union[IntExpr, FloatExpr, ComplexExpr]:
# Str(string s)
@with_line
def visit_Str(self, n: ast3.Str) -> Union[UnicodeExpr, StrExpr]:
if self.pyversion[0] >= 3 or self.is_stub:
# Hack: assume all string literals in Python 2 stubs are normal
# strs (i.e. not unicode). All stubs are parsed with the Python 3
# parser, which causes unprefixed string literals to be interpreted
# as unicode instead of bytes. This hack is generally okay,
# because mypy considers str literals to be compatible with
# unicode.
return StrExpr(n.s)
else:
return UnicodeExpr(n.s)
# Hack: assume all string literals in Python 2 stubs are normal
# strs (i.e. not unicode). All stubs are parsed with the Python 3
# parser, which causes unprefixed string literals to be interpreted
# as unicode instead of bytes. This hack is generally okay,
# because mypy considers str literals to be compatible with
# unicode.
return StrExpr(n.s)
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem at first unrelated to --no-implicit-optional -- why are they here?

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 made this change in lieu of updating self.pyversion -> self.options.python_version because we've asserted that this is a Python 3 or stub file in the constructor (so only one codepath is ever taken here).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK. There should be no major version checks in fastparse{,2}.py at all.

Copy link
Member

Choose a reason for hiding this comment

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

(In the light of the bug you fixed, as a follow-up, it seems there should be no major version checks in fastparse2.py, but those in fastparse.py should be kept, since much of that file is still shared with fastparse2.py.)


# Only available with typed_ast >= 0.6.2
if hasattr(ast3, 'JoinedStr'):
Expand Down Expand Up @@ -894,11 +890,7 @@ def visit_Bytes(self, n: ast3.Bytes) -> Union[BytesExpr, StrExpr]:
# The following line is a bit hacky, but is the best way to maintain
# compatibility with how mypy currently parses the contents of bytes literals.
contents = str(n.s)[2:-1]

if self.pyversion[0] >= 3:
return BytesExpr(contents)
else:
return StrExpr(contents)
return BytesExpr(contents)

# NameConstant(singleton value)
def visit_NameConstant(self, n: ast3.NameConstant) -> NameExpr:
Expand Down
37 changes: 12 additions & 25 deletions mypy/fastparse2.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@
from mypy.types import (
Type, CallableType, AnyType, UnboundType, EllipsisType
)
from mypy import defaults
from mypy import experiments
from mypy import messages
from mypy.errors import Errors
from mypy.fastparse import TypeConverter, parse_type_comment
from mypy.options import Options

try:
from typed_ast import ast27
Expand Down Expand Up @@ -74,14 +74,11 @@


def parse(source: Union[str, bytes], fnam: str = None, errors: Errors = None,
pyversion: Tuple[int, int] = defaults.PYTHON3_VERSION,
custom_typing_module: str = None) -> MypyFile:
options: Options = Options()) -> MypyFile:
"""Parse a source file, without doing any semantic analysis.

Return the parse tree. If errors is not provided, raise ParseError
on failure. Otherwise, use the errors object to report parse errors.

The pyversion (major, minor) argument determines the Python syntax variant.
"""
raise_on_error = False
if errors is None:
Expand All @@ -90,12 +87,11 @@ def parse(source: Union[str, bytes], fnam: str = None, errors: Errors = None,
errors.set_file('<input>' if fnam is None else fnam, None)
is_stub_file = bool(fnam) and fnam.endswith('.pyi')
try:
assert pyversion[0] < 3 and not is_stub_file
assert options.python_version[0] < 3 and not is_stub_file
ast = ast27.parse(source, fnam, 'exec')
tree = ASTConverter(pyversion=pyversion,
tree = ASTConverter(options=options,
is_stub=is_stub_file,
errors=errors,
custom_typing_module=custom_typing_module,
).visit(ast)
assert isinstance(tree, MypyFile)
tree.path = fnam
Expand Down Expand Up @@ -137,17 +133,15 @@ def is_no_type_check_decorator(expr: ast27.expr) -> bool:

class ASTConverter(ast27.NodeTransformer):
def __init__(self,
pyversion: Tuple[int, int],
options: Options,
is_stub: bool,
errors: Errors,
custom_typing_module: str = None) -> None:
errors: Errors) -> None:
self.class_nesting = 0
self.imports = [] # type: List[ImportBase]

self.pyversion = pyversion
self.options = options
self.is_stub = is_stub
self.errors = errors
self.custom_typing_module = custom_typing_module

def fail(self, msg: str, line: int, column: int) -> None:
self.errors.report(line, column, msg)
Expand Down Expand Up @@ -262,9 +256,9 @@ def translate_module_id(self, id: str) -> str:

For example, translate '__builtin__' in Python 2 to 'builtins'.
"""
if id == self.custom_typing_module:
if id == self.options.custom_typing_module:
return 'typing'
elif id == '__builtin__' and self.pyversion[0] == 2:
elif id == '__builtin__':
# HACK: __builtin__ in Python 2 is aliases to builtins. However, the implementation
# is named __builtin__.py (there is another layer of translation elsewhere).
return 'builtins'
Expand Down Expand Up @@ -370,7 +364,7 @@ def visit_FunctionDef(self, n: ast27.FunctionDef) -> Statement:
return func_def

def set_type_optional(self, type: Type, initializer: Expression) -> None:
if not experiments.STRICT_OPTIONAL:
if self.options.no_implicit_optional or not experiments.STRICT_OPTIONAL:
return
# Indicate that type should be wrapped in an Optional if arg is initialized to None.
optional = isinstance(initializer, NameExpr) and initializer.name == 'None'
Expand Down Expand Up @@ -872,16 +866,9 @@ def visit_Str(self, s: ast27.Str) -> Expression:
# The following line is a bit hacky, but is the best way to maintain
# compatibility with how mypy currently parses the contents of bytes literals.
contents = str(n)[2:-1]

if self.pyversion[0] >= 3:
return BytesExpr(contents)
else:
return StrExpr(contents)
return StrExpr(contents)
else:
if self.pyversion[0] >= 3 or self.is_stub:
return StrExpr(s.s)
else:
return UnicodeExpr(s.s)
return UnicodeExpr(s.s)

# Ellipsis
def visit_Ellipsis(self, n: ast27.Ellipsis) -> EllipsisExpr:
Expand Down
2 changes: 2 additions & 0 deletions mypy/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ def add_invertible_flag(flag: str,
add_invertible_flag('--show-error-context', default=False,
dest='show_error_context',
help='Precede errors with "note:" messages explaining context')
add_invertible_flag('--no-implicit-optional', default=False, strict_flag=True,
help="don't assume arguments with default values of None are Optional")
parser.add_argument('-i', '--incremental', action='store_true',
help="enable module cache")
parser.add_argument('--quick-and-dirty', action='store_true',
Expand Down
4 changes: 4 additions & 0 deletions mypy/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class Options:
"warn_return_any",
"ignore_errors",
"strict_boolean",
"no_implicit_optional",
}

OPTIONS_AFFECTING_CACHE = PER_MODULE_OPTIONS | {"strict_optional", "quick_and_dirty"}
Expand Down Expand Up @@ -92,6 +93,9 @@ def __init__(self) -> None:
# Alternate way to show/hide strict-None-checking related errors
self.show_none_errors = True

# Don't assume arguments with default values of None are Optional
self.no_implicit_optional = False

# Use script name instead of __main__
self.scripts_are_modules = False

Expand Down
6 changes: 2 additions & 4 deletions mypy/parse.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,10 @@ def parse(source: Union[str, bytes],
return mypy.fastparse.parse(source,
fnam=fnam,
errors=errors,
pyversion=options.python_version,
custom_typing_module=options.custom_typing_module)
options=options)
else:
import mypy.fastparse2
return mypy.fastparse2.parse(source,
fnam=fnam,
errors=errors,
pyversion=options.python_version,
custom_typing_module=options.custom_typing_module)
options=options)
18 changes: 8 additions & 10 deletions test-data/unit/check-optional.test
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,10 @@ def f(x: int = None) -> None:
f(None)
[out]

[case testInferOptionalFromDefaultNoneWithFastParser]

def f(x: int = None) -> None:
x + 1 # E: Unsupported left operand type for + (some union)
f(None)
[case testNoInferOptionalFromDefaultNone]
# flags: --no-implicit-optional
def f(x: int = None) -> None: # E: Incompatible types in assignment (expression has type None, variable has type "int")
pass
[out]

[case testInferOptionalFromDefaultNoneComment]
Expand All @@ -139,12 +138,11 @@ def f(x=None):
f(None)
[out]

[case testInferOptionalFromDefaultNoneCommentWithFastParser]

def f(x=None):
[case testNoInferOptionalFromDefaultNoneComment]
# flags: --no-implicit-optional
def f(x=None): # E: Incompatible types in assignment (expression has type None, variable has type "int")
# type: (int) -> None
x + 1 # E: Unsupported left operand type for + (some union)
f(None)
pass
[out]

[case testInferOptionalType]
Expand Down