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

✅️Option<&T>; ❌️&Option<T> #13054

Closed
Rudxain opened this issue Jul 5, 2024 · 8 comments · Fixed by #13336
Closed

✅️Option<&T>; ❌️&Option<T> #13054

Rudxain opened this issue Jul 5, 2024 · 8 comments · Fixed by #13336
Assignees
Labels
A-lint Area: New lints

Comments

@Rudxain
Copy link
Contributor

Rudxain commented Jul 5, 2024

What it does

Title says it all, but it could be extended to Result and other enums

Advantage

helix-editor/helix#11091

Drawbacks

If a lib has this lint, it could lead to public API breakage. However, it may be worth it, considering this lint could prevent further breakages

Example

fn f<T>(a: &Option<T>) {
    let _ = a.as_ref();
}

Could be written as:

fn f<T>(a: Option<&T>) {
    let _ = a;
}
@Rudxain Rudxain added the A-lint Area: New lints label Jul 5, 2024
@TornaxO7

This comment was marked as duplicate.

@Rudxain
Copy link
Contributor Author

Rudxain commented Jul 6, 2024

I've already (on the Helix PR) linked to Logan Smith's video. I didn't include it here, because I thought it was redundant

@TornaxO7

This comment was marked as off-topic.

@Rudxain

This comment was marked as off-topic.

@nyurik
Copy link
Contributor

nyurik commented Sep 3, 2024

There is already a ref_option_ref lint that suggests this change:

// bad
let x: &Option<&u32> = &Some(&0u32);
// good
let x: Option<&u32> = Some(&0u32);

I guess it focuses on the internal code, whereas this issue is about fn signature, but if it is expanded to enums and other places, it might make sense to either split them into multiple lints, or rethink the whole approach?

@nyurik
Copy link
Contributor

nyurik commented Sep 3, 2024

@rustbot claim

@nyurik
Copy link
Contributor

nyurik commented Sep 3, 2024

@Rudxain I implemented rough draft of this in #13336 - but it could use some unit tests. If you have some time, could you provide a few to ensure it works well? Thx!

@Rudxain
Copy link
Contributor Author

Rudxain commented Sep 6, 2024

it could use some unit tests. If you have some time, could you provide a few to ensure it works well? Thx!

Do you mean snippet examples? Or do I have to locally build the fork and test clippy on some "real" code?

If I understood correctly, you mean internal test cases for the functions that the lint depends on. Am I right?

@bors bors closed this as completed in 7b566c2 Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants