[pyflakes] Add secondary annotation showing previous definition (F811)
#19900
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| F811 | 76 | 38 | 38 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+38 -38 violations, +0 -0 fixes in 3 projects; 52 projects unchanged)
aws/aws-sam-cli (+2 -2 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview
- tests/unit/commands/deploy/test_deploy_context.py:11:47: F811 [*] Redefinition of unused `DeployFailedError` from line 9 + tests/unit/commands/deploy/test_deploy_context.py:11:47: F811 [*] Redefinition of unused `DeployFailedError` from line 9: `DeployFailedError` redefined here - tests/unit/local/docker/test_lambda_image.py:8:27: F811 [*] Redefinition of unused `parameterized` from line 5 + tests/unit/local/docker/test_lambda_image.py:8:27: F811 [*] Redefinition of unused `parameterized` from line 5: `parameterized` redefined here
binary-husky/gpt_academic (+35 -35 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview
- crazy_functions/Rag_Interface.py:8:41: F811 [*] Redefinition of unused `validate_path_safety` from line 4 + crazy_functions/Rag_Interface.py:8:41: F811 [*] Redefinition of unused `validate_path_safety` from line 4: `validate_path_safety` redefined here - crazy_functions/agent_fns/python_comment_agent.py:268:9: F811 Redefinition of unused `dedent` from line 5 + crazy_functions/agent_fns/python_comment_agent.py:268:9: F811 Redefinition of unused `dedent` from line 5: `dedent` redefined here - crazy_functions/doc_fns/AI_review_doc.py:10:39: F811 [*] Redefinition of unused `Inches` from line 9 + crazy_functions/doc_fns/AI_review_doc.py:10:39: F811 [*] Redefinition of unused `Inches` from line 9: `Inches` redefined here - crazy_functions/doc_fns/batch_file_query_doc.py:10:39: F811 [*] Redefinition of unused `Inches` from line 9 + crazy_functions/doc_fns/batch_file_query_doc.py:10:39: F811 [*] Redefinition of unused `Inches` from line 9: `Inches` redefined here - crazy_functions/gen_fns/gen_fns_shared.py:4:48: F811 [*] Redefinition of unused `gen_time_str` from line 3 + crazy_functions/gen_fns/gen_fns_shared.py:4:48: F811 [*] Redefinition of unused `gen_time_str` from line 3: `gen_time_str` redefined here - crazy_functions/gen_fns/gen_fns_shared.py:4:62: F811 [*] Redefinition of unused `trimmed_format_exc` from line 3 + crazy_functions/gen_fns/gen_fns_shared.py:4:62: F811 [*] Redefinition of unused `trimmed_format_exc` from line 3: `trimmed_format_exc` redefined here - crazy_functions/gen_fns/gen_fns_shared.py:5:51: F811 [*] Redefinition of unused `get_log_folder` from line 3 + crazy_functions/gen_fns/gen_fns_shared.py:5:51: F811 [*] Redefinition of unused `get_log_folder` from line 3: `get_log_folder` redefined here - crazy_functions/latex_fns/latex_actions.py:392:16: F811 [*] Redefinition of unused `os` from line 348 + crazy_functions/latex_fns/latex_actions.py:392:16: F811 [*] Redefinition of unused `os` from line 348: `os` redefined here - crazy_functions/pdf_fns/parse_pdf.py:4:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 3 + crazy_functions/pdf_fns/parse_pdf.py:4:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 3: `promote_file_to_downloadzone` redefined here - crazy_functions/pdf_fns/parse_pdf_grobid.py:4:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 3 + crazy_functions/pdf_fns/parse_pdf_grobid.py:4:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 3: `promote_file_to_downloadzone` redefined here - crazy_functions/pdf_fns/parse_pdf_legacy.py:3:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 2 + crazy_functions/pdf_fns/parse_pdf_legacy.py:3:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 2: `promote_file_to_downloadzone` redefined here - crazy_functions/pdf_fns/parse_pdf_via_doc2x.py:3:21: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 2 + crazy_functions/pdf_fns/parse_pdf_via_doc2x.py:3:21: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 2: `promote_file_to_downloadzone` redefined here - crazy_functions/pdf_fns/report_gen_html.py:52:29: F811 [*] Redefinition of unused `get_log_folder` from line 1 + crazy_functions/pdf_fns/report_gen_html.py:52:29: F811 [*] Redefinition of unused `get_log_folder` from line 1: `get_log_folder` redefined here - crazy_functions/vector_fns/vector_database.py:4:8: F811 [*] Redefinition of unused `os` from line 3 + crazy_functions/vector_fns/vector_database.py:4:8: F811 [*] Redefinition of unused `os` from line 3: `os` redefined here - crazy_functions/批量总结PDF文档.py:5:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 3 + crazy_functions/批量总结PDF文档.py:5:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 3: `promote_file_to_downloadzone` redefined here - crazy_functions/批量翻译PDF文档_NOUGAT.py:3:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 2 + crazy_functions/批量翻译PDF文档_NOUGAT.py:3:44: F811 [*] Redefinition of unused `promote_file_to_downloadzone` from line 2: `promote_file_to_downloadzone` redefined here - crazy_functions/批量翻译PDF文档_NOUGAT.py:99:12: F811 [*] Redefinition of unused `copy` from line 9 + crazy_functions/批量翻译PDF文档_NOUGAT.py:99:12: F811 [*] Redefinition of unused `copy` from line 9: `copy` redefined here - request_llms/bridge_chatglm.py:23:16: F811 [*] Redefinition of unused `os` from line 22 + request_llms/bridge_chatglm.py:23:16: F811 [*] Redefinition of unused `os` from line 22: `os` redefined here - request_llms/bridge_chatglm3.py:23:16: F811 [*] Redefinition of unused `os` from line 22 + request_llms/bridge_chatglm3.py:23:16: F811 [*] Redefinition of unused `os` from line 22: `os` redefined here - request_llms/bridge_claude.py:25:21: F811 [*] Redefinition of unused `get_conf` from line 18 + request_llms/bridge_claude.py:25:21: F811 [*] Redefinition of unused `get_conf` from line 18: `get_conf` redefined here - request_llms/bridge_claude.py:25:31: F811 [*] Redefinition of unused `update_ui` from line 18 + request_llms/bridge_claude.py:25:31: F811 [*] Redefinition of unused `update_ui` from line 18: `update_ui` redefined here - request_llms/bridge_claude.py:25:42: F811 [*] Redefinition of unused `trimmed_format_exc` from line 18 + request_llms/bridge_claude.py:25:42: F811 [*] Redefinition of unused `trimmed_format_exc` from line 18: `trimmed_format_exc` redefined here - request_llms/bridge_internlm.py:53:56: F811 [*] Redefinition of unused `AutoTokenizer` from line 4 + request_llms/bridge_internlm.py:53:56: F811 [*] Redefinition of unused `AutoTokenizer` from line 4: `AutoTokenizer` redefined here ... 24 additional changes omitted for project
latchbio/latch (+1 -1 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview
- tests/test_ls.py:6:13: F811 Redefinition of unused `test_account_jwt` from line 3 + tests/test_ls.py:6:13: F811 Redefinition of unused `test_account_jwt` from line 3: `test_account_jwt` redefined here
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| F811 | 76 | 38 | 38 | 0 | 0 |
pyflakes] Add sub-diagnostic (F811)
|
We have a similar diagnostic in ty for detecting when symbols declared as Although... now that I look at it, it's a bit weird that the "Symbol later reassigned here" annotation is rendered before the "Symbol declared as |
|
Oh interesting. I don't think that should have changed. Are those two screenshots from the same input? I think the rendering depends on exactly how many lines are between the two snippets. |
Great point, they're not! Here's a screenshot on I guess I should file a bug report at ty to improve that diagnostic if there are many lines between the two snippets (which is probably the common case!) |
|
This might be the relevant code, but I haven't tested it: ruff/crates/ruff_db/src/diagnostic/render.rs Lines 372 to 373 in ab685c1 We sort the annotations by their start range earlier in this function but then sort the snippets to put the primary one first, which I think aligns with the behavior in your first screenshot. |
|
Nice! I'm not entirely sure I follow
Or, maybe it's just that I dislike that part :) @BurntSushi will know better but I think we try to avoid having info/help text between sub diagnostics with snippets because it looks somewhat odd. Either way, I do prefer what we have in ty. It's more concise because it doesn't need the extra info text to explain the annotation; the explanation is right in the annotation. |
| @@ -3331,7 +3349,8 @@ impl Drop for DiagnosticGuard<'_, '_> { | |||
| return; | |||
| } | |||
|
|
|||
| if let Some(diagnostic) = self.diagnostic.take() { | |||
| if let Some(mut diagnostic) = self.diagnostic.take() { | |||
| diagnostic.sort_sub_diagnostics(); | |||
There was a problem hiding this comment.
I'm not sure about this. I think it's important that a lint rule author is in full control about how a diagnostic is rendered. If they decide that a sub diagnostic with lower severity should come first, than so be it. I'm sure there's a reason for it.
There was a problem hiding this comment.
I know what you mean, I was just trying to work around the automatic help sub-diagnostic for every rule that has a fix as easily as possible. I'm assuming we don't want lint authors to have to control that, but maybe we can just expose the sub-diagnostic Vec or an API that lets them insert additional sub-diagnostics wherever they want. Or another level of DiagnosticGuard that pushes the help/fix_title last instead of immediately when the Diagnostic is created from a Violation.
There was a problem hiding this comment.
A more strimlined version of report_diagnostic, e.g. report_custom or similar, seems the most reasonable to me. Ideally, we'd just remove fix and help, but I believe those fields are used on the website or other output formats? If not, that would be the simplest: just remove help and fix_title and set them manually
|
I like the ty example too, and it will avoid needing to deal with the sub-diagnostic sorting, at least for now. I'll switch to a secondary annotation! |
|
How do y'all feel about the primary annotation ( However, this has the downside of changing our Adding the secondary annotation doesn't really make sense in the concise diagnostic: However, that is exactly what rustc does: I guess ty must do the same too. |
|
I think ideally the concise diagnostic message would not include the text of the primary annotation in this case (since it kinda duplicates the information already in the diagnostic summary). However, I don't think this is awful: I'd say it's fine for now! |
|
(and yeah, I think ty does do similarly right now!) |
pyflakes] Add sub-diagnostic (F811)pyflakes] Add secondary annotation showing previous definition (F811)
|
A thought triggered by your message
It might be a nice addition for our testing framework to at least capture some concise diagnostics because, unlike you, I don't think many folks (myself included) verify the output manually as you did. |
|
This actually did appear in a CLI snapshot I added in the past, in this case, so I got a bit lucky :) But yes, that sounds like a good idea in general! |
|
We could potentially just dump a second rendering of the diagnostics in ruff/crates/ruff_linter/src/test.rs Lines 360 to 363 in 22c284a I don't think that would be too bad to look at. It reminds me a bit of the parser tests that print the AST. |
Summary -- This is a quick implementation of the idea in #19900 (comment). Especially as we start using new diagnostic features, it seems like a good idea to keep an eye on the `concise` output format as well as the `full`. This PR just appends a second rendering of the diagnostics in our rule snapshots in the `concise` format. The concise message is typically reused in the other output formats too, so this gives us a bit of coverage for all the output formats. I noticed that some of our concise messages are still quite long. That might be another good class of diagnostic to use new features for. Some explanatory pieces might fit better as sub-diagnostics. Test Plan -- Existing snapshots. Again, I clicked through these individually, but very, very quickly
Summary -- This is a quick implementation of the idea in #19900 (comment). Especially as we start using new diagnostic features, it seems like a good idea to keep an eye on the `concise` output format as well as the `full`. This PR just appends a second rendering of the diagnostics in our rule snapshots in the `concise` format. The concise message is typically reused in the other output formats too, so this gives us a bit of coverage for all the output formats. I noticed that some of our concise messages are still quite long. That might be another good class of diagnostic to use new features for. Some explanatory pieces might fit better as sub-diagnostics. Test Plan -- Existing snapshots. Again, I clicked through these individually, but very, very quickly
|
I guess we have some coverage for concise in the ecosystem checks. It only means that we'd rely on the issue being present in at least one of our ecosystem projects which might be a bit too fragile |
| .file | ||
| .relative_path(resolver) | ||
| .to_str() | ||
| .unwrap_or_else(|| ann.span.file.path(resolver)); |
There was a problem hiding this comment.
Out of curiosity, what is the motivation for this change?
There was a problem hiding this comment.
Just to match the path in from_diagnostic above. This isn't used in this PR now that we're using a secondary annotation, so I can revert it!
I'll double-check if it's actually needed for full diagnostics too.
There was a problem hiding this comment.
Using path instead of relative_path changes some of our CLI snapshots for the main diagnostic, so I think we'll need this for sub-diagnostics too, once we use any sub-diagnostics with paths in Ruff.
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
0 0 │ success: false
1 1 │ exit_code: 1
2 2 │ ----- stdout -----
3 3 │ F401 [*] `bar` imported but unused
4 │- --> bar.py:2:8
4 │+ --> /tmp/.tmpJqw1Qi/bar.py:2:8
5 5 │ |
6 6 │ 2 | import bar # unused import
7 7 │ | ^^^
8 8 │ |
9 9 │ help: Remove unused import: `bar`
10 10 │
11 11 │ F401 [*] `foo` imported but unused
12 │- --> foo.py:2:8
12 │+ --> /tmp/.tmpJqw1Qi/foo.py:2:8
13 13 │ |
14 14 │ 2 | import foo # unused import
15 15 │ | ^^^
16 16 │ |
────────────┴───────────────────────────────────────────────────────────────────
There was a problem hiding this comment.
(I think I'll go ahead and leave this instead of reverting, since it seems like it will be useful in the future)
|
This is a nice find for a rule that can make use of the new features because it is one that's very commonly used. |
* main: [ty] Add diagnostics for invalid `await` expressions (#19711) [ty] Synthesize read-only properties for all declared members on `NamedTuple` classes (#19899) [ty] Remove use of `ClassBase::try_from_type` from `super()` machinery (#19902) [ty] Speedup project file discovery (#19913) [`pyflakes`] Add secondary annotation showing previous definition (`F811`) (#19900) Bump 0.12.9 (#19917) [ty] support `kw_only=True` for `dataclass()` and `field()` (#19677)
Summary
This is a second attempt at a first use of a new diagnostic feature after #19886. I'll blame rustc for this one because it also has a similar diagnostic:
We end up with a very similar diagnostic:
Test Plan
New snapshots and manual tests above