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

Differentiate between suggestions which are programmatically applicable and those which are not #39254

Closed
Manishearth opened this issue Jan 23, 2017 · 8 comments · Fixed by #47540
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

@killercup has rustfix, which parses suggestion output and attempts to apply it.

However, sometimes the suggestions will have fillers (_ or ...) in them because they can't generate a snippet for that code. Or it is just not possible to generate an accurate suggestion.

We still want to generate these incomplete suggestions for human readers to manually go the last mile and apply, but JSON consumers probably should differentiate between the two.

It would be nice if the span_suggestion APIs had a bool argument (that is included in the output json) we could use to differentiate between suggestions which are programmatically applicable and those which aren't.

cc @jonathandturner @GuillaumeGomez

@Manishearth Manishearth added the A-diagnostics Area: Messages for errors, warnings, and lints label Jan 23, 2017
@killercup
Copy link
Member

It might also be advisable to choose a specific placeholder for all fillers in suggestions, that is not valid Rust code and will immediately alert the user of a suggestion gone wrong. For examples, ####.

@GuillaumeGomez
Copy link
Member

Suggestions are a bit broken in some (very few) cases, and I think that applying them might cause great troubles. As long as such problems exist (even if there are very few), I think we should not propose to apply provided suggestions.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 1, 2017

duplicate of #37085 and #33691

@nrc
Copy link
Member

nrc commented Feb 3, 2017

As long as such problems exist (even if there are very few), I think we should not propose to apply provided suggestions

This seems wrong - we are not proposing automatically changing code, we are saying the user will run a tool and opt-in to each change, so there is a chance to verify the change, and if it is wrong they can undo (either literally in an IDE, or using a VCS or whatever). So, I don't think we have to be at all perfect to be useful (in fact I don't think we ever can be).

@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 26, 2017
@Manishearth
Copy link
Member Author

I'm thinking of adding this given that rustfix is maturing.

I'm going to add the boolean field to all the span_suggestion* methods, and do a rough audit of the suggestions in rustc and see when they're applicable. Alternatively, I could not add the field and let them all be applicable-by-default, and then add a separate method for this. Thoughts?

@killercup
Copy link
Member

I'd go with making it opt-in to avoid burning people trying this, and also to ensure the suggestions have passed some amount of review. While we can't ensure that the suggestions always work, we can get a good estimate by only setting auto_applicable: true for suggestions we have tests for/that have been reviewed.

We should probably do a "help us review all our suggestions" thing, to get awareness of this feature once rustfix/fixing-in-vscode-via- RLS is more mature. And we can always add an override option for rustfix/rls to apply all suggestions for those who want to help test them. (Let's call it the "improve our error messages" initiative of 2018!)

@Manishearth
Copy link
Member Author

Actually, it seems like most of the Rust ones are all machine applicable. I'll just add two new methods, and if @killercup discovers any bad suggestions we can migrate them over.

In clippy, I'd expect this to look like:

let machine = true;
// span_to_snippet sets `machine` to false if it ends up using the fallback
let snippet1 = span_to_snippet(e.span, "<type>", &mut machine);
...
span_lint_with_suggestion_approximate(cx, ....., machine)

The methods I'm adding do not take a bool parameter, but we'll add a helper in clippy that does.

@Manishearth
Copy link
Member Author

#47540

Manishearth added a commit to Manishearth/rust that referenced this issue Jan 29, 2018
This adds `span_approximate_suggestion()` that lets you emit a
suggestion marked as "approximate" in the JSON output. UI
users see no difference. This is for when rustc and clippy wish to
 emit suggestions which will make sense to the reader (e.g. they may
have placeholders like `<type>`) but are not source-applicable, so that
rustfix/etc can ignore these.

fixes rust-lang#39254
bors added a commit that referenced this issue Feb 1, 2018
Add approximate suggestions for rustfix

This adds `span_approximate_suggestion()` that lets you emit a
suggestion marked as "non-machine applicable" in the JSON output. UI
users see no difference. This is for when rustc and clippy wish to
 emit suggestions which will make sense to the reader (e.g. they may
have placeholders like `<type>`) but are not source-applicable, so that
rustfix/etc can ignore these.

fixes #39254
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants