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: allow specific functions to be tagged #[must_use] #22348

Closed
wants to merge 2 commits into from

Conversation

huonw
Copy link
Member

@huonw huonw commented Feb 14, 2015

A function marked with #[must_use] behaves as if it returns a
#[must_use] type, no matter what the actual return type is, e.g.

#[must_use}
fn foo() -> i32 { 1 }

foo(); // error: unused result which must be used

The #[must_use] lint is currently restricted to types; types that should
usually not be ignored can be tagged with it to warn the user. This
serves most purposes well, but it doesn't always work, since a
function/method may be just acting as a go-between for a must-use type
and a more general/flexible one, without doing anything
significant (i.e. not really counting as a "use" of the must-use type).

In this case, the function can be marked #[must_use] to ensure that
its own result can be used, and avoid accidental ignoring of must-use
types.

The second commit marks Result::{ok, err} #[must_use] since they're "pure" Result adaptors.

A function marked with `#[must_use]` behaves as if it returns a
`#[must_use]` type, not matter what the actual return type is, e.g.

    #[must_use}
    fn foo() -> i32 { 1 }

    foo(); // error: unused result which must be used

The #[must_use] lint is currently restricted to types; types that should
*usually* not be ignored can be tagged with it to warn the user. This
serves most purposes well, but it doesn't always work, since a
function/method may be just acting as a go-between for a must-use type
and a more general/flexible one, without doing anything
significant (i.e. not really counting as a "use" of the must-use type).

In this case, the function can be marked `#[must_use]` to ensure that
its own result can be used, and avoid accidental ignoring of must-use
types.
These functions are just adapters for `Result`, and something like
`returns_result().ok()` is likely to be accidentally not handling the
possibility of error as intended: `ok` does not *require* that the
variant is `Ok`, and so errors are just silently ignord. Something like
`returns_result().ok().unwrap()` is more likely to be what was
desired. Similarly for `err`.

Users who do wish to just ignore the result can use `let _ =
returns_result();` or `drop(returns_result());` instead of calling
one of these (effectively) no-op methods.
@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@brson
Copy link
Contributor

brson commented Feb 15, 2015

cc @larsbergstrom @Manishearth I think I saw you mentioning this problem with .ok().

This appears prudent to me. Wondering what others think about the expanded scope of must_use (and whether it needs an RFC).

@larsbergstrom
Copy link
Contributor

@brson I'm a huge fan of this, as it would prevent creating 'dummy' types just to #[must_use] them. Some of the work being done on a session types encoding (cc @Munksgaard @laumann) might able to be slightly reformulated, too...

@Manishearth
Copy link
Member

Looks good to me.

I don't think it needs an RFC, however this is technically a breaking change (breaks #[deny(unused_result)])

@Manishearth
Copy link
Member

@larsbergstrom FWIW we'll probably end up doing most of the session types stuff in tree anyway.

If they want to have a discussion on the (practical) capabilities of the plugin system so that they can formulate their research in that way, I don't mind scheduling one :)

@huonw
Copy link
Member Author

huonw commented Feb 15, 2015

I think @alexcrichton implemented #[must_use] originally, and so may have some thoughts.

@alexcrichton
Copy link
Member

This seems like a fine extension to #[must_use] to me, although I personally always tend to favor "keep it super duper simple as long as possible" and would be slightly biased against this. That said this doesn't really complicate the idea of #[must_use] that much, it just means it applies to more things, so it's not that bad.

I feel like adding #[must_use] to ok() and err() is slightly RFC-worthy, but the lint modifications itself aren't necessarily RFC-worthy in my opinion.

@Munksgaard
Copy link
Contributor

I'm not sure if this has any use for our case specifically (session types), but it would definitely add a lot of granularity to the must_use system, and it seems like a natural extension to the current system.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 16, 2015

.ok() is currently the way to explicitly ignore the return value. This breaks stable functions so its clearly not just "slightly" RFC worthy.

@Manishearth
Copy link
Member

It doesn't break anything since this is a warning (though I did mention that putting it on deny would make it a slightly breaking change)

@mahkoh
Copy link
Contributor

mahkoh commented Feb 16, 2015

It doesn't break anything since this is a warning (though I did mention that putting it on deny would make it a slightly breaking change)

You claim that it doesn't break anything and then mention a case where it breaks something?

@Manishearth
Copy link
Member

I mean it doesn't break other stable functions as you said

@mahkoh
Copy link
Contributor

mahkoh commented Feb 16, 2015

#![deny(unused_must_use)]

fn f() -> Result<(), ()> {
    Ok(())
}

fn main() {
    f().ok();
}

A program using only stable features that this breaks.

@Manishearth
Copy link
Member

"stable functions". I agree with that.

@tomjakubowski
Copy link
Contributor

.ok() is currently the way to explicitly ignore the return value.

Says who? The most I can say is that it was a way to do this.

@huonw
Copy link
Member Author

huonw commented Feb 17, 2015

There is exactly two obvious uses of ok to ignore the return value in the whole rust-lang/rust codebase, in a test and its seems like they could really be .unwrap() anyway (the regexes below find instances of .ok(); where there's no = before it on the line).

$ git grep -C 3 '\.ok();' | grep 'rs:[0-9]\+:[^=]*.ok();' -C 3
src/test/bench/rt-messaging-ping-pong.rs-51-            }
src/test/bench/rt-messaging-ping-pong.rs-52-        });
src/test/bench/rt-messaging-ping-pong.rs-53-
src/test/bench/rt-messaging-ping-pong.rs:54:        guard_a.join().ok();
src/test/bench/rt-messaging-ping-pong.rs:55:        guard_b.join().ok();
src/test/bench/rt-messaging-ping-pong.rs-56-    }
src/test/bench/rt-messaging-ping-pong.rs-57-
src/test/bench/rt-messaging-ping-pong.rs-58-    for _ in 0..m {

ok is definitely not the way to ignore the return value. let _ = ...; has nearly 400 uses in rust-lang/rust (git grep 'let _ =' | wc -l), from a quick glance many of these are definitely ignoring results (but obviously not all; either way, orders of magnitude more than .ok()).

@huonw
Copy link
Member Author

huonw commented Feb 17, 2015

servo/servo has 4 uses of ok() for ignoring data (although, there's every possibility they should be unwrap instead), and 61 uses of let _ = (essentially all of which appear to be ignoring Results, rather than something else).

@mahkoh
Copy link
Contributor

mahkoh commented Feb 17, 2015

From what I recall, the original reason not to use let _ had something to do with questionable drop semantics. Either way, I believe the need for an RFC has been demonstrated.

@huonw
Copy link
Member Author

huonw commented Feb 17, 2015

Additionally, we're still in alpha, so breaking-in-edge-cases changes like this are fine.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 17, 2015

Breaking #[stable] functions in the prelude is now fine because ...? Just because you don't use it doesn't mean that it's an "edge-case", whatever that means.

@Manishearth
Copy link
Member

It isn't "now fine", it was always fine. In alpha, breaking changes will happen even to stable things and things not behind a feature gate. In beta, there should be very few, and in release, none unless there is some major unavoidable bug.

FWIW Servo does use .ok() for ignoring the return values a lot, but that is just because of the send()/recv() changes -- we usually make quick fixes during rustups and patch them up later. Ideally, these should be moved to .ok().expect() or let _ = once audited.

@huonw
Copy link
Member Author

huonw commented Feb 17, 2015

Conversely, something can still be an edge case even if someone personally uses it a lot. In this case, it's not just me that doesn't use it.

And yes, there's been a regular stream of breaking changes for prelude functionality in the alpha.

(@Manishearth for the current HEAD 4ab928728e3d65ac4c6ca72cd6c8aa0c75fde33b, there are 4 and 55 instances of .ok(); and let _ = respectively, which doesn't seem like a lot).

@Manishearth
Copy link
Member

Ah, nice. They got reduced :)

@mahkoh
Copy link
Contributor

mahkoh commented Feb 17, 2015

Currently our policy is that any modifications to the prelude need to have a corresponding RFC.

#18067 (comment)

@Manishearth
Copy link
Member

The context of that is that you're adding something to the prelude. Major changes of functionality, or introduction/removal of things from the prelude require an RfC. Something that might break a function used in the prelude if used in a certain way ... eh, not really.

Besides, IIRC lints are informally considered unstable for now until we figure out a stability story for them (there's a bug on this somewhere)

@mahkoh
Copy link
Contributor

mahkoh commented Feb 17, 2015

How convenient for you.

@Manishearth
Copy link
Member

#21761

@huonw
Copy link
Member Author

huonw commented Mar 5, 2015

(Closing in favour of the RFC, rust-lang/rfcs#886; to keep the queue nice.)

@huonw huonw closed this Mar 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants