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

Implement per-file strict Optional #3206

Merged
merged 9 commits into from Jun 28, 2017

Conversation

Projects
None yet
4 participants
@ddfisher
Collaborator

ddfisher commented Apr 21, 2017

I don't think this is quite 100%, but I think it's close. In particular, suggestions for more tests would be very helpful.

This definitely feels a little hacky, but I'm not convinced there's much of a better way to do it. It currently runs cleanly against the smaller of our internal repos, and with only 6 errors against the larger.

@gvanrossum

What do you think of the idea of moving the context managers into the call sites? Much less code churn, which somehow is always high on my list of PR quality issues.

with self.binder.top_frame_context():
for d in self.tree.defs:
self.accept(d)
with experiments.strict_optional_set(self.options.strict_optional):

This comment has been minimized.

@gvanrossum

gvanrossum Apr 23, 2017

Member

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

@gvanrossum

gvanrossum Apr 23, 2017

Member

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

This comment has been minimized.

@ddfisher

ddfisher Apr 25, 2017

Collaborator

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.

@ddfisher

ddfisher Apr 25, 2017

Collaborator

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.

This comment has been minimized.

@gvanrossum

gvanrossum Apr 25, 2017

Member

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?

@gvanrossum

gvanrossum Apr 25, 2017

Member

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?

This comment has been minimized.

@ddfisher

ddfisher Apr 25, 2017

Collaborator

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

@ddfisher

ddfisher Apr 25, 2017

Collaborator

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

This comment has been minimized.

@gvanrossum

gvanrossum Apr 25, 2017

Member

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

@gvanrossum

gvanrossum Apr 25, 2017

Member

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

saved = STRICT_OPTIONAL
STRICT_OPTIONAL = value
yield
STRICT_OPTIONAL = saved

This comment has been minimized.

@gvanrossum

gvanrossum Apr 23, 2017

Member

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

@gvanrossum

gvanrossum Apr 23, 2017

Member

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

This comment has been minimized.

@ddfisher

ddfisher Apr 25, 2017

Collaborator

Yep. :/

@ddfisher

ddfisher Apr 25, 2017

Collaborator

Yep. :/

@@ -407,7 +407,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]'

This comment has been minimized.

@gvanrossum

gvanrossum Apr 23, 2017

Member

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

@gvanrossum

gvanrossum Apr 23, 2017

Member

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

This comment has been minimized.

@ddfisher

ddfisher Apr 25, 2017

Collaborator

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.

@ddfisher

ddfisher Apr 25, 2017

Collaborator

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.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Apr 23, 2017

Collaborator

What are the plans concerning --strict-optional in the roadmap? I think it is important to make it default soon. I could volunteer to make necessary changes to annotations in mypy to make mypy --strict-optional mypy pass (if this will help).

Collaborator

ilevkivskyi commented Apr 23, 2017

What are the plans concerning --strict-optional in the roadmap? I think it is important to make it default soon. I could volunteer to make necessary changes to annotations in mypy to make mypy --strict-optional mypy pass (if this will help).

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Apr 23, 2017

Member

Regarding the roadmap, we all want this to become the default, but there are several roadblocks.

  • mypy itself is not easy to make strict-optional-clean (we tried when we first started).
  • Our (Dropbox) internal codebases are no strict-optional-clean by a long shot.
  • Ditto for other large codebases we know of.

It would be acceptable to turn on the option by default only when we at least have decent tactics to prevent regressions in mypy itself and the Dropbox codebases. For mypy, that tactic could be to enable it only for a few files, and then gradually increase coverage. That's relatively straightforward (unlike actually getting 100% coverage).

For the Dropbox codebases it's a little more complicated, because we are already actively using the old strict-optional settings in some places, and we have a complicated setup where we run mypy twice, once without strict-optional, and once with it, but suppressing errors in those files that aren't yet strict-optional-clean. I want us to switch to the new approach represented by this PR first before we can even think of making it the default in mypy, just so we don't have a flag that the whole world uses except us (there would be serious quality concerns there).

Really, you could argue with all of this (why should Dropbox be the bar) but I'd like to have some more real-world experience before turning it on by default.

If you find it simple to make mypy itself strict-optional-clean, please be our guest, but hopefully you can submit many small PRs rather than huge one that causes merge conflicts everywhere else.

Member

gvanrossum commented Apr 23, 2017

Regarding the roadmap, we all want this to become the default, but there are several roadblocks.

  • mypy itself is not easy to make strict-optional-clean (we tried when we first started).
  • Our (Dropbox) internal codebases are no strict-optional-clean by a long shot.
  • Ditto for other large codebases we know of.

It would be acceptable to turn on the option by default only when we at least have decent tactics to prevent regressions in mypy itself and the Dropbox codebases. For mypy, that tactic could be to enable it only for a few files, and then gradually increase coverage. That's relatively straightforward (unlike actually getting 100% coverage).

For the Dropbox codebases it's a little more complicated, because we are already actively using the old strict-optional settings in some places, and we have a complicated setup where we run mypy twice, once without strict-optional, and once with it, but suppressing errors in those files that aren't yet strict-optional-clean. I want us to switch to the new approach represented by this PR first before we can even think of making it the default in mypy, just so we don't have a flag that the whole world uses except us (there would be serious quality concerns there).

Really, you could argue with all of this (why should Dropbox be the bar) but I'd like to have some more real-world experience before turning it on by default.

If you find it simple to make mypy itself strict-optional-clean, please be our guest, but hopefully you can submit many small PRs rather than huge one that causes merge conflicts everywhere else.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Apr 23, 2017

Collaborator

@gvanrossum

If you find it simple to make mypy itself strict-optional-clean, please be our guest, but hopefully you can submit many small PRs rather than huge one that causes merge conflicts everywhere else.

This is what I was thinking about. There are in total 547 --strict-optional errors in mypy. PR #3228 fixes 26 errors by modifying 46 lines. After another 20 PRs like #3228 we will have clean --strict-optional.

Collaborator

ilevkivskyi commented Apr 23, 2017

@gvanrossum

If you find it simple to make mypy itself strict-optional-clean, please be our guest, but hopefully you can submit many small PRs rather than huge one that causes merge conflicts everywhere else.

This is what I was thinking about. There are in total 547 --strict-optional errors in mypy. PR #3228 fixes 26 errors by modifying 46 lines. After another 20 PRs like #3228 we will have clean --strict-optional.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Apr 23, 2017

Member

This is what I was thinking about. There are in total 547 --strict-optional errors in mypy. PR #3228 fixes 26 errors by modifying 46 lines. After another 20 PRs like #3228 we will have clean --strict-optional.

Hmm. Sometimes fixing an error in one place makes several new errors appear. Yet I am optimistic.

Member

gvanrossum commented Apr 23, 2017

This is what I was thinking about. There are in total 547 --strict-optional errors in mypy. PR #3228 fixes 26 errors by modifying 46 lines. After another 20 PRs like #3228 we will have clean --strict-optional.

Hmm. Sometimes fixing an error in one place makes several new errors appear. Yet I am optimistic.

@gvanrossum

Can you also make all the tests pass?

Show outdated Hide outdated mypy/checker.py Outdated
Show outdated Hide outdated mypy/checker.py Outdated
Show outdated Hide outdated mypy/checker.py Outdated
Show outdated Hide outdated mypy/types.py Outdated

ddfisher added some commits Apr 25, 2017

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Apr 25, 2017

Collaborator

🎉!

I think this still needs a little bit more work before merging -- a few more places need tweaking so it doesn't cause errors in our internal codebase. I'll ping here again once I've done that!

Collaborator

ddfisher commented Apr 25, 2017

🎉!

I think this still needs a little bit more work before merging -- a few more places need tweaking so it doesn't cause errors in our internal codebase. I'll ping here again once I've done that!

@gvanrossum

I really would like to make another plea for doing the proper refactor (enabling the decorator-based solution) now rather than piling up tech debt.

Highly indented code is less readable.

You could make two decorators, one that uses self.options, the other uses options passed in.

x = 0 # type: Optional[int]
y = None # type: None
[file mypy.ini]

This comment has been minimized.

@JukkaL

JukkaL May 2, 2017

Collaborator

Would it make sense to add tests with invariant collections such as List? In non-strict-optional files, should List[Optional[int]] be compatible with List[int] and vice versa?

@JukkaL

JukkaL May 2, 2017

Collaborator

Would it make sense to add tests with invariant collections such as List? In non-strict-optional files, should List[Optional[int]] be compatible with List[int] and vice versa?

This comment has been minimized.

@ddfisher

ddfisher Jun 24, 2017

Collaborator

Added!

@ddfisher

ddfisher Jun 24, 2017

Collaborator

Added!

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi May 4, 2017

Collaborator

Just an interesting observation: 6 --strict-optional errors were introduced by recent PRs in files that I previously cleaned (there are 20 such files now). None creeps in much faster than I expected :-)

Collaborator

ilevkivskyi commented May 4, 2017

Just an interesting observation: 6 --strict-optional errors were introduced by recent PRs in files that I previously cleaned (there are 20 such files now). None creeps in much faster than I expected :-)

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 26, 2017

Member

@ilevkivskyi wrote

errors were introduced by recent PRs in files that I previously cleaned

You can guard against that by adding per-module strict_optional = True settings to mypy_strict_optional.ini.

@ddfisher Can you resolve the conflicts and address the reviews?

Member

gvanrossum commented May 26, 2017

@ilevkivskyi wrote

errors were introduced by recent PRs in files that I previously cleaned

You can guard against that by adding per-module strict_optional = True settings to mypy_strict_optional.ini.

@ddfisher Can you resolve the conflicts and address the reviews?

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 20, 2017

Collaborator

@gvanrossum

You can guard against that by adding per-module strict_optional = True settings to mypy_strict_optional.ini.

Do I understand correctly that will only work after this PR is merged? Or can I continue working on making mypy strict-optional-clean without waiting for this PR?

Collaborator

ilevkivskyi commented Jun 20, 2017

@gvanrossum

You can guard against that by adding per-module strict_optional = True settings to mypy_strict_optional.ini.

Do I understand correctly that will only work after this PR is merged? Or can I continue working on making mypy strict-optional-clean without waiting for this PR?

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 20, 2017

Member

Sorry, yes, that option will only work once this PR is merged. We apologize for the delay, it's been on the back burner since around PyCon, but the plan is to finish the work before July starts.

Member

gvanrossum commented Jun 20, 2017

Sorry, yes, that option will only work once this PR is merged. We apologize for the delay, it's been on the back burner since around PyCon, but the plan is to finish the work before July starts.

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jun 24, 2017

Collaborator

(Not yet ready to merge -- there are several errors in internal codebases that still need to be fixed.)

Collaborator

ddfisher commented Jun 24, 2017

(Not yet ready to merge -- there are several errors in internal codebases that still need to be fixed.)

@gvanrossum

I am fine with this landing.

[file optional.py]
from typing import Optional, List
def f(x: List[int]) -> None: pass

This comment has been minimized.

@gvanrossum

gvanrossum Jun 27, 2017

Member

Didn't you mean to involve f() in some test in this file?

@gvanrossum

gvanrossum Jun 27, 2017

Member

Didn't you mean to involve f() in some test in this file?

This comment has been minimized.

@ddfisher

ddfisher Jun 28, 2017

Collaborator

I think I left it for parallel structure with the other tests, but it doesn't actually participate in this one.

@ddfisher

ddfisher Jun 28, 2017

Collaborator

I think I left it for parallel structure with the other tests, but it doesn't actually participate in this one.

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jun 28, 2017

Collaborator

This should now be ready to land. It still causes a small number of errors across our two main internal codebases, but the errors are all legitimate (and it's not clear to me why some of them aren't caught currently).

Collaborator

ddfisher commented Jun 28, 2017

This should now be ready to land. It still causes a small number of errors across our two main internal codebases, but the errors are all legitimate (and it's not clear to me why some of them aren't caught currently).

@gvanrossum gvanrossum merged commit 0619e26 into master Jun 28, 2017

3 checks passed

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 per-file-strict-optional branch Jun 28, 2017

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 28, 2017

Member

If there's an issue requesting this feature can you track it down and close it?

Member

gvanrossum commented Jun 28, 2017

If there's an issue requesting this feature can you track it down and close it?

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jun 28, 2017

Collaborator

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

Collaborator

ddfisher commented Jun 28, 2017

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jun 28, 2017

Collaborator

I don't see an issue for this, actually.

Collaborator

ddfisher commented Jun 28, 2017

I don't see an issue for this, actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment