Permit to lint to the end in case of crash on a file #4810

Pierre-Sassoulas · 2021-08-07T13:36:30Z

Type of Changes

	Type
✓	✨ New feature

Description

Permit to lint to the end in case of crash on a file and add a pre-filled issue template so it's easier to open an issue.
Will permit to keep using pylint when there is a crash on a code base. Also easier issue and probably more report.

Related to all the issue with a crash that are opened and completely prevent pylint from being run on a code base. I've done that prior to #4775

coveralls · 2021-08-07T13:41:52Z

Pull Request Test Coverage Report for Build 1125212127

36 of 38 (94.74%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 92.648%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/lint/pylinter.py	15	16	93.75%
pylint/utils/ast_walker.py	1	2	50.0%

Totals
Change from base Build 1124806342:	0.02%
Covered Lines:	13320
Relevant Lines:	14377

💛 - Coveralls

cdce8p

I'm not sure this would be a good change. IMO there are multiple issues with it:

pylint is in equal parts run locally an in CI environments. Especially larger projects only use CI. Artifacts aren't stored, thus it will be difficult to see the error if it's only written to a temp file.
I expect most devs won't appreciate an error file being written to their project directory. It's way too easy for it to be accidentally in the git repo.
A fatal crash should really be the exception. We try our best to fix them as fast as we can. So they don't stay for long.
Until the issue is fixed, the easiest solution (and probably the one most choose) would be to continue using an older version.
I don't think this will increase bug reports. pylint is used by devs after all. If they don't open a new issue it's most likely that they don't care enough about it. In that case we shouldn't care either. If the issue is bigger, someone will open a new issue.
Devs might unknowingly share confidential code.
Just having a lot of low quality issues will only increase our workload without providing much benefit. It's much easier to find a good solution if the one opening the issue is self invested and can help debug it if necessary.

Sorry for being that harsh on your idea, but at the moment I don't think we should implement it.

Pierre-Sassoulas · 2021-08-07T21:04:23Z

The created template is only part of the merge request. Right now if there is a crash on a single file in pylint, it just crash and it is literally unusable for the whole code base. This change also permit to have a fatal error message instead of a crash and pylint will work as intended on other files.

cdce8p · 2021-08-07T21:09:13Z

I must be missing something.

If you have a crash with a new pylint version, you're not going to use it in production. You'll stick with the old / working one until the issue is fixed.

Pierre-Sassoulas · 2021-08-08T07:47:14Z

Right now if there is a single crash on a single file, it's impossible to use pylint for the whole codebase. A user might want to test the new pylint on their codebase. If a single file crash, it's better if they still can benefit of the new checkers and fixes for all the other files. Or they might add something to the codebase that make pylint crash after having been on a version for months. I mean I already had the case, and the previous version also had problems. What was I supposed to do ? Check each older version until I find one that seems to work ? Should I check multiple version of astroid for each of those old pylint too ? What I did - and what user with some respect for their time will do- was disable pylint for the project. If I was not already a fan, I would have thought about not using it at all for my next project. I think having the last version and a file to exclude is preferable. In particular consider the useless suppression. Is the user better off reverting to a previous version and adding back all the disable from the false positive we removed in the latest one ? What about a crash in the pylint version included in your vscode when you just started programming ? This is a user experience nightmare.

Also once you handle the crash this way, you have to do something with the stacktrace or it won't be reported. And we might as well help the user make the report.

Regarding your point:

pylint is in equal parts run locally an in CI environments. Especially larger projects only use CI. Artifacts aren't stored, thus it will be difficult to see the error if it's only written to a temp file.

If they want to report the issue they will reproduce locally, so not a problem.

I expect most devs won't appreciate an error file being written to their project directory. It's way too easy for it to be accidentally in the git repo.

True, we should write this in pylint cache or a tmp directory.

A fatal crash should really be the exception. We try our best to fix them as fast as we can. So they don't stay for long.

We're also parsing a language, which is no easy task. Crashes are bound to happen (think of gcc) and in fact do happens. There are 100 crashes issues currently, 8 are still opened.

Until the issue is fixed, the easiest solution (and probably the one most choose) would be to continue using an older version.

See my main point, I don't think it's "easy". Also if you've been on a version for a long time and pylint start to crash reverting to an older version might create other problems.

I don't think this will increase bug reports. pylint is used by devs after all. If they don't open a new issue it's most likely that they don't care enough about it. In that case we shouldn't care either. If the issue is bigger, someone will open a new issue.

The point is not to increase bug reports, the point is to make them possible to do at all while making pylint resilient, which is the main point. Printing the traceback in the middle of the report is not that elegant or user friendly. By the way the warning fatal and astroid-error exists for this reason and were not used.

Devs might unknowingly share confidential code.

They will be creating the issue themselves, so it's on them. I hope they will read the issue before reporting it. I'm pretty sure the one that are making minimal reproducible example because they can't share the code will continue to do so.

Just having a lot of low quality issues will only increase our workload without providing much benefit. It's much easier to find a good solution if the one opening the issue is self invested and can help debug it if necessary.

True, but contradictory with the point about this not increasing bug report. Also the template is of better quality than some issue we have right now. Imo if you help the reporter by pre-redacting the issue and pointing to the issue tracker, they will spend more time doing something useful before creating the issue and the quality will increase.

Also see how plantuml is handling this problem similarly : http://www.plantuml.com/plantuml/uml/SoWkIImgAStDuKh9B4bCv798pKi1yW00

cdce8p · 2021-08-10T20:47:15Z

My initial response to your proposal wasn't ideal. It might have been because it hasn't been an issue for me personally. If I remember correctly, it always continued even after encountering an exception. However that might be related to multithreading or I didn't recall it correctly.

In case you still like to move forward with this one, I would appreciate if you could at least print the exception traceback as well. That way it wouldn't be buried in a log file that might not be accessible on a CI server. Here might be a good position for it:
https://github.com/PyCQA/pylint/blob/b8021d33a4f34b410d73fa389621e2c4ae5e22c0/pylint/utils/ast_walker.py#L80-L85

Pierre-Sassoulas · 2021-08-12T07:10:14Z

Right now the message for a crash would be:

Exception on node <ImportFrom l.2 at 0x7f47685594f0> in file '/home/pierre/pylint/a.py'
************* Module a
a.py:1:0: F0002: a.py: Fatal error while checking 'a.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/pierre/pylint/pylint-crash-2021-08-12-09.txt'. (astroid-error)

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Do you prefer to have:

Exception on node <ImportFrom l.2 at 0x7f15c89ce4c0> in file '/home/pierre/pylint/a.py'
Traceback (most recent call last):
  File "/home/pierre/pylint/venv/bin/pylint", line 11, in <module>
    load_entry_point('pylint', 'console_scripts', 'pylint')()
  File "/home/pierre/pylint/pylint/__init__.py", line 24, in run_pylint
    PylintRun(sys.argv[1:])
  File "/home/pierre/pylint/pylint/lint/run.py", line 385, in __init__
    linter.check(args)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 983, in check
    self._check_files(
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1017, in _check_files
    self._check_file(get_ast, check_astroid_module, name, filepath, modname)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1043, in _check_file
    check_astroid_module(ast_node)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1180, in check_astroid_module
    retval = self._check_astroid_module(
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1225, in _check_astroid_module
    walker.walk(ast_node)
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 77, in walk
    self.walk(child)
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 74, in walk
    callback(astroid)
  File "/home/pierre/pylint/pylint/checkers/deprecated.py", line 112, in visit_importfrom
    basename = get_import_name(node, basename)
  File "/home/pierre/pylint/pylint/checkers/utils.py", line 1552, in get_import_name
    modname = root.relative_to_absolute_name(
  File "/home/pierre/astroid/astroid/nodes/scoped_nodes.py", line 728, in relative_to_absolute_name
    raise TooManyLevelsError(level=level, name=self.name)
astroid.exceptions.TooManyLevelsError: Relative import with too many levels (1) for module 'a'
************* Module a
a.py:1:0: F0002: a.py: Fatal error while checking 'a.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/pierre/pylint/pylint-crash-2021-08-12-09.txt'. (astroid-error)

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Or:

Exception on node <ImportFrom l.2 at 0x7f15c89ce4c0> in file '/home/pierre/pylint/a.py'
************* Module a
a.py:1:0: F0002: a.py: Fatal error while checking 'a.py':
Traceback (most recent call last):
  File "/home/pierre/pylint/venv/bin/pylint", line 11, in <module>
    load_entry_point('pylint', 'console_scripts', 'pylint')()
  File "/home/pierre/pylint/pylint/__init__.py", line 24, in run_pylint
    PylintRun(sys.argv[1:])
  File "/home/pierre/pylint/pylint/lint/run.py", line 385, in __init__
    linter.check(args)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 983, in check
    self._check_files(
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1017, in _check_files
    self._check_file(get_ast, check_astroid_module, name, filepath, modname)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1043, in _check_file
    check_astroid_module(ast_node)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1180, in check_astroid_module
    retval = self._check_astroid_module(
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1225, in _check_astroid_module
    walker.walk(ast_node)
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 77, in walk
    self.walk(child)
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 74, in walk
    callback(astroid)
  File "/home/pierre/pylint/pylint/checkers/deprecated.py", line 112, in visit_importfrom
    basename = get_import_name(node, basename)
  File "/home/pierre/pylint/pylint/checkers/utils.py", line 1552, in get_import_name
    modname = root.relative_to_absolute_name(
  File "/home/pierre/astroid/astroid/nodes/scoped_nodes.py", line 728, in relative_to_absolute_name
    raise TooManyLevelsError(level=level, name=self.name)
astroid.exceptions.TooManyLevelsError: Relative import with too many levels (1) for module 'a'
Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/pierre/pylint/pylint-crash-2021-08-12-09.txt' (astroid-error)

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Also, I checked the lib for tempdir and apparently you're not guaranteed that the user will be able to open a tempfile.NamedTemporaryFile on Windows NT:

Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).

https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile

Do you have an alternative ? If not maybe it's better to have the file in the current directory where it will be visible and removed than in the cache where it will stay ad infinitum ?

Pierre-Sassoulas · 2021-08-12T07:11:21Z

By the way I appreciated your constructive criticism and the final result will be better because of it ;)

And add pre-filled issue template so it's easier to open an issue.

cdce8p · 2021-08-12T15:28:47Z

My gut feeling would be that option (1) would be a bit better, as the Traceback is directly below the Exception.
That would also be pretty easy to do by adding another print in the section I linked earlier.

Regarding the file: Can't we use PYLINTHOME for that? If not, I would lean more towards not creating it at all if I'm being completely honest. Especially if the traceback is now printed. Maybe it would be enough if we were to add a link to the issue tracker in the error message 🤔

See discussion here: pylint-dev#4810

Pierre-Sassoulas · 2021-08-12T19:45:08Z

We have a shorter stacktrace printed because we print it earlier which is "nice", well as nice as a crash can be:

Exception on node <ImportFrom l.2 at 0x7ff3a754f490> in file '/home/pierre/pylint/a.py'
Traceback (most recent call last):
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 75, in walk
    callback(astroid)
  File "/home/pierre/pylint/pylint/checkers/deprecated.py", line 112, in visit_importfrom
    basename = get_import_name(node, basename)
  File "/home/pierre/pylint/pylint/checkers/utils.py", line 1557, in get_import_name
    modname = root.relative_to_absolute_name(
  File "/home/pierre/astroid/astroid/nodes/scoped_nodes.py", line 727, in relative_to_absolute_name
    raise TooManyLevelsError(level=level, name=self.name)
astroid.exceptions.TooManyLevelsError: Relative import with too many levels (1) for module 'a'
************* Module a
a.py:1:0: F0002: a.py: Fatal error while checking 'a.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/pierre/.cache/pylint/pylint-crash-2021-08-12-21.txt'. (astroid-error)

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

cdce8p

I can't review it if you merge it so quickly 😜

cdce8p · 2021-08-12T20:08:18Z

pylint/lint/pylinter.py

+                if error is not None:
+                    msg = get_fatal_error_message(filepath, template_path)
+                    if isinstance(error, AstroidError):
+                        symbol = "astroid-error"
+                        msg = (filepath, msg)
+                    else:
+                        symbol = "fatal"
+                    self.add_message(symbol, args=msg)


Why not do everything in the except handler? That way you wouldn't need to reassign the exception.

Pierre-Sassoulas · 2021-08-12T20:27:20Z

Sorry I thought you already reviewed the full MR the first time 😅 I'm going to address your comment in another MR.

Following review see pylint-dev#4810

Following review see #4810

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Aug 7, 2021

Pierre-Sassoulas added this to the 2.10.0 milestone Aug 7, 2021

Pierre-Sassoulas force-pushed the pre-fill-template-for-crash-and-keep-linting branch from 058ebda to a734621 Compare August 7, 2021 20:29

Pierre-Sassoulas requested a review from cdce8p August 7, 2021 20:31

cdce8p reviewed Aug 7, 2021

View reviewed changes

Pierre-Sassoulas force-pushed the pre-fill-template-for-crash-and-keep-linting branch from a734621 to 7d2852c Compare August 7, 2021 21:00

Pierre-Sassoulas force-pushed the pre-fill-template-for-crash-and-keep-linting branch from 7d2852c to 7bc6109 Compare August 12, 2021 07:14

Pierre-Sassoulas added 5 commits August 12, 2021 09:39

Permit to lint to the end in case of crash on a file

d30216f

And add pre-filled issue template so it's easier to open an issue.

Add test for pylinter

61b6697

Refactor crash handling in pylinter and add tests

23933ff

Add test for crash file and add a class attribute for pylinter

67fee6c

Refactor the crash file path for easier changes

06be74f

Pierre-Sassoulas added 2 commits August 12, 2021 21:27

Change the directory for the crash issue template to pylinthome

fdabbab

Print full traceback when a crash occur

f132802

See discussion here: pylint-dev#4810

Pierre-Sassoulas force-pushed the pre-fill-template-for-crash-and-keep-linting branch from 7bc6109 to f132802 Compare August 12, 2021 19:42

Pierre-Sassoulas requested a review from cdce8p August 12, 2021 19:45

Pierre-Sassoulas merged commit 0b34b5f into pylint-dev:main Aug 12, 2021

Pierre-Sassoulas deleted the pre-fill-template-for-crash-and-keep-linting branch August 12, 2021 20:05

cdce8p reviewed Aug 12, 2021

View reviewed changes

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 12, 2021

Simplification in error handling inside Pylinter

e57faee

Following review see pylint-dev#4810

Pierre-Sassoulas mentioned this pull request Aug 12, 2021

Simplification in error handling inside Pylinter #4834

Merged

Pierre-Sassoulas added a commit that referenced this pull request Aug 12, 2021

Simplification in error handling inside Pylinter (#4834)

4f57320

Following review see #4810

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit to lint to the end in case of crash on a file #4810

Permit to lint to the end in case of crash on a file #4810

Pierre-Sassoulas commented Aug 7, 2021 •

edited

coveralls commented Aug 7, 2021 •

edited

cdce8p left a comment •

edited

Pierre-Sassoulas commented Aug 7, 2021

cdce8p commented Aug 7, 2021

Pierre-Sassoulas commented Aug 8, 2021 •

edited

cdce8p commented Aug 10, 2021

Pierre-Sassoulas commented Aug 12, 2021

Pierre-Sassoulas commented Aug 12, 2021

cdce8p commented Aug 12, 2021

Pierre-Sassoulas commented Aug 12, 2021

cdce8p left a comment

cdce8p Aug 12, 2021

Pierre-Sassoulas commented Aug 12, 2021

Permit to lint to the end in case of crash on a file #4810

Permit to lint to the end in case of crash on a file #4810

Conversation

Pierre-Sassoulas commented Aug 7, 2021 • edited

Type of Changes

Description

coveralls commented Aug 7, 2021 • edited

Pull Request Test Coverage Report for Build 1125212127

💛 - Coveralls

cdce8p left a comment • edited

Choose a reason for hiding this comment

Pierre-Sassoulas commented Aug 7, 2021

cdce8p commented Aug 7, 2021

Pierre-Sassoulas commented Aug 8, 2021 • edited

cdce8p commented Aug 10, 2021

Pierre-Sassoulas commented Aug 12, 2021

Pierre-Sassoulas commented Aug 12, 2021

cdce8p commented Aug 12, 2021

Pierre-Sassoulas commented Aug 12, 2021

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p Aug 12, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas commented Aug 12, 2021

Pierre-Sassoulas commented Aug 7, 2021 •

edited

coveralls commented Aug 7, 2021 •

edited

cdce8p left a comment •

edited

Pierre-Sassoulas commented Aug 8, 2021 •

edited