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

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

Merged
merged 3 commits into from May 26, 2017

Conversation

Projects
None yet
3 participants
@ddfisher
Collaborator

ddfisher commented Apr 25, 2017

Fix python/typing#275.

(Likely needs a bit more discussion.)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Apr 25, 2017

Member

It would be much better if this started out as a command-line flag. (Probably global, unless it's really easy to make it per-module.)

Member

gvanrossum commented Apr 25, 2017

It would be much better if this started out as a command-line flag. (Probably global, unless it's really easy to make it per-module.)

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Apr 26, 2017

Collaborator

I agree that this would be better as a flag, at least in the next release or two.

Collaborator

JukkaL commented Apr 26, 2017

I agree that this would be better as a flag, at least in the next release or two.

@gvanrossum

Please make this a flag. (Global if you have to, per-mod if it's easy.)

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher May 4, 2017

Collaborator

Updated to use an off-by-default per-module flag.

Collaborator

ddfisher commented May 4, 2017

Updated to use an off-by-default per-module flag.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 9, 2017

Member

I haven't looked in detail at your code yet, but when I tried this, the typeshed tests for Python 2.7 started failing without enabling the new flag. To repro, check out this PR, then

cd typeshed
./tests/mypy_test.py -p2

I don't want to post the entire list of errors (too long and boring) but here are the first few and last few:

running mypy --python-version 2.7 --strict-optional # with 566 files
stdlib/2/__builtin__.pyi:29: error: Incompatible types in assignment (expression has type "Optional[str]", base class "object" defined the type as "Optional[str]")
stdlib/2/__builtin__.pyi:30: error: Incompatible types in assignment (expression has type "__builtin__.type", base class "object" defined the type as "builtins.type")
stdlib/2/__builtin__.pyi:31: error: Incompatible types in assignment (expression has type "Union[str, unicode, Iterable[Union[str, unicode]], None]", base class "object" defined the type as "Union[str, unicode, Iterable[Union[str, unicode]], None]")
stdlib/2/__builtin__.pyi:32: error: Incompatible types in assignment (expression has type "__builtin__.str", base class "object" defined the type as "builtins.str")
stdlib/2/__builtin__.pyi:36: error: Argument 1 of "__setattr__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:37: error: Argument 1 of "__eq__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:37: error: Return type of "__eq__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:38: error: Argument 1 of "__ne__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:38: error: Return type of "__ne__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:39: error: Return type of "__str__" incompatible with supertype "object"
.
.
.
stdlib/2/__builtin__.pyi:970: error: Argument 1 of "seek" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:970: error: Argument 2 of "seek" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:970: error: Return type of "seek" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:971: error: Return type of "tell" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:972: error: Argument 1 of "readline" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:972: error: Return type of "readline" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:973: error: Argument 1 of "readlines" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:973: error: Return type of "readlines" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:974: error: Argument 1 of "write" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:974: error: Return type of "write" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:975: error: Argument 1 of "writelines" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:976: error: Argument 1 of "truncate" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:976: error: Return type of "truncate" incompatible with supertype "IO"
--- exit status 1 ---
Member

gvanrossum commented May 9, 2017

I haven't looked in detail at your code yet, but when I tried this, the typeshed tests for Python 2.7 started failing without enabling the new flag. To repro, check out this PR, then

cd typeshed
./tests/mypy_test.py -p2

I don't want to post the entire list of errors (too long and boring) but here are the first few and last few:

running mypy --python-version 2.7 --strict-optional # with 566 files
stdlib/2/__builtin__.pyi:29: error: Incompatible types in assignment (expression has type "Optional[str]", base class "object" defined the type as "Optional[str]")
stdlib/2/__builtin__.pyi:30: error: Incompatible types in assignment (expression has type "__builtin__.type", base class "object" defined the type as "builtins.type")
stdlib/2/__builtin__.pyi:31: error: Incompatible types in assignment (expression has type "Union[str, unicode, Iterable[Union[str, unicode]], None]", base class "object" defined the type as "Union[str, unicode, Iterable[Union[str, unicode]], None]")
stdlib/2/__builtin__.pyi:32: error: Incompatible types in assignment (expression has type "__builtin__.str", base class "object" defined the type as "builtins.str")
stdlib/2/__builtin__.pyi:36: error: Argument 1 of "__setattr__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:37: error: Argument 1 of "__eq__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:37: error: Return type of "__eq__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:38: error: Argument 1 of "__ne__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:38: error: Return type of "__ne__" incompatible with supertype "object"
stdlib/2/__builtin__.pyi:39: error: Return type of "__str__" incompatible with supertype "object"
.
.
.
stdlib/2/__builtin__.pyi:970: error: Argument 1 of "seek" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:970: error: Argument 2 of "seek" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:970: error: Return type of "seek" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:971: error: Return type of "tell" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:972: error: Argument 1 of "readline" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:972: error: Return type of "readline" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:973: error: Argument 1 of "readlines" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:973: error: Return type of "readlines" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:974: error: Argument 1 of "write" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:974: error: Return type of "write" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:975: error: Argument 1 of "writelines" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:976: error: Argument 1 of "truncate" incompatible with supertype "IO"
stdlib/2/__builtin__.pyi:976: error: Return type of "truncate" incompatible with supertype "IO"
--- exit status 1 ---
# as unicode instead of bytes. This hack is generally okay,
# because mypy considers str literals to be compatible with
# unicode.
return StrExpr(n.s)

This comment has been minimized.

@gvanrossum

gvanrossum May 9, 2017

Member

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

@gvanrossum

gvanrossum May 9, 2017

Member

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

This comment has been minimized.

@ddfisher

ddfisher May 23, 2017

Collaborator

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).

@ddfisher

ddfisher May 23, 2017

Collaborator

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).

This comment has been minimized.

@gvanrossum

gvanrossum May 23, 2017

Member

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

@gvanrossum

gvanrossum May 23, 2017

Member

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

This comment has been minimized.

@gvanrossum

gvanrossum May 26, 2017

Member

(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.)

@gvanrossum

gvanrossum May 26, 2017

Member

(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.)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 23, 2017

Member

So I can still repro the issue with typeshed. Can you look into that?

Member

gvanrossum commented May 23, 2017

So I can still repro the issue with typeshed. Can you look into that?

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher May 24, 2017

Collaborator

Yep! I repro'd them too. I was overzealous in removing version checks -- one is actually needed in fastparse.py (see the commit I just pushed which fixes it).

Collaborator

ddfisher commented May 24, 2017

Yep! I repro'd them too. I was overzealous in removing version checks -- one is actually needed in fastparse.py (see the commit I just pushed which fixes it).

@gvanrossum gvanrossum merged commit 443c479 into master May 26, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gvanrossum gvanrossum deleted the no-implicit-optional branch May 26, 2017

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 26, 2017

Member

I tried to use this and realized it only has an effect when --strict-optional is also set. Fine.

But the error that it produces is "Incompatible types in assignment (expression has type None, variable has type ...)".

I think it deserves a more specialized error, e.g. "Default for argument XXX is incompatible with its type (expression has type None, argument has type ...)".

If you agree, can you file a new issue to track that task?

Member

gvanrossum commented May 26, 2017

I tried to use this and realized it only has an effect when --strict-optional is also set. Fine.

But the error that it produces is "Incompatible types in assignment (expression has type None, variable has type ...)".

I think it deserves a more specialized error, e.g. "Default for argument XXX is incompatible with its type (expression has type None, argument has type ...)".

If you agree, can you file a new issue to track that task?

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL May 26, 2017

Collaborator

Might be worth explicitly adding a note that suggests using Optional[...] after the error, and maybe also mention that implicit optional types for arguments are no longer supported.

Collaborator

JukkaL commented May 26, 2017

Might be worth explicitly adding a note that suggests using Optional[...] after the error, and maybe also mention that implicit optional types for arguments are no longer supported.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 26, 2017

Member

... are no longer supported

Should probably be "are not supported with --no-implicit-optional".

Member

gvanrossum commented May 26, 2017

... are no longer supported

Should probably be "are not supported with --no-implicit-optional".

carljm added a commit to carljm/mypy that referenced this pull request May 30, 2017

Merge branch 'master' into module-alias
* master: (23 commits)
  Make return type of open() more precise (#3477)
  Add test cases that delete a file during incremental checking (#3461)
  Parse each format-string component separately (#3390)
  Don't warn about returning Any if it is a proper subtype of the return type (#3473)
  Add __setattr__ support (#3451)
  Remove bundled lib-typing (#3337)
  Move version of extensions to post-release (#3348)
  Fix None slice bounds with strict-optional (#3445)
  Allow NewType subclassing NewType. (#3465)
  Add console scripts (#3074)
  Fix 'variance' label.
  Change label for variance section to just 'variance' (#3429)
  Better error message for invalid package names passed to mypy (#3447)
  Fix last character cut in html-report if file does not end with newline (#3466)
  Print pytest output as it happens (#3463)
  Add mypy roadmap (#3460)
  Add flag to avoid interpreting arguments with a default of None as Optional (#3248)
  Add type checking plugin support for functions (#3299)
  Mismatch of inferred type and return type note (#3428)
  Sync typeshed (#3449)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment