Add orelse_lineno and orelse_col_offset to nodes.If

[DanielNoord]

Steps

• For new features or bug fixes, add a ChangeLog entry describing what your PR does.
• Write a good description on what the PR does.

Description

This helps to identify where the else keyword is. It can be very useful, for example, for this issue: pylint-dev/pylint#872.
With a line number for the else keyword we can separate the different blocks in the if...else block and handle the disables accordingly.

Let me know what you guys think of this approach.

Type of Changes

	Type
✓	✨ New feature

Related Issue

[Pierre-Sassoulas]

LGTM, that would be very useful indeed.

[DanielNoord]

Drafting this. Thought about this some more over the weekend and I think it makes sense to also pick up the elif when available.
For:

if:
    ...
elif:
    ...
else:
    ...

The elif is the orelse of the first if. I think it makes sense to pick up that keyword as well.

[coveralls]

Pull Request Test Coverage Report for Build 2307861275

• 30 of 30 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 91.69%

---

Totals
Change from base Build 2303054388:	0.03%
Covered Lines:	9169
Relevant Lines:	10000

---

💛 - Coveralls

[Pierre-Sassoulas]

Looks good, I think a review by Marc would be nice but if he does not have the time this can go :)

[cdce8p]

The current solution is quite inefficient. In worse case, the whole token list is generated multiple times for a single node. It would be better to calculate the end_line beforehand and using the generate_tokens iterator directly. For _get_position_info I've used node.end_lineno (were available) or iterated until the last line. Since it's a generator, we can exit early easily.

Besides the performance issue, I'm not sure a tokenize solution is the right approach. We need to search a whole range of lines which can contain basically any other nodes. For example, what about a nested function with an IfExp node in its body? There is also the issue of else which isn't exclusive to if - else. It can also be used with try and for. All in all, you would need to keep track of a lot of different things which I imaging will be pretty complicated.

Thinking about a different solution, is something preventing us to look at the first node in orelse and using its lineno and col_offset?

Some other questions:

• If that works, do we really need to add new node attributes in astroid? (Or can pylint deal with it itself.)
• What about other block nodes for with else, try with except, else, and finally? How do we deal with them?

In the end, is a change worth the effort?

[DanielNoord]

Thinking about a different solution, is something preventing us to look at the first node in orelse and using its lineno and col_offset?

if condition:
    do_something()
elif other_condition:
    do_something_different()
else:
    # We don't want to do anything here
    # But we don't to disable a message: # pylint: disable=invalid-name
    dont_do_anything()

The issue with the approach and the code above is that we don't think comments into account. I thought this might become problematic, especially since the use-case for this is the handle disable comments in these blocks better. Doing orelse[0].lineno - 1 doesn't guarantee we actually hit the else line.

The issue then became that I had to take quite large range of lines in order not to pass SyntaxError raising code to generate_tokens...

[DanielNoord]

@cdce8p I changed the code to use a pretty naive but working solution. I'm not sure what the impact of rstrip is but it should be much better than generate_tokens right?

Let me know what you think!

[cdce8p]

Not particularly pretty, but I guess it should work.

Could you add some more test cases with for - else and try - except - else in addition to the nested one? This shouldn't be an issue with the current implementation, but those cases are never the less good as regression tests if we ever need to touch this part again.

[DanielNoord]

@cdce8p After your comments I found out that we were actually missing some nodes which could have access to the same attributes.
IfExp is now the only node which has a orelse attribute but not a orelse_lineno attribute as it doesn't really seem relevant there. If you want I can add it there as well, but it is probably best to do so in a follow-up when we actually need it.

[cdce8p]

@cdce8p After your comments I found out that we were actually missing some nodes which could have access to the same attributes.
IfExp is now the only node which has a orelse attribute but not a orelse_lineno attribute as it doesn't really seem relevant there. If you want I can add it there as well, but it is probably best to do so in a follow-up when we actually need it.

Let's step back for a moment. Just for me to understand, once the PR is merged how would you integrate the new information so that the issue is fixed? If I remember correctly, wouldn't you need to modify the block_range methods?

Something else, although I'm not sure it's an issue, with this PR we're only addressing else blocks. What about finally and except? Furthermore, is col_offset necessary or lineno enough?

[DanielNoord]

@cdce8p Current iteration is with changing block_range. As you can see this will fail some tests as we would define scopes differently. That's why I originally opted for a completely separate system.

The issue is basically the following example:

if x:
    1
elif y:  # pylint: disable=pointless-statement
    1
else:
    1

Which lines should be covered by the disable? With this proposal it would be 3 and 4. Similar examples can be seen in the tests. The PR would make it so that every keyword creates its own scope "disable-wise". That's not completely backwards compatible I think, but it does seem a little bit more logical and also more in line with other tools such as coverage.py I think.

[DanielNoord]

@Pierre-Sassoulas @jacobtylerwalls I can dust off this PR if we want. I think the last comment is a good summary of the changes, and is still valuable to make. What do you think?

See also pylint-dev/pylint#872

[Pierre-Sassoulas]

I think this is very nice but it's going to break some expectations and external documentation (stackoverflow) about disable. Ho well..

[jacobtylerwalls]

👍 from me, we'll just need to update docs and decide if the pylint change goes in a minor or major.