Fixed a crash when inferring a value and using its qname on a slice

[Pierre-Sassoulas]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #8067

[DanielNoord]

I don't really like using hasattr as it is not exactly type safe. Can't we use isinstance?

[codecov]

Codecov Report

Merging #8070 (e1a478d) into main (4a5b4e6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8070      +/-   ##
==========================================
+ Coverage   95.43%   95.44%   +0.01%     
==========================================
  Files         176      176              
  Lines       18545    18545              
==========================================
+ Hits        17698    17700       +2     
+ Misses        847      845       -2     

Impacted Files	Coverage Δ
pylint/checkers/method_args.py	100.00% <100.00%> (ø)
pylint/checkers/refactoring/refactoring_checker.py	98.33% <100.00%> (+0.20%)	⬆️

[Pierre-Sassoulas]

Maybe we could check that it's a base.Instance ? I've seen that somewhere. Did you have something in mind ?

[DanielNoord]

Maybe we could check that it's a base.Instance ? I've seen that somewhere. Did you have something in mind ?

Purely based on the next line (self.linter.config.timeout_methods) I guess we're looking for FunctionDefs here?

[Pierre-Sassoulas]

Given our current test, it can be

<class 'astroid.nodes.scoped_nodes.scoped_nodes.FunctionDef'>
<class 'astroid.nodes.scoped_nodes.scoped_nodes.ClassDef'>
<class 'astroid.bases.BoundMethod'>

(and create false negative according to our tests if we don't handle those cases). What's wrong with hasattr exactly ?

[DanielNoord]

Given our current test, it can be

<class 'astroid.nodes.scoped_nodes.scoped_nodes.FunctionDef'>
<class 'astroid.nodes.scoped_nodes.scoped_nodes.ClassDef'>
<class 'astroid.bases.BoundMethod'>

(and create false negative according to our tests if we don't handle those cases). What's wrong with hasattr exactly ?

class A:
    def qname(self):
        return 1

😄

There is nothing that says that qname returns a string like we expect it to. I'm also not opposed to adding it to all nodes which seems much simpler than tackling these one by one.

[jacobtylerwalls]

I'm also not opposed to adding it to all nodes which seems much simpler than tackling these one by one.

Marc was against that here, but it also comes up regularly, see pylint-dev/astroid#1373.

[Pierre-Sassoulas]

Hmm yeah, right thank you I understand. The problematic version would be this:

class A:
    def qname(self):
        return "threading.lock.acquire"

Because then it's a false positive for consider-using-with. Are you proposing to add qname to all node in astroid directly ? I was going to propose that because clearly Slice has in fact a qname, and it's builtin.Slice, right ?

[Pierre-Sassoulas]

Being able to use qname on all nodes would obviously be great in order to not crash when we get unexpected nodes. (Could assignAttr qname be the name of the variable with full namespace ?) But it's a big decisions with a required astroid upgrade, maybe hasattr is a decent temporary fix.

[DanielNoord]

Yeah I'm not sure. hasattr is just asking for trouble further down the line. When we add py.typed to astroid all of our incorrect calls to qname() for nodes that don't have it will be flagged anyway.
Here it does feel like we could get away with doing a check for ClassDef, FunctionDef and the "lowest type" of either UnboundMethod or BoundMethod (I always forget which inherits which).

[Pierre-Sassoulas]

Let's the how the primers react then

[Pierre-Sassoulas]

There's a whole lot of other call to hasattr accross the codebase (qname, attrname, name). Probably something to do in another refactor merge request or after the astroid qname discussion.

[tushar-deepsource]

Hey! This does fix the first crash in the issue, but it doesn't fix the second crash in RefactoringChecker, mentioned here.

Dropping the code snippet here as well:

class Foo:
    def __init__(self, foo=slice(42)):
        self.foo = foo


def bar():
    i = Foo()
    i.foo()
    return 100

[DanielNoord]

Clearly node is typed incorrectly, but oh well 😄

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit e1a478d