Use span_suggestion_with_applicability instead of span_suggestion #3191

vi · 2018-09-15T16:19:57Z

killercup · 2018-09-15T22:06:59Z

this might duplicate parts of #2943

vi · 2018-09-15T22:12:05Z

I've posted it because of in my linked pull request I suggest deprecation of non-Applicability-accepting suggestion functions. It is meant to stop potential influx of new suggestions without Applicability.

This pull request is only refactoring: all changed suggestions stay Unspecified. Deciding actual applicability status should happen elsewhere (i.e. in #2943).

Git merge strategy parameter (-Xours on #2943 if applied after this one or -Xtheirs if #2943 applied first) may be useful.

flip1995 · 2018-09-16T11:03:31Z

This PR only addresses calls to span_lint_and_then with a |db| db.span_suggestion() closure? If that's the case, this shouldn't interfere with #2943 too much.

The formatting of many db.span_suggestion_with_applicability() is pretty off. Mainly weird/inconsistent indentations. It would be nice, if those would be prettified!

If rust-lang/rust#54241 gets accepted and merged, I'm ok with merging this too.

flip1995 · 2018-09-16T11:04:17Z

clippy_lints/src/assign_ops.rs

+                                            "or", 
+                                            long,
+                                            Applicability::Unspecified,
+                                            );


weird indentation (for example)

Is it cargo fmt-able? If not, I'm not sure what is the correct indentation.

I was doing it manually and used judgement call it is too far right or not and whether it is distinguishable as arguments or not...

rustfmt converts this to

db.span_suggestion_with_applicability( expr.span, "or", long, Applicability::Unspecified, );

clippy_lints/src/booleans.rs

flip1995

Since rust-lang/rust#54241 is S-waiting-on-bors we should move forward with this.

Let's also add Applicability levels here. I annotated the functions with the Applicability levels I think would be fitting. I took @estebank approached and annotated as much as possible with MachineApplicable.

Since I'm not sure how to handle cases where a call to snippet(_, _, "placeholder"); is involved, I annotated them with ", snippet". This could produce suggestions with Placeholders, so maybe HasPlaceholders would be better here.

If I wasn't sure how the suggestion is generated, I just didn't add a comment. I would leave those cases as Unspecified for now.

I would be happy if @killercup and @estebank could also look over my suggestions, in case I misjudged them.

Could you also apply the style @estebank suggested in rust-lang/rust#54241 (review) to the modified methods?

clippy_lints/src/assign_ops.rs

clippy_lints/src/attrs.rs

flip1995 · 2018-09-18T13:03:07Z

clippy_lints/src/bit_mask.rs

+                        e.span,
+                        "try",
+                        format!("{}.trailing_zeros() >= {}", sugg, n.count_ones()),
+                        Applicability::Unspecified,


MaybeIncorrect

And indentation of the whole |db| closure

clippy_lints/src/ranges.rs

clippy_lints/src/returns.rs

clippy_lints/src/swap.rs

vi · 2018-09-18T14:45:25Z

@flip1995, What to do with #2943 then?

flip1995 · 2018-09-18T15:03:22Z

#2943 changes the utils::* lint funtions, not the db.span_suggestion methods inside utils::span_lint_and_then calls. So it is pretty much unrelated. It is pretty old though and I just saw that it needs a huge rebase, probably even bigger with this change.

I think closing it and starting over would be a better approach. Maybe also implement this #2943 (comment) before adding Applicability levels to the utils lint functions.

killercup

some comments but haven read all of it

killercup · 2018-09-18T16:11:37Z

clippy_lints/src/large_enum_variant.rs

                                        span,
                                        "consider boxing the large fields to reduce the total size of the \
                                         enum",
                                        format!("Box<{}>", snip),
+                                        Applicability::Unspecified,


not MachineApplicable since changing the enum field can stop the code from compiling (since we don't also change all the sites where you destructure the enum and would need to add the box to the pattern matching)

killercup · 2018-09-18T16:14:26Z

clippy_lints/src/let_if_seq.rs

+                                                    span,
+                                                    "it is more idiomatic to write",
+                                                    sug,
+                                                    Applicability::Unspecified,


Applicability::HasPlaceholders, actually: https://github.com/rust-lang-nursery/rust-clippy/blob/125907ad08853b92d35e86aecebcf0f784f348d5/tests/ui/let_if_seq.stderr#L8

we might be able to make this MachineApplicable in cases where there is no placeholders: https://github.com/rust-lang-nursery/rust-clippy/blob/125907ad08853b92d35e86aecebcf0f784f348d5/tests/ui/let_if_seq.stderr#L36

killercup · 2018-09-18T16:19:51Z

clippy_lints/src/matches.rs

+                                    expr.span,
+                                    "consider using an if/else expression",
+                                    sugg,
+                                    Applicability::Unspecified,


Hm, in https://github.com/rust-lang-nursery/rust-clippy/blob/125907ad08853b92d35e86aecebcf0f784f348d5/tests/ui/map_unit_fn.stderr I can see a few cases we can apply. Should probably track if there are any placeholders use (which is a good idea in general!).

Any false positives I missed?

killercup · 2018-09-18T16:47:29Z

clippy_lints/src/misc.rs

                                               "try",
                                               format!("let {name}{tyopt} = {initref};",
                                                       name=snippet(cx, i.span, "_"),
                                                       tyopt=tyopt,
-                                                       initref=initref));
+                                                       initref=initref),
+                                               Applicability::Unspecified,


Can we be certain it almost always compiles, even without a type annotation or with just : _?

vi · 2018-09-18T17:19:43Z

Fixed formatting
Applied Applicabilityies

About 25 more Unspecifieds remain.

killercup · 2018-09-18T17:20:17Z

@vi oh I think you marked some comments as "resolved" which meant I didn't notice them so I didn't comment on it

vi · 2018-09-18T17:22:17Z

I marked each review as Resolved as soon as I edited and saved the respective file, to keep track. Is it wrong?

killercup · 2018-09-18T17:26:23Z

Ah, no, not per se. It's just that @flip1995 worte

I would be happy if @killercup and @estebank could also look over my suggestions, in case I misjudged them.

but I didn't see all their suggestions because I didn't expand the resolved comments

vi · 2018-09-18T17:34:08Z

As Applicability application is a separate commit, it can be viewed (and line-commented) as alternative to expending the resolved review comments.

This way it would also catch mistakes in case I incorrectly transferred values from review comments to the code.

flip1995 · 2018-09-18T18:34:39Z

Thanks @killercup for looking over my suggestions.

And even a bigger thanks @vi for doing this!

There are 3 Applicability levels that need to change again:
clippy_lints/src/large_enum_variant.rs
clippy_lints/src/let_if_seq.rs
clippy_lints/src/matches.rs L347

I marked them as unresolved, but now they're showing as outdated.

vi · 2018-09-18T19:44:42Z

Applied those three changes, please check.

Note that "Allow edits from maintainers." is checked, so no need to involve me for every edit.

vi · 2018-09-20T10:07:16Z

What's next?

flip1995 · 2018-09-20T12:44:38Z

Needed a rebase due to #3194 and adjustment of applicability from comment of @estebank #3191 (comment)

flip1995 · 2018-09-20T12:54:15Z

List of `Applicability::Unspecified`:

clippy_lints/src/needless_pass_by_value.rs:235
clippy_lints/src/needless_pass_by_value.rs:247
clippy_lints/src/needless_pass_by_value.rs:264
clippy_lints/src/needless_pass_by_value.rs:276
clippy_lints/src/map_unit_fn.rs:219
clippy_lints/src/map_unit_fn.rs:246
clippy_lints/src/booleans.rs:400
clippy_lints/src/booleans.rs:430
clippy_lints/src/methods.rs:1187
clippy_lints/src/ptr.rs:189
clippy_lints/src/ptr.rs:200
clippy_lints/src/ptr.rs:218
clippy_lints/src/ptr.rs:228
clippy_lints/src/ptr.rs:259
clippy_lints/src/swap.rs:144
clippy_lints/src/transmute.rs:253
clippy_lints/src/transmute.rs:268
clippy_lints/src/transmute.rs:330
clippy_lints/src/transmute.rs:351
clippy_lints/src/transmute.rs:381
clippy_lints/src/transmute.rs:405
clippy_lints/src/transmute.rs:424
clippy_lints/src/transmute.rs:441
clippy_lints/src/transmute.rs:468

Manishearth · 2018-09-27T15:35:37Z

Hi! Could you leave the requested comment in #3230 for relicensing?

vi mentioned this pull request Sep 15, 2018

Remove usages of span_suggestion without Applicability rust-lang/rust#54241

Merged

flip1995 reviewed Sep 16, 2018

View reviewed changes

clippy_lints/src/booleans.rs Outdated Show resolved Hide resolved

killercup mentioned this pull request Sep 18, 2018

Add lint for mem::replace(.., None). #3195

Merged

flip1995 requested changes Sep 18, 2018

View reviewed changes

killercup reviewed Sep 18, 2018

View reviewed changes

vi and others added 8 commits September 20, 2018 14:38

Use span_suggestion_with_applicability instead of span_suggestion

92034e2

Supplement DiagnosticBuilderExt with Applicability

d4c994e

Add forgotten function: span_suggestion*s* to the previous refactoting

3e853a6

Apply subset of "cargo fmt".

2781cac

Fix indents

3eccccb

Fill in Applicability from review comments by @flip1995

5872934

Applicability adjustment per additional comments

52fb7d4

Another Applicability adjustment

987b34d

flip1995 force-pushed the suggest_with_applicability branch from 9043de4 to 987b34d Compare September 20, 2018 12:42

flip1995 approved these changes Sep 20, 2018

View reviewed changes

flip1995 merged commit af2d6a0 into rust-lang:master Sep 23, 2018

flip1995 mentioned this pull request Sep 23, 2018

Meta-issue: Relicense under dual MIT / Apache v2 #3093

Closed

This was referenced Oct 16, 2018

WIP: Add Applicability to suggestion lints #2943

Closed

Add lint for calling mem::discriminant on a non-enum type #3346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use span_suggestion_with_applicability instead of span_suggestion #3191

Use span_suggestion_with_applicability instead of span_suggestion #3191

vi commented Sep 15, 2018

killercup commented Sep 15, 2018

vi commented Sep 15, 2018 •

edited

Loading

flip1995 commented Sep 16, 2018

flip1995 Sep 16, 2018

vi Sep 16, 2018

flip1995 Sep 16, 2018 •

edited

Loading

flip1995 left a comment •

edited

Loading

flip1995 Sep 18, 2018

vi commented Sep 18, 2018

flip1995 commented Sep 18, 2018 •

edited

Loading

killercup left a comment

killercup Sep 18, 2018

killercup Sep 18, 2018

killercup Sep 18, 2018

killercup Sep 18, 2018

vi commented Sep 18, 2018

killercup commented Sep 18, 2018

vi commented Sep 18, 2018 •

edited

Loading

killercup commented Sep 18, 2018

vi commented Sep 18, 2018

flip1995 commented Sep 18, 2018

vi commented Sep 18, 2018 •

edited

Loading

vi commented Sep 20, 2018

flip1995 commented Sep 20, 2018

flip1995 commented Sep 20, 2018

Manishearth commented Sep 27, 2018

Use span_suggestion_with_applicability instead of span_suggestion #3191

Use span_suggestion_with_applicability instead of span_suggestion #3191

Conversation

vi commented Sep 15, 2018

killercup commented Sep 15, 2018

vi commented Sep 15, 2018 • edited Loading

flip1995 commented Sep 16, 2018

flip1995 Sep 16, 2018

Choose a reason for hiding this comment

vi Sep 16, 2018

Choose a reason for hiding this comment

flip1995 Sep 16, 2018 • edited Loading

Choose a reason for hiding this comment

flip1995 left a comment • edited Loading

Choose a reason for hiding this comment

flip1995 Sep 18, 2018

Choose a reason for hiding this comment

vi commented Sep 18, 2018

flip1995 commented Sep 18, 2018 • edited Loading

killercup left a comment

Choose a reason for hiding this comment

killercup Sep 18, 2018

Choose a reason for hiding this comment

killercup Sep 18, 2018

Choose a reason for hiding this comment

killercup Sep 18, 2018

Choose a reason for hiding this comment

killercup Sep 18, 2018

Choose a reason for hiding this comment

vi commented Sep 18, 2018

killercup commented Sep 18, 2018

vi commented Sep 18, 2018 • edited Loading

killercup commented Sep 18, 2018

vi commented Sep 18, 2018

flip1995 commented Sep 18, 2018

vi commented Sep 18, 2018 • edited Loading

vi commented Sep 20, 2018

flip1995 commented Sep 20, 2018

flip1995 commented Sep 20, 2018

Manishearth commented Sep 27, 2018

vi commented Sep 15, 2018 •

edited

Loading

flip1995 Sep 16, 2018 •

edited

Loading

flip1995 left a comment •

edited

Loading

flip1995 commented Sep 18, 2018 •

edited

Loading

vi commented Sep 18, 2018 •

edited

Loading

vi commented Sep 18, 2018 •

edited

Loading