Skip to content
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

[clean strict optional] Use dummy TypeInfo instead of None in Instance.type (and few more fixes) #3285

Merged
merged 7 commits into from Apr 30, 2017

Conversation

ilevkivskyi
Copy link
Member

This PR fixes 146 --strict-optional errors and makes the following clean:

  • types.py
  • subtypes.py
  • fixup.py

Main change is the use of dummy TypeInfo in Instance.type after de-serialization.

Also I have noticed a conflict between tests and docs for TypeVarDef.values: there are two comments that explicitly say "empty list if no restriction", while there are two tests that use None. I decided to follow the docs.

@ilevkivskyi
Copy link
Member Author

There are still 343 --strict-optional errors left. As compared to initial 547 errors, this looks like a progress.

@gvanrossum
Copy link
Member

Hm, I'm not so keen on the dummy TypeInfo. I don't think it makes the code clearer, and None really is intended for this kind of case. I understand the problem it's trying to solve (parts of the code require a value that's not None but other parts clearly may have None, for the same attribute) but I thought that assert x is not None is less disruptive and in a sense clearer. The failure will also be clearer -- with an assert it's pretty clear the problem is not the user's fault.

(TBH, I haven't reviewed the whole PR yet, this just jumped out.)

@ilevkivskyi
Copy link
Member Author

@gvanrossum
This was proposed by @JukkaL in #3265 (comment) and I very much agree with him, because othervise we will need around 80-100 asserts.

@gvanrossum
Copy link
Member

OK, fair enough. That's definitely too many asserts! Maybe the __getattr__ should raise AssertionError then? (And another comment pointing future maintainers in the right direction when this error actually triggers would also be helpful -- none of us may be around then. :-)

@gvanrossum
Copy link
Member

Other than that LGTM, and if you feel like it you can merge it when you've changed the RuntimeError into AssertionError.

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.

Waiting for a change to FakeInfo to raise AssertionError (or a rebuttal :-).

@ilevkivskyi
Copy link
Member Author

Sorry, made these changes just now and pushed (was away from computers most of the day).

@gvanrossum
Copy link
Member

was away from computers most of the day

Lucky you. :-)

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.

LGTM! You could add an explanation of why None isn't a good dummy value, up to you. You can merge once the tests pass.

# 1. This will require around 80-100 asserts to make 'mypy --strict-optional mypy'
# pass cleanly.
# 2. If NOT_READY value is accidentally used somewhere, it will be obvious where the value
# is from, whereas a 'None' value could come from anywhere.
Copy link
Member

Choose a reason for hiding this comment

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

Excellent reasons!

@gvanrossum gvanrossum merged commit 8ecccda into python:master Apr 30, 2017
# pass cleanly.
# 2. If NOT_READY value is accidentally used somewhere, it will be obvious where the value
# is from, whereas a 'None' value could come from anywhere.
def __getattr__(self, attr: str) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ilevkivskyi @gvanrossum This should probably be __getattribute__ instead. Now attributes defined in the body of TypeInfo will not trigger an AssertionError, as far as I know, and thus FakeInfo can mask some actual errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case __getattribute__ looks more robust. Will make a PR soon.

Copy link
Member

Choose a reason for hiding this comment

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

OK, but then I suspect that #3281 will come back. So we should at least have a test in place to verify it doesn't. (Because that's the kind of error that's being masked here -- and since in that particular case it was just getting the wrong full name of some type for some error message I'd rather have a poor error than a crash -- our users really don't like crashes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gvanrossum
Yes, #3281 comes back, I tried your crash scenario and it indeed crashes (although in a different place). But it looks like Jukka's idea works! At least I have found two actual bugs in fixup.py, both related to metaclass de-serialization. After fixing those, your scenario from #3281 doe snot crash anymore. I will make a PR now so that you can try it.

gvanrossum pushed a commit that referenced this pull request May 11, 2017
The idea was proposed by Jukka in #3285 (comment)

This could (hopefully) fix #3281 for real.
Also fixes #3278
@ilevkivskyi ilevkivskyi deleted the fix-instance-optional branch October 2, 2017 07:19
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.

None yet

3 participants