FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Make strict Optional type system changes standard by ddfisher · Pull Request #3024 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Make strict Optional type system changes standard - #3024

Merged
JukkaL merged 7 commits into
masterfrom
per-file-strict-optional
Mar 19, 2017
ConversationCommits (7)ChecksFiles changed
Merged

Make strict Optional type system changes standard#3024
JukkaL merged 7 commits into
masterfrom
per-file-strict-optional

Conversation

ddfisher commented Mar 19, 2017

Copy link
Copy Markdown
Collaborator

After this lands, per-file strict Optional should be straightforward. This should fix a lot of bugs around None. Introduces a small bug with partial types (where mypy is overly permissive) which will be fixed in a follow up PR.

ddfisher force-pushed the per-file-strict-optional branch from 06ae294 to c0350ca Compare March 19, 2017 00:44
ddfisher force-pushed the per-file-strict-optional branch from c0350ca to 9f73487 Compare March 19, 2017 00:49
ddfisher force-pushed the per-file-strict-optional branch from 9f73487 to 5c44790 Compare March 19, 2017 01:32
JelleZijlstra reviewed Mar 19, 2017
View reviewed changes
Comment thread test-data/unit/check-classes.test
def g(self) -> None: pass
[out]
main:6: error: Return type of "f" incompatible with supertype "A"
main:7: error: Return type of "g" incompatible with supertype "A"

JelleZijlstra Mar 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why is this no longer an error?

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

None is a subtype of A, so this is valid.

gvanrossum Mar 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Well, only without --strict-optional, or if the base class has -> Optional[A].

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure, but --strict-optional is still not enabled by default.

gvanrossum Mar 19, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, I was confused by the PR title (and didn't read the whole thing).

ddfisher mentioned this pull request Mar 19, 2017
Closed
JukkaL reviewed Mar 19, 2017
View reviewed changes

JukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice! This simplifies things a lot and improves consistency.

Comment thread mypy/checker.py
# Lambdas are allowed to have None returns.
# Functions returning a value of type None are allowed to have a None return.
if isinstance(self.scope.top_function(), FuncExpr) or isinstance(typ, NoneTyp):
if is_lambda or isinstance(typ, NoneTyp):

JukkaL Mar 19, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is unclear.

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added some comments

Comment thread test-data/unit/check-inference.test Outdated
def ff() -> None:
x = f() # E: Need type annotation for variable
x = f()
reveal_type(x)

JukkaL Mar 19, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There's no output from reveal_type. The desired behavior would probably be to require an annotation for x.

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Filed issue for this instead: #3032.

Comment thread test-data/unit/check-inference.test
def g(a: T) -> None: pass
[out]

[case testUnsolvableInferenceResult]

JukkaL Mar 19, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Consider enabling this test case and renaming it if the output doesn't correspond to the original intent of the test case.

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a test involving void and is no longer relevant. It no longer tests anything interesting that is untested elsewhere.

Comment thread test-data/unit/check-inference.test
a = x

for y in []: # E: Need type annotation for variable
for y in []:

JukkaL Mar 19, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What's the type of y here? I think that the original message was fine. At least add a reveal_type.

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

y is inferred as None. Added a reveal_type.

Comment thread test-data/unit/check-inference.test
a = y

for e, f in [[]]: # E: Need type annotation for variable
for e, f in [[]]:

JukkaL Mar 19, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Similar to above; at least add reveal_types.

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

Comment thread test-data/unit/check-inference.test

[case testPartiallyInitializedToNoneAndThenToIncompleteType]
[case testPartiallyInitializedToNoneAndThenToIncompleteType-skip]
# TODO(ddfisher): fix partial type bug and re-enable

JukkaL Mar 19, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Add an issue for this so that we won't lose track of it.

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

#3031

Comment thread test-data/unit/pythoneval-asyncio.test Outdated
def test() -> 'Generator[Any, None, None]':
yield from greet()
x = yield from greet() # Error
x = yield from greet()

JukkaL Mar 19, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For consistency, we should retain the original error message (... does not return a value).

ddfisher Mar 19, 2017

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

ddfisher mentioned this pull request Mar 19, 2017
Closed
JukkaL merged commit 660018c into master Mar 19, 2017
ddfisher deleted the per-file-strict-optional branch March 19, 2017 20:13
JelleZijlstra mentioned this pull request Apr 23, 2017
Closed
ddfisher mentioned this pull request May 1, 2017
Open
ddfisher mentioned this pull request Jun 28, 2017
Closed
gvanrossum mentioned this pull request Jul 21, 2017
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JelleZijlstra JelleZijlstra left review comments
JukkaL JukkaL left review comments
gvanrossum gvanrossum left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL