Skip to content

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

Merged
Pierre-Sassoulas merged 1 commit into
pylint-dev:mainfrom
Pierre-Sassoulas:fix-crash-utf9
Jul 30, 2022
Merged

[syntax error] Fix a crash when the line and column can't be retrieved#7097
Pierre-Sassoulas merged 1 commit into
pylint-dev:mainfrom
Pierre-Sassoulas:fix-crash-utf9

Conversation

@Pierre-Sassoulas
Copy link
Copy Markdown
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Jun 30, 2022

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
Copy Markdown
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?

Comment thread pylint/lint/pylinter.py Outdated
@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
Copy Markdown

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
Copy Markdown
Member Author

Pierre-Sassoulas commented Jul 9, 2022

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
Copy Markdown
Collaborator

DanielNoord commented Jul 10, 2022

@Pierre-Sassoulas Do we understand the primer results?

@Pierre-Sassoulas
Copy link
Copy Markdown
Member Author

Pierre-Sassoulas commented Jul 10, 2022

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

@DanielNoord
Copy link
Copy Markdown
Collaborator

DanielNoord commented Jul 10, 2022

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
Copy Markdown
Member Author

Pierre-Sassoulas commented Jul 10, 2022

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
Copy Markdown
Collaborator

DanielNoord commented Jul 10, 2022

See #7156 (comment).

DanielNoord
DanielNoord requested changes Jul 10, 2022
View reviewed changes
Comment thread pylint/lint/pylinter.py Outdated
Pierre-Sassoulas
Pierre-Sassoulas commented Jul 10, 2022
View reviewed changes
Comment thread pylint/lint/pylinter.py Outdated
@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
Copy Markdown
Member Author

Pierre-Sassoulas commented Jul 17, 2022

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
Comment thread tests/test_self.py Outdated
from unittest.mock import patch

import pytest
from astroid import PY310_PLUS
Copy link
Copy Markdown
Collaborator

@DanielNoord DanielNoord Jul 18, 2022

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
Copy Markdown
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Jul 18, 2022

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 😄

Comment thread tests/test_self.py Outdated
Comment thread 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
Copy Markdown
Collaborator

@DanielNoord DanielNoord Jul 18, 2022

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
Copy Markdown
Collaborator

@DanielNoord DanielNoord Jul 18, 2022

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
Copy Markdown
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Jul 19, 2022

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
Copy Markdown
Member Author

Pierre-Sassoulas commented Jul 21, 2022

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
Comment thread pylint/checkers/imports.py Outdated
Comment thread pylint/lint/pylinter.py Outdated
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
Copy Markdown
Collaborator

@DanielNoord DanielNoord Jul 21, 2022

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
Copy Markdown
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Jul 22, 2022

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
Copy Markdown
Collaborator

@DanielNoord DanielNoord Jul 22, 2022

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
Copy Markdown
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Jul 30, 2022

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.

Comment thread tests/functional/s/syntax/syntax_error.rc Outdated
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 30, 2022

🤖 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