GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-123881: make compiler add the .generic_base base class without constructing AST nodes by iritkatriel · Pull Request #123883 · python/cpython

Conversation

@iritkatriel

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Fixes #123881.

  • Issue: Compiler should not need to know how to construct AST nodes #123881

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
iritkatriel added 4 commits September 9, 2024 19:38
@iritkatriel
add action helper for ClassDef
031c697
@iritkatriel
add .generic_base to bases in AST so compiler doesn't need to do it
6547576
@iritkatriel
update tests
412fe99
@iritkatriel
add news
781fc79
@iritkatriel
iritkatriel requested a review from jeremyhylton September 9, 2024 21:07
@iritkatriel
iritkatriel requested review from JelleZijlstra, isidentical, lysnikolaou, markshannon and pablogsal as code owners September 9, 2024 21:07
@bedevere-app bedevere-app Bot added the awaiting core review label Sep 9, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Sep 9, 2024
Compiler should not need to know how to construct AST nodes #123881
Closed
iritkatriel
iritkatriel commented Sep 9, 2024
View reviewed changes
Comment thread Parser/action_helpers.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra requested changes Sep 9, 2024
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting core review labels Sep 9, 2024
@bedevere-app

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
add whatsnew entry
8200b77
@iritkatriel

iritkatriel commented Sep 9, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra reviewed Sep 9, 2024
View reviewed changes
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,

@JelleZijlstra JelleZijlstra Sep 9, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iritkatriel iritkatriel Sep 9, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@JelleZijlstra JelleZijlstra Sep 9, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
iritkatriel added 6 commits September 9, 2024 22:25
@iritkatriel
Revert "add whatsnew entry"
d75532a
This reverts commit 8200b77.
@iritkatriel
Revert "add news"
22228eb
This reverts commit 781fc79.
@iritkatriel
Revert "update tests"
4831765
This reverts commit 412fe99.
@iritkatriel
Revert "add .generic_base to bases in AST so compiler doesn't need to…
5218173
… do it"

This reverts commit 6547576.
@iritkatriel
Revert "add action helper for ClassDef"
0e52c0f
This reverts commit 031c697.
@iritkatriel
keep it in the compiler
341f8a5
@iritkatriel

iritkatriel commented Sep 9, 2024

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Sep 9, 2024
@bedevere-app

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app
bedevere-app Bot requested a review from JelleZijlstra September 9, 2024 21:54
@iritkatriel iritkatriel added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) and removed awaiting change review labels Sep 9, 2024
JelleZijlstra
JelleZijlstra reviewed Sep 9, 2024
View reviewed changes
Comment thread Python/codegen.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 2 commits September 10, 2024 13:05
@iritkatriel
handle stararg case
e92c432
@iritkatriel
Merge remote-tracking branch 'upstream/main' into generic
40046c1
@iritkatriel 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
JelleZijlstra
JelleZijlstra approved these changes Sep 10, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added the awaiting merge label Sep 10, 2024
iritkatriel and others added 2 commits September 10, 2024 16:30
@iritkatriel
the compiler struct no longer needs the arena
4611b92
@iritkatriel
Merge branch 'main' into generic
c6c3cff
@iritkatriel
iritkatriel enabled auto-merge (squash) September 10, 2024 15:50
@iritkatriel
iritkatriel merged commit a2d0818 into python:main Sep 10, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@jeremyhylton jeremyhylton Awaiting requested review from jeremyhylton
@isidentical isidentical Awaiting requested review from isidentical
@pablogsal pablogsal Awaiting requested review from pablogsal
@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Compiler should not need to know how to construct AST nodes

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@iritkatriel @JelleZijlstra
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.