Skip to content

gh-98811: use full source location to simplify __future__ imports error checking. This also fixes an incorrect error offset. #98812

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

Merged
merged 7 commits into from
Oct 31, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Oct 28, 2022

…ts error checking. Also fixes the offset of the error in one case
@iritkatriel iritkatriel added type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.12 only security fixes labels Oct 28, 2022
@@ -60,7 +60,7 @@ def test_badfuture6(self):
def test_badfuture7(self):
with self.assertRaises(SyntaxError) as cm:
from test import badsyntax_future7
self.check_syntax_error(cm.exception, "badsyntax_future7", 3, 53)
self.check_syntax_error(cm.exception, "badsyntax_future7", 3, 54)
Copy link
Member Author

@iritkatriel iritkatriel Oct 28, 2022

Choose a reason for hiding this comment

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

53 was wrong. It caused this:

python.exe -c "import test.badsyntax_future7.py"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/iritkatriel/src/cpython/Lib/test/badsyntax_future7.py", line 3
    from __future__ import nested_scopes; import string; from __future__ import \
                                                        ^
SyntaxError: from __future__ imports must occur at the beginning of the file

which should be this:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/iritkatriel/src/cpython-654/Lib/test/badsyntax_future7.py", line 3
    from __future__ import nested_scopes; import string; from __future__ import \
                                                         ^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: from __future__ imports must occur at the beginning of the file

Co-authored-by: Alex Waygood <[email protected]>
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe convert a macro to an inline function?

Python/compile.c Outdated

#define LOCATION(LNO, END_LNO, COL, END_COL) \
((const location){(LNO), (END_LNO), (COL), (END_COL)})

static location NO_LOCATION = {-1, -1, -1, -1};

#define LOCATION_IS_GT(L1, L2) \
Copy link
Member

Choose a reason for hiding this comment

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

Make this an inline function, please.
Also, maybe compare_location(l1, l2) for more generality?

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 not a total order - how would this function work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be "lt, gt, or neither", and then you can't use it for equality. So we add an equality testing function too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make a function just for gt for now - checking both directions would be less efficient (if it's not GT we don't care about the other distinction) and we don't have a use case for it now. We can always add a location_compare that does two GT calls if we need it.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a total order - how would this function work?

Good point.

int end_lineno;
int col_offset;
int end_col_offset;
} PyCompilerSrcLocation;
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if I should put an underscore so it's _PyCompilerSrcLocation. Are we ready to make this public?

@iritkatriel iritkatriel merged commit 39448ad into python:main Oct 31, 2022
@iritkatriel iritkatriel deleted the future branch July 25, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__futures__ parsing can be simplified using complete source locations for error detection. Also the error's offset is off by one sometimes.
4 participants