gh-137814: Fix __qualname__ of __annotate__#137842
gh-137814: Fix __qualname__ of __annotate__#137842JelleZijlstra wants to merge 7 commits intopython:mainfrom
__qualname__ of __annotate__#137842Conversation
JelleZijlstra
requested review from
carljm,
iritkatriel and
markshannon
as code owners
| PyObject *ste_id; /* int: key in ste_table->st_blocks */ | ||
| PyObject *ste_symbols; /* dict: variable names to flags */ | ||
| PyObject *ste_name; /* string: name of current block */ | ||
| PyObject *ste_function_name; /* string or NULL: for annotation blocks: name of the corresponding functions */ |
There was a problem hiding this comment.
We can’t use ste_name for this?
There was a problem hiding this comment.
It's possible but I don't know if it's better. I just pushed a version that does this.
Python/symtable.c
Outdated
| return Py_NewRef(&_Py_ID(__annotate__)); | ||
| } | ||
| return Py_NewRef(ste->ste_name); | ||
| } |
There was a problem hiding this comment.
I was assuming that the ste_name of an annotations ste would be set to __annotations__ so you wouldn’t need to do this. Is that not possible?
There was a problem hiding this comment.
Your previous comment suggested using the ste_name to store the name of the corresponding function instead. We can't do both.
There was a problem hiding this comment.
Can we set the ste_name to funcname.__annotations__?
There was a problem hiding this comment.
Then we'd end up with that in the __name__ of the function. I guess we could do that, but having a period in the __name__ seems odd.
There was a problem hiding this comment.
Ok, I'm not sure which option is best. I'll approve with whichever you choose.
There was a problem hiding this comment.
There are at least two options, the initial version of this PR (adding ste_function_name) and the one I pushed earlier this morning after your comment (overloading ste_name). The advantage of the ste_function_name version is that it's simpler: less code and ste_name always means the same thing. The advantage of the version that overloads ste_name is that we save a little on memory usage: one fewer pointer in the symbol table struct.
Personally I prefer the ste_function_name version since it's simpler even if it uses slightly more memory during compilation, but if you prefer the other version, I'm OK with that.
There was a problem hiding this comment.
I'm happy with the original version. Thank you for indulging my musings.
This reverts commit b4ae25b.
|
I mentioned on the issue that this needs a magic number bump. Maybe that makes it not worth backporting into 3.14? |
… the interpreter I'd still like to do python#137842 on 3.15+, but that requires changing bytecode and we can't really afford to do that in 3.14. So to fix this in 3.14, let's patch things up in the ceval loop instead. This is safe because the compiler only sets __annotate__ to just-created dedicated annotate functions.
3 participants
__annotate__functions generated by CPython for class methods have an incorrect__qualname__#137814