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.

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

Add an exception for IndexError inside uninferable_final_decorator #6532

Merged

Conversation

mbyrnepr2
Copy link
Collaborator

@mbyrnepr2 mbyrnepr2 commented May 6, 2022

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

Refs #6531

@coveralls
Copy link

coveralls commented May 6, 2022

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 Coverage Status
Change from base Build 2283754721: 0.0%
Covered Lines: 15985
Relevant Lines: 16769

πŸ’› - Coveralls

@mbyrnepr2 mbyrnepr2 force-pushed the crash_uninferable_final_decorator branch from 1b6ba12 to 2b33cf5 Compare May 6, 2022 23:22
@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review May 6, 2022 23:22
@mbyrnepr2 mbyrnepr2 marked this pull request as draft May 6, 2022 23:28
@mbyrnepr2 mbyrnepr2 force-pushed the crash_uninferable_final_decorator branch from 2b33cf5 to e7053b7 Compare May 6, 2022 23:52
@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review May 6, 2022 23:54
@Pierre-Sassoulas Pierre-Sassoulas added Bug πŸͺ² Crash πŸ’₯ A bug that makes pylint crash labels May 7, 2022
ChangeLog Outdated Show resolved Hide resolved
doc/whatsnew/2.14.rst Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.14.0, 2.13.9 May 7, 2022
@Pierre-Sassoulas 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
@@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@mbyrnepr2 mbyrnepr2 marked this pull request as draft May 7, 2022 08:34
@mbyrnepr2 mbyrnepr2 force-pushed the crash_uninferable_final_decorator branch from f644326 to 38cbeeb Compare May 7, 2022 08:40
@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review May 7, 2022 09:03
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@mbyrnepr2 mbyrnepr2 May 7, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
mbyrnepr2 and others added 2 commits May 7, 2022 11:49
…r.py

Co-authored-by: DaniΓ«l van Noord <13665637+DanielNoord@users.noreply.github.com>
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The new code is a lot easier to understand, that's great πŸ‘Œ

@mbyrnepr2
Copy link
Collaborator Author

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

Copy link
Member

@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.

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!

@Pierre-Sassoulas Pierre-Sassoulas merged commit ccd87b4 into PyCQA:main May 7, 2022
33 checks passed
@mbyrnepr2 mbyrnepr2 deleted the crash_uninferable_final_decorator branch May 7, 2022 19:29
@mbyrnepr2
Copy link
Collaborator Author

Thank you @DanielNoord & @Pierre-Sassoulas for your help πŸ‘

@Pierre-Sassoulas 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
Pierre-Sassoulas added a commit that referenced this pull request May 9, 2022
#6532)


Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Co-authored-by: DaniΓ«l van Noord <13665637+DanielNoord@users.noreply.github.com>
Pierre-Sassoulas added a commit that referenced this pull request May 13, 2022
#6532)


Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Co-authored-by: DaniΓ«l van Noord <13665637+DanielNoord@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants