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 CodeFix::apply_solution and impl Clone #14092

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

Alexendoo
Copy link
Member

What does this PR try to resolve?

In Clippy we have a good few lints that produce mutually exclusive suggestions e.g.

error: octal-looking escape in a literal
  --> tests/ui/octal_escapes.rs:6:19
   |
LL |     let _bad2 = b"\033[0m";
   |                   ^^^^
   |
help: if an octal escape is intended, use a hex escape instead
   |
LL |     let _bad2 = b"\x1b[0m";
   |                   ~~~~
help: if a null escape is intended, disambiguate using
   |
LL |     let _bad2 = b"\x0033[0m";
   |                   ~~~~~~

For these we have to disable rustfix tests since the suggestions are overlapping, this PR adds a method to rustfix that ui_test could use in order to produce multiple .fixed files, one for each alternative suggestion

Additional information

It does not work for for multiple suggestions coming from a single subdiagnostic (Diag::span_suggestions) e.g.

help: consider importing one of these items
  |
1 + use std::collections::HashMap;
  |
1 + use ahash::HashMap;

Solving this would be blocked on rust-lang/rust#53934, on the Clippy side we only have one use of span_suggestions however so it's still very useful without this

The test cases use Clippy lints that I generated by setting the parse_and_replace test to use clippy-driver because of familiarity, if there's a rustc case that does multiple suggestions it would be good to go with that

@rustbot
Copy link
Collaborator

rustbot commented Jun 18, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2024
@Alexendoo
Copy link
Member Author

Ah I misread how the test suite worked, thought it only ran rustc on bless so I'll need to find some rustc examples that produce multiple suggestions

@rust-cloud-vms rust-cloud-vms bot force-pushed the rustfix-separate-suggestions branch from 50fa303 to 148105a Compare June 18, 2024 12:18
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. I saw you've changed the test snapshot to rustc diagnostics. Would you mind

  • Providing issue links and code in Clippy of where Clippy disables rustfix tests.
  • Sharing the Clippy diagnositc JSON which rustfix is not happy with.
  • Also how Clippy is going to use that new public interface so we won't mess up stuff in the future.

That would make the intent clear and connected. Thank you!

/// Currently individual subdiagnostics with multiple solutions are not
/// detected, they are treated as a single combined solution. See
/// [rust-lang/rust#53934](https://github.com/rust-lang/rust/issues/53934).
pub fn apply_suggestions_separately(
Copy link
Member

Choose a reason for hiding this comment

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

Do we consider duplicate solutions inside a suggestion? We should only apply once in that situation. See #13728.
That said, it can be considered a rustc bug and we do nothing here if the only consumer is clippy.

@Alexendoo
Copy link
Member Author

I don't think we have an issue open for it but here's a few test examples with disabled rustfix due to having multiple suggestions

https://github.com/rust-lang/cargo/blob/50fa303d5c3e61c95e1a87b5b12ef3de19d415db/crates/rustfix/tests/separate/separate.json is a good Clippy diagnostic example

The plan would be to add support to ui_test that Clippy uses for its tests, similar to the added tests here there would be multiple fixed files when suggestions contain multiple solutions

I just noticed though that since the fields of Suggestion are pub this could be done without an API change to rustfix. Do you think this should live in rustfix or be implemented outside? The Clone/apply_solution additions for CodeFix would still be useful if it's implemented outside

@weihanglo
Copy link
Member

Thanks for the information! Now I can clearly see where it came from :)

I just noticed though that since the fields of Suggestion are pub this could be done without an API change to rustfix. Do you think this should live in rustfix or be implemented outside?

Sounds that it's better being done on Clippy/ui_test side if it is the only consumer. It also reduces cross repo works if something needs to change in the futre.

The Clone/apply_solution additions for CodeFix would still be useful if it's implemented outside

Yeah I am good with these additions.

@rust-cloud-vms rust-cloud-vms bot force-pushed the rustfix-separate-suggestions branch from 148105a to 47eb0e8 Compare June 19, 2024 19:05
@Alexendoo Alexendoo changed the title Add rustfix::apply_suggestions_separately Add CodeFix::apply_solution and impl Clone Jun 19, 2024
@rust-cloud-vms rust-cloud-vms bot force-pushed the rustfix-separate-suggestions branch from 47eb0e8 to 5a15397 Compare June 19, 2024 19:06
@Alexendoo
Copy link
Member Author

A much smaller PR now 😄

@weihanglo
Copy link
Member

Thanks!

@bors+

BTW, do you know what rust-cloud-vms is? I saw it force-pushed code you commited. Or, are you a bot?

@Alexendoo
Copy link
Member Author

🤖

It's because I pushed from one of the dev desktops, they have a funky credential helper setup going on that I guess appears as though it's coming from the bot

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2024

📌 Commit 5a15397 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 19, 2024
@bors
Copy link
Contributor

bors commented Jun 19, 2024

⌛ Testing commit 5a15397 with merge 99361c4...

@bors
Copy link
Contributor

bors commented Jun 19, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 99361c4 to master...

@bors bors merged commit 99361c4 into rust-lang:master Jun 19, 2024
24 checks passed
@Alexendoo Alexendoo deleted the rustfix-separate-suggestions branch June 19, 2024 21:28
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 22, 2024
Update cargo

17 commits in 3ed207e416fb2f678a40cc79c02dcf4f936a21ce..bc89bffa5987d4af8f71011c7557119b39e44a65
2024-06-18 19:18:22 +0000 to 2024-06-22 00:36:36 +0000
- test: migrate weak_dep_features, workspaces and yank to snapbox (rust-lang/cargo#14111)
- test: migrate features and features(2|_namespaced) to snapbox (rust-lang/cargo#14100)
- test: Add auto-redaction for not found error (rust-lang/cargo#14124)
- test: migrate build to snapbox (rust-lang/cargo#14068)
- test: migrate unit_graph, update and vendor to snapbox (rust-lang/cargo#14119)
- fix(test): Un-redact Packaged files (rust-lang/cargo#14123)
- test: Auto-redact file number (rust-lang/cargo#14121)
- test: migrate lints_table and lints/(mod|unknown_lints) to snapbox (rust-lang/cargo#14104)
- Simplify checking feature syntax (rust-lang/cargo#14106)
- test: migrate testsuites to snapbox (rust-lang/cargo#14091)
- Make `-Cmetadata` consistent across platforms (rust-lang/cargo#14107)
- fix(toml): Warn when edition is unuset, even when MSRV is unset (rust-lang/cargo#14110)
- Add `CodeFix::apply_solution` and impl `Clone` (rust-lang/cargo#14092)
- test: migrate `cargo_alias_config&cargo_config/mod` to snapbox (rust-lang/cargo#14093)
- Simplify checking for dependency cycles (rust-lang/cargo#14089)
- test: Migrate `pub_priv.rs` to snapshot (rust-lang/cargo#14103)
- test: migrate rustdoc and rustdocflags to snapbox (rust-lang/cargo#14098)

<!--
r? ghost
-->
@rustbot rustbot added this to the 1.81.0 milestone Jun 22, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 23, 2024
Update cargo

17 commits in 3ed207e416fb2f678a40cc79c02dcf4f936a21ce..bc89bffa5987d4af8f71011c7557119b39e44a65
2024-06-18 19:18:22 +0000 to 2024-06-22 00:36:36 +0000
- test: migrate weak_dep_features, workspaces and yank to snapbox (rust-lang/cargo#14111)
- test: migrate features and features(2|_namespaced) to snapbox (rust-lang/cargo#14100)
- test: Add auto-redaction for not found error (rust-lang/cargo#14124)
- test: migrate build to snapbox (rust-lang/cargo#14068)
- test: migrate unit_graph, update and vendor to snapbox (rust-lang/cargo#14119)
- fix(test): Un-redact Packaged files (rust-lang/cargo#14123)
- test: Auto-redact file number (rust-lang/cargo#14121)
- test: migrate lints_table and lints/(mod|unknown_lints) to snapbox (rust-lang/cargo#14104)
- Simplify checking feature syntax (rust-lang/cargo#14106)
- test: migrate testsuites to snapbox (rust-lang/cargo#14091)
- Make `-Cmetadata` consistent across platforms (rust-lang/cargo#14107)
- fix(toml): Warn when edition is unuset, even when MSRV is unset (rust-lang/cargo#14110)
- Add `CodeFix::apply_solution` and impl `Clone` (rust-lang/cargo#14092)
- test: migrate `cargo_alias_config&cargo_config/mod` to snapbox (rust-lang/cargo#14093)
- Simplify checking for dependency cycles (rust-lang/cargo#14089)
- test: Migrate `pub_priv.rs` to snapshot (rust-lang/cargo#14103)
- test: migrate rustdoc and rustdocflags to snapbox (rust-lang/cargo#14098)

<!--
r? ghost
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants