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

Fix partial type crash during protocol checking #9495

Merged
merged 3 commits into from Sep 29, 2020

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Sep 28, 2020

In particular, this affected hashables, as in #9437

Maybe there's a more standard way of falling back the PartialType? We could also consider moving this logic into is_subtype, but I'd be hesitant to do so. I'm okay with returning False based on the fallback Instance constructed here, but not returning True.

hauntsaninja added 2 commits September 27, 2020 23:59
In particular, this affected hashables.
@hauntsaninja hauntsaninja marked this pull request as ready for review September 29, 2020 01:40
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.

This is a relief. Thanks very much for identifying the true cause and providing a fix!

test-data/unit/check-protocols.test Outdated Show resolved Hide resolved


[case testPartialTypeProtocolHashable]
# flags: --strict-optional
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this? I thought it was the default. Or are the tests run multiple times with different default flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I meant to use --no-strict-optional as in #9437 (comment) — but a test a couple lines up used --strict-optional and I copied it by accident. Thanks!

Hmm. It actually looks like --no-strict-optional is the default in tests. I just discovered this file lurking in the shadows:

Strict optional is disabled be default

Also object in the test stubs doesn't have __hash__... so this test didn't actually test anything different from Blooper (and crashes regardless of --strict-optional). Fixed now (ie, the new test and old code only crashes with --no-strict-optional).

This test case was meant to use `--no-strict-optional`, not
`--strict-optional`, but happened to crash anyway because object in the
test stubs doesn't have __hash__. Both changes made.
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.

Yay!

@hauntsaninja hauntsaninja merged commit 9ab622c into python:master Sep 29, 2020
JukkaL pushed a commit that referenced this pull request Oct 1, 2020
In particular, this affected hashables. Fixes #9437

Co-authored-by: hauntsaninja <>
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

2 participants