[mypyc] Fix ClassVar self-references in class bodies#21011
[mypyc] Fix ClassVar self-references in class bodies#21011VaggelisD wants to merge 2 commits intopython:masterfrom
ClassVar self-references in class bodies#21011Conversation
In CPython, the class body executes as a function where earlier
assignments are available to later ones (e.g. C = A | B where A
is a ClassVar defined earlier in the same class). mypyc previously
resolved such names via load_global(), looking them up in the module
globals dict where they don't exist — causing a KeyError at runtime.
Fix by tracking ClassVar names as they're defined during class body
processing, and redirecting lookups to the class being built: the
type object (py_get_attr) for extension classes, or the class dict
(dict_get_item_op) for non-extension classes.
This enables patterns like:
class Parser:
TYPE_TOKENS: ClassVar = {"INT", "VARCHAR"}
FUNC_TOKENS: ClassVar = TYPE_TOKENS | {"FUNCTION"}
for more information, see https://pre-commit.ci
|
IIUC this fixes mypyc/mypyc#972. If yes, could you please updated PR description to start with "Fixes mypyc/mypyc#972"? |
|
Btw thanks for all the PRs! Please note that we will have a release soon, so if you want these PRs included in v1.20, please ping @JukkaL (who will be release manager). |
|
Hey @ilevkivskyi, we actually ran into this in independently in SQLGlot, thus the dict-based repro instead of the much simpler
Thank you for the project! We were able to make SQLGlot faster by 4-5x on avg and we only just got started. Hopefully we will be contributing even more fixes and features. Regarding pinging, is asking for a review in Github the preferred way? I'm sure the core contributors are oversubscribed so I've avoided pinging in general. I had also joined the Gitter to discuss these solutions but I'm not sure if it's active anymore? |
Yeah, you can request review or
Yeah, Gitter is not active. We want to create a new Discord server at some point soon, but didn't get to this yet. |
Stating the obvious here but a Discord would be awesome; I can vouch that having to manage a community channel is not easy but it'd be of great help for external contributors, especially for more complex PRs in which upfront coordination & design can massively reduce PR friction later down the road. We'll jump in when it's ready! |
p-sawicki
left a comment
p-sawicki
left a comment
There was a problem hiding this comment.
it would be good to add irbuild tests with the new method calls to confirm they are generated as expected.
| # Class body context: tracks ClassVar names defined so far when processing | ||
| # a class body, so that intra-class references (e.g. C = A | B where A is | ||
| # a ClassVar defined earlier in the same class) can be resolved correctly. | ||
| # Without this, mypyc looks up such names in module globals, which fails. | ||
| self.class_body_classvars: dict[str, None] = {} | ||
| self.class_body_obj: Value | None = None | ||
| self.class_body_is_ext: bool = False |
There was a problem hiding this comment.
would it make sense to move these instance variables to ClassIR and add a pointer in IRBuilder to the IR that's currently processed? seems like that could simplify things a bit, since for example class_body_is_ext effectively duplicates ClassIR.is_ext_class.
| # Restore previous class body context (handles nested classes). | ||
| builder.class_body_classvars = saved_classvars | ||
| builder.class_body_obj = saved_obj | ||
| builder.class_body_is_ext = saved_is_ext |
There was a problem hiding this comment.
nested classes are currently not supported (they should be rejected on line 103) so this isn't needed.
| @mypyc_attr(allow_interpreted_subclasses=True) | ||
| class NonExt: | ||
| A: ClassVar[Set[str]] = {"a", "b"} | ||
| B: ClassVar[Set[str]] = {"c"} | ||
| C: ClassVar[Set[str]] = A | B | ||
|
|
||
| @mypyc_attr(allow_interpreted_subclasses=True) | ||
| class NonExtDict: | ||
| BASE: ClassVar[Dict[str, int]] = {"x": 1} | ||
| EXTENDED: ClassVar[Dict[str, int]] = {**BASE, "y": 2} | ||
|
|
||
| @mypyc_attr(allow_interpreted_subclasses=True) |
There was a problem hiding this comment.
native classes may allow interpreted subclasses so this isn't enough to make these non-ext. you need @mypyc_attr(native_class=False).
3 participants
Fixes mypyc/mypyc#972.
In CPython, the class body executes as a function where earlier assignments are available to later ones e.g this is possible:
mypyc previously resolved such names via
load_global(), looking them up in the module globals dict where they don't exist causing a KeyError at runtime.This PR fixes this by tracking
ClassVarnames as they're defined during class body processing, and redirecting lookups to the class being built: the type object (py_get_attr) for extension classes, or the class dict (dict_get_item_op) for non-extension classes.