Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

Allow multiple solutions in a suggestion #155

Merged
merged 4 commits into from
Dec 9, 2018

Conversation

pietroalbini
Copy link
Member

At the moment, rustfix discards all the suggestions with more than one solution. This prevent implementing machine-applicable fixes for unused_lints where multiple sections of the line needs to be removed:

warning: unused imports: `HashMap`, `HashSet`
 --> src/main.rs:2:24
  |
2 | use std::collections::{HashMap, VecDeque, HashSet};
  |                        ^^^^^^^            ^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default
help: remove this
  |
2 | use std::collections::{VecDeque};
  |                       --      --

This patch changes the behavior to allow multiple solutions.

@pietroalbini
Copy link
Member Author

Also removed a test that was checking for this behavior.

r? @killercup

@killercup
Copy link
Member

Thanks, this looks good! Can you add a new test with a >1 solutions case?

@pietroalbini
Copy link
Member Author

Ok, changed the test suite to use the cached json instead of calling rustc (since it's not possible to do so until the lint is merged into rustc, which is blocked on this) and added a test.

Copy link
Member

@killercup killercup left a comment

Choose a reason for hiding this comment

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

Thanks!

@zackmdavis
Copy link
Member

@pietroalbini @killercup Can we also get a test for multiple distinct suggestions? That is, if I understand correctly, the test added in 4595c3b is using JSON that was generated by calling multipart_suggestion_with_applicability in the compiler (in the open PR rust-lang/rust#56645), but I'm concerned that we want rustfix to handle this differently than JSON generated by calling .span_suggestions_with_applicability (or calling .span_suggestion twice),

(I regret the lack of initiative implied by me leaving this mere comment rather than submitting a PR (as it is written, "patch or STFU"), but I'm afraid I don't have time today.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants