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

Make Option<T> #[must_use] if T is #71368

Open
ctiller opened this issue Apr 20, 2020 · 10 comments
Open

Make Option<T> #[must_use] if T is #71368

ctiller opened this issue Apr 20, 2020 · 10 comments
Labels
A-async-await AsyncAwait-Triaged P-medium T-lang

Comments

@ctiller
Copy link

@ctiller ctiller commented Apr 20, 2020

I was recently refactoring some code that originally looked like this (minimized):

fn foo() {}
async fn bar() {
  Some(foo());
}

bar() was far away from the definition of foo().
I had need to refactor the code so that foo was also async, and so now I had:

async fn foo() {}
async fn bar() {
  Some(foo());
}

This gives no compiler diagnostic and instead the body of foo is not executed.

@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Apr 20, 2020

Why do you create an option and do nothing with it?

@ctiller
Copy link
Author

@ctiller ctiller commented Apr 20, 2020

did I minimize this example too far?... actual code was closer to:

some_option.map(|value| foo(value))

@tmandry
Copy link
Contributor

@tmandry tmandry commented Apr 20, 2020

The issue is that Option<T> is not #[must_use] even when T is. We can't change this backwards compatibly, but perhaps adding a warning is possible.

@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Apr 20, 2020

Also see #67387

(#[must_use] is just a warning, so it can be added without breaking code)

@tmandry tmandry added A-async-await T-lang labels Apr 20, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 21, 2020

Nominating for @rust-lang/lang consideration. This is another case where extending #[must_use] makes sense, I think -- in particular, an Option<impl Future> should warn when it is unused, just as a impl Future does.

I was pondering why it makes sense, and I think the reason is pretty clear -- an Option contains its T in a public field and doesn't in any sense try to "abstract over it" or "encapsulate it" (which were some of the reasons that we were reluctant to extend #[must_use] uniformly across all types).

@tmandry tmandry added this to On deck in wg-async work via automation Apr 21, 2020
@tmandry tmandry added the P-medium label Apr 21, 2020
@tmandry
Copy link
Contributor

@tmandry tmandry commented Apr 21, 2020

From wg-async-foundations triage: marking On Deck, because forgetting to await is a fairly common thing, and part of the learnability story for async/await is making sure you don't do that.

@tmandry tmandry added the AsyncAwait-Triaged label Apr 21, 2020
@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Apr 23, 2020

How feasible would it be to detect taking a #[must_use] value and putting it in a value that subsequently never gets touched? That might give far less false positives than trying to make Option<impl Future> a #[must_use] type.

@ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Apr 23, 2020

What about making the enum variant constructors themselves #[must_use]? In this case that would be Option::Some (the function that returns an Option, not the variant itself).

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 29, 2020

Marking the variant constructor must_use seems like it would have too much fallout, since it would mean that code like Ok(22); would warn as well, wouldn't it?

Also, I think we might well want to warn on code like the following:

fn foo() -> Option<impl Future> { ... }

fn bar() {
   foo();
}

I'm not quite I understand what @joshtriplett is proposing. It sounds like saying "if you construct a enum or struct variant that contains a #[must_use] value, and that struct is unused". I'm reluctant to do that, because it implies that (a) it would apply to all enum/struct types, or at least "most", and we've historically felt that is too broad (which I still think is true), and also (b) it doesn't address cases like foo above, although I think those cases are a bit more borderline than the original example.

I'd like to ask: what is the resistance to making Option<T> be "must-use" if T is must-use? I guess it would be worth doing a bit of a crater run to see the effects before making a final decision, since it may be that cases like None; raise a bunch of false positives, but in general it seems ok to extend "must-use" to wrapper types like Option that don't "encapsulate" their contents.

@tmandry tmandry moved this from On deck to In progress in wg-async work May 5, 2020
@tmandry tmandry moved this from In progress to Blocked in wg-async work May 12, 2020
@jonas-schievink jonas-schievink removed their assignment Jun 6, 2020
@tmandry tmandry moved this from Blocked to On deck in wg-async work Jun 16, 2020
@tmandry tmandry removed this from On deck in wg-async work Jun 16, 2020
@gebressler
Copy link

@gebressler gebressler commented Mar 17, 2021

I support making Option<T>``#[must_use] if T is.

In https://fuchsia-review.googlesource.com/c/fuchsia/+/501960/9/src/sys/component_manager/src/capability.rs#173, I wanted to have a trait method returning a Result of Option:

trait Foo {
    fn bar(&self) -> Result<Option<MustUseType>, Error>;
}

I wanted Option<MustUseType> to be #[must_use]. Result itself is #[must_use], but this doesn't help because that only applies to the Result, which is "used" as soon as the ? operator is invoked on it. If this fn were directly returning Option<MustUseType>, I could have marked the fn #[must_use], but I need to be able to return an error. In the end I resorted to creating a custom wrapper type for Option<MustUseType>.

@tmandry tmandry changed the title Async execution can get lost during refactoring Make Option<T> #[must_use] if T is Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await AsyncAwait-Triaged P-medium T-lang
Projects
None yet
Development

No branches or pull requests

7 participants