Tracking issue for RFC 1940: allow `#[must_use]` on functions

[aturon]

This is a tracking issue for the RFC "allow #[must_use] on functions" (rust-lang/rfcs#1940).

Steps:

• [x] Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
• [x] Adjust documentation (see instructions on forge)
• [x] Stabilization PR (see instructions on forge)

[zackmdavis]

I think I should have an implementation shortly.

[crumblingstatue]

I'm just adding a note here that the current implementation of #1940 does not lint for unused ==, which was the main motivation for it.

In fact, the RFC itself states in the motivation

By marking PartialEq #[must_use] the compiler would complain about things like:

   modem_reset_flag == false; //warning
   modem_reset_flag = false; //ok

[zackmdavis]

Yeah, that's a little awkward. I can add the comparison operators in a follow-on PR.

[eddyb]

Also, this issue shouldn't have been closed. Ahhh I may have not checked for a feature gate in the implementation. I'm a bad reviewer.

[zackmdavis]

I have an implementation for linting == and the other comparison operators. (It's based off the feature-gating work, so we'll let that land first.)

[nikomatsakis]

Hi @zackmdavis / @eddyb -- so I've not had time to check on the PRs, but I wanted to make a request. In preparation for the decision to stabilize, would it be possible for you to collect the various tests into a subdirectory (e.g., src/test/ui/rfc_1940_must_use_on_functions/) for easier review? I've found that when deciding whether or not to stabilize, being able to quickly survey how the compiler currently behaves on a bunch of examples is the best way to judge the current state of the feature (and, of course, the number of tests can also give you an idea of how well vetted the implementation is). If tests have already been collected, my apologies!

Either way, can you add a link somewhere (I can copy it into the header of the issue).

Thanks. ❤️

[nikomatsakis]

How are people feeling about this? Shall we consider stabilization? Are we using this for the various iterator methods and other purposes that it was envisioned for?

[kennytm]

It is currently applied on methods in PartialOrd and PartialEq only :S

[nikomatsakis]

@kennytm was there a plan to use it elsewhere? I forget. If so, is there any reason we are not doing so?

[kennytm]

@nikomatsakis There was a plan to add #[must_use] to Result::ok rust-lang/rfcs#886.

rust-lang/rfcs#1940 only suggests applying to eq() and friends (which turns out isn't enough to fulfill the RFC's motivation).

I don't think there were any plans to use it on iterator methods coming from these two RFCs. IIRC there's some discussion to put #[must_use] on collect() to avoid newbies using it to consume the whole iterator.

[Ixrec]

I definitely wouldn't stabilize this if it's not working for the == operator yet, but https://github.com/rust-lang/rust/pull/44103 claims to make it work for == and the playground agrees that it's working, so the RFC's primary motivation seems fulfilled. I don't think we need to block stabilization on deciding what iterator methods should also have it.

[scottmcm]

It looks like linting of { x == y; } is also feature gated right now. Can we at least stabilize that?

[oli-obk]

ping

this seems like it's ready to get stabilized

[zackmdavis]

Yes, this feature is very straightforward; the only thing I'd want to consider before stabilization is whether we should add #[must_use] to any other standard library methods (like .collect()).

(And even then, the only scenarios in which that makes a difference are ones in which we decide to make more functions must-use without stabilizing, and then nightly users point out that we were wrong to do so, but the instability lets us revert without breaking any deny-warnings stable code.)

Give me a few days to find some time to look through the standard library for any other candidate must-use functions, and I'll report back with comments and/or a stabilization PR for 1.25? (Looks like we have until 15 February before the next train leaves.)

[zackmdavis]

(Of course, it would also be possible to immediately stabilize just the often-demanded operator lints—which are only under the fn_must_use feature gate due to the historical contingency of RFC discussion not sufficiently forseeing implementation details—but continue to evaluate must-use for functions. However, I agree with the point that came up at least a couple times in the Rust2018 discussions that we're sitting on too many unshipped features: ungating all the fn_must_use functionality in 1.25 seems reasonable.)

[zackmdavis]

There's a perspective that says everything in the standard library that has no side effects that doesn't already return a must-use type (like Result) should be must-use. But that feels like a weirdly excessive number of attributes (at least dozens, likely hundreds), right?

[scottmcm]

As it's a lint, I'm in favour of just stabilizing and worrying about which functions get it later.

That said, I'd also like to put this on things like clone. And maybe things like to_string, since I saw someone on IRC do x.to_string(); and wonder why x wasn't now a String...

[nikomatsakis]

I agree we should stabilize. I have no strong opinion about which functions get the lint but I feel like it's ok if it's a lot, if that is useful. =)

@zackmdavis would you be up for preparing a stabilization report? Basically, include a brief summary of what the feature does, and what tests exist for each part of that (so we can verify that it is working correctly). Special attention to edge cases that came up during implementation and how they are being tested or what was decided about them (particularly if it deviates from or was not mentioned in the RFC).

The audience would be people who have been following along, so it doesn't have to explain the feature from first principles -- i.e., no need to delve into motivation per se. (Though a bit more never hurts, e.g. for people coming in from This Week In Rust)

[zackmdavis]

(I regret the delay; I'm penciling in some time Saturday to put together a stabilization PR.)

[Havvy]

As it's been exactly four months, what's the status on this?

[zackmdavis]

The status is that I haven't forgotten that I said I was going to put together a stabilization PR; I'm just terrible at managing my time. :cry: (And writing new code is more fun than stabilizing existing code.)

The psychological pressure induced by having been caught breaking my word is probably enough to get me to do it tonight (Pacific time).

[zackmdavis]

So, this is unexpected—and really the sort of thing that we would hope to find out before trying to put together the stabilization PR—but we have a serious bug. Building the getopts crate with the fn-must-use code (either after ripping out the feature gate, or with the feature attribute inserted) panics (!) when qpath_def gives us a Def::Local for this line and we try to take its DefId (which isn't a real thing). I haven't found a comprehensibly reduced test case yet.

[zackmdavis]

Minimized:

#![feature(fn_must_use)]

fn main() {
    let c = || { true };
    c();
}

yields

error: internal compiler error: librustc/hir/def.rs:178: attempted .def_id() on invalid def: Local(NodeId(8))

Sadly, this does make me wonder how effective the "let nightly users find bugs in unstable features" strategy is, if so few people have kicked the tires on #[feature(fn_must_use)] that no one noticed that it (not even using the functionality, just enabling the flag!) breaks programs containing closures that return a non-unit value.

[zackmdavis]

stabilization PR is #48925; separate issue #48926 created for the question of #[must_use] elsewhere in the standard library

[nikomatsakis]

@zackmdavis do you think you can prepare a "stabilization issue" (or just re-use this issue, if there are no further "pieces" to consider stabilizing). It should look something like this:

https://github.com/rust-lang/rust/issues/48453

That is, summarize briefly what is going on, give a few example tests showing the feature in use in various representative ways (and also highlight any changes that have happened since the RFC). Then I can move to FCP.

[zackmdavis]

@nikomatsakis

It it proposed that we stabilize the code currently under the fn_must_use feature gate.

This commits us to the following behaviors:

• A #[must_use] attribute on a function or method will cause the unused-must-use lint to fire when the return value of that function or method is ignored, just (up to the wording of the lint message) like the unused-must-use lint currently (on stable Rust) fires on ignored values whose type has the #[must_use] attribute.

  • As for must-use types (the codepath is the same), the attribute can take an optional string literal, which is added to the message. (UI test source, output)
  • For trait methods, the attribute must appear on the method signature in the trait definition; annotating a trait method implementation is a no-op.
  • Examples of usage or desired usage in the ecosystem:
    • matrix operations in Servo's geometry library
    • for C types: 1 2
    • possibly documenting memory leakage scenarios?

• Unused values of comparison operators (<, >, <=, >= and ==) are likewise linted.

Changes from the authorizing RFC 1940:

• Many RFC discussants seemed to expect that the must-use operator lints would automatically fall out of must-use function lints and marking the comparison trait methods as unused, but the implementation turned out to not work that way. The linting code handles the operators separately.

Status changes since implementation (#43728, #43776, #44103):

• The code on master at time of writing actually has a serious bug: if the fn_must_use feature is on, programs containing closures that return with non-unit values will ICE (!!).
  • But this is easy to fix. The patch is included in the stabilization PR #48925, which is blocked on our final-comment-period here.

Documentation status:

• An unstable book section exists (proposed to be deleted in the stabilization PR, but the text could be cannabalized for other docs).

• Thanks to @Havvy, there's an open PR for the Reference pending our decision here.

Unresolved questions and issues (which need not necessarily block stabilization):

• Do we want to add #[must_use] attributes to functions and methods in the standard libary? (Currently, only the comparison methods .gt, .lt, &c. have the attribute.) If we stabilize now, future #[must_use] attributes would be insta-stable. Dedicated issue: #48926

  • Discussed possible uses:
    • String.replace (#48423)
    • .collect (discussion, this issue)
    • Result.ok and Result.err (an explicit motivator of the unapproved predecessor RFC #886)

• Should the compiler warn when #[must_use] appears on a trait implementation, rather than the declaration? (This is currently a no-op, but users might expect it to work and be confused when it doesn't.) (discussion on #48486)

• Maybe the further explanation from the optional string literal that the attribute can take would look better æsthetically and perhaps be more visible in a note rather than being appended to main lint message?

warning: unused return value of `foo` which must be used: further info
  --> $DIR/fn_must_use.rs:67:5
   |
LL |     foo();
   |     ^^^^^^

vs.

warning: unused return value of `foo` which must be used
  --> $DIR/fn_must_use.rs:67:5
   |
LL |     foo();
   |     ^^^^^^
   |
note: further info

[SimonSapin]

Should the compiler warn when #[must_use] appears on a trait implementation, rather than the declaration? (This is currently a no-op, but users might expect it to work and be confused when it doesn't.)

Can we make this attribute a compilation error when used in places where it would be a no-op? This leaves the option of making it work later without changing the meaning of programs that compile.

[nikomatsakis]

@rfcbot fcp merge

I propose that we stabilize must-use lints. @zackmdavis has prepared an excellent summary here.

However, I am also in favor of @SimonSapin's change to report an error if #[must_use] is attached to a method in a trait impl (rather than having that be a no-op), and I'd prefer not to stabilize until we agree to that change as well (though this could be added to the stabilization PR).

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

• [x] @aturon
• [x] @cramertj
• [x] @eddyb
• [x] @nikomatsakis
• [x] @nrc
• [x] @pnkfelix
• [ ] @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[golddranks]

Is there any issue that tracks which std APIs are going to have this annotation? I just shot myself into foot with Write::write() – I checked the Result with ?, but ignored the amount of the written bytes it returned. I should have used write_all(); if it warned me about ignoring the return value, I would have noticed it and it would have saved me from a serious bug.

Another thought: is the annotation going to work with APIs that return Result, but where even the happy path value needs not to be ignored? (I.e. write() here) I would imagine that it would be quite possible to do this with calls like write()?, but the general case with all kinds of combinators before returning the error is much much harder.

[oli-obk]

Clippy has a correctness lint for that: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_io_amount

I don't think it can be caught by must_use, but a targeted lint can.

[zackmdavis]

Is there any issue that tracks which std APIs are going to have this annotation?

Please, contribute your thoughts on #48926.

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[rfcbot]

The final comment period is now complete.

[TimNN]

@nikomatsakis: It looks like it was decided to stabilize this, can I assume that #48925 is now un-blocked?

[nikomatsakis]

@TimNN yep

[zackmdavis]

@SimonSapin @nikomatsakis

Can we make this attribute a compilation error when used in places where it would be a no-op? This leaves the option of making it work later without changing the meaning of programs that compile.

I'm confused about how exactly how literally we take our stability guarantee. #[must_use] on trait-impls (and other non-ADT locations) is already a no-op on stable; are we allowed to break programs with such attributes by making it an error?

This seems like it ought to be sheerly hypothetical—why would anyone have already written a program with such no-op attributes? But the fn_must_use feature gate ended up emitting a warning rather an error (which took some extra work) for precisely this reason, which warnings turned out to reveal at least one crate in the wild that was trying to use the #[must_use] attributes even though they were no-oping.

[SimonSapin]

#[must_use] on trait-impls (and other non-ADT locations) is already a no-op on stable

This is the situation I was hoping to prevent, I didn’t realize it was already the case.

[Havvy]

We can make it a hard error on the next edition then.

[Havvy]

@aturon The stabilization PR has already gone through. I'm not sure if all the necessary docs are updated yet, although the reference has been.

[Centril]

@Havvy is there anything left here to do?

[Havvy]

I don't know. It's documented in the reference and there's an issue for the API guidelines book. But I don't think it's talked about in TRPL and definitely untouched in Rust By Example, if it should even be in there.

[Centril]

Closing as completed.