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

Add orelse_lineno and orelse_col_offset to nodes.If #1480

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

DanielNoord
Copy link
Collaborator

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

@DanielNoord DanielNoord added the Enhancement ✨ Improvement to a component label Mar 19, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.12.0 milestone Mar 19, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, that would be very useful indeed.

astroid/rebuilder.py Outdated Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

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.

@DanielNoord DanielNoord marked this pull request as draft March 21, 2022 08:15
@DanielNoord DanielNoord marked this pull request as ready for review March 21, 2022 12:25
@coveralls
Copy link

coveralls commented Mar 22, 2022

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 Coverage Status
Change from base Build 2303054388: 0.03%
Covered Lines: 9169
Relevant Lines: 10000

💛 - Coveralls

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DanielNoord DanielNoord requested a review from cdce8p March 28, 2022 09:00
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@cdce8p cdce8p removed this from the 2.12.0 milestone Apr 22, 2022
@DanielNoord
Copy link
Collaborator Author

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

@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 DanielNoord requested a review from cdce8p April 25, 2022 17:55
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

astroid/rebuilder.py Outdated Show resolved Hide resolved
astroid/rebuilder.py Outdated Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

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

astroid/rebuilder.py Outdated Show resolved Hide resolved
astroid/rebuilder.py Outdated Show resolved Hide resolved
@cdce8p
Copy link
Member

cdce8p commented May 12, 2022

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

DanielNoord commented Jun 1, 2022

@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 DanielNoord added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jun 24, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Work in progress and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Sep 24, 2023
@Pierre-Sassoulas Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs review 🔍 Needs to be reviewed by one or multiple more persons Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants