[mypyc] fix: isinstance_native fast path conflict w/ interp. subclasses [1/1] #19957
Conversation
BobTheBuidler
changed the title
i think it would make sense for Edit: though looking at the internals of i see there's one more use of could you also add an IR build test with code that hits this bug? |
|
This is preventing me from updating one of my projects to 1.18, what would you say about cherry-picking this and the crash fix in #19960 to a 1.18.3? |
BobTheBuidler
changed the title
|
not sure if there's going to be a 1.18.3 since #19764 is closed but maybe you could suggest it there. |
mypyc/ir/class_ir.py
Outdated
| return f"{self.module_name}.{self.name}" | ||
|
|
||
| @property | ||
| def allow_interpreted_subclasses(self) -> bool: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
I think allow_interpreted_subclasses is 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 for is_ext_class separately. So instead of modifying the definition here, my suggestion is to add a check for is_ext_class and/or is_final_class in the isinstance fast path code (if required).
Also, mypyc should generate a compile error if allow_interpreted_subclasses=True is used with a non-native class or a final class (not sure if this is checked right now).
There was a problem hiding this comment.
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
mypyc/test-data/irbuild-classes.test
Outdated
| L0: | ||
| r0 = __main__.NonNative :: type | ||
| r1 = get_element_ptr x ob_type :: PyObject | ||
| r2 :: borrow load_mem r1 :: builtins.object* |
There was a problem hiding this comment.
this IR will change once we address the property question
|
Some tests are failing. |
|
|
This branch is fixed up and ready for review. I've partially implemented the mypyc_attrs validation code but that isn't quite finished yet, we can continue on that here: #20242 |
| r1 = get_element_ptr x ob_type :: PyObject | ||
| r2 = borrow load_mem r1 :: builtins.object* | ||
| keep_alive x | ||
| r3 = r2 == r0 |
There was a problem hiding this comment.
Non-native classes implicitly allow interpreted subclasses, so this should also use CPy_TypeCheck. Can you also add a run test for these that defines the subclasses and uses them with isinstance?
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@JukkaL did something change on master recently which could lead to the current errors? I don't see any relation between the line I just added and the error we see, and last time that happened it was due to something with lib-rt |
for more information, see https://pre-commit.ci
3 participants
There is a bug in
isinstance_nativewhen one or more of the classes allows interpreted subclasses.When interpreted subclasses are allowed, we cannot make any assumptions about the number of subclasses at compile time.
I'm not sure if this fix is better suited in
all_concrete_classes