Skip to content
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

Lint for using Option::as_deref #4918

Closed
jan-auer opened this issue Dec 19, 2019 · 3 comments · Fixed by #4945
Closed

Lint for using Option::as_deref #4918

jan-auer opened this issue Dec 19, 2019 · 3 comments · Fixed by #4945
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-complexity Lint: Belongs in the complexity lint group

Comments

@jan-auer
Copy link

With Rust 1.40.0, Option::as_deref has landed.

It would be great to have a clippy lint that detects Option::as_ref chained by Option::map<F> where F: FnOnce(&T) -> &<T as Deref>::Target. This would then automatically account for cases where AsRef::as_ref or a custom method like String::as_str are used.

By far the most compelling case is to apply it to Option<String> or Option<PathBuf>, with probably lots of code bases now containing opt.as_ref().map(String::as_str) or similar constructs.

Example in:

let opt = Some(" hello ".to_owned());
let trimmed = opt.as_ref().map(String::as_str).map(str::trim);

Example out:

let opt = Some(" hello ".to_owned());
let trimmed = opt.as_deref().map(str::trim);

There are probably more cases that could be detected, like ? chaining or match statements, but I think those will get gradually less useful to be worth the effort.

Icing on the cake would be compatibility to rustfix :)

@jan-auer
Copy link
Author

I have to say that I'm highly interested in attempting to implement this if someone agrees that this is useful. Realistically though, I might not get to it as I know myself, so I guess this is up for grabs.

@ghost
Copy link

ghost commented Dec 21, 2019

It would be great to have a clippy lint that detects Option::as_ref chained by Option::map where F: FnOnce(&T) -> &::Target. This would then automatically account for cases where AsRef::as_ref or a custom method like String::as_str are used.

Aren't we going to get false positives if we check based on the signature of the closure? The code for as_def is self.as_ref().map(|t| t.deref()). IMO, we lint should only generate warnings for this code or special cases that are equivalent.

@jan-auer
Copy link
Author

You’re right. Especially with closures, there could be false positives. I missed that.

I wonder if there is a nice way to avoid whitelisting all well-known cases...

@flip1995 flip1995 added L-complexity Lint: Belongs in the complexity lint group E-medium Call for participation: Medium difficulty level problem and requires some initial experience. A-lint Area: New lints labels Dec 21, 2019
@bors bors closed this as completed in 6763447 Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-complexity Lint: Belongs in the complexity lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants