rustc recommends removing desired str method instead of adding .to_owned()

[GinoMan]

Summary

When you have a function that returns a String but add .trim() to the return value to remove extraneous whitespace, you have a type error because .trim() returns a &str. But the suggestion is not to add .to_owned() or to_string() to fix the problem but to remove the trimming behavior entirely even if that is exactly what the programmer intended.

When using cargo run, it additionally fails to show the line containing .trim() in the suggested change.

Lint Name

error[E0308]: mismatched types

Reproducer

I tried this code:

use std::io::stdin;

fn get_name() -> String {
	let mut your_name = String::new();
	stdin()
		.read_line(&mut your_name)
		.expect("Failed to read the line for some reason");
	your_name
		.trim()
}

fn main() {
	println!("Hello, What is your name? ");
	let your_name = get_name();
	println!("Hello, {}", your_name)
}

I saw this happen:

    Checking treehouse v0.1.0 (C:\Users\OpenM\Code\Projects\Rust\treehouse)
error[E0308]: mismatched types
 --> src\main.rs:8:2
  |
3 |   fn get_name() -> String {
  |                    ------ expected `std::string::String` because of return type
...
8 | /     your_name
9 | |         .trim()
  | |_______________^ expected `String`, found `&str`
  |
help: try removing the method call
  |
8 -     your_name
9 -         .trim()
8 +     your_name
  |

For more information about this error, try `rustc --explain E0308`.
error: could not compile `treehouse` (bin "treehouse") due to previous error

I expected to see this happen:

    Checking treehouse v0.1.0 (C:\Users\OpenM\Code\Projects\Rust\treehouse)
error[E0308]: mismatched types
 --> src\main.rs:8:2
  |
3 |   fn get_name() -> String {
  |                    ------ expected `std::string::String` because of return type
...
8 | /     your_name
9 | |         .trim()
  | |_______________^ expected `String`, found `&str`
  |
help: add .to_owned() to return a String instead of a &str
  |
8 -     your_name
9 -         .trim()
8 +     your_name
9 +         .trim().to_owned()
  |

For more information about this error, try `rustc --explain E0308`.
error: could not compile `treehouse` (bin "treehouse") due to previous error

Version

rustc 1.68.0 (2c8cc3432 2023-03-06)
binary: rustc
commit-hash: 2c8cc343237b8f7d5a3c3703e3a87f2eb2c54a74
commit-date: 2023-03-06
host: x86_64-pc-windows-msvc
release: 1.68.0
LLVM version: 15.0.6

Additional Labels

No response

[Centri3]

Not really a clippy issue, as it's a wrapper around cargo check with additional lints (and thus any bad diagnostics there will be an issue here as well). This should be changed in rustc

[compiler-errors]

This suggestion should only be applied for methods that have no "side-effects" -- things like ast_str, as_ptr, etc.

[gurry]

This suggestion should only be applied for methods that have no "side-effects" -- things like ast_str, as_ptr, etc.

How do you determine whether the method has "side-effects" or not?

Sorry if this is a stupid question. I'm an absolute newbie to rustc development, but would be keen to start on this issue.

[compiler-errors]

No, that's a fine question. The way I would do it is probably just hard-code a set of known methods. Like... [sym::as_str, sym::as_ptr, sym::to_string] etc.

[gurry]

Thanks :) That's what I was thinking as well.

Is it okay if I claim this issue?

I presume the error originates in type checking and hence the change would go into some HIR related code. Is that correct?

[gurry]

As per my analysis the fix can go into the following if condition in the suggest_remove_last_method_call() method:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

Lines 253 to 255 in fca59ab

	if let hir::ExprKind::MethodCall(hir::PathSegment { ident: method, .. }, recv_expr, &[], _) = expr.kind &&
	let Some(recv_ty) = self.typeck_results.borrow().expr_ty_opt(recv_expr) &&
	self.can_coerce(recv_ty, expected) {

We can add an extra check here for methods with side effects as shown below:

 if let hir::ExprKind::MethodCall(hir::PathSegment { ident: method, .. }, recv_expr, &[], _) = expr.kind &&
            let Some(recv_ty) = self.typeck_results.borrow().expr_ty_opt(recv_expr) &&
            self.can_coerce(recv_ty, expected) &&
            self.has_no_side_effects(expr) {
           }

where the has_no_side_effects() method could be as follows:

fn has_no_side_effects(expr: &hir::Expr<'tcx>) -> bool {
    if (/* expr value is &str && 
        expected type is String &&
        the last called method is in [sym::as_str, sym::as_ptr, sym::to_string...] */) {
         true
    } else {
        false
    }
}

As a result of this change, suggest_remove_last_method_call() will return false in the situation reported in the bug and the control will eventually get to the suggest_deref_ref_or_into() method which should automatically suggest a conversion using to_string()

There's a method hir::Expr::can_have_side_effects()() which seems to be something we could use instead of has_no_side_effects(), but it considers all method call exprs to have side effects. Therefore it may not be a good fit.

With regard to tests, we could enhance this test to verify our case. We could also enhance this test. Or we could write completely new tests.

What are your thoughts @compiler-errors?

[gurry]

While filtering on a set of hard-coded method symbols may work for String, it's not a scalable solution. What we need is a general way to tell whether any given method on any given type could have side effects or not. Since achieving something like that would be non-trivial, I don't think we have a good solution here.

Perhaps a conservative, but likely unacceptable, option is to never suggest removal of methods. Then at least the suggestions in this context will always be correct although not necessarily most natural.

[GinoMan]

While filtering on a set of hard-coded method symbols may work for String, it's not a scalable solution. What we need is a general way to tell whether any given method on any given type could have side effects or not. Since achieving something like that would be non-trivial, I don't think we have a good solution here.

Perhaps a conservative, but likely unacceptable, option is to never suggest removal of methods. Then at least the suggestions in this context will always be correct although not necessarily most natural.

I personally think that would be the best solution. I don't really care that it's suggesting I fix the code, I care that it suggests I fix it by removing a method at all that the programmer likely intended to call. Though I get that a lot of people learn Rust by letting the compiler teach them. I'm in the same boat as a learner myself. Still, seems kinda silly for the compiler to be like "Oh, you want trimming? No! The type doesn't match. Remove the trim and just deal with the spaces." when "to_owned()" handily fixes the problem. Maybe the compiler can tell you how to convert when the solution is as simple as "to_owned()"?