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.

Sign up for GitHub

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

Jump to bottom

[syntax error] Fix a crash when the line and column can't be retrieved #7097

Merged
merged 1 commit into from
Jul 30, 2022
Merged

[syntax error] Fix a crash when the line and column can't be retrieved #7097

merged 1 commit into from
Jul 30, 2022

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
✓ 🐛 Bug fix

Description

Closes #3860

@Pierre-Sassoulas Pierre-Sassoulas added Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer Crash 💥 A bug that makes pylint crash labels Jun 30, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.5 milestone Jun 30, 2022
@Pierre-Sassoulas Pierre-Sassoulas changed the title [astroid syntax error] Fix a crash when the line and column can't be retrieved [syntax error] Fix a crash when the line and column can't be retrieved Jun 30, 2022
DanielNoord
DanielNoord requested changes Jun 30, 2022
View reviewed changes
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

One of the issues with SyntaxErrors is that their messages changes across versions and implementations. They don't really lend themselves well for our functional test framework.

Perhaps an unittest would work better here?

pylint/lint/pylinter.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Jun 30, 2022
@github-actions

This comment has been minimized.

Sign in to view
@coveralls
Copy link

coveralls commented Jun 30, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2765124596

  • 4 of 5 (80.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 95.255%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/lint/pylinter.py 1 2 50.0%
Totals Coverage Status
Change from base Build 2760747244: 0.005%
Covered Lines: 16822
Relevant Lines: 17660
💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas removed the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Jul 9, 2022
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the fix-crash-utf9 branch 2 times, most recently from dede3cc to 319a5c9 Compare July 9, 2022 19:38
@Pierre-Sassoulas
Copy link
Member Author

I'm back with a new machine with python 3.10 / ubuntu 22.04 😄 everything is nicer, there's progress bars in pip !

@github-actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas Pierre-Sassoulas added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Jul 10, 2022
@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas Do we understand the primer results?

@Pierre-Sassoulas
Copy link
Member Author

No idea, I think it's a primer cache bug ?

@DanielNoord
Copy link
Collaborator

It's weird as you don't have a main merge commit which can sometimes cause issues. Could you re-push and see if it fixes itself?

@Pierre-Sassoulas
Copy link
Member Author

It seems I was already based on b9419a2 which is the latest commit from main so I don't know what I can do.

@DanielNoord
Copy link
Collaborator

See #7156 (comment).

DanielNoord
DanielNoord requested changes Jul 10, 2022
View reviewed changes
pylint/lint/pylinter.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

Sign in to view
@DanielNoord DanielNoord mentioned this pull request Jul 11, 2022
Merged
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Jul 13, 2022
@github-actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.14.5 milestone Jul 17, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.0 milestone Jul 17, 2022
@Pierre-Sassoulas
Copy link
Member Author

We can't reproduce the syntax error with functional tests, will need to create a unit test with a mock, but this can wait 2.15.0

@github-actions

This comment has been minimized.

Sign in to view
DanielNoord
DanielNoord requested changes Jul 18, 2022
View reviewed changes
tests/test_self.py Outdated
@@ -27,6 +27,7 @@
from unittest.mock import patch

import pytest
from astroid import PY310_PLUS
Copy link
Collaborator

Choose a reason for hiding this comment

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

? Why from astroid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I imported the wrong one 😄

tests/test_self.py Outdated Show resolved Hide resolved
tests/test_self.py Outdated
@pytest.mark.skipif(
PY310_PLUS, reason="Not a syntax error for python 3.10 or superior"
)
def test_astroid_syntax_error(self, tmp_path) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def test_astroid_syntax_error(self, tmp_path) -> None:
def test_astroid_syntax_error(self, tmp_path: TempPathFixture?) -> None:

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Pierre-Sassoulas this was a question. Sorry should have been clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hurgh, no that's my bad. It's a pathlib.Path.

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas
Copy link
Member Author

This is the same error than we have on other branches... I wonder if we should deactivate the tests for pypy. It's likely we'll never think of reactivating it if we do that.

@github-actions

This comment has been minimized.

Sign in to view
DanielNoord
DanielNoord reviewed Jul 21, 2022
View reviewed changes
pylint/checkers/imports.py Outdated Show resolved Hide resolved
pylint/lint/pylinter.py Outdated
@@ -918,7 +918,8 @@ def get_ast(
"syntax-error",
line=getattr(ex.error, "lineno", 0),
col_offset=getattr(ex.error, "offset", None),
args=str(ex.error),
args=f"Parsing Python code failed:\n{ex.error}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems unnecessary? The message is already called syntax-error?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's other syntax error for example when there's a syntax error in something we want to import. I think it does not hurt to be explicit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind too much, but I don't really understand what it adds. Seems like it just adds unnecessary verbosity. In IDEs that might actually reduce usability.

But if you think this is better please go ahead!

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 removed the line break that could be problematic in IDE.

tests/functional/s/syntax/syntax_error.rc Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 1af2c02

@Pierre-Sassoulas Pierre-Sassoulas merged commit 36a6723 into pylint-dev:main Jul 30, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the fix-crash-utf9 branch July 30, 2022 08:46
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Sep 6, 2022
This was referenced Oct 21, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielNoord DanielNoord DanielNoord approved these changes

Assignees
No one assigned
Labels
Crash 💥 A bug that makes pylint crash
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

Traceback on unknown encoding
3 participants
@Pierre-Sassoulas @coveralls @DanielNoord