pylint icon indicating copy to clipboard operation
pylint copied to clipboard

Disables immediately after an else clause do not work properly.

Open dbaum opened this issue 8 years ago • 6 comments

def my_func(x):
    if x:
        pass
    else:
        # pylint: disable=protected-access
        return x._foo

The disable has no effect here and a warning is generated. If any statement is insert between the else and the disable then it works properly. I suspect the line numbering for the else portion of the ast starts at the first child of the else clause rather than the else keyword itself.

dbaum avatar Apr 13 '16 19:04 dbaum

Investigating a little further, it seems like the culprit is BlockRangeMixIn._elsed_block_range:

https://github.com/PyCQA/astroid/blob/master/astroid/mixins.py#L43

Specifically, the logic here is that when an else clause is present, anything before the first line of the first else statement is considered to have occurred before the else. Would it make more sense for this check to be based on the last line of the last statement of the body of the if? I suppose that has the problem that a disable at the end of the body would impact the else...

if ..
  foo()
  # pylint: disable=protected-access
else:
  return x._foo  # disabled because of the comment above

It would be great if we had the line number of the else keyword itself, though I suspect that's a limitation from the underlying python parser.

I'm also not sure about the repercussions for chained if/elif/else statements or the other uses of BlockRangeMixIn (Try, While, etc).

dbaum avatar Apr 13 '16 21:04 dbaum

Indeed, this is definitely problematic, which is one of the reasons we will try to switch to other parsers in https://github.com/PyCQA/astroid/issues/329. Right now I think that trying to fine tune the numbering with the builtin ast module will still result in these kind of peculiar cases, where we don't have an exact state of source code.

PCManticore avatar Apr 18 '16 07:04 PCManticore

The issue is still here.

stanislavlevin avatar Jul 30 '21 15:07 stanislavlevin

See https://github.com/PyCQA/pylint/issues/3898#issuecomment-755332618 if you want to fix this:

I think the issue ultimately comes from astroid. From what I can tell, the disable pragmas are assigned to line ranges based on the associated node's block_range (in file_state.py). In this case the node is an If but the line number corresponds to the orelse portion; the orelse attribute of the If node is just a list of the expressions in the block. Since the pylint comment comes before those expressions, _elsed_block_range ends returning a range for the orelse block that doesn't actually include the expressions in it (since the only way to determine the line range of the orelse is to take the start of the first expression and the end of the last)

Pierre-Sassoulas avatar Aug 04 '21 17:08 Pierre-Sassoulas

Not sure if this is helpful, but pylint:disable also didn't work directly above an else in my case (pylint 2.8.2):

class PolarPoint:
    _private: int


x = PolarPoint()
y = PolarPoint()

# Fails
if x == y:
    pass
else:
    # pylint:disable=protected-access
    print(x._private)

# Fails
if x == y:
    pass
# pylint:disable=protected-access
else:
    print(x._private)

# Success
# pylint:disable=protected-access
if x == y:
    pass
else:
    print(x._private)

NickGoog avatar Oct 04 '21 13:10 NickGoog

I have opened an astroid PR (https://github.com/pylint-dev/astroid/pull/1480) that would allows us to retrieve the position of the else keyword. That should make this fixable.

DanielNoord avatar Mar 19 '22 11:03 DanielNoord