string_add_assign: add autofix

[lapla-cogito]

changelog: [string_add_assign]: add autofix

[rustbot]

r? @Centri3

rustbot has assigned @Centri3.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

I am surprised that s += "foobar" is not a recognized form, only s = s + "foobar" seems to be.

Also, every test case seems to suggest both .push()/.push_str() and s += ….

[lapla-cogito]

@samueltardieu

Also, every test case seems to suggest both .push()/.push_str() and s += ….

I don't think so. In previous test cases, assign_op_pattern was not explicitly allowed, resulting in a warning. OTOH, string_add_assign suggests only push or push_str both before and after this change.

[samueltardieu]

@samueltardieu

Also, every test case seems to suggest both .push()/.push_str() and s += ….

I don't think so. In previous test cases, assign_op_pattern was not explicitly allowed, resulting in a warning. OTOH, string_add_assign suggests only push or push_str both before and after this change.

Yes, I meant that without allowing it, they both trigger at any s = s + … occurrence. Shouldn't assign_op_pattern not trigger if string_add_assign does to avoid the double (contradictory) suggestion?

[lapla-cogito]

I see. However, it seems non-trivial to decide which lint is preferred (i.e., there could be an implementation that suppresses string_add_assign, not assign_op_pattern).

[lapla-cogito]

I think the main differences between them are their categories (assign_op_pattern is a style lint, while string_add_assign is pedantic) or their suggestion styles, as there is no performance difference—+= internally calls .push_str.
cf) https://doc.rust-lang.org/src/alloc/string.rs.html#2557-2565

[samueltardieu]

I think the main differences between them are their categories (assign_op_pattern is a style lint, while string_add_assign is pedantic) or their suggestion styles, as there is no performance difference—+= internally calls .push_str. cf) https://doc.rust-lang.org/src/alloc/string.rs.html#2557-2565

So it looks like this is an easy choice: if string_add_assign would trigger, the other one should not. No?

[lapla-cogito]

I fail to see why it is an easy choice that string_add_assign should be preferred over assign_op_pattern. Could there be an implementation that prevents string_add_assign from triggering when assign_op_pattern is triggered (It may not be something to include in this patch.)? Or am I missing something?

[samueltardieu]

My reasoning is:

• one of the lints is activated by default (assign_op_pattern), while the other one is opt-in (string_add_assign) – the opt-in should have priority
• one of the lints is generic (assign_op_pattern), while the other one is specialized for one data type (string_add_assign) – the specialized one should have priority

You're right in that it doesn't strictly belong to this PR, but this PR adds a second, conflicting autofix suggestion. The following code

#![allow(unused)]

fn main() {
    let mut s = String::from("hello");
    s = s + ", world!";
    println!("s = {s}");
}

when linted in pedantic mode will get:

warning: you assigned the result of adding something to this string. Consider using `String::push_str()` or `String::push()` instead
 --> src/main.rs:5:5
  |
5 |     s = s + ", world!";
  |     ^^^^^^^^^^^^^^^^^^ help: try: `s.push_str(", world!")`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#string_add_assign
  = note: `-W clippy::string-add-assign` implied by `-W clippy::pedantic`
  = help: to override `-W clippy::pedantic` add `#[allow(clippy::string_add_assign)]`

warning: manual implementation of an assign operation
 --> src/main.rs:5:5
  |
5 |     s = s + ", world!";
  |     ^^^^^^^^^^^^^^^^^^ help: replace it with: `s += ", world!"`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
  = note: `#[warn(clippy::assign_op_pattern)]` on by default

Moreover, when applying the --fix, it will become

#![allow(unused)]

fn main() {
    let mut s = String::from("hello");
    s += ", world!";
    println!("s = {s}");
}

Only the second fix seems to have been applied, or more exactly the second fix seems to have overwritten the first one. I just don't find it very clean.

[lapla-cogito]

OK, I'll prepare a patch to inhibit assign_op_pattern from triggering at that point when string_add_assign triggers. Thanks.

So I think the discussion that has arisen so far has been resolved and can be reviewed. @Centri3

[Centri3]

@samueltardieu, you're free to review here if y'want :)

[samueltardieu]

The empty string case ought to be treated separately with a proper removal suggestion if it is a statement or the last expression in a block. Or it can be replaced by () in other cases.

Also, you can prevent assign_op_pattern from triggering by inserting something like

                if is_type_lang_item(cx, ty.peel_refs(), LangItem::String)
                    && !is_lint_allowed(cx, STRING_ADD_ASSIGN, e.hir_id)
                {
                    // Do not lint if the more specialized `clippy::string_add_assign` lint will triggger.
                    return;
                }

to assign_op_pattern.rs.

It would also be great to add a single codepoint test to ensure that single quotes are used, and that there is no regression if the code is changed later.

[samueltardieu]

This should take an app: &mut Applicability, for use with snippet_with_applicability instead of snippet.

Also, this could return an Option<String>, returning None if the string to replace is empty, so that the lint can be specialized.

[samueltardieu]

Suggested change

	format!(".push_str({})", snippet(cx, s.span, ".."))
	format!(".push_str({})", snippet_with_applicability(cx, s.span, "_", &mut app))

[samueltardieu]

Applicability may not always be MachineApplicable.

[samueltardieu]

This should use snippet_with_applicability().

Also, if sugg_push_or_pushstr was tested before linting, depending on the result, if it returns an Option, you could specialize the lint for the empty string, with something like:

                        let (span, help, sugg) = match cx.tcx.parent_hir_node(e.hir_id) {
                            Node::Stmt(stmt) => (stmt.span, "remove it", ""),
                            Node::Block(_) => (e.span, "remove it", ""),
                            _ => (e.span, "replace it with", "()")
                        };
                        span_lint_and_sugg(
                            cx,
                            STRING_ADD_ASSIGN,
                            span,
                            "adding an empty string to this string has no effect",
                            help,
                            String::from(sugg),
                            Applicability::MachineApplicable,
                        );

with tests for

    x = x + "";
    //~^ string_add_assign

    { x = x + "" }
    //~^ string_add_assign

    let _: () = x = x + "";
    //~^ string_add_assign

    x = x + ""
    //~^ string_add_assign

[samueltardieu]

Also, situations such as x += "foo" ought to be linted as well, but it can always be done later.