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

fix syntax error reporting from stdin (#357) #716

Merged
merged 1 commit into from Jul 19, 2022

Conversation

stevenkaras
Copy link
Contributor

In b73a3d1, there was an assumption that text is None only if there was an
encoding error with the file. However this was the case for all pythons before
3.9 when reading code from stdin.

This takes care to correctly report as much context as possible, so errors
aren't silently dropped with the unhelpful "problem decoding source" message.

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

could you add a test for this?

pyflakes/test/test_api.py Outdated Show resolved Hide resolved
pyflakes/test/test_api.py Outdated Show resolved Hide resolved
@stevenkaras
Copy link
Contributor Author

Added explicit tests of the new behavior and the stdin example as described in the issue.

Comment on lines 267 to 268
reporter.syntaxError('<stdin>', 'a problem', 0, 0,
None)
Copy link
Member

Choose a reason for hiding this comment

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

this should generate an actual syntax error and validate it so we can know when to delete this behaviour (3.9+)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is testing the reporting of a syntax error. Specifically, the behavior of what to do when the offsets are nonsensical (which they are for actual decoding errors in all versions of python including 3.9+). I'll attempt to clarify this in the test comment.

Copy link
Member

Choose a reason for hiding this comment

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

ah I see -- also I think the None fits on the line above within the character limit too if you want to move that as well! ideally we should set up some auto-formatting

Copy link
Member

Choose a reason for hiding this comment

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

but that's probably for another PR!

Comment on lines +667 to +672
def test_stdinReportsErrors(self):
"""
L{check} reports syntax errors from stdin
"""
source = "max(1 for i in range(10), key=lambda x: x+1)\n"
err = io.StringIO()
count = withStderrTo(err, check, source, "<stdin>")
self.assertEqual(count, 1)
errlines = err.getvalue().split("\n")[:-1]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this actually tests the behaviour -- and we already have a SyntaxError test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior is triggered when ast.parse throws a SyntaxError from code with a filepath that doesn't exist (stdin).

I'll see if I can trigger this with a different syntax error that has more consistent messaging to avoid the rats nest of expected messages by version. The offset behavior is a bit concerning though. It looks like pypy has special handling that is objectively wrong in certain situations (such as this one).

Copy link
Member

Choose a reason for hiding this comment

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

pypy is usually pretty attentive to fixes for syntax error offsets (with a little help from me they inspired the improvements to cpython 3.10's error messages)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant pyflakes handling of pypy's offsets as here:

pyflakes/pyflakes/api.py

Lines 46 to 56 in 44ef321

if checker.PYPY:
if text is None:
lines = codeString.splitlines()
if len(lines) >= lineno:
text = lines[lineno - 1]
if isinstance(text, bytes):
try:
text = text.decode('ascii')
except UnicodeDecodeError:
text = None
offset -= 1

It's probably something that used to be an issue with pypy and was fixed since on the other end. I tested with pypy3.7-7.3.9, but tracking down which version of pypy changed offsets in syntax errors is out of scope for here.

Copy link
Member

Choose a reason for hiding this comment

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

oh yeah we should totally kill that -- forgot pyflakes had special cases (probably from like 2015-era pypy)

In b73a3d1, there was an assumption that text is None only if there was an
encoding error with the file. However this was the case for all pythons before
3.9 when reading code from stdin.

This takes care to correctly report as much context as possible, so errors
aren't silently dropped with the unhelpful "problem decoding source" message.
@asottile asottile merged commit d875b02 into PyCQA:master Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants