Add approximate suggestions for rustfix #47540

Manishearth · 2018-01-18T11:50:28Z

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

rust-highfive · 2018-01-18T11:50:39Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

Manishearth · 2018-01-18T11:50:40Z

Not tested yet, but will test soon

Manishearth · 2018-01-18T11:50:46Z

r? @nrc

nrc · 2018-01-18T19:46:51Z

I've opposed this in the past, because I don't think it is ever possible to mechanically apply suggestions without user input. However, this might be something different, let's discuss at the meeting.

oli-obk · 2018-01-18T20:02:32Z

With the rfc being closed due to an experimental implementation getting greenlit (rust-lang/rfcs#1941 (comment)) I assumed that this was fine.

oli-obk · 2018-01-18T21:25:04Z

rereading the RFC, the json output should only be changed if -Zunstable-features is enabled, so we don't start emitting the flag unless explicitly desired. Maybe even under its own -Z flag

Manishearth · 2018-01-19T02:26:57Z

@nrc rustfix already does this pretty well. Of course there will still be human confirmation of the diff, but we can at least try and remove things we know are not source suggestions.

And yeah, I was under the impression we had already agreed to add this API in the RFC. I can put it behind a flag.

nrc · 2018-01-19T04:50:15Z

Yeah, I'd forgotten where we'd landed with the RFC. We were all broadly in favour at the dev-tools meeting.

For handling the unstable-ness, rather than have a flag, we should have some API to turn this on, so that only tools which extend the compiler can activate it. I think that is both simpler to implement and gives better safeguards than a flag. killercup was ok with this at the meeting for rustfix.

nrc

Looks fine, modulo the comments inline and adding using a function to turn on the flag.

nrc · 2018-01-19T04:52:21Z

src/libsyntax/json.rs

@@ -121,6 +121,8 @@ struct DiagnosticSpan {
    /// If we are suggesting a replacement, this will contain text
    /// that should be sliced in atop this span.
    suggested_replacement: Option<String>,
+    /// If the suggestion is machine-applicable
+    suggestion_machine_applicable: Option<bool>,


Why is this an Option<bool> rather than just a bool?

nrc · 2018-01-19T04:56:18Z

src/libsyntax/json.rs

@@ -220,7 +222,7 @@ impl Diagnostic {

 impl DiagnosticSpan {
    fn from_span_label(span: SpanLabel,
-                       suggestion: Option<&String>,
+                       suggestion: Option<(&String, bool)>,


I find it a bit weird that we have three different ways to represent the suggestion and whether it is machine applicable (two options, one option and a bool, one option of a tuple). It would be good to stick to one representation, if possible.

Manishearth · 2018-01-19T05:03:07Z

It has to be option<bool> so that it will be excluded from the JSON output when it's not a suggestion (also if the feature is off) It's a separate field so as not to break consumers of the suggestion field.

Manishearth · 2018-01-19T09:27:51Z

Gah. So it turns out that rustc-serialize's JSON stuff will unconditionally output suggestion: machine_applicable: null in case we disable it, which is not something we wish to happen. I'm not sure what to do here, rustc-serialize just isn't that configurable. I could easily add a "skip if null" gated attribute that we only use internally but that feels like scope creep. Alternatively I can define a second struct and add a .json() function that formats it. Kinda icky but not really.

oli-obk · 2018-01-19T10:32:57Z

Can't we do it like with MIR output? Just tell everyone that the field is unstable and should not be used by naming it __do_not_use_machine_applicable?

Manishearth · 2018-01-19T10:41:04Z

that works for me. @nrc ?

…

-Manish Goregaokar

On Fri, Jan 19, 2018 at 4:03 PM, Oliver Schneider ***@***.***> wrote: Can't we do it like with MIR output? Just tell everyone that the field is unstable and should not be used by naming it __do_not_use_machine_ applicable? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#47540 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSMvuXNbHxW8E3kfg6IUo7kIs4h8xks5tMG9tgaJpZM4RizYF> .

bors · 2018-01-23T20:01:10Z

☔ The latest upstream changes (presumably #47678) made this pull request unmergeable. Please resolve the merge conflicts.

Manishearth · 2018-01-24T06:21:09Z

Rebased. I tweaked rustc-serialize with a small unstable attribute that lets fields opt out of serialization if they are None.

oli-obk · 2018-01-24T12:43:49Z

tidy error: /checkout/src/libsyntax_ext/deriving/encodable.rs:215: trailing whitespace

nrc · 2018-01-29T06:02:10Z

Code looks fine (thanks for the changes!), I do still have some qualms about the naming, however: there is one concept here, but two names: machine_applicable and approximate and unfortunately, they have opposite truth values. I think it would be best to choose one or the other and only use that term. I would prefer machine_applicable, but that would mean that the default for the suggestion functions would be false and you would use machine_applicable_suggestion, etc. (I actually think that is the right default, but I'm willing to believe it is not).

Anyway, I'm happy either way, but I would like just one term.

Manishearth · 2018-01-29T06:03:55Z

I'll use approximate so that the default continues to make sense.

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

…he field only exist in the json if the flag is passed

Manishearth · 2018-01-29T06:11:30Z

Done. r?

nrc · 2018-02-01T01:07:44Z

Thanks!

@bors: r+

bors · 2018-02-01T01:07:45Z

📌 Commit 540f95d has been approved by nrc

bors · 2018-02-01T04:47:53Z

⌛ Testing commit 540f95d with merge 26792f0...

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

bors · 2018-02-01T07:33:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 26792f0 to master...

rust-highfive assigned nikomatsakis Jan 18, 2018

rust-highfive assigned nrc and unassigned nikomatsakis Jan 18, 2018

Manishearth mentioned this pull request Jan 18, 2018

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

Closed

Manishearth force-pushed the suggestion branch from 997e25c to 8855ee8 Compare January 18, 2018 12:27

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2018

nrc added I-nominated T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Jan 18, 2018

nrc reviewed Jan 19, 2018

View reviewed changes

Manishearth force-pushed the suggestion branch from 5a854c4 to 065a108 Compare January 19, 2018 09:15

Manishearth force-pushed the suggestion branch from 065a108 to 0131ed1 Compare January 24, 2018 06:20

Manishearth force-pushed the suggestion branch from 0131ed1 to a85e891 Compare January 24, 2018 14:05

Manishearth added 2 commits January 29, 2018 11:41

Add -Zapproximate-suggestions

a53bdc6

Add internal-only rustc_serialize_exclude_null attribute for making t…

540f95d

…he field only exist in the json if the flag is passed

Manishearth force-pushed the suggestion branch from a85e891 to 540f95d Compare January 29, 2018 06:11

bors merged commit 540f95d into rust-lang:master Feb 1, 2018

oli-obk mentioned this pull request Feb 1, 2018

Unergonomic structured suggestions in rustc #47927

Open

ehuss mentioned this pull request Feb 2, 2018

Build failure on nightly-2018-02-01 rust-lang/rust-clippy#2429

Closed

nikomatsakis mentioned this pull request Mar 6, 2018

Epoch lints don't handle macros very well #48704

Closed

zackmdavis mentioned this pull request May 13, 2018

Applicability-ify suggestions #50723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add approximate suggestions for rustfix #47540

Add approximate suggestions for rustfix #47540

Manishearth commented Jan 18, 2018

rust-highfive commented Jan 18, 2018

Manishearth commented Jan 18, 2018

Manishearth commented Jan 18, 2018

nrc commented Jan 18, 2018

oli-obk commented Jan 18, 2018

oli-obk commented Jan 18, 2018

Manishearth commented Jan 19, 2018

nrc commented Jan 19, 2018

nrc left a comment

nrc Jan 19, 2018

nrc Jan 19, 2018

Manishearth commented Jan 19, 2018 via email •

edited

Loading

Manishearth commented Jan 19, 2018

oli-obk commented Jan 19, 2018

Manishearth commented Jan 19, 2018 via email

bors commented Jan 23, 2018

Manishearth commented Jan 24, 2018

oli-obk commented Jan 24, 2018

nrc commented Jan 29, 2018

Manishearth commented Jan 29, 2018

Manishearth commented Jan 29, 2018

nrc commented Feb 1, 2018

bors commented Feb 1, 2018

bors commented Feb 1, 2018

bors commented Feb 1, 2018

Add approximate suggestions for rustfix #47540

Add approximate suggestions for rustfix #47540

Conversation

Manishearth commented Jan 18, 2018

rust-highfive commented Jan 18, 2018

Manishearth commented Jan 18, 2018

Manishearth commented Jan 18, 2018

nrc commented Jan 18, 2018

oli-obk commented Jan 18, 2018

oli-obk commented Jan 18, 2018

Manishearth commented Jan 19, 2018

nrc commented Jan 19, 2018

nrc left a comment

Choose a reason for hiding this comment

nrc Jan 19, 2018

Choose a reason for hiding this comment

nrc Jan 19, 2018

Choose a reason for hiding this comment

Manishearth commented Jan 19, 2018 via email • edited Loading

Manishearth commented Jan 19, 2018

oli-obk commented Jan 19, 2018

Manishearth commented Jan 19, 2018 via email

bors commented Jan 23, 2018

Manishearth commented Jan 24, 2018

oli-obk commented Jan 24, 2018

nrc commented Jan 29, 2018

Manishearth commented Jan 29, 2018

Manishearth commented Jan 29, 2018

nrc commented Feb 1, 2018

bors commented Feb 1, 2018

bors commented Feb 1, 2018

bors commented Feb 1, 2018

Manishearth commented Jan 19, 2018 via email •

edited

Loading