← 返回首页
gh-123881: make compiler add the .generic_base base class without constructing AST nodes by iritkatriel · Pull Request #123883 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

gh-123881: make compiler add the .generic_base base class without constructing AST nodes#123883

Merged
iritkatriel merged 15 commits into
python:mainfrom
iritkatriel:generic
Sep 10, 2024
Merged

gh-123881: make compiler add the .generic_base base class without constructing AST nodes#123883
iritkatriel merged 15 commits into
python:mainfrom
iritkatriel:generic

Conversation

iritkatriel commented Sep 9, 2024
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

Comment thread Parser/action_helpers.c Outdated Show resolved Hide resolved

JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I feel this is worse, since it makes the AST returned by ast.parse different from what users actually see. Users dealing with the AST will have to learn about this new oddity.

The issue this is attached to says the compiler modifies the AST, but it doesn't; it creates a new ASDL seq and inserts an extra entry into it. The original AST is not modified.

bedevere-app Bot commented Sep 9, 2024

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Copy link
Copy Markdown
Member Author

it makes the AST returned by ast.parse different from what users actually see.

This is true anyway, the AST currently gives no indication that class C[T]: pass has a base class.

Comment thread Python/codegen.c Outdated
asdl_seq_SET(bases, original_len, name_node);
RETURN_IF_ERROR_IN_SCOPE(c, codegen_call_helper(c, loc, 2,
bases,
s->v.ClassDef.bases,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

If it's important to you to remove the code creating a fake AST node here, you could instead add an extra argument to codegen_call_helper that is usually NULL and maps to an extra argument to add at the end of the list. I think I did that initially while working on PEP 695, but it seemed worse than creating a new virtual asdl_seq as the current code does. (Possibly you were involved in discussing that in person, don't remember exactly.)

But I'd like to keep the extra base class as an implementation detail in the compiler, not something that gets exposed in the AST layer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Ok, I'll try something like that, should be quite simple.
It's possible I was involved in the discussion at the time, I don't remember the details.

I think it would be better if the compiler didn't need to know how to create AST nodes. (I take your point about it not modifying the AST, I'll correct the wording in the issue)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I found back the discussion from last time: #103764 (comment).

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

bedevere-app Bot commented Sep 9, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

@JelleZijlstra: please review the changes made to this pull request.

bedevere-app Bot requested a review from JelleZijlstra September 9, 2024 21:54
iritkatriel added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) and removed awaiting change review labels Sep 9, 2024
Comment thread Python/codegen.c Show resolved Hide resolved
iritkatriel changed the title gh-123881: add the .generic_base base class in the parser rather than the compiler gh-123881: make compiler add the .generic_base base class without constructing AST nodes Sep 10, 2024
iritkatriel enabled auto-merge (squash) September 10, 2024 15:50
iritkatriel merged commit a2d0818 into python:main Sep 10, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compiler should not need to know how to construct AST nodes

2 participants

Footer

© 2026 GitHub, Inc.