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 — bpo-42218: Correctly handle errors in left-recursive rules by lysnikolaou · Pull Request #23065 · python/cpython

Conversation

@lysnikolaou

@lysnikolaou lysnikolaou commented Oct 31, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Left-recursive rules need to check for errors explicitly, since
even if the rule returns NULL, the parsing might continue and lead
to long-distance failures.

https://bugs.python.org/issue42218

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lysnikolaou
bpo-42218: Correctly handle errors in left-recursive rules
2aa81a8 
Left-recursive rules need to check for errors explicitly, since
even if the rule returns NULL, the parsing might continue and lead
to long-distance failures.
@lysnikolaou
lysnikolaou requested review from gvanrossum, isidentical, pablogsal and serhiy-storchaka October 31, 2020 17:40
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 31, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Oct 31, 2020
@lysnikolaou lysnikolaou added the needs backport to 3.9 label Oct 31, 2020
pablogsal
pablogsal approved these changes Oct 31, 2020
View reviewed changes

@pablogsal pablogsal 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.

Oh, great catch, @lysnikolaou! 🎣

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Oct 31, 2020
@blurb-it
📜🤖 Added by blurb_it.
7fe6209
pablogsal
pablogsal reviewed Oct 31, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-10-31-17-50-23.bpo-42218.Dp_Z3v.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou @pablogsal
Update Misc/NEWS.d/next/Core and Builtins/2020-10-31-17-50-23.bpo-422…
53ed0f4 
…18.Dp_Z3v.rst

Co-authored-by: Pablo Galindo <[email protected]>
@pablogsal

pablogsal commented Oct 31, 2020

Copy link
Copy Markdown
Member

@lysnikolaou (For a different PR/issue) I was thinking that maybe we should add an assert(!p->error_indicator) to the beginning of each function in the pegen.c file to avoid similar situations in the future. What do you think?
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed Oct 31, 2020
View reviewed changes
Comment thread Lib/test/test_syntax.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou
Add one more test case
9f431b2
@lysnikolaou

lysnikolaou commented Oct 31, 2020

Copy link
Copy Markdown
Member Author

@lysnikolaou (For a different PR/issue) I was thinking that maybe we should add an assert(!p->error_indicator) to the beginning of each function in the pegen.c file to avoid similar situations in the future. What do you think?

That's a good idea, though every function might be a bit too much. Are we 100% sure that the assertion should apply to every single rule?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou
lysnikolaou merged commit 02cdfc9 into python:master Oct 31, 2020
@miss-islington

miss-islington commented Oct 31, 2020

Copy link
Copy Markdown
Contributor

Thanks @lysnikolaou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 31, 2020
@lysnikolaou
lysnikolaou deleted the left-recursive-error-indicator branch October 31, 2020 18:31
serhiy-storchaka
serhiy-storchaka approved these changes Oct 31, 2020
View reviewed changes
@bedevere-bot bedevere-bot added the awaiting merge label Oct 31, 2020
@miss-islington

miss-islington commented Oct 31, 2020

Copy link
Copy Markdown
Contributor

Sorry, @lysnikolaou, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 02cdfc93f82fecdb7eae97a868d4ee222b9875d9 3.9
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned lysnikolaou Oct 31, 2020
lysnikolaou added a commit to lysnikolaou/cpython that referenced this pull request Oct 31, 2020
@lysnikolaou @pablogsal
[3.9] bpo-42218: Correctly handle errors in left-recursive rules (pyt…
0817e5d 
…honGH-23065)

Left-recursive rules need to check for errors explicitly, since
even if the rule returns NULL, the parsing might continue and lead
to long-distance failures.

Co-authored-by: Pablo Galindo <[email protected]>
(cherry picked from commit 02cdfc9)
@bedevere-bot

bedevere-bot commented Oct 31, 2020

Copy link
Copy Markdown

GH-23066 is a backport of this pull request to the 3.9 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Oct 31, 2020
@pablogsal

pablogsal commented Oct 31, 2020

Copy link
Copy Markdown
Member

That's a good idea, though every function might be a bit too much. Are we 100% sure that the assertion should apply to every single rule?

I am confident it applies to most of the functions if not all. The key here is that non of these functions should be called with the error indicator set, ever. As the assertion is also only on debug mode (and is just a simple check of an integer) I think the cost is not too elevated. The most annoying part is checking what functions don't make sense to include the check.
All reactions

Sorry, something went wrong.

Uh oh!

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

lysnikolaou commented Oct 31, 2020

Copy link
Copy Markdown
Member Author

The most annoying part is checking what functions don't make sense to include the check.

Right, that's what I wanted to say before as well. Figuring out where might be a bit annoying, but I agree that it's definitely worth it.
All reactions

Sorry, something went wrong.

Uh oh!

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

pablogsal commented Oct 31, 2020

Copy link
Copy Markdown
Member

Right, that's what I wanted to say before as well. Figuring out where might be a bit annoying, but I agree that it's definitely worth it.

Ok, I will try to make a proposal PR soon :)
All reactions

Sorry, something went wrong.

Uh oh!

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

lysnikolaou commented Oct 31, 2020

Copy link
Copy Markdown
Member Author

Let me know, in case you don't find the bandwidth. I can take it up, if needed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lysnikolaou lysnikolaou mentioned this pull request Oct 31, 2020
'awaiting merge' label gets applied even if PR is closed python/bedevere#291
Closed
miss-islington pushed a commit that referenced this pull request Oct 31, 2020
@lysnikolaou
[3.9] bpo-42218: Correctly handle errors in left-recursive rules (GH-…
cfcb952 
…23065) (GH-23066)

Left-recursive rules need to check for errors explicitly, since
even if the rule returns NULL, the parsing might continue and lead
to long-distance failures.

Co-authored-by: Pablo Galindo <[email protected]>
(cherry picked from commit 02cdfc9)

Automerge-Triggered-By: GH:lysnikolaou
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@lysnikolaou @pablogsal @adorilson
bpo-42218: Correctly handle errors in left-recursive rules (pythonGH-…
b8bb7bd 
…23065)

Left-recursive rules need to check for errors explicitly, since
even if the rule returns NULL, the parsing might continue and lead
to long-distance failures.

Co-authored-by: Pablo Galindo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@isidentical isidentical Awaiting requested review from isidentical

Uh oh!

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

Assignees

@lysnikolaou lysnikolaou

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

6 participants

@lysnikolaou @pablogsal @miss-islington @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni
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.