False positive invalid-unary-operand-type with elasticsearch_dsl

[glostis]

Steps to reproduce

Consider the following file, which imports elasticsearch_dsl:

from elasticsearch_dsl import Q


def main():
    return ~Q("exists", field="foo")

Current behavior

pylint on this file returns:

test.py:5:11: E1130: bad operand type for unary ~: str (invalid-unary-operand-type)

Expected behavior

pylint should not complain about this line as it is valid syntax.

Q("exists", field="foo") is of type elasticsearch_dsl.query.Exists which inherits from elasticsearch_dsl.query.Query which implements an __invert__ method, so the ~ operator on such an object is legal.

I am not very familiar with how astroid infers types, but it could be confused by the fact that elasticsearch_dsl.query.Query inherits from elasticsearch_dsl.utils.DslMeta which in turn inherits from elasticsearch_dsl.utils.DslMeta through six's add_metaclass decorator.

I have seen several similar issues concerning numpy (#2436 for example) which were solved by adding a brain tip to astroid. Being quite new to pylint and astroid, I don't know if this would be a good approach to solve the problem I am facing.

pylint --version output

pylint 2.4.4
astroid 2.3.3
Python 3.7.5 (v3.7.5:5c02a39a0b, Oct 14 2019, 18:49:57)
[Clang 6.0 (clang-600.0.57)]

[PCManticore]

Thanks for reporting the issue!

Here's what I presume happens. The Q function looks like this:

def Q(name_or_query='match_all', **params):
    if isinstance(name_or_query, collections_abc.Mapping):
        if params:
            raise ValueError('Q() cannot accept parameters when passing in a dict.')
        if len(name_or_query) != 1:
            raise ValueError('Q() can only accept dict with a single query ({"match": {...}}). Instead it got (%r)' % name_or_query)
        (name, params) = name_or_query.copy().popitem()
        return Query.get_dsl_class(name)(_expand__to_dot=False, **params)
    if isinstance(name_or_query, Query):
        if params:
            raise ValueError('Q() cannot accept parameters when passing in a Query object.')
        return name_or_query
    if hasattr(name_or_query, '_proxied'):
        return name_or_query._proxied
    return Query.get_dsl_class(name_or_query)(**params)

Later on we check that name_or_query is a type of Query, but the initial type of the parameter, the default one, is a string. Right now pylint does not have any control flow type guards, so it disregards completely the isinstance guards that it finds a function. And because the function is returning name_or_query later on, it assumes name_or_query can also be a string, leading to the error you are seeing.

We want to have support for isinstnace guards at some point, but I'm not sure when that is going to happen.
So right now the best bet is to add a brain transform to astroid similar to the ones for numpy for example, in which we instruct pylint what Q actually returns (and it can return a query or something that makes sense for elasticsearch_dsl)

[glostis]

Thanks for your explanations @PCManticore, things start to get clearer!

I have tried to change the Q function definition a bit to understand what goes on, but I don't understand astroid's behavior.

If I change the default value of name_or_query to a tuple for example:

def Q(name_or_query=(1,), **params):

the pylint error is still: bad operand type for unary ~: str even though name_or_query is not a string anymore.

However, I have seen that by commenting the if isinstance(name_or_query, Query): ... lines in the Q definition, the pylint error disappears:

def Q(name_or_query=(1,), **params):
    if isinstance(name_or_query, collections_abc.Mapping):
        if params:
            raise ValueError('Q() cannot accept parameters when passing in a dict.')
        if len(name_or_query) != 1:
            raise ValueError('Q() can only accept dict with a single query ({"match": {...}}). '
                 'Instead it got (%r)' % name_or_query)
        name, params = name_or_query.copy().popitem()
        return Query.get_dsl_class(name)(_expand__to_dot=False, **params)

    # if isinstance(name_or_query, Query):
    #     if params:
    #         raise ValueError('Q() cannot accept parameters when passing in a Query object.')
    #     return name_or_query

    if hasattr(name_or_query, '_proxied'):
        return name_or_query._proxied

    return Query.get_dsl_class(name_or_query)(**params)

How does astroid work when a function, like Q, has several return statements inside flow control? It seems that the return statement I have commented out was the one that was misleading astroid, so removing it removes the error.

[PCManticore]

@glostis astroid will consider all return values from a function, regardless their flow control, so any if guards are completely ignored. Basically this means for the above example it will look at return name_or_query, return Query.get_dsl_class(name_or_query)(**params) and the rest of the return statements.

Even if you change the type of the parameter, pylint is partially able to propagate the exists string argument from return ~Q("exists", field="foo") thus making it believe that it is a string.

A transform like the following should do the trick:

import astroid


def q_transform():
    return astroid.parse('''
    def Q(name_or_query=(1,), **params):
        if isinstance(name_or_query, collections_abc.Mapping):
            if params:
                raise ValueError('Q() cannot accept parameters when passing in a dict.')
            if len(name_or_query) != 1:
                raise ValueError('Q() can only accept dict with a single query ({"match": {...}}). '
                     'Instead it got (%r)' % name_or_query)
            name, params = name_or_query.copy().popitem()
            return Query.get_dsl_class(name)(_expand__to_dot=False, **params)
        
        if hasattr(name_or_query, '_proxied'):
            return name_or_query._proxied
    
        return Query.get_dsl_class(name_or_query)(**params)
    

    ''')


astroid.register_module_extender(
    astroid.MANAGER, 'elasticsearch_dsl.query', q_transform
)

[glostis]

Thanks @PCManticore, the code you provided does indeed do the trick!

Should I open a PR on astroid to add a brain_elasticsearch_dsl.py file, or do you want to avoid cluttering the repo?
Another solution would be for me to make a pylint plugin with this code.

[PCManticore]

Adding it in astroid sounds good, as well as releasing a separate pylint plugin just for this library. We have quite a few plugins in astroid baked in and I don't have any clear rules on what should go there or not (certain libraries are more popular than others and we'd want to support them out of the box)

[glostis]

After second thought, I'm not sure if elasticsearch-dsl is popular enough to justify baking it into astroid, so I've released the transform as a pylint plugin here: https://github.com/glostis/pylint-elasticsearch-dsl