gh-120323: Remove class check for redundant _CHECK_ATTR_CLASS #121002
Conversation
| op(_CHECK_ATTR_CLASS, (type_version/2, owner -- owner)) { | ||
| assert(type_version); | ||
| if (sym_is_a_class(owner) && | ||
| sym_matches_type_version(owner, type_version)) { |
There was a problem hiding this comment.
This is incorrect. sym_matches_type_version matches the type version of the symbol's class, not that of the symbol.
There was a problem hiding this comment.
Please stop marking comments as resolved when they are not.
It wastes a lot of time forcing me double check all resolved issues, if I cannot trust that they are resolved.
There was a problem hiding this comment.
Sorry again, this was an old PR and was back when I more freely resolved things.
|
When you're done making the requested changes, leave the comment: |
There was a problem hiding this comment.
This still needs tests.
It seems like you aren't making the distinction between the version of the symbol's class, and the version of a symbol that is a class.
This distinction is a subset of the more general distinction between a symbol representing a value (or finite set of values) and a symbol representing an instance of a class.
| @@ -117,6 +117,12 @@ dummy_func(void) { | |||
|
|
|||
| op(_GUARD_TYPE_VERSION, (type_version/2, owner -- owner)) { | |||
| assert(type_version); | |||
| // This is a contradiction. | |||
There was a problem hiding this comment.
This isn't necessarily a contradiction. Py_TYPE(owner)->tp_version == type_version can be true even if owner is a class.
I suspect we don't check the version number of metaclasses anywhere now, but it is definitely possible for enums.
sym_matches_type_version should check for the contradiction, so this check shouldn't be necessary.
| op(_CHECK_ATTR_CLASS, (type_version/2, owner -- owner)) { | ||
| assert(type_version); | ||
| if (sym_is_a_class(owner) && | ||
| sym_matches_type_version(owner, type_version)) { |
There was a problem hiding this comment.
Please stop marking comments as resolved when they are not.
It wastes a lot of time forcing me double check all resolved issues, if I cannot trust that they are resolved.
| @@ -207,6 +216,15 @@ _Py_uop_sym_set_non_null(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym) | |||
| sym_set_flag(sym, NOT_NULL); | |||
| } | |||
|
|
|||
| void | |||
| _Py_uop_sym_set_is_a_class(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, unsigned int type_version) | |||
There was a problem hiding this comment.
This needs a better name. The is in the middle reads strangely.
Maybe just _Py_uop_sym_set_class?
| @@ -121,6 +124,12 @@ _Py_uop_sym_is_null(_Py_UopsSymbol *sym) | |||
| return sym->flags == IS_NULL; | |||
| } | |||
|
|
|||
| bool | |||
| _Py_uop_sym_is_a_class(_Py_UopsSymbol *sym, unsigned int type_version) | |||
There was a problem hiding this comment.
These need tests.
Specifically tests setting the symbol to be a class, and then setting its class's version.
|
@Fidget-Spinner do plan to continue working on this? |
|
Not in the immediate term. If anyone else wants to work on this they are free to pick it up. Else I will reliok before 3.14 beta. |
When is the release date for version 3.14 beta? This job looks really interesting, and if I have time, I think I can give it a try. |
|
May 2025. Feel free to take over the PR. |
|
All commit authors signed the Contributor License Agreement. |
As requested, this I separated this PR out into just one with the type guard removal.