-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix partial type crash during protocol checking #9495
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
Conversation
In particular, this affected hashables.
There was a problem hiding this 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
|
||
|
||
[case testPartialTypeProtocolHashable] | ||
# flags: --strict-optional |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Line 8 in 8bf770d
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay!
In particular, this affected hashables. Fixes #9437 Co-authored-by: hauntsaninja <>
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.