add suggestion applicabilities to librustc and libsyntax #50724

zackmdavis · 2018-05-13T21:15:27Z

A down payment on #50723. Interested in feedback on whether my MaybeIncorrect vs. MachineApplicable judgement calls are well-calibrated (and that we have a consensus on what this means).

r? @Manishearth
cc @killercup @estebank

rust-highfive · 2018-05-13T21:30:01Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:08] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:09] tidy error: /checkout/src/libsyntax/ext/expand.rs:335: line longer than 100 chars
[00:05:10] some tidy checks failed
[00:05:10] 
[00:05:10] 
[00:05:10] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:10] 
[00:05:10] 
[00:05:10] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:10] Build completed unsuccessfully in 0:02:05
[00:05:10] Build completed unsuccessfully in 0:02:05
[00:05:10] make: *** [tidy] Error 1
[00:05:10] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:06c730bc
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

killercup · 2018-05-13T21:35:08Z

Thanks for filing #50723 and starting this! This looks pretty good already.

There is one more thing I'd do, though: Whenever we say something is MachineApplicable, we should probably also add a rustfix test (by adding // run-rustfix to a UI test and adding a .fixed file that contains the fixed state of the source code). For some of the fixes we might want to wait for #50713 and rust-lang/rustfix#97.

Manishearth · 2018-05-14T04:21:04Z

src/librustc/infer/error_reporting/mod.rs

-                err.span_suggestion_short(sp, consider, suggestion);
+                err.span_suggestion_short_with_applicability(
+                    sp, consider, suggestion, Applicability::MaybeIncorrect
+                );


Can applicabilities that aren't MachineApplicable all have a comment explaining why?

In most cases, the explanation is just "I don't feel Overwhelmingly Confident that this suggestion will always be what the user wants, even if I don't have an explicit counterexample of reasonable code for which this suggestion is incorrect", because I was imagining MachineApplicability was a very high standard. (Like you said, we don't want rustfix to guess when it doesn't know.)

If the standard is slightly lower than that (such that MaybeIncorrect requires a commentable justification stronger than "Well, I'm not sure that it's always correct"), then I'd want to change a lot of these to MachineApplicable. Thinking on it, that's probably the best long-term decision, for the same reason assertions and static typing are good ideas: better to make slightly too strong assumptions (and then fix the bugs as they're reported), rather than too weak assumptions (and have no way of knowing whether we could successfully auto-fix more things).

I feel that having the convention of having a comment outlining the reasons the suggestion might be incorrect could be a good start. We can in the future encode this using types.

Manishearth · 2018-05-14T04:21:38Z

src/librustc/middle/liveness.rs

-                        err.span_suggestion(sp, "try ignoring the field",
-                                            format!("{}: _", name));
+                        err.span_suggestion_with_applicability(sp, "try ignoring the field",
+                                                               format!("{}: _", name),


These may need macro checks?

I'm not happy with the situation at the moment. Almost every single suggestion needs to have macro checks. I'll be trying something out to encode this in the API and avoid the current game of whack-a-mole.

Yeah.

So in clippy my plan was that span_to_snippet would take an &mut applicability parameter, and whenever it detects a macro in the thing you're using it would automatically flip the applicability.

But also the span you're suggesting for would need a good macro check. idk.

Manishearth · 2018-05-14T06:10:25Z

Yeah, the bar is slightly higher for edition stuff. It's basically that _known_ and _likely_ problems should not be autofixed. But this is speculative, it's better to mark it machine applicable and then fix in case there are bugs.

…

On Mon, May 14, 2018, 1:08 AM Zack M. Davis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/librustc/infer/error_reporting/mod.rs <#50724 (comment)>: > @@ -1097,7 +1097,9 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { if let Some((sp, has_lifetimes)) = type_param_span { let tail = if has_lifetimes { " + " } else { "" }; let suggestion = format!("{}: {}{}", bound_kind, sub, tail); - err.span_suggestion_short(sp, consider, suggestion); + err.span_suggestion_short_with_applicability( + sp, consider, suggestion, Applicability::MaybeIncorrect + ); In most cases, the explanation is just "I don't feel Overwhelmingly Confident that this suggestion will always be what the user wants, even if I don't have an explicit counterexample of reasonable code for which this suggestion is incorrect", because I was imagining MachineApplicability was a very high standard. (Like you said <#50476 (comment)>, we don't want rustfix to guess when it doesn't *know*.) If the standard is slightly lower than that (such that MaybeIncorrect requires a commentable justification stronger than "Well, I'm not *sure* that it's always correct"), then I'd want to change a lot of these to MachineApplicable. Thinking on it, that's probably the best long-term decision, for the same reason assertions and static typing are good ideas: better to make slightly too strong assumptions (and then fix the bugs as they're reported), rather than too weak assumptions (and have no way of knowing whether we could successfully auto-fix more things). — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#50724 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSM54C8hp7ESzN8oKolLJ79-EG4cuks5tyR9rgaJpZM4T89d0> .

bors · 2018-05-18T02:45:58Z

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

Some would argue that this 40-character method name is ludicrously unwieldy (even ironic), but it's the unique continuation of the precedent set by the other suggestion methods. (And there is some hope that someday we'll just fold `Applicability` into the signature of the "basic" method `span_suggestion`.) This is in support of rust-lang#50723.

Consider this a down payment on rust-lang#50723. To recap, an `Applicability` enum was recently (rust-lang#50204) added, to convey to Rustfix and other tools whether we think it's OK for them to blindly apply the suggestion, or whether to prompt a human for guidance (because the suggestion might contain placeholders that we can't infer, or because we think it has a sufficiently high probability of being wrong even though it's— presumably—right often enough to be worth emitting in the first place). When a suggestion is marked as `MaybeIncorrect`, we try to use comments to indicate precisely why (although there are a few places where we just say `// speculative` because the present author's subjective judgement balked at the idea that the suggestion has no false positives). The `run-rustfix` directive is opporunistically set on some relevant UI tests (and a couple tests that were in the `test/ui/suggestions` directory, even if the suggestions didn't originate in librustc or libsyntax). This is less trivial than it sounds, because a surprising number of test files aren't equipped to be tested as fixed even when they contain successfully fixable errors, because, e.g., there are more, not-directly-related errors after fixing. Some test files need an attribute or underscore to avoid unused warnings tripping up the "fixed code is still producing diagnostics" check despite the fixes being correct; this is an interesting contrast-to/inconsistency-with the behavior of UI tests (which secretly pass `-A unused`), a behavior which we probably ought to resolve one way or the other (filed issue rust-lang#50926). A few suggestion labels are reworded (e.g., to avoid phrasing it as a question, which which is discouraged by the style guidelines listed in `.span_suggestion`'s doc-comment).

zackmdavis · 2018-05-20T23:18:22Z

@Manishearth (updated to be less conservative about choosing MachineApplicable, with comments for MaybeIncorrects; and run-rustfix added to several UI tests; see 98a0429's commit message for more detailed discussion of changes)

pietroalbini · 2018-05-28T08:51:59Z

Ping from triage @Manishearth! This PR needs your review.

Manishearth · 2018-05-28T09:29:50Z

src/libsyntax/ext/expand.rs

-                            err.span_suggestion(span, "try an outer attribute", suggestion);
+                            err.span_suggestion_with_applicability(
+                                span, "try an outer attribute", suggestion,
+                                // We don't 𝑘𝑛𝑜𝑤 that the following item is an ADT


Manishearth

LGTM

Manishearth · 2018-05-28T09:46:03Z

@bors r+

thanks! Are there more left?

FWIW there are also a bunch of "suggestions" that don't use the suggestion API; for example I think some of the "maybe try this variable name instead" suggestions don't use the suggestion API.

With PossiblyIncorrect we can still have rustfix prompt about these in interactive mode.

bors · 2018-05-28T09:46:04Z

📌 Commit 98a0429 has been approved by Manishearth

bors · 2018-05-28T10:11:33Z

⌛ Testing commit 98a0429 with merge 16cd84e...

@Manishearth

add suggestion applicabilities to librustc and libsyntax A down payment on #50723. Interested in feedback on whether my `MaybeIncorrect` vs. `MachineApplicable` judgement calls are well-calibrated (and that we have a consensus on what this means). r? @Manishearth cc @killercup @estebank

bors · 2018-05-28T12:38:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Manishearth
Pushing 16cd84e to master...

zackmdavis · 2018-05-28T16:31:32Z

Are there more left?

Yes, I semi-arbitrarily chose two crates (libsyntax and librustc) for this initial PR. (More convenient to get and iterate on feedback on this than a diff that would be five times as large.)

@eminence

…ebank App-lint-cability @eminence recently pointed out (rust-lang/cargo#5846) that it's surprising that `cargo fix` (now shipping with Cargo itself!) doesn't fix very common lint warnings, which is as good of a reminder as any that we should finish rust-lang#50723. (Previously, we did this on the librustc and libsyntax crates in rust-lang#50724. I filed rust-lang/this-week-in-rust#685 in hopes of recruiting new contributors to do the rest.) r? @estebank

rust-highfive assigned Manishearth May 13, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2018

zackmdavis force-pushed the applicability_rush branch from 4d43eb1 to 94965b9 Compare May 13, 2018 21:33

Manishearth reviewed May 14, 2018

View reviewed changes

zackmdavis force-pushed the applicability_rush branch from 94965b9 to fe0bece Compare May 20, 2018 20:34

This comment has been minimized.

Sign in to view

zackmdavis force-pushed the applicability_rush branch from fe0bece to 98a0429 Compare May 20, 2018 21:15

Manishearth reviewed May 28, 2018

View reviewed changes

Manishearth approved these changes May 28, 2018

View reviewed changes

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 May 28, 2018

bors merged commit 98a0429 into rust-lang:master May 28, 2018

bors mentioned this pull request May 28, 2018

operate on HirId instead of NodeId in hir::Pat::each_binding, and consequences of that #50929

Merged

zackmdavis deleted the applicability_rush branch May 28, 2018 18:27

This was referenced Aug 1, 2018

cargo-fix: what kind of things can be fixed? rust-lang/cargo#5846

Closed

App-lint-cability #52968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add suggestion applicabilities to librustc and libsyntax #50724

add suggestion applicabilities to librustc and libsyntax #50724

zackmdavis commented May 13, 2018

rust-highfive commented May 13, 2018

killercup commented May 13, 2018

Manishearth May 14, 2018

zackmdavis May 14, 2018

estebank May 14, 2018

Manishearth May 14, 2018

estebank May 14, 2018

Manishearth May 14, 2018

Manishearth commented May 14, 2018 via email

bors commented May 18, 2018

This comment has been minimized.

zackmdavis commented May 20, 2018

pietroalbini commented May 28, 2018

Manishearth May 28, 2018

Manishearth left a comment

Manishearth commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

zackmdavis commented May 28, 2018

add suggestion applicabilities to librustc and libsyntax #50724

add suggestion applicabilities to librustc and libsyntax #50724

Conversation

zackmdavis commented May 13, 2018

rust-highfive commented May 13, 2018

killercup commented May 13, 2018

Manishearth May 14, 2018

Choose a reason for hiding this comment

zackmdavis May 14, 2018

Choose a reason for hiding this comment

estebank May 14, 2018

Choose a reason for hiding this comment

Manishearth May 14, 2018

Choose a reason for hiding this comment

estebank May 14, 2018

Choose a reason for hiding this comment

Manishearth May 14, 2018

Choose a reason for hiding this comment

Manishearth commented May 14, 2018 via email

bors commented May 18, 2018

This comment has been minimized.

zackmdavis commented May 20, 2018

pietroalbini commented May 28, 2018

Manishearth May 28, 2018

Choose a reason for hiding this comment

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

bors commented May 28, 2018

zackmdavis commented May 28, 2018