-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Fix SyntaxError crash line to show file, line, and column #14899
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
base: main
Are you sure you want to change the base?
Changes from all commits
0c88848
427a995
4a35f0e
8ca36dd
384afbf
58b358d
88f44c3
3f240dc
e4c4ac7
c1d5871
fe93273
6b40d16
4d8030b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| :class:`SyntaxError` crashes now contain an extra `FILE:LINE:COLUMN ERROR` line, as in other traceback lines. This format is widely supported by most editors/IDEs. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -532,9 +532,13 @@ def importtestmodule( | |
| consider_namespace_packages=config.getini("consider_namespace_packages"), | ||
| ) | ||
| except SyntaxError as e: | ||
| raise nodes.Collector.CollectError( | ||
| ExceptionInfo.from_current().getrepr(style="short") | ||
| ) from e | ||
| excinfo = ExceptionInfo.from_current() | ||
| repr_ = excinfo.getrepr(style="short") | ||
| reprcrash = excinfo._getreprcrash() | ||
| msg = str(repr_) | ||
| if reprcrash is not None and reprcrash.column is not None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. str(reprcrash) is safe with column=None, but the guard isn't about safety. Dropping the guard here would append a near-duplicate location line in the no-column case. The column is the signal that we have the SyntaxError's own precise location worth appending, without it we keep the existing traceback line. What do you think?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make the intent more explicit then, preferably without just slapping a comment there... 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course, here's my code proposal, wdyt? msg = str(repr_)
syntax_error_has_precise_location = reprcrash is not None and reprcrash.column is not None
if syntax_error_has_precise_location:
msg += "\n" + str(reprcrash) |
||
| msg += "\n" + str(reprcrash) | ||
| raise nodes.Collector.CollectError(msg) from e | ||
| except ImportPathMismatchError as e: | ||
| raise nodes.Collector.CollectError( | ||
| "import file mismatch:\n" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -339,6 +339,70 @@ def f(): | |
| f() | ||
| assert excinfo._getreprcrash() is None | ||
|
|
||
| def test_getreprcrash_syntax_error(self): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of the tests are constructing a |
||
| with pytest.raises(SyntaxError) as excinfo: | ||
| raise SyntaxError("bad syntax", ("file.py", 1, 5, "def foo(:", 1, 6)) | ||
| reprcrash = excinfo._getreprcrash() | ||
| assert reprcrash is not None | ||
| assert reprcrash.path == "file.py" | ||
| assert reprcrash.lineno == 1 | ||
| assert reprcrash.column == 5 | ||
| assert reprcrash.message == "SyntaxError: bad syntax" | ||
| assert str(reprcrash) == "file.py:1:5: SyntaxError: bad syntax" | ||
|
|
||
| def test_getreprcrash_syntax_error_without_offset(self): | ||
| def f(): | ||
| raise SyntaxError("no location") | ||
|
|
||
| with pytest.raises(SyntaxError) as excinfo: | ||
| f() | ||
| reprcrash = excinfo._getreprcrash() | ||
| assert reprcrash is not None | ||
| assert reprcrash.column is None | ||
| assert reprcrash.message == "SyntaxError: no location" | ||
|
|
||
| def test_getreprcrash_syntax_error_without_filename(self): | ||
| def f(): | ||
| raise SyntaxError("bad syntax", (None, 1, 5, "def foo(:", 1, 6)) | ||
|
|
||
| with pytest.raises(SyntaxError) as excinfo: | ||
| f() | ||
| reprcrash = excinfo._getreprcrash() | ||
| assert reprcrash is not None | ||
| co = _pytest._code.Code.from_function(f) | ||
| assert reprcrash.path == str(co.path) | ||
| assert reprcrash.lineno == co.firstlineno + 1 + 1 | ||
| assert reprcrash.column is None | ||
| assert reprcrash.message.endswith("SyntaxError: bad syntax") | ||
|
|
||
| def test_getreprcrash_indentation_error(self): | ||
| with pytest.raises(IndentationError) as excinfo: | ||
| raise IndentationError( | ||
| "unexpected indent", ("file.py", 3, 5, " foo", 3, 6) | ||
| ) | ||
| reprcrash = excinfo._getreprcrash() | ||
| assert reprcrash is not None | ||
| assert reprcrash.path == "file.py" | ||
| assert reprcrash.lineno == 3 | ||
| assert reprcrash.column == 5 | ||
| assert reprcrash.message == "IndentationError: unexpected indent" | ||
| assert str(reprcrash) == "file.py:3:5: IndentationError: unexpected indent" | ||
|
|
||
| def test_getreprcrash_syntax_error_without_lineno(self): | ||
| def f(): | ||
| raise SyntaxError( | ||
| "bad syntax", ("file.py", None, 5, "def foo(:", None, None) | ||
| ) | ||
|
|
||
| with pytest.raises(SyntaxError) as excinfo: | ||
| f() | ||
| reprcrash = excinfo._getreprcrash() | ||
| assert reprcrash is not None | ||
| assert reprcrash.column is None | ||
| co = _pytest._code.Code.from_function(f) | ||
| assert reprcrash.path == str(co.path) | ||
| assert reprcrash.lineno == co.firstlineno + 1 + 1 | ||
|
|
||
|
|
||
| def test_excinfo_exconly(): | ||
| with pytest.raises(ValueError) as excinfo: | ||
|
|
@@ -1116,6 +1180,82 @@ def entry(): | |
| assert repr.reprcrash.message == "ValueError" | ||
| assert str(repr.reprcrash).endswith("mod.py:3: ValueError") | ||
|
|
||
| def test_repr_excinfo_reprcrash_syntax_error(self, importasmod) -> None: | ||
| mod = importasmod( | ||
| """ | ||
| def entry(): | ||
| raise SyntaxError("bad syntax", ("file.py", 1, 5, "def foo(:", 1, 6)) | ||
| """ | ||
| ) | ||
| with pytest.raises(SyntaxError) as excinfo: | ||
| mod.entry() | ||
| repr = excinfo.getrepr() | ||
| assert repr.reprcrash is not None | ||
| assert repr.reprcrash.path == "file.py" | ||
| assert repr.reprcrash.lineno == 1 | ||
| assert repr.reprcrash.column == 5 | ||
| assert repr.reprcrash.message == "SyntaxError: bad syntax" | ||
| assert str(repr.reprcrash) == "file.py:1:5: SyntaxError: bad syntax" | ||
|
|
||
| def test_syntax_error_default_tb_long(self, pytester: Pytester) -> None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we raising manual SyntaxErrors here, instead of just writing code with syntax errors? |
||
| pytester.makepyfile( | ||
| """ | ||
| def entry(): | ||
| raise SyntaxError("bad syntax", ("file.py", 1, 5, "def foo(:", 1, 6)) | ||
| def test_x(): | ||
| entry() | ||
| """ | ||
| ) | ||
| result = pytester.runpytest("--tb=long") | ||
| result.stdout.fnmatch_lines(["file.py:1:5: SyntaxError"]) | ||
|
|
||
| def test_syntax_error_default_tb_short(self, pytester: Pytester) -> None: | ||
| pytester.makepyfile( | ||
| """ | ||
| def entry(): | ||
| raise SyntaxError("bad syntax", ("file.py", 1, 5, "def foo(:", 1, 6)) | ||
| def test_x(): | ||
| entry() | ||
| """ | ||
| ) | ||
| result = pytester.runpytest("--tb=short") | ||
| result.stdout.fnmatch_lines(["file.py:1:5: in entry"]) | ||
| result.stdout.fnmatch_lines(["*SyntaxError: bad syntax*"]) | ||
|
|
||
| def test_syntax_error_tb_line(self, pytester: Pytester) -> None: | ||
| pytester.makepyfile( | ||
| """ | ||
| def entry(): | ||
| raise SyntaxError("bad syntax", ("file.py", 1, 5, "def foo(:", 1, 6)) | ||
| def test_x(): | ||
| entry() | ||
| """ | ||
| ) | ||
| result = pytester.runpytest("--tb=line") | ||
| result.stdout.fnmatch_lines(["file.py:1:5: SyntaxError: bad syntax"]) | ||
|
|
||
| def test_syntax_error_no_offset_fallback(self, pytester: Pytester) -> None: | ||
| pytester.makepyfile( | ||
| """ | ||
| def entry(): | ||
| raise SyntaxError("no location") | ||
| def test_x(): | ||
| entry() | ||
| """ | ||
| ) | ||
| result = pytester.runpytest("--tb=long") | ||
| result.stdout.fnmatch_lines(["*SyntaxError: no location*"]) | ||
|
|
||
| def test_syntax_error_collection(self, pytester: Pytester) -> None: | ||
| pytester.makepyfile("def broken(:\n pass\n") | ||
| result = pytester.runpytest() | ||
| result.stdout.fnmatch_lines(["*.py:1:*: SyntaxError*"]) | ||
|
|
||
| def test_indentation_error_collection(self, pytester: Pytester) -> None: | ||
| pytester.makepyfile("def f():\n x = 1\n y = 2\n") | ||
| result = pytester.runpytest() | ||
| result.stdout.fnmatch_lines(["*.py:3:*: IndentationError*"]) | ||
|
|
||
| def test_repr_traceback_recursion(self, importasmod): | ||
| mod = importasmod( | ||
| """ | ||
|
|
||
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.
Move
path = self._makepath(entry_path)from above to here:Uh oh!
There was an error while loading. Please reload this page.
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.
Hmmm this is good. But when I think about it, is this a bandaid? Because if _syntax_error_location is ever relaxed to return loc even when exc.filename is falsy, then filename or path would evaluate the undefined path, and immediately raise a NameError? Yes mine is also a bandaid I believe, the
or pathis a dead code, should be deleted and be like this:self._makepath(filename)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.
Not sure I follow... if
_syntax_error_locationchanges, mypy should catch any discrepancies.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.
Ok fair enough.
What do you think about this?
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.
Let's leave it for now.