Syntax error caret confused by indentation

[mystor]

BPO	25677
Nosy	@berkerpeksag, @vadmium, @serhiy-storchaka, @yan12125
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	cpython25677.patch cpython25677.patch: v2 - patch + test cpython25677.patch cpython25677.patch cpython25677.patch cpython25677.v6.patch: Strip form feeds

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/vadmium'
closed_at = <Date 2016-12-19.20:29:54.996>
created_at = <Date 2015-11-20.05:50:55.967>
labels = ['interpreter-core', 'easy', 'type-bug', '3.7']
title = 'Syntax error caret confused by indentation'
updated_at = <Date 2017-03-31.16:36:35.392>
user = 'https://bugs.python.org/mystor'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:35.392>
actor = 'dstufft'
assignee = 'martin.panter'
closed = True
closed_date = <Date 2016-12-19.20:29:54.996>
closer = 'martin.panter'
components = ['Interpreter Core']
creation = <Date 2015-11-20.05:50:55.967>
creator = 'mystor'
dependencies = []
files = ['41100', '41163', '41169', '41539', '41559', '45844']
hgrepos = []
issue_num = 25677
keywords = ['patch', 'easy']
message_count = 25.0
messages = ['254951', '254979', '254981', '254992', '255007', '255370', '255376', '255397', '255418', '255435', '256178', '257789', '257864', '257870', '279978', '280205', '280206', '280207', '280215', '280220', '280223', '282845', '282890', '282905', '283592']
nosy_count = 6.0
nosy_names = ['python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'yan12125', 'mystor']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue25677'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[mystor]

It appears that syntax errors generated by checking the AST will get confused by indentation and place the caret incorrectly.

With no indentation:
===
1 + 1 = 2
===
File "/Users/mlayzell/test.py", line 1
1 + 1 = 2
^
SyntaxError: can't assign to operator
===

With 4 spaces of indentation:
===

if True:
    1 + 1 = 2

===
File "/Users/mlayzell/test.py", line 2
1 + 1 = 2
^
SyntaxError: can't assign to operator
===

As you can see, the caret appears to be placed randomly in the middle of the second statement, apparently offset (probably by the 4 space indentation).

The above examples were run with Python3.5 on Mac OS X 10.10.4, though my somewhat-recent trunk clone exhibits the same problems.

[bitdancer]

It appears as though the line is always printed without the leading whitespace, but the caret takes the whitespace into account (try indenting the line even further). So the problem is that the caret and the printing of the source line in the error message are not consistent about how whitespace is handled.

[bitdancer]

By the way, python3.4 also shows the behavior. Python2 does not, because python2 does not print the caret. The bug presumably crept in when the caret was added. I'm marking this as easy...presumably the hardest part of fixing it is figuring out exactly how the message is constructed :)

[mystor]

This should fix the problem. It appears as though the indentation was being stripped from the program text differently depending on how the error was produced. In the case of a syntax error from the parser, the indentation was maintained until it was printed (in print_error_text, correctly handling the offset value). In contrast, errors from the AST were created with PyErr_ProgramText*, which would also strip the indentation (without mutating any offset value).

This patch causes the second code path to not strip whitespace. This shouldn't cause I problem (I think), as it seems to only be used to instantiate SyntaxErrors.

[serhiy-storchaka]

Would be nice to have a test.

[mystor]

Sorry for the delay, I finally got around to adding a test.

I'm mildly concerned about the portability of the check, but it seems to work locally. If people have suggestions about how to make the check more portable, let me know!

[bitdancer]

Two options on portability, I think: either use splitlines() on the decoded result, or add a 'universal_newlines' keyword argument to assert_python_failure and friends that would get passed to Popen. I'd favor the latter, as other users of script_helpers may find it useful. On the other hand, that may involve a whole separate discussion, so you could use splitlines here and we could open a new issue for a script_helpers enhancement :)

Also, rather than a direct re, I'd use assertRegex. That will give a more useful error message if the test fails.

[vadmium]

Another way to get universal newlines I have used in other cases is to use TextIOWrapper. It might be easier if you really need that multi-line RE search:

text = TextIOWrapper(BytesIO(stderr), "ascii").read()

[mystor]

Martin's solution seemed like the least work, so that's what I ended up using. I also switched over to assertRegex, and I agree that it produces better error messages.

I added a comment to explain the use of the ugly multiline regex, and removed some of its unnecessary parts.

[vadmium]

Michael, see the review comments if you already haven’t. There is an unused variable “p” in the last two patches.

[berkerpeksag]

With the latest patch applied, the output is

File "z.py", line 2
1 + 2 = 3
^
SyntaxError: can't assign to operator

I think the caret is still in wrong place.

I would expect the following output:

File "z.py", line 2
1 + 2 = 3
^
SyntaxError: can't assign to operator

[mystor]

Sorry for missing the review comments earlier, I didn't notice them (I've not used this bug tracker before - sorry).

I've attached an updated patch which should not have the variable problem, and also takes some of the other review comments into account.

Berker: I agree that the current positioning of the caret is undesirable, and that ideally we would report the error at the site of the operator rather than the first character of the expression. I think that changing this behavior is a different bug though.

P.S: Sorry for disappearing for a month, I don't seem to get emails when there are changes made to the bug, and thus forgot to check up on the bug...