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

Fix crash when using enumerate with start and a class attribute #7824

Merged
merged 4 commits into from Nov 24, 2022

Conversation

clavedeluna
Copy link
Collaborator

@clavedeluna clavedeluna commented Nov 22, 2022

Type of Changes

Type
🐛 Bug fix

Description

fixes

    start_val = node.value
AttributeError: 'Attribute' object has no attribute 'value'

Closes #7821

@@ -2303,7 +2303,7 @@ def _enumerate_with_start(
def _get_start_value(self, node: nodes.NodeNG) -> tuple[int | None, Confidence]:
confidence = HIGH

if isinstance(node, (nodes.Name, nodes.Call)):
if isinstance(node, (nodes.Name, nodes.Call, nodes.Attribute)):
inferred = utils.safe_infer(node)
start_val = inferred.value if inferred else None
confidence = INFERENCE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in this entire func I'm negotiating between knowing what type of node will be and crashing... would you prefer I make the else the "catch all" instead and ensure we never crash regardless of not knowing what the node could be?

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 really understand your point and I'm not sure I missed this in an earlier review but the broad else doesn't make a lot of sense.
mypy only allows it because astroid isn't typed and therefore every astroid object is Any as far as mypy can see. Doing node.value is not type safe sadly so we should use instance checks.

Copy link
Member

Choose a reason for hiding this comment

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

Oftentimes if a node is not something we expect we return early. I.e. we use the pattern we talked about in #7787 (comment). It's a way to make sure we have false negatives instead of false positive or crashes. To be clearer listing the node we expect to treat is safer than thinking we know what we'll have to treat. It's really hard to think of all possible python construct around a particular situation. Might be a principle that should be documented / checked in review btw.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright so... I think this would meet both of your comments?

    def _get_start_value(self, node: nodes.NodeNG) -> tuple[int | None, Confidence]:
        if isinstance(node, (nodes.Name, nodes.Call, nodes.Attribute)):
            inferred = utils.safe_infer(node)
            start_val = inferred.value if inferred else None
            return start_val, INFERENCE
        if isinstance(node, nodes.UnaryOp):
            return node.operand.value, HIGH
        if isinstance(node, nodes.Const):
            return node.value, HIGH

        return None, HIGH

?

@coveralls
Copy link

coveralls commented Nov 22, 2022

Pull Request Test Coverage Report for Build 3544004706

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 95.428%

Totals Coverage Status
Change from base Build 3543573374: 0.005%
Covered Lines: 17552
Relevant Lines: 18393

💛 - Coveralls

@clavedeluna
Copy link
Collaborator Author

Primer stdlib error appears to be unrelated and probably hitting other PRs, too

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Nov 23, 2022
@clavedeluna clavedeluna removed the Blocked 🚧 Blocked by a particular issue label Nov 23, 2022
@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Nov 23, 2022
@Pierre-Sassoulas
Copy link
Member

Blocked by #7826

@Pierre-Sassoulas
Copy link
Member

Could you rebase/merge main now that #7826 is merged please ? :)

@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Nov 24, 2022
@clavedeluna
Copy link
Collaborator Author

I'm going to add the test case provided in #7821 (comment) and rebase and hopefully we should be good

@github-actions

This comment has been minimized.

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.

Good job on adding the second test case.

@Pierre-Sassoulas Pierre-Sassoulas changed the title Fix bug crash for Attribute Fix crash when using enumerate with start and a class attribute Nov 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas merged commit 6abd0a0 into pylint-dev:main Nov 24, 2022
@github-actions

This comment has been minimized.

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

@Pierre-Sassoulas Pierre-Sassoulas added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Nov 25, 2022
Pierre-Sassoulas added a commit that referenced this pull request Nov 25, 2022
…te (#7824)

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Nov 26, 2022
Pierre-Sassoulas added a commit that referenced this pull request Nov 29, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.15.7, 2.16.0 Nov 29, 2022
@tushar-deepsource tushar-deepsource mentioned this pull request Dec 5, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.0, 2.15.9 Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported Crash 💥 A bug that makes pylint crash Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pylint crashed with a AstroidError
4 participants