Skip to content
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

gh-123969: refactor _PyErr_RaiseSyntaxError and _PyErr_EmitSyntaxWarning out of compiler #123972

Merged
merged 8 commits into from
Sep 16, 2024

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Sep 11, 2024

These were local to the compiler, but after the splitting it would be good to be able to use them without needing to pass in a compiler struct (which is only used for the filename).

Fixes #123969.

Python/errors.c Outdated Show resolved Hide resolved
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Python/errors.c Outdated Show resolved Hide resolved
Python/errors.c Outdated Show resolved Hide resolved
Python/errors.c Outdated Show resolved Hide resolved
Python/errors.c Outdated Show resolved Hide resolved
Python/errors.c Outdated Show resolved Hide resolved
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

This change by itself feels unnecessary, but I'm guessing you'll use these functions in more places in the future. Would it make sense to change some of those other places in this PR so you can know whether the API will work for all cases.

Python/errors.c Outdated Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Python/errors.c Outdated
text = Py_NewRef(Py_None);
}
PyObject *args = Py_BuildValue("O(OiiOii)", msg, filename,
lineno, col_offset + 1, text,
Copy link
Member

Choose a reason for hiding this comment

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

You still do the +1 here, so at the end, it's +2 which is wrong, no?

@iritkatriel
Copy link
Member Author

This change by itself feels unnecessary, but I'm guessing you'll use these functions in more places in the future. Would it make sense to change some of those other places in this PR so you can know whether the API will work for all cases.

I want to use it in order to raise the assert(tuple) warning from ast_opt (and move the assert optimization there). It felt like putting it in the PR would lump together two different things, but I could do it that way.

@vstinner
Copy link
Member

I prefer a separated PR just for these two PyErr functions.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

Is the +1 on col_offset needed because compile.c counts starting at 0 and the displayed error starts counting at 1?

@iritkatriel
Copy link
Member Author

LGTM.

Is the +1 on col_offset needed because compile.c counts starting at 0 and the displayed error starts counting at 1?

SyntaxError is defined to start at 1: https://docs.python.org/3/library/exceptions.html#SyntaxError

@iritkatriel
Copy link
Member Author

LGTM.
Is the +1 on col_offset needed because compile.c counts starting at 0 and the displayed error starts counting at 1?

SyntaxError is defined to start at 1: https://docs.python.org/3/library/exceptions.html#SyntaxError

The location comes from the AST, where it starts at 0.

@iritkatriel iritkatriel merged commit aba42c0 into python:main Sep 16, 2024
35 checks passed
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
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.

Add _PyErr_RaiseSyntaxError and _PyErr_EmitSyntaxWarning
5 participants