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

PLR1702: Visually noisy diagnostic ranges #15570

Open
beauxq opened this issue Jan 18, 2025 · 13 comments · May be fixed by #15578
Open

PLR1702: Visually noisy diagnostic ranges #15570

beauxq opened this issue Jan 18, 2025 · 13 comments · May be fixed by #15578
Labels
breaking Breaking API change diagnostics Related to reporting of diagnostics. needs-design Needs further design before implementation

Comments

@beauxq
Copy link

beauxq commented Jan 18, 2025

If a function has nested blocks to violate PLR1702, almost the entire function is marked for it, including lines that are not violations.

Even lines that are only nested 2 or 3 deep are marked with squigglies.

Even when there are multiple lines in a block nested 9 deep, I think it would be enough to only mark the first line of that deeply nested block.

Image

@beauxq
Copy link
Author

beauxq commented Jan 18, 2025

PLR1702 is currently marking every line inside this function:

def foo():
    while a:  # should not be marked - not a violation
        if b:  # should not be marked - not a violation
            for c in range(3):  # should not be marked - not a violation
                if d:  # should not be marked - not a violation
                    while e:  # should not be marked - not a violation
                        if f:  # should not be marked - not a violation
                            for g in z:  # not bad to mark this line, but not needed if only marking the max
                                print(p)  # this line should be marked
                                pass  # this line doesn't need to be marked, because the previous line is marked
        else:  # should not be marked - not a violation
            print(q)  # should not be marked - not a violation

@dylwil3 dylwil3 added diagnostics Related to reporting of diagnostics. needs-mre Needs more information for reproduction labels Jan 18, 2025
@dylwil3
Copy link
Collaborator

dylwil3 commented Jan 18, 2025

That sounds annoying, I'm sorry! When I run this locally or in the playground, I only see a single diagnostic, though the whole range is highlighted. Similarly, when I use the VSCode extension, I see the entire block highlighted but the pop-up only displays a single diagnostic no matter where I hover.

Could you help me reproduce what you're seeing? What's your setup - how are you calling Ruff, and what version are you using?

Playground link

VSCode:

Image

Terminal:

cat tmp.py
def foo():
    while a:  # should not be marked - not a violation
        if b:  # should not be marked - not a violation
            for c in range(3):  # should not be marked - not a violation
                if d:  # should not be marked - not a violation
                    while e:  # should not be marked - not a violation
                        if f:  # should not be marked - not a violation
                            for g in z:  # not bad to mark this line, but not needed if only marking the max
                                print(p)  # this line should be marked
                                pass  # this line doesn't need to be marked, because the previous line is marked
        else:  # should not be marked - not a violation
            print(q)  # should not be marked - not a violationuvx ruff check tmp.py --isolated --select PLR --preview --no-cache
tmp.py:2:5: PLR1702 Too many nested blocks (7 > 5)
   |
 1 |   def foo():
 2 | /     while a:  # should not be marked - not a violation
 3 | |         if b:  # should not be marked - not a violation
 4 | |             for c in range(3):  # should not be marked - not a violation
 5 | |                 if d:  # should not be marked - not a violation
 6 | |                     while e:  # should not be marked - not a violation
 7 | |                         if f:  # should not be marked - not a violation
 8 | |                             for g in z:  # not bad to mark this line, but not needed if only marking the max
 9 | |                                 print(p)  # this line should be marked
10 | |                                 pass  # this line doesn't need to be marked, because the previous line is marked
11 | |         else:  # should not be marked - not a violation
12 | |             print(q)  # should not be marked - not a violation
   | |____________________^ PLR1702
   |

Found 1 error.

@beauxq
Copy link
Author

beauxq commented Jan 18, 2025

Here's an example with multiple diagnostics.
But I think the multiple diagnostics is not as big of a problem as everything being highlighted.

def foo():
    while a:  # should not be marked - not a violation
        if b:  # should not be marked - not a violation
            for c in range(3):  # should not be marked - not a violation
                if d:  # should not be marked - not a violation
                    while e:  # should not be marked - not a violation
                        if f:  # should not be marked - not a violation
                            for g in z:  # not bad to mark this line, but not needed if only marking the max
                                print(p)  # this line should be marked
                                pass  # this line doesn't need to be marked, because the previous line is marked
                        else:
                            if h:
                                print(r)
        else:  # should not be marked - not a violation
            print(q)  # should not be marked - not a violation

@beauxq
Copy link
Author

beauxq commented Jan 18, 2025

In the original example of the first screenshot, there 250 lines all completely highlighted, making it difficult to see other diagnostics.
That's not helpful highlighting.

If there are 5 violations, highlighting 5 lines is enough.

@dylwil3 dylwil3 removed the needs-mre Needs more information for reproduction label Jan 18, 2025
@dylwil3
Copy link
Collaborator

dylwil3 commented Jan 18, 2025

Thank you, that's helpful!

I think there's a small design question about how to handle this situation, and we should be conscious of whether we are creating a breaking change by modifying where a suppression comment can go. But I agree that we can do better here.

@dylwil3 dylwil3 added the needs-design Needs further design before implementation label Jan 18, 2025
@InSyncWithFoo InSyncWithFoo linked a pull request Jan 19, 2025 that will close this issue
@InSyncWithFoo
Copy link
Contributor

I whipped up a draft PR for this. It doesn't quite work yet, but here's how I envision it:

| def foo():
|     while a:                              # \
|         if b:                             # |
|             for c in range(3):            # | These should not be reported,
|                 if d:                     # | as they don't exceed the max depth.
|                     while e:              # |
|                         if f:             # /
# 
|                             for g in z:   # This statement is the first to exceed the limit.
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Report range
|                                 print(p)  # Thus, it is reported but not any of its substatements.
|                                 pass      #
# 
|                             with y:       # The former statement was already reported.
|                                 print(x)  # Thus, reporting these is redundant.
|                             print(u)      #
# 
|         else:                             # Other blocks of an ancestor statement
|             print(q)                      # are also not reported.
| def foo():
|     while a:                              # \
|         if b:                             # |
|             for c in range(3):            # | These should not be reported,
|                 if d:                     # | as they don't exceed the max depth.
|                     while e:              # |
|                         if f:             # /
# 
|                             if x == y:    # This statement is the first to exceed the limit.
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Report range
|                                 print(p)  # It is therefore reported.
# 
|                             elif y > x:   # This block belongs to the same statement,
|                                 print(p)  # and so it is not reported on its own.

Indentation seems to be the most sensible place to report an overnesting.

@MichaReiser MichaReiser changed the title PLR1702 spam PLR1702: Visually noisy diagnostic ranges Jan 19, 2025
@MichaReiser MichaReiser added the breaking Breaking API change label Jan 19, 2025
@InSyncWithFoo
Copy link
Contributor

A refined explanation can be found at this test file.

I'm torn between this current approach and one that simply reports the first block that exceeds the limit:

def foo():
	if a:
		if b:
			if c:
				if d:
					if e:
						if f:
							if g:             # PLR1702: Too many nested blocks (only n allowed)
								if h:
									print(i)

But that would require some recursion to find the max depth.

@MichaReiser
Copy link
Member

MichaReiser commented Jan 20, 2025

I'm torn on this.

Both too-many-branches and complex-structure report the entire function (and I expect so do other too-many-* rules). I also think that highlighting the function itself sort of makes sense, considering:

Checks for functions or methods with too many nested blocks.

There's a Ruff helper somewhere (I wasn't able to find it just now) that only extracts the range of the function's name. This would reduce the highlighting range but doesn't point the user to the relevant code.

That's where highlighting the first "violating" block would definitely be an improvement. But this is a breaking change (it's still a preview rule so it might be fine).

I'm not convinced that highlighting the indent is the right solution. It incorrectly suggests that something is wrong with the indent, which there isn't.

Has anyone looked into what the upstream rule odes or what other tools do in this situation?

Side note: The rule should probably be updated to support match statements

@dhruvmanila
Copy link
Member

(This is a scenario where related diagnostics might help where the main diagnostic will highlight the function name but related diagnostics will also highlight the block which makes Ruff consider this as exceeding the limit.)

@InSyncWithFoo
Copy link
Contributor

I'm not convinced that highlighting the indent is the right solution. It incorrectly suggests that something is wrong with the indent, which there isn't.

I do think deep indentation is the problem in this case. Not character-wise, no, but semantically.

@MichaReiser
Copy link
Member

I do think deep indentation is the problem in this case. Not character-wise, no, but semantically.

Ideally the same approach could be taken for all too-* rules and most of them aren't about the indent.

@InSyncWithFoo
Copy link
Contributor

Ideally the same approach could be taken for all too-* rules and most of them aren't about the indent.

I take it that you are suggesting other rules should be updated as well?

If so, here's my take: The reported range is that of the first that exceeds the limit.

This aligns with, say, E501, which reports all characters from the $(n + 1)$-th onward. The collective range for all exceeders would be just as bad as the name of the function/class, if not worse, so the first of them would perhaps be representative enough.

(Off-topic: PLR0913 and PLR0917 will benefit from a rename and documentation update. They are currently not very distinct.)

@InSyncWithFoo
Copy link
Contributor

Reporting the keywords is a solid choice too:

if a:
	...
	if b:
#   ^^ PLR1702
		...
	for d in e:
#   ^^^ PLR1702
		...
	while f < g:
#   ^^^^^ PLR1702
		...
	match h:
#   ^^^^^ PLR1702
		case I(j=k):
			...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change diagnostics Related to reporting of diagnostics. needs-design Needs further design before implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants