Add an exception for `IndexError` inside `uninferable_final_decorator` #6532

mbyrnepr2 · 2022-05-06T22:34:53Z

Type of Changes

	Type
✓	🐛 Bug fix
	✨ New feature
	🔨 Refactoring
	📜 Docs

Description

Refs #6531

coveralls · 2022-05-06T23:11:15Z

Pull Request Test Coverage Report for Build 2286792493

7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.325%

Totals
Change from base Build 2283754721:	0.0%
Covered Lines:	15985
Relevant Lines:	16769

💛 - Coveralls

…s` method.

ChangeLog

doc/whatsnew/2.14.rst

Pierre-Sassoulas · 2022-05-07T06:45:11Z

pylint/checkers/utils.py

@@ -850,7 +850,7 @@ def uninferable_final_decorators(
        if isinstance(decorator, nodes.Attribute):
            try:
                import_node = decorator.expr.lookup(decorator.expr.name)[1][0]
-            except AttributeError:
+            except (AttributeError, IndexError):


I'm wondering if we should distinguish the two errors and try to handle the case where we can't do [1][0]. I guess Daniel will have an opinion :)

I believe the logic is simpler now with 38cbeeb & it more closely matches the nodes.Name case beneath it in the same function.

No, need to re-add the try-except since the primer test fails without it.

DanielNoord · 2022-05-07T09:06:15Z

pylint/checkers/utils.py

@@ -849,9 +849,12 @@ def uninferable_final_decorators(
    for decorator in getattr(node, "nodes", []):
        if isinstance(decorator, nodes.Attribute):
            try:
-                import_node = decorator.expr.lookup(decorator.expr.name)[1][0]
+                _, import_nodes = decorator.expr.lookup(decorator.expr.name)
            except AttributeError:


Looking at the failed test this should be fixable with an isinstance on decorator.expr. I think we assume expr to a LocalsDictNodeNG while apparently in those regression tests it is a nodes.Attribute. Should we try and fix that while we're at it?
In general I prefer explicitness over broader try...excepts.

decorator.expr has no attribute loookup (it is an instance of nodes.Attribute). I could try adding a hasattr instead of an isinstance if I understood you.

But normally it does right? My hunch is that this code works well for @decorator.func() but not for @decorator.module.func(). Apparently sometimes expr sometimes does have lookup so instead of using hasattr can't we reliable determine what instance expr has and whether that instance is supposed to have lookup?

pylint/checkers/utils.py

tests/functional/r/regression/regression_6531_crash_index_error.py

…r.py Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Pierre-Sassoulas

The new code is a lot easier to understand, that's great 👌

mbyrnepr2 · 2022-05-07T12:28:32Z

Thanks Pierre. I do want to address the suggestion from @DanielNoord later tonight. If we can wait that would be good. Otherwise I can follow up in another MR

for more information, see https://pre-commit.ci

DanielNoord

I'm on mobile so I can't really check if the coverage is correct. But the code LGTM.

Thanks @mbyrnepr2 for the extra effort you put in! I think this actually improves the status quo as well!

mbyrnepr2 · 2022-05-07T19:30:16Z

Thank you @DanielNoord & @Pierre-Sassoulas for your help 👍

#6532) Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com> Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

mbyrnepr2 mentioned this pull request May 6, 2022

False Positive W0621: Redefining name %r from outer scope when using pytest fixtures #6531

Open

mbyrnepr2 marked this pull request as draft May 6, 2022 22:44

DanielNoord self-requested a review May 6, 2022 22:47

mbyrnepr2 force-pushed the crash_uninferable_final_decorator branch from 3f9553b to 1b6ba12 Compare May 6, 2022 23:02

mbyrnepr2 force-pushed the crash_uninferable_final_decorator branch from 1b6ba12 to 2b33cf5 Compare May 6, 2022 23:22

mbyrnepr2 marked this pull request as ready for review May 6, 2022 23:22

mbyrnepr2 marked this pull request as draft May 6, 2022 23:28

Add an exception for IndexError inside `uninferable_final_decorator…

e7053b7

…s` method.

mbyrnepr2 force-pushed the crash_uninferable_final_decorator branch from 2b33cf5 to e7053b7 Compare May 6, 2022 23:52

mbyrnepr2 marked this pull request as ready for review May 6, 2022 23:54

Pierre-Sassoulas added Bug 🪲 Crash 💥 A bug that makes pylint crash labels May 7, 2022

Pierre-Sassoulas reviewed May 7, 2022

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

doc/whatsnew/2.14.rst Outdated Show resolved Hide resolved

Pierre-Sassoulas modified the milestones: 2.14.0, 2.13.9 May 7, 2022

Pierre-Sassoulas added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label May 7, 2022

Pierre-Sassoulas added 2 commits May 7, 2022 08:36

Apply suggestions from code review

714c1d2

Move changelog to 2.13.9

8357070

Pierre-Sassoulas reviewed May 7, 2022

View reviewed changes

mbyrnepr2 marked this pull request as draft May 7, 2022 08:34

Simplify the logic - Handle IndexError & AttributeError situations.

38cbeeb

mbyrnepr2 force-pushed the crash_uninferable_final_decorator branch from f644326 to 38cbeeb Compare May 7, 2022 08:40

Re-add the try-except to catch an AttributeError.

071251d

mbyrnepr2 marked this pull request as ready for review May 7, 2022 09:03

DanielNoord requested changes May 7, 2022

View reviewed changes

mbyrnepr2 and others added 2 commits May 7, 2022 11:49

Update tests/functional/r/regression/regression_6531_crash_index_erro…

64a5b75

…r.py Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Remove try-except. Replace it with a hasattr check.

42bcfc1

Pierre-Sassoulas approved these changes May 7, 2022

View reviewed changes

mbyrnepr2 and others added 9 commits May 7, 2022 16:41

Narrow the scope for error-handling

d663879

[pre-commit.ci] auto fixes from pre-commit.com hooks

6253936

for more information, see https://pre-commit.ci

rm Tuple

7dc1beb

typo

1e35c35

Fix typing

547ab5e

[pre-commit.ci] auto fixes from pre-commit.com hooks

e2350df

for more information, see https://pre-commit.ci

import typing.Union

8aedda2

Fix the typing syntax.

83f696f

Only consider lookup when the module is named typing

51a3df8

DanielNoord approved these changes May 7, 2022

View reviewed changes

Pierre-Sassoulas merged commit ccd87b4 into pylint-dev:main May 7, 2022

mbyrnepr2 deleted the crash_uninferable_final_decorator branch May 7, 2022 19:29

Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels May 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an exception for `IndexError` inside `uninferable_final_decorator` #6532

Add an exception for `IndexError` inside `uninferable_final_decorator` #6532

mbyrnepr2 commented May 6, 2022 •

edited by Pierre-Sassoulas

Loading

coveralls commented May 6, 2022 •

edited

Loading

Pierre-Sassoulas May 7, 2022

mbyrnepr2 May 7, 2022

mbyrnepr2 May 7, 2022

DanielNoord May 7, 2022

mbyrnepr2 May 7, 2022 •

edited

Loading

DanielNoord May 7, 2022 •

edited

Loading

Pierre-Sassoulas left a comment

mbyrnepr2 commented May 7, 2022

DanielNoord left a comment

mbyrnepr2 commented May 7, 2022

Add an exception for IndexError inside uninferable_final_decorator #6532

Add an exception for IndexError inside uninferable_final_decorator #6532

Conversation

mbyrnepr2 commented May 6, 2022 • edited by Pierre-Sassoulas Loading

Type of Changes

Description

coveralls commented May 6, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2286792493

💛 - Coveralls

Pierre-Sassoulas May 7, 2022

Choose a reason for hiding this comment

mbyrnepr2 May 7, 2022

Choose a reason for hiding this comment

mbyrnepr2 May 7, 2022

Choose a reason for hiding this comment

DanielNoord May 7, 2022

Choose a reason for hiding this comment

mbyrnepr2 May 7, 2022 • edited Loading

Choose a reason for hiding this comment

DanielNoord May 7, 2022 • edited Loading

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

mbyrnepr2 commented May 7, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

mbyrnepr2 commented May 7, 2022

Add an exception for `IndexError` inside `uninferable_final_decorator` #6532

Add an exception for `IndexError` inside `uninferable_final_decorator` #6532

mbyrnepr2 commented May 6, 2022 •

edited by Pierre-Sassoulas

Loading

coveralls commented May 6, 2022 •

edited

Loading

mbyrnepr2 May 7, 2022 •

edited

Loading

DanielNoord May 7, 2022 •

edited

Loading