Suggest Option<&T> instead of &Option<T>

[nyurik]

closes #13054

// bad code
fn foo(a: &Option<T>) {}
fn bar(&self) -> &Option<T> {}

// Use instead
fn foo(a: Option<&T>) {}
fn bar(&self) -> Option<&T> {}

Handles argument types and return types in functions, methods, and closures with explicit types. Honors avoid_breaking_exported_api parameter.

See this great YouTube video with the in-depth explanation.

Open Questions

These are not blocking, and could be done in separate PRs if needed.

• Should &Option<Box<T>> be suggested as Option<&T> -- without the box? Handled by clippy::borrowed_box
• Should &Option<String> be suggested as Option<&str> -- using de-refed type?

Possible Future Improvements

These cases might also be good to handle, probably in a separate PR.

fn lambdas() {
  let x = |a: &Option<String>| {};
  let x = |a: &Option<String>| -> &Option<String> { todo!() };
}

fn mut_ref_to_ref(a: &mut &Option<u8>) {}

changelog: [ref_option]: Suggest Option<&T> instead of &Option<T>

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
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

[llogiq]

I'd shorten the name to ref_option. Also are you going to work on the TODOs within this PR or do you want me to review it as is and follow up with another PR later?

[nyurik]

@llogiq I would rather finish at least the first two TODOs before merging, but I can already use your help because there are a lot of in-code TODOs that I could use some help with. Thx!

[nyurik]

@llogiq I think I'm done with the changes I wanted to make - now it is all open questions and review :)

[PatchMixolydic]

Should &mut Option<T> parameter be included? (currently yes)

This could break code that mutates the Option directly (eg. by setting it to None, or by overwriting a None with Some(concrete_value)).

[nyurik]

@PatchMixolydic could you give a full example to make sure we evaluate it properly? I am suspecting mut might need to be excluded, but just to ensure we are talking about the same thing

[llogiq]

Example:

let x = &mut Some(42);
*x = None;

[nyurik]

@llogiq thx, but we should discuss it as a full "semi-working" example - i.e. as a function param. Although if you are also certain this should not be checked, i will just exclude it from the lint

[llogiq]

Yes, I am certain this should not be linted. Please exclude it by only linting immutable RPtrs.

[nyurik]

Should both of these cases be ignored?

fn mut_1(a: &mut Option<String>) {}
fn mut_2(a: &mut &Option<String>) {}

[llogiq]

No, only the direct &mut Option. A &mut &T only means that the reference to T might be exchanged, but the object itself cannot be modified through the immutable reference.

[llogiq]

I left some feedback re your TODOs. Hopefully this should give you a clear path forward.

[llogiq]

Suggested change

	/// See [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE)
	/// See this [YouTube video](https://www.youtube.com/watch?v=6c7pZYP_iIE)

[Rudxain]

Adding the video title and author/channel would also be nice, as it would serve as alt text

https://en.wikipedia.org/wiki/Wikipedia:Bare_URLs

[nyurik]

@Rudxain how can you add an alt text to a non-image/video link? I thought the ![Alt text](...) is only for images and such. Please give an example of what you think this markdown should be like. I couldn't find any other code in the rust repo that had alt text like this. Thx!

[jieyouxu]

If possible, you could try to summarize succintly the key reasoning from the YouTube video for why this might enable better optimizations (i.e. it's unlikely people are going to want to watch a youtube video linked from doc comments).

[nyurik]

thanks, updated.

[jieyouxu]

You should also copy-pasta your summary to your PR description, makes it easier to understand why your PR included these changes 😸

[Rudxain]

add an alt text to a non-image/video link?

I'm sorry for not being clear. I meant it as "equivalent to alt" not literally an alt. It doesn't matter how you add the extra information, what matters is the information itself.

I was thinking of a good ol' link: the "name" of the link can be used as an alt:

[This link points to an example web page](https://example.org)

[nyurik]

@flip1995 thx for all the help with this PR on Zullip! @llogiq same goes to you for the feedback here. I think this is ready for another round of reviews. Thx!

[jieyouxu]

Remark: it would be good if we can reference any guarantees that these optimizations are performed for justification of this lint. We should double-check Option<T> representation guarantees by std docs: https://doc.rust-lang.org/std/option/index.html#representation.

[nyurik]

thx, i updated the text with the link

[jieyouxu]

Question: is this always guaranteed, or is it only guaranteed if T: Sized (I haven't looked very hard myself)?

[PatchMixolydic]

(I haven't looked into this too far; please take everything I say with a grain of salt)

Option's documentation in std only guarantees this for &{,mut} impl Sized, but it seems like the Unsafe Code Guidelines Working Group tenatively posit that similar optimizations occur for all values with a niche, which would include &{,mut} impl !Sized (as all references must be nonnull, even if their pointee is !Sized).

Obviously, though, Option<&impl!Thin> cannot be optimized to a thin pointer, and there does not seem to be any guarantee about what the metadata component would be in None::<&impl !Thin>.

[jieyouxu]

Yeah, which is why I asked, because I wasn't sure if this is always the case.

[nyurik]

The real question is if we want to have all this complexity explained in a lint? I agree we should link to the options doc, and perhaps talk about possible performance improvement. Any suggestions on the wording?

An even more important question is: how common is a case where API is deliberately, as oppose to accidentally, designed to take or return &Option<T>? If this is extremely rare, I think it makes sense to promote this to style, at least in the longer term.

[llogiq]

Looks ok to me. Thank you!

@bors r+

[bors]

📌 Commit 10e02cf has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit 10e02cf with merge 7b566c2...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 7b566c2 to master...