pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.31k stars 1.13k forks source link

linting code containing sqlalchemy FunctionElement and compiler.compiles crashes pylint with an InferenceError exception #5679

Closed davekirby closed 2 years ago

davekirby commented 2 years ago

Bug description

After upgrading pylint from 2.2.2 to 2.12.2 and astroid from 2.1.0 to 2.9.3, a file that contained sqlalchemy user function definitions caused pylint to crash with an InferenceError exception. I have cut the file down to the example code below, and it still crashes. Curiously if you remove either the FunctionElement subclass OR the expression.compiles decorator it works, even though they are completely separate bits of code.

from sqlalchemy.ext.compiler import compiles
from sqlalchemy.sql.expression import FunctionElement

class concat(FunctionElement):  # pylint: disable=invalid-name,too-many-ancestors
    """Simple string concatenation function (variable length arguments one)."""
    name = "compat"

@compiles(concat, "postgresql")
def compile_concat_for_postgresql(element, compiler, **kw):
    """Implementation of compat for PostgreSQL."""
    return f"compat({compiler.process(element.clauses, **kw)})"

class months_between(
    FunctionElement
):  # pylint: disable=invalid-name,too-many-ancestors
    """Amount of months between two dates represented as integer value."""
    name = "months_between"

    def __init__(self, *clauses, **kwargs):
        super().__init__(*clauses, **kwargs)

Command used

pylint test.py

Pylint output

pylint crashed with a ``InferenceError`` and with the following stacktrace:

Traceback (most recent call last):
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/lint/pylinter.py", line 1034, in _check_files
    self._check_file(get_ast, check_astroid_module, file)
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/lint/pylinter.py", line 1069, in _check_file
    check_astroid_module(ast_node)
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/lint/pylinter.py", line 1204, in check_astroid_module
    ast_node, walker, rawcheckers, tokencheckers
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/lint/pylinter.py", line 1250, in _check_astroid_module
    walker.walk(node)
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  [Previous line repeated 2 more times]
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/utils/ast_walker.py", line 72, in walk
    callback(astroid)
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/pylint/checkers/typecheck.py", line 1041, in visit_attribute
    for n in owner.getattr(node.attrname)
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/astroid/objects.py", line 210, in getattr
    return list(self.igetattr(name, context=context))
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/astroid/objects.py", line 176, in igetattr
    for inferred in bases._infer_stmts([cls[name]], context, frame=self):
  File "/opt/dbconda/envs/dbsleuth/lib/python3.6/site-packages/astroid/bases.py", line 170, in _infer_stmts
    context=context,
astroid.exceptions.InferenceError: Inference failed for all members of [<FunctionDef.__init__ l.None at 0x7f4606fdd0f0>].

Expected behavior

Pylint to run to completion and report any errors in the file.

Pylint version

pylint 2.12.2
astroid 2.9.3
Python 3.6.7 | packaged by conda-forge | (default, Nov 6 2019, 16:19:42)

OS / Environment

RHEL 8

Additional dependencies

SQLAlchemy==1.4.29

Pierre-Sassoulas commented 2 years ago

Thank you for opening the bug. This is quite an upgrade you did there !

DanielNoord commented 2 years ago

The following diff would fix this issue:

diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py
index bb9c3de52..25837c343 100644
--- a/pylint/checkers/typecheck.py
+++ b/pylint/checkers/typecheck.py
@@ -1087,6 +1087,8 @@ accessed. Python regular expressions are accepted.",
                     continue
                 missingattr.add((owner, name))
                 continue
+            except astroid.InferenceError:
+                continue
             # stop on the first found
             break
         else:

However, I'd like to investigate if we can't prevent this InferenceError. It seems this might be preventable. Similarly, we might want to add the try...except to getattr in astroid instead of pylint as getattr does not really reflect that it using inference and that it should be guarded by an except.

DanielNoord commented 2 years ago

I'm trying to narrow down the issue. I think I have found a solution that goes beyond catching the exception, but I wonder if my new fix isn't masking a deeper issue.

@davekirby I got the reproducible example down to:

from sqlalchemy.sql.expression import FunctionElement

def compile_concat_for_postgresql(compiler, **kw):
    """Implementation of compat for PostgreSQL."""
    return f"compat({compiler.process(**kw)})"

class months_between(FunctionElement):
    """Amount of months between two dates represented as integer value."""

    def __init__(self):
        super().__init__()

I don't really understand why compile_concat_for_postgresql is necessary for this to crash. I have no experience with sqlalchemy and I grepped for the function name but couldn't find anything. Could you try and explain what the function does? I don't immediately see why its definition interferes with astroid's inference of months_between but it seems to have some effect...

DanielNoord commented 2 years ago

Brought the reproducible example down to: test.py:

from sqlalchemy.sql import roles
from test2 import FromClause

class HasMemoized(object):
    ...

class Generative(HasMemoized):
    ...

class ColumnElement(
    roles.ColumnArgumentOrKeyRole,
    roles.BinaryElementRole,
    roles.OrderByRole,
    roles.ColumnsClauseRole,
    roles.LimitOffsetRole,
    roles.DMLColumnRole,
    roles.DDLConstraintColumnRole,
    roles.StatementRole,
    Generative,
):
    ...

class FunctionElement(ColumnElement, FromClause):
    ...

class months_between(FunctionElement):
    """Amount of months between two dates represented as integer value."""
    def __init__(self):
        super().__init__()

test2.py:

from operator import attrgetter
from sqlalchemy.sql import roles

class HasCacheKey(object):
    ...

class HasMemoized(object):
    ...

class MemoizedHasCacheKey(HasCacheKey, HasMemoized):
    ...

class ClauseElement(MemoizedHasCacheKey):
    ...

class ReturnsRows(roles.ReturnsRowsRole, ClauseElement):
    ...

class Selectable(ReturnsRows):
    ...

class FromClause(roles.AnonymizedFromClauseRole, Selectable):
    c = property(attrgetter("columns"))

sqlalchemy.sql.roles:

from operator import attrgetter
from sqlalchemy.sql import roles

class HasCacheKey(object):
    ...

class HasMemoized(object):
    ...

class MemoizedHasCacheKey(HasCacheKey, HasMemoized):
    ...

class ClauseElement(MemoizedHasCacheKey):
    ...

class ReturnsRows(roles.ReturnsRowsRole, ClauseElement):
    ...

class Selectable(ReturnsRows):
    ...

class FromClause(roles.AnonymizedFromClauseRole, Selectable):
    c = property(attrgetter("columns"))

I believe the issue is related to the imports in roles. There are probably too many ancestors for the cache and therefore astroid trips up.

DanielNoord commented 2 years ago

I have found a fix and submitted a PR to astroid. I expect it to land in 2.10 which I hope we can release before pylint 2.13 is released.