False negative `trailing-comma-tuple`

[mattclay]

Bug description

A false negative with the trailing-comma-tuple rule can occur due to code preceding the use of a comma.

"""detected.py"""
# pylint: disable=expression-not-assigned

print("demo"),

"""undetected.py"""
# pylint: disable=expression-not-assigned

DEMO = True

print("demo"),

Configuration

No response

Command used

pylint detected.py undetected.py

Pylint output

************* Module detected
detected.py:4:0: R1707: Disallow trailing comma tuple (trailing-comma-tuple)

Expected behavior

************* Module detected
detected.py:4:0: R1707: Disallow trailing comma tuple (trailing-comma-tuple)
************* Module undetected
undetected.py:6:0: R1707: Disallow trailing comma tuple (trailing-comma-tuple)

Pylint version

pylint 2.16.1
astroid 2.14.1
Python 3.11.1 (main, Jan 28 2023, 18:47:50) [GCC 11.3.0]

OS / Environment

Ubuntu 22.04.1

Additional dependencies

No response

[Pierre-Sassoulas]

Wow, this one has WTF written all over it. (Thank you for creating the issue.)

[nickdrozd]

Adding a comma after the variable leads to the warning getting raised:

# pylint: disable=expression-not-assigned

DEMO = True,

print("demo"),

Apparently the warning is raised from a function that processes tokens:

    def process_tokens(self, tokens: list[tokenize.TokenInfo]) -> None:
        # Process tokens and look for 'if' or 'elif'
        for index, token in enumerate(tokens):
            token_string = token[1]
            if token_string == "elif":
                # AST exists by the time process_tokens is called, so
                # it's safe to assume tokens[index+1] exists.
                # tokens[index+1][2] is the elif's position as
                # reported by CPython and PyPy,
                # token[2] is the actual position and also is
                # reported by IronPython.
                self._elifs.extend([token[2], tokens[index + 1][2]])
            elif _is_trailing_comma(tokens, index):
                if self.linter.is_message_enabled("trailing-comma-tuple"):
                    self.add_message("trailing-comma-tuple", line=token.start[0])

I think what's happening is that the token processor doesn't really understand the structure of the code, it's just looking for where the commas are. Tuples can be broken up across multiple lines, so it has to look back to previous lines, looking for certain keywords.

Maybe the token-processing approach has been pushed a little too far.

[nickdrozd]

Just to finish the thought, one of the keywords the token processor currently looks for is the character =. That is = at the textual level. The pylint: disable= pragma contains that character, so its particular placement can affect whether the warning is emitted. Thus this one raises no warnings:

print("demo"),  # pylint: disable = expression-not-assigned

While this one gets the trailing comma warning:

# pylint: disable = expression-not-assigned
print("demo"),

No warning:

# pylint: disable = expression-not-assigned

DEMO = True

print("demo"),

Gets the warning:

# pylint: disable = expression-not-assigned

DEMO = True

#=
print("demo"),

[mattclay]

@nickdrozd Ah, that explains something else we were seeing. This isn't reported:

"""demo.py"""
# pylint: disable=expression-not-assigned

try:
    pass
except KeyboardInterrupt:
    print("not reported"),

But this is:

"""demo.py"""
# pylint: disable=expression-not-assigned

try:
    pass
except KeyboardInterrupt:
    print("is = reported"),

[Pierre-Sassoulas]

It sounds like we do not need a mere bug fix but a proper rework using the ast instead ?

[nickdrozd]

Thinking about it further, I don't think it would be possible to implement this check with the AST. The presence or absence of particular characters isn't available, because the AST is A. It may be crummy, but I think this will have to be done at the token level. (I could be wrong, of course. How do Flake et al handle this?)

@mattclay In the meantime, you might consider enabling expression-not-assigned. Technically, the expression print('demo'), evaluates to a tuple of one element (namely None). As far as Pylint is concerned, it's just a tuple. But the tuple expression isn't assigned to anything, which is suspicious. So if that warning were enabled, there would at least be some kind of indication that something is amiss.