Remove usages of span_suggestion without Applicability #54241

vi · 2018-09-15T02:40:09Z

Use Applicability::Unspecified for all of them instead.

Shall deprecations for the non-_with_applicability functions be added?

Shall clippy be addressed somehow?

zackmdavis · 2018-09-15T04:38:40Z

Use Applicability::Unspecified for all of them instead.

I'd rather we make the call between MachineApplicable and MaybeIncorrect at the same time we change to _with_applicability; otherwise, we'll just have to do it later in order to get the benefits. It's OK to guess and let reviewers—or, if we're unlucky, bug reporters (recent example)—debate and iterate on which diagnostics should have which applicability values.

vi · 2018-09-15T11:08:17Z

@zackmdavis , There is advantage of doing it in two steps, given influx of new span_suggestion without applicability and each decision about concrete applicability being a potential subject of debate.

First step is obvious and doesn't make any decisions about applicability of litens, yet prevents [accidental] creation of new lints with unspecified applicability.

Second step can be gradual and dialogue-heavy, without time pressure.

vi · 2018-09-15T11:22:22Z

Just noticed there are also span_suggestions and multipart_suggestion.

killercup · 2018-09-15T13:42:28Z

It would be great to add rustfix tests as well. That should probably happen in increments, though, because it leads to much larger diffs.

vi · 2018-09-15T16:33:56Z

Implemented similar pull request for Clippy: rust-lang/rust-clippy#3191

Both pull reqeusts should come as one package.

Use Applicability::Unspecified for all of them instead.

estebank

Thank you so much for taking the work of going through these!

I'm asking for a bunch of changes, mainly fixing indentation to match the current style (arguments on their own line and one indentation level down when they don't fit in a single line) and using an appropriate Applicability when it makes sense.

Other than that, r=me.

src/librustc/session/mod.rs

src/librustc_borrowck/borrowck/mod.rs

src/librustc_typeck/check/op.rs

src/libsyntax/config.rs

src/libsyntax/ext/tt/macro_rules.rs

src/libsyntax/parse/parser.rs

src/libsyntax_ext/format.rs

estebank · 2018-09-16T22:59:31Z

It would be great to add rustfix tests as well. That should probably happen in increments, though, because it leads to much larger diffs.

@killercup how does rustfix handle when there are multiple suggestions to change the same piece of code (for example, multiple mutable access to immutable binding suggesting to add mut to the binding)?

vi · 2018-09-16T23:23:01Z

... using an appropriate Applicability when it makes sense.

I expected that it's better to split "mechanical" refactoring to explicit Unspecified and "political" task of assigning concrete applicability levels to two separate pull requests: the first deprecates the function (preventing influx of new underspecified lints) and is more time-sensitive, the second can be done gradually, lint after lint and be discussed thoroughly.

Or is the whole problem just not big enough to warrant such approach?

estebank · 2018-09-16T23:35:42Z

Or is the whole problem just not big enough to warrant such approach?

The reality is that there's a small amount of people that know wether a given suggestion is always (or even a majority of the time) correct, namely the person that wrote it and the person that reviewed it (at best, sometimes the interactions can be too subtle even for the writer). We sometimes do get it wrong and have to go back and change them, which is why when in doubt I lean towards marking as much as possible as MachineApplicable and change if we find errors in the wild. It is also true that if we misjudge something as MaybeApplicable few people would complain, and it is a "safe" failure mode because no one would be adversely affected.

I understand your position and I would tend to agree in the general case as the correct approach for this kind of changes, but for this task you're already doing the brunt of the work, highlighting the places where the changes need to happen. I (and others in this thread) have enough context to suggest with a high degree of confidence the appropriate Applicability level, which makes me think it is worth it to include that change in this PR. I also invite @zackmdavis, @killercup, @michaelwoerister @oli-obk and @varkor to take a look at the suggested Applicability levels in case I severely misjudged any of them.

vi · 2018-09-16T23:42:59Z

OK, I'll make changing Applicability from Unspecified to something a separate commit.

…nt comma

Also fix some formatting along the way.

vi · 2018-09-17T00:20:40Z

Applied the Applicabilitiyies. There are at least 10 remaining occurrences of Unspecified:

$ git grep Applicability::Unspecified | grep -v src/librustc_errors/diagnostic.rs
src/librustc/infer/error_reporting/nice_region_error/named_anon_conflict.rs:            Applicability::Unspecified,
src/librustc/infer/error_reporting/nice_region_error/static_impl_trait.rs:                                Applicability::Unspecified,
src/librustc/session/mod.rs:                                                                    Applicability::Unspecified,
src/librustc_borrowck/borrowck/mod.rs:                                                                      Applicability::Unspecified,
src/librustc_borrowck/borrowck/mod.rs:                                                                      Applicability::Unspecified,
src/librustc_borrowck/borrowck/mod.rs:                                Applicability::Unspecified,
src/librustc_mir/borrow_check/move_errors.rs:                        Applicability::Unspecified,
src/librustc_mir/borrow_check/move_errors.rs:                        Applicability::Unspecified,
src/librustc_resolve/lib.rs:                                                Applicability::Unspecified);
src/librustc_typeck/check/cast.rs:                                              Applicability::Unspecified,

estebank

Thanks for the changes. However consistency with the style guide lowers the mental burden for people that come later to read the code.

When foo(bar, baz, quz); doesn't fit in one line under 100 cols, the appropriate indentation is

foo(
    bar,
    baz,
    qux,
);

There are multiple instances of

foo(bar,
    baz,
    qux);

from before the current style guide was arrived to, which is why you'll see inconsistencies, but we're slowly moving to the new style.

Would you mind making that change wherever the method calls that you're already modifying don't match that style?

Beyond that, the PR is in good shape. r=me.

estebank · 2018-09-17T05:51:47Z

src/librustc_errors/diagnostic_builder.rs

@@ -39,7 +39,7 @@ pub struct DiagnosticBuilder<'a> {
 /// it easy to declare such methods on the builder.
 macro_rules! forward {
    // Forward pattern for &self -> &Self
-    (pub fn $n:ident(&self, $($name:ident: $ty:ty),*) -> &Self) => {
+    (pub fn $n:ident(&self, $($name:ident: $ty:ty,)*) -> &Self) => {


What's the reason for this change? It seems to me that including the , inside the repetition makes it slightly more annoying to use elsewhere in the file, right?

Inside the macro call, it was requested to update it to new style (each argument on separate line). This looks better with trailing commas, but original macro didn't support trailing commas. Seeing no easy way to make it accept optional trailing comma, I just made it accept non-optional trailing comma. This cascaded into changing all invocations (and one-line invocations just got in the way).

slightly more annoying

For me diff/merge-friendliness brought by trailing commas typically trumps niceness for eye. So if there can only be one, it should be with the commas.

Remembered that there's $(,)* trick, now it can be both with and without.

estebank · 2018-09-17T06:00:50Z

Applied the Applicabilitiyies. There are at least 10 remaining occurrences of Unspecified

We can deal with them in a separate PR :)

killercup · 2018-09-17T09:00:00Z

I'll have a quick look at the chosen applicability levels but I generally trust @estebank's judgement in that regard.

It would be great to add rustfix tests as well. That should probably happen in increments, though, because it leads to much larger diffs.

@killercup how does rustfix handle when there are multiple suggestions to change the same piece of code (for example, multiple mutable access to immutable binding suggesting to add mut to the binding)?

@estebank right now, it'll do weird things. Sadly, the rustfix mode of the compile tests is not as powerful as cargo-fix. We should update it sooner rather than later :)

killercup

Great work! There are some small nits but otherwise r=me, too!

killercup · 2018-09-17T09:02:33Z

src/librustc/session/mod.rs

+                                                                    message,
+                                                                    suggestion,
+                                                                    Applicability::Unspecified,
+                                                                    );


Ah, I see what the formatting commits were about now: Please, for all the people who like split diffs, indent the arguments just one level and not visually :)

@vi the only thing left is the formatting here (and elsewhere) should have the arguments indented one level (four spaces):

diag_builder.span_suggestion_with_applicability( span, message, suggestion, Applicability::Unspecified, );

killercup · 2018-09-17T09:11:11Z

src/librustc_borrowck/borrowck/mod.rs

+                                // reference, I don't know how rustfix handles it, it might
+                                // attempt fixing them multiple times.
+                                //                              @estebank
+                                Applicability::Unspecified,


cargo fix will actually try to iteratively apply suggestions when necessary: rust-lang/cargo#5842. IIUC, this will generate the same suggestion over and over again? This should work.

@killercup Won't it cause the final code to be something along the lines of let mut mut mut mut x?

rustfix works by replacing specific byte ranges. Once a part of the file has been replaced, rustfix will yell at you when you try to replace it against. cargo-fix will then try to resolve this in some ways (see link above)

killercup · 2018-09-17T09:25:28Z

src/librustc_mir/borrow_check/mutability_errors.rs

                            upvar_ident.span,
                            "consider changing this to be mutable",
                            format!("mut {}", upvar_ident.name),
+                            Applicability::MachineApplicable,


"Upvar" implies this is only for closures and closure-like things (maybe generators?), right? I'm asking because I'm not sure I'd want use to change function signatures -- which are potentially public API. Actually, scratch that, if it's a valid fix, we should fix it.

killercup · 2018-09-17T09:35:05Z

src/librustc_typeck/check/cast.rs

                                            "try casting to a `Box` instead",
-                                            format!("Box<{}>", s));
+                                            format!("Box<{}>", s),
+                                            Applicability::MachineApplicable,


Side question: If I read this correctly, s is a trait here. Should we suggest Box<dyn {}> here? (That's how you use the dyn keyword, right?) Probably the same for "try casting to a reference …" above

killercup · 2018-09-17T09:38:25Z

src/libsyntax/ext/tt/macro_rules.rs

+                                                comma_span,
+                                                "missing comma here",
+                                                ", ".to_string(),
+                                                Applicability::MachineApplicable,


This might be the first syntax error fix we have -- are we sure this is 100% correct in this case, @estebank?

I don't think it is the first one, but this suggestion will always be syntactically correct, as in the new code will match one of the macro matches. I can imagine cases where that wouldn't be the expected case, but I'm pretty sure >90% we'll be correct.

killercup · 2018-09-17T09:41:17Z

src/libsyntax_ext/format.rs

+                        diag.multipart_suggestion_with_applicability(
+                                                    "format specifiers use curly braces",
+                                                    suggestions,
+                                                    Applicability::MachineApplicable,


I love this fix. This is just magic. I hope it actually works each time it yields suggestions :D

(Not sure if it is correct although).

michaelwoerister · 2018-09-17T12:36:19Z

I took a quick look but I don't know the intricacies about most of the things touched, so I can't really help here.

src/librustc_errors/diagnostic_builder.rs

vi · 2018-09-17T15:14:03Z

S-waiting-on-author

What's more to be done?

estebank · 2018-09-17T21:49:54Z

@bors r+

bors · 2018-09-17T21:49:55Z

📌 Commit d0790c4 has been approved by estebank

bors · 2018-09-20T06:34:31Z

⌛ Testing commit d0790c4 with merge 992d1e4...

@estebank

Remove usages of span_suggestion without Applicability Use `Applicability::Unspecified` for all of them instead. Shall deprecations for the non-`_with_applicability` functions be added? Shall clippy be addressed somehow? r? @estebank

bors · 2018-09-20T09:02:40Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 992d1e4 to master...

rust-highfive assigned estebank Sep 15, 2018

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

This comment has been minimized.

Sign in to view

vi mentioned this pull request Sep 15, 2018

Use span_suggestion_with_applicability instead of span_suggestion rust-lang/rust-clippy#3191

Merged

This comment has been minimized.

Sign in to view

vi added 5 commits September 16, 2018 21:42

Remove usages of span_suggestion without Applicability

b6fea32

Use Applicability::Unspecified for all of them instead.

Add multipart_suggestion_with_applicability

2f5cb6d

Attach Applicability to multipart_suggestion and span_suggestions

4b05128

Deprecate *_suggestion* that are without explicit applicability

959fc61

trailing whitespace fix

3be24c6

vi force-pushed the suggest_with_applicability branch from 4ebfa7b to 3be24c6 Compare September 16, 2018 18:43

This comment has been minimized.

Sign in to view

Fixup inaccurate rebase

f395072

estebank reviewed Sep 16, 2018

View reviewed changes

estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2018

vi added 4 commits September 17, 2018 02:49

Fix style according to review comments.

6ebb916

Change diagnostic_builder's forward! macro to enforce trailing argume…

0ad1c0e

…nt comma

Fix tidy's too long lines.

c61f4a7

Fill in suggestions Applicability according to @estebank

2b77760

Also fix some formatting along the way.

estebank approved these changes Sep 17, 2018

View reviewed changes

killercup approved these changes Sep 17, 2018

View reviewed changes

Fixed some remaining whitespace issues.

0269e66

(Not sure if it is correct although).

varkor reviewed Sep 17, 2018

View reviewed changes

src/librustc_errors/diagnostic_builder.rs Outdated Show resolved Hide resolved

Better trick for allowing trailing comma at forward!

15982fe

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 17, 2018

Whitespace fix again.

d0790c4

vi force-pushed the suggest_with_applicability branch from 18f25bd to d0790c4 Compare September 17, 2018 17:26

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 Sep 17, 2018

bors merged commit d0790c4 into rust-lang:master Sep 20, 2018

Remove usages of span_suggestion without Applicability #54241

Remove usages of span_suggestion without Applicability #54241

Conversation

vi commented Sep 15, 2018 • edited Loading

zackmdavis commented Sep 15, 2018

vi commented Sep 15, 2018

vi commented Sep 15, 2018

This comment has been minimized.

killercup commented Sep 15, 2018

vi commented Sep 15, 2018 • edited Loading

This comment has been minimized.

This comment has been minimized.

estebank left a comment

Choose a reason for hiding this comment

estebank commented Sep 16, 2018

vi commented Sep 16, 2018

estebank commented Sep 16, 2018

vi commented Sep 16, 2018

vi commented Sep 17, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Sep 17, 2018

killercup commented Sep 17, 2018

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelwoerister commented Sep 17, 2018

vi commented Sep 17, 2018

estebank commented Sep 17, 2018

bors commented Sep 17, 2018

bors commented Sep 20, 2018

bors commented Sep 20, 2018

vi commented Sep 15, 2018 •

edited

Loading

vi commented Sep 15, 2018 •

edited

Loading