add special handling for use suggestions
#37
Conversation
|
|
||
| /// We need some special care for use suggestions, | ||
| /// to eliminate duplicates, for example for HashMap, | ||
| /// rustc suggests 4 variants: |
There was a problem hiding this comment.
We should not be eliminating suggestions. If there are exact duplicates that's fine, but if we'd eliminate the duplicate for something like Iter, we're basically guaranteed to get the wrong one.
| .map(|v| { | ||
| let new_v = Suggestion { | ||
| line_range: LineRange { | ||
| start: LinePosition { line: 1, column: 1}, |
There was a problem hiding this comment.
We should not be hacking around with the insertion span. Let's fix rust-lang/rust#42835 instead.
There was a problem hiding this comment.
Actually, until we get that fixed, let's just leave this as it is. It's better that to be completely broken
There was a problem hiding this comment.
Thanks! In addition to Oliver's comments, I wrote a bit (okay, a lot) on detecting the suggestions by its error code. I'd love for this to become a template to how we can filter specific suggestions. Let me know what you think. If you don't want to/have to time to change this, that's fine too :)
| @@ -41,6 +41,12 @@ pub struct Suggestion { | |||
| pub replacement: String, | |||
| } | |||
|
|
|||
| impl Suggestion { | |||
| pub fn is_use_suggestion(&self) -> bool { | |||
| self.replacement.starts_with("use ") && self.replacement.ends_with(";\n") | |||
There was a problem hiding this comment.
Can we get a more stable detection here?
Rust's errors have codes that are present in the JSON output like this:
{
message: {
children: [{
code: null, level: "help", message: "", suggested_replacement: "..."
}],
code: {
code: "E0425", explanation: "..."
},
level: "error",
message: "...",
spans: []
}
}So, here, we can probably look for the message.code.code == E0425. This might require adjusting the Suggestions struct.
For future extensions to other lints: Most clippy lint messages start with something like
"children": [
{
"children": [],
"code": null,
"level": "note",
"message": "#[warn(needless_borrow)] on by default",
"rendered": null,
"spans": []
},
{
"children": [],
"code": null,
"level": "help",
"message": "for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_borrow",
"rendered": null,
"spans": []
}
]Parsing the "#[warn(needless_borrow)] on by default" message seems a bit less fragile than "everything that begins with use and ends with a semicolon". It would be even more awesome to get the actual lint name as structured data (maybe even lint group as well as the child lint that actually created the diagnostic message), but that's something that needs changes in rustc (and some discussion, maybe even an RFC).
There was a problem hiding this comment.
What do you think about placing the lint name into the code field?
There was a problem hiding this comment.
As a string or something like "code": { "code": null, "lint": "needless_borrow" }?
There was a problem hiding this comment.
I'd have thought "code": { "code": "needless_borrow", "explanation": "..." }
There was a problem hiding this comment.
Sure. I have no idea if error codes and lints ever overlap (i.e., if a lint can have an error code or an error with a code a lint name)
| @@ -56,6 +57,9 @@ fn try_main() -> Result<(), ProgramError> { | |||
| .arg(Arg::with_name("yolo") | |||
| .long("yolo") | |||
| .help("Automatically apply all unambiguous suggestions")) | |||
| .arg(Arg::with_name("apply-only-use") | |||
| .long("apply-only-use") | |||
| .help("Apply only use fix suggestions")) | |||
There was a problem hiding this comment.
I'd rather we introduce an --apply or --enable or --only flag (can be used multiple times) and, when set, rustfix will filter the given suggestions to only fix these lints.
For now this can be hardcoded to only accept "use" (or whatever the actual lint name is) to keep the code simple.
|
Resolved by #42 |
comment #36