Skip to content

gh-137814: Fix __qualname__ of __annotate__ #137842

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
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Aug 16, 2025

@@ -90,6 +90,7 @@ typedef struct _symtable_entry {
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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can’t use ste_name for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible but I don't know if it's better. I just pushed a version that does this.

return Py_NewRef(&_Py_ID(__annotate__));
}
return Py_NewRef(ste->ste_name);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your previous comment suggested using the ste_name to store the name of the corresponding function instead. We can't do both.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set the ste_name to funcname.__annotations__?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm not sure which option is best. I'll approve with whichever you choose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the original version. Thank you for indulging my musings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants