Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disables immediately after an else clause do not work properly. #872

Open
dbaum opened this issue Apr 13, 2016 · 6 comments
Open

Disables immediately after an else clause do not work properly. #872

dbaum opened this issue Apr 13, 2016 · 6 comments
Labels
Bug 🪲 C: Pragma's Needs astroid update Needs an astroid update (probably a release too) before being mergable Needs review 🔍 Needs to be reviewed by one or multiple more persons New parser Requires a new AST parser (upstream) Work in progress

Comments

@dbaum
Copy link

dbaum commented Apr 13, 2016

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
Copy link
Author

dbaum commented Apr 13, 2016

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).

@PCManticore
Copy link
Contributor

Indeed, this is definitely problematic, which is one of the reasons we will try to switch to other parsers in pylint-dev/astroid#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 PCManticore added the New parser Requires a new AST parser (upstream) label Jan 22, 2017
@stanislavlevin
Copy link
Contributor

The issue is still here.

@Pierre-Sassoulas Pierre-Sassoulas added Good first issue Friendly and approachable by new contributors Help wanted 🙏 Outside help would be appreciated, good for new contributors labels Aug 4, 2021
@Pierre-Sassoulas
Copy link
Member

See #3898 (comment) 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 Pierre-Sassoulas added the Minor 💅 Polishing pylint is always nice label Aug 4, 2021
@NickGoog
Copy link

NickGoog commented Oct 4, 2021

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)

@DanielNoord
Copy link
Collaborator

DanielNoord commented Mar 19, 2022

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

@cdce8p cdce8p removed Help wanted 🙏 Outside help would be appreciated, good for new contributors Minor 💅 Polishing pylint is always nice Good first issue Friendly and approachable by new contributors Hacktoberfest labels Apr 22, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jul 18, 2022
@clavedeluna clavedeluna added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Dec 22, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Work in progress Needs review 🔍 Needs to be reviewed by one or multiple more persons and removed Needs PR This issue is accepted, sufficiently specified and now needs an implementation labels Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 C: Pragma's Needs astroid update Needs an astroid update (probably a release too) before being mergable Needs review 🔍 Needs to be reviewed by one or multiple more persons New parser Requires a new AST parser (upstream) Work in progress
Projects
None yet
Development

No branches or pull requests

8 participants