Add ``orelse_lineno`` and ``orelse_col_offset`` to ``nodes.If``

[DanielNoord]

Steps

• [x] For new features or bug fixes, add a ChangeLog entry describing what your PR does.
• [x] 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: https://github.com/PyCQA/pylint/issues/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
✓	:sparkles: New feature

Related Issue

[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

[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!

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