-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[mypyc] fix: isinstance_native fast path conflict w/ interp. subclasses [1/1] #19957
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
Open
BobTheBuidler
wants to merge
31
commits into
python:master
Choose a base branch
from
BobTheBuidler:fix-isinstance-native-interpreted-subclass
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 18 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
9a358fc
[mypyc] fix: isinstance_native fast path conflict w/ subclasses
BobTheBuidler 8446150
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] f84f576
handle non-native final
BobTheBuidler 376356b
Update class_ir.py
BobTheBuidler c22e6a4
Update ll_builder.py
BobTheBuidler 70cdff9
Update class_ir.py
BobTheBuidler 9a50f54
Merge branch 'master' into fix-isinstance-native-interpreted-subclass
BobTheBuidler 7459701
Update irbuild-classes.test
BobTheBuidler 86e5b00
refactor
BobTheBuidler d8602a0
Update irbuild-classes.test
BobTheBuidler 9fce629
Update irbuild-classes.test
BobTheBuidler b90961e
Update irbuild-classes.test
BobTheBuidler a554654
Update irbuild-classes.test
BobTheBuidler f514765
Update class_ir.py
BobTheBuidler bee44dc
Update class_ir.py
BobTheBuidler 3158d15
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 71a3c96
Update class_ir.py
BobTheBuidler 89b2e8f
Merge branch 'master' into fix-isinstance-native-interpreted-subclass
BobTheBuidler da41e3f
Merge branch 'master' into fix-isinstance-native-interpreted-subclass
BobTheBuidler 997364e
Update class_ir.py
BobTheBuidler 726e36e
Update class_ir.py
BobTheBuidler d8b2f2b
fix: change :: to =
BobTheBuidler 40876a8
fix: non-native class
BobTheBuidler ec59e5b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] cfac061
Update class_ir.py
BobTheBuidler 9dbc600
Update ll_builder.py
BobTheBuidler a379621
Merge branch 'master' into fix-isinstance-native-interpreted-subclass
BobTheBuidler b617fed
Update class_ir.py
BobTheBuidler b8aadd6
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] f06e6b6
Update class_ir.py
BobTheBuidler e47b5f9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2852,3 +2852,40 @@ class InvalidKwarg: | |
| @mypyc_attr(str()) # E: All "mypyc_attr" positional arguments must be string literals. | ||
| class InvalidLiteral: | ||
| pass | ||
|
|
||
| [case testIsInstanceInterpretedSubclasses] | ||
| from typing import Any | ||
| from mypy_extensions import mypyc_attr | ||
|
|
||
| @mypyc_attr(native_class=False) | ||
| class NonNative: | ||
| pass | ||
|
|
||
| @mypyc_attr(allow_interpreted_subclasses=True) | ||
| class InterpSubclasses: | ||
| pass | ||
|
|
||
| def isinstance_nonnative(x: Any) -> bool: | ||
| return isinstance(x, NonNative) | ||
| def isinstance_interpreted_subclasses(x: Any) -> bool: | ||
| return isinstance(x, InterpSubclasses) | ||
| [out] | ||
| def isinstance_nonnative(x): | ||
| x, r0 :: object | ||
| r1 :: ptr | ||
| r2 :: object | ||
| r3 :: bit | ||
| L0: | ||
| r0 = __main__.NonNative :: type | ||
| r1 = get_element_ptr x ob_type :: PyObject | ||
| r2 :: borrow load_mem r1 :: builtins.object* | ||
|
||
| keep_alive x | ||
| r3 :: r2 == r0 | ||
BobTheBuidler marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return r3 | ||
| def isinstance_interpreted_subclasses(x): | ||
| x, r0 :: object | ||
| r1 :: bool | ||
| L0: | ||
| r0 = __main__.InterpSubclasses :: type | ||
| r1 = CPy_TypeCheck(x, r0) | ||
| return r1 | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
I'm not entirely sure that this is correct in terms of the rest of the codebase right now, but semantically it feels like the correct thing to do
Any class with is_ext_class=False and is_final=False will inherently allow interpreted subclasses. But this new property bricked the codebase so clearly I'm misunderstanding something about how these 3 components interact
I can remove the property, fix just this one case, but then there will be other places in the codebase where allow_interpreted_subclasses could return False for a class that actually does allow interpreted subclasses. That's the state of things currently, but should it be?
Uh oh!
There was an error while loading. Please reload this page.
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.
@JukkaL this is the reason the tests currently fail, I can revert to a working commit if you agree that my above concern is not relevant
Or if it is relevant but is a non-blocker, we can continue conversation here when its more convenient: mypyc/mypyc#1148
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.
I think
allow_interpreted_subclassesis currently directly derived from@mypyc_attr(allow_interpreted_subclasses=True), and it indicates whether a native class could have interpreted subclassses. I think this is the most natural way to do this, and we can add a comment to make it more explicit if this is unclear right now. Various parts of the code that check for whether there could be interpreted subclasses may need to check foris_ext_classseparately. So instead of modifying the definition here, my suggestion is to add a check foris_ext_classand/oris_final_classin the isinstance fast path code (if required).Also, mypyc should generate a compile error if
allow_interpreted_subclasses=Trueis used with a non-native class or a final class (not sure if this is checked right now).Uh oh!
There was an error while loading. Please reload this page.
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.
Ah okay great that makes sense, thanks for this.
I already opened this a while back, adding a brief docstring to mypyc_attr with an attached proposal to enhance it with an Args: section, could I get a greenlight to proceed with that, adding a note about this and a short blurb on the other mypyc_attr kwargs?
Separately, I have a second PR to validate mypyc_attr arguments, if that's a welcome change I can extend it to account for this allows_interpreted_subclasses/native_class conflict
I'll get this PR fixed up tonight