Improve suggestions

[mcarton]

Fix #1052.
This is a work in progress. I probably have forgotten most suggestions. I'll review them all tomorrow.

[mcarton]

Feeling like cheating anyone?

[mcarton]

(This is now based on #1054.)

Some more better suggestions. Introducing multispan_sugg, a function to build a suggestion made from several replacements, without playing with spans manually.
Still a WIP, there are much more suggestions to review.

[mcarton]

Ok, this PR has become big enough. I think it's ready to be merged.

[oli-obk]

The semicolon is wrong here, if the if-then-else is a match arm (or a function argument or sth)

[Manishearth]

I've had a broad look and am very much 🎉 🎈 🎆 for this, but haven't had the time for a thorough review, yet. Thanks for doing this, though!

[oli-obk]

I'm through. The Sugg struct is awesome. A lot of code became more readable. Especially where the ad-hoc parenthesis adders were factored out.

Some parts are a little confusing, like sugg.as_ty(&format!("{} as {}", x, y)). I get that the "type" is just a string, so we can dump anything there, and that there's not much advantage over sugg.as_ty(x.to_string()).as_ty(y.to_string()), but it feels weird. But it's not a blocker imo.

Except for the needless bool refactoring, I could not find any bugs that were introduced from the refactoring.

[mcarton]

Some parts are a little confusing, like sugg.as_ty(&format!("{} as {}", x, y)). I get that the "type" is just a string, so we can dump anything there, and that there's not much advantage over sugg.as_ty(x.to_string()).as_ty(y.to_string()), but it feels weird. But it's not a blocker imo.

But sugg.as_ty(x.to_string()).as_ty(y.to_string()) is 17% longer as sugg.as_ty(&format!("{} as {}", x, y))! 😅

Except for the needless bool refactoring, I could not find any bugs that were introduced from the refactoring.

Let's see what rustfmt suggests about that:

% cat a.rs
fn foo() -> i32 {
    return match 42 {
        1337 => 314,
        _ => 0xdeadbeef,
    }
}
% rustfmt a.rs
% cat a.rs
fn foo() -> i32 {
    return match 42 {
        1337 => 314,
        _ => 0xdeadbeef,
    };
}
% cat b.rs
fn foo() -> i32 {
    return if true {
        42
    } else {
        1337
    }
}
% rustfmt b.rs
% cat b.rs
fn foo() -> i32 {
    return if true {
        42
    } else {
        1337
    };
}

I think the convention is to always have a semi-colon with an explicit return.

[Manishearth]

@nrc Btw, this API might be useful for a rustfix tool in the future.

[Manishearth]

Theoretically we could tweak this API so that it can actually fix the code by editing it directly! 😄

I've always wanted such a mode for rustc and clippy. Still involves some work, but this PR gets most of it done

[oli-obk]

I meant match x { a => return foo, _ => {} }

[oli-obk]

is 17% longer

cough make the method generic over Display cough

[mcarton]

cough make the method generic over Display cough

Duh me.

I meant match x { a => return foo, _ => {} }

Oh right!

Theoretically we could tweak this API so that it can actually fix the code by editing it directly! 😄

Needs more works 😄, but that would be great.

[Manishearth]

And now, https://github.com/killercup/rustfix exists!

[killercup]

@mcarton, before I open a new issue: Does this change by any chance fix that some suggestions for lines that end with ; do not currently end with a ;?

Just got this one in my rustfix demo:

Info: transmute from a pointer type (`*mut libc::c_void`) to a reference type (`&mut Box<for<'r, 'r> std::ops::FnMut(&'r menus::MenuItem, &'r windows::Window)>`)
      #[warn(transmute_ptr_to_ref)] on by default
  --> src/menus.rs:50:17-51:76
Suggestion - Replace:

    mem::transmute::<*mut c_void,
                     &mut Box<FnMut(&MenuItem, &Window)>>(data)(&menu_item, &window);

with:

    &mut *(data as *mut Box<for<'r, 'r> std::ops::FnMut(&'r menus::MenuItem, &'r windows::Window)>)

(note the missing semicolon in the suggestion).

If not, feel free to copy this comment into a new issue 😄

[mcarton]

@killercup not yet, the context isn't used. But it should. I've been planning to do a second round with that.

[killercup]

@mcarton Okay, cool. I'll just paste that into a new issue then :)

Already worked around this with an ugly patch anyway 😄

[mcarton]

@killercup the thing is not all suggestions might need a ;. Imagine the needless_ret lint might suggest to replace return x; with x.

[killercup]

@mcarton Right, I totally missed that. Are there other lints that might do that?

I opened #1076, btw.

[mcarton]

I'm not sure. For that specific example probably not many.
Don't hesitate to report other things like that! I'd really want a “just fix it” tool 🎉 ✨

[llogiq]

So let's merge this already!