False positive: not-callable for property that returns a lambda function conditionally

[tushar-deepsource]

Bug description

class C:
    def __init__(self):
        self._x = None

    @property
    def foo(self):
        if self._x is None:
            self._x = lambda: None
        return self._x

c = C()
c.foo()

Configuration

No response

Command used

pylint myfile.py | grep E1102

Pylint output

myfile.py:12:0: E1102: c.foo is not callable (not-callable)

Expected behavior

No error. The code runs when run under python.

Pylint version

pylint 2.12.2
astroid 2.9.3
Python 3.10.1 (main, Dec  6 2021, 22:25:40) [Clang 13.0.0 (clang-1300.0.29.3)]

OS / Environment

MacOS Monterey

Additional dependencies

No response

[DanielNoord]

This requires understanding of control flow as we can't infer that the return value of foo() is a function easily.

Note that c._x() won't get you a false positive, so to avoid the warning in the meantime you might want to explore if that's a possibility. Or assigning lambda: None instead of None in the __init__ works?

class C:
    def __init__(self):
        self._x = lambda: None

    @property
    def foo(self):
        return self._x


c = C()
c.foo()

[tushar-deepsource]

@DanielNoord Somehow the @property is very relevant here, as this does not error:

class C:
    def __init__(self):
        self._x = None

    def foo(self):
        if self._x is None:
            self._x = lambda: None
        return self._x

c = C()
c.foo()()

Doing c.foo()()() raises the correct error: E1102: c.foo()() is not callable.

So it seems that the control flow analysis is already present and working. There's just some edge case with properties.

[DanielNoord]

Yeah, the code that checks this relies on the property decorator and then checks if all return values are callable. Since None is not callable (and doesn't get removed due to lacking control-flow) we warn that the call is "not callable".

[tushar-deepsource]

If control flow wasn't there, my 2nd example should've also raised E1102 right?

[DanielNoord]

In the second example the outer () gets ignored because the func attribute of the nodes.Call node is not a nodes.Attribute. The decision to do so was made when we first started checking properties in 979538e. See L302-303.

For the inner () we then fail the decorated_with_property(attr) check, so indeed the property decorator is necessary here.

I haven't thought about the implications of that first check, but perhaps that is a little too restrictive.

That said, I think we're (correctly) being a little stricter when we find the property decorator and check if the return values are indeed not callable. For other calls we don't seem to check return values. If we infer that a node returns either None or lambda: None and we do a call on it, without control-flow inference we won't know for sure that this is not-callable.

We could obviously be less strict until we have better control-flow (at some point...), but is that worth it? I don't know your use case, but doesn't initialising to the empty lambda work?

[tushar-deepsource]

Found this:

https://github.com/PyCQA/pylint/blob/5324fecb32b0c86a70c88fc196360d721085fdfc/pylint/checkers/typecheck.py#L1231-L1242

The inference seems to work just fine. Here, attr.infer_call_result gives us both Const.None and Lambda objects.

But since we're checking if all the inferred results are callable, E1102 is getting raised (None is not callable).

Even if some of the inferred results are callable, pylint reports that it is not callable. Instead of that, maybe it should raise "possibly-not-callable"?

[tushar-deepsource]

Proposed solution:

• If we were able to infer call results, and none of the inferred values were callable, raise not-callable.
• If we were able to infer call results, and some of the inferred values were callable, raise possibly-not-callable.
• If we were not able to infer call results, do nothing

[DanielNoord]

@tushar-deepsource What about changing possibly-not-callable to another lint: assigning-callable-to-non-callable or something like that?
That seems to be a bigger issue imo?

[tushar-deepsource]

@DanielNoord sounds good to me! Did you mean "assigning non callable to callable"? As in if a variable has multiple possible values and some of them aren't callable.

[DanielNoord]

Yeah! I think we should exclude None for now but assigning both a string and a callable to the same variable seems problematic and could be a lint for the Refactoring checker I think.

[tushar-deepsource]

I'll make a new issue for that.

For this issue, is it okay to not raise not-callable in this case then? Mind if I make a PR that only raises not-callable when no inferred values are callable?

[DanielNoord]

@tushar-deepsource I think that sounds like the right way forward!