Skip to content

Recommend Option::as_deref when given Option<&String> in place of Option<&str> #89856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
NoraCodes opened this issue Oct 13, 2021 · 3 comments · Fixed by #90645
Closed

Recommend Option::as_deref when given Option<&String> in place of Option<&str> #89856

NoraCodes opened this issue Oct 13, 2021 · 3 comments · Fixed by #90645
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@NoraCodes
Copy link
Contributor

NoraCodes commented Oct 13, 2021

&String can easily be converted to &str, but we give a somewhat opaque E0308 when faced with the following situation:

fn take_str_maybe(x: Option<&str>) -> Option<&str> { None }

fn main() {
    let string = String::from("Hello, world");
    let option = Some(&string);
    take_str_maybe(option);
}

This results in:

error[E0308]: mismatched types
 --> src/main.rs:8:20
  |
8 |     take_str_maybe(option);
  |                    ^^^^^^ expected `str`, found struct `String`
  |
  = note: expected enum `Option<&str>`
             found enum `Option<&String>`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `playground` due to previous error

playground

Which is not totally clear to people who do not understand the Deref trait and/or do not know that it is implemented for these types.

We should suggest that the user use Option::as_deref to convert between these types.

@NoraCodes NoraCodes changed the title Recommend Option::as_deref when give Option<&String> in place of Option<&str> Recommend Option::as_deref when given Option<&String> in place of Option<&str> Oct 13, 2021
@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 14, 2021
@terrarier2111
Copy link
Contributor

@rustbot claim

@terrarier2111
Copy link
Contributor

@estebank Option::as_deref only works for String and not for &String
There are a couple of options I thought about, if there is a better one i'd be happy to use it:

  • tell the user to pass a String instead of a &String and use Option::deref
  • try to find where the &String parameter comes from and tell the user to remove the & sign and use Option::deref
    if this is the last instance where the String was used
  • just handle the cases in which a String is passed
  • try to find a Copy or Clone impl on the struct and suggest to use it and use Option::deref (this one seems like the most unsafe for arbitrary types, but in this particular case it would work)

sorry for the inconvenience, I am new to rustc and I am currently trying to get started with this issue.

@estebank
Copy link
Contributor

@terrarier2111 another option that would remain more local than finding where the Option<&String> was constructed is to try and suggest .map(|x| &**x) for that case in particular. Having said that, finding where the Option<&String> was constructed would be great because it's a type that is almost always wrong to use, unless it comes from a place where you end up with that because of type params (like a hashmap or another container), so it'd be good to lead people towards better code.

What you can do is indeed split your work and focus on making the Option<String> case work on its own PR, and then follow up with an attempt to handle Option<&String>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-papercut Diagnostics: An error or lint that needs small tweaks. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants