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

Add core::hint::must_use #94723

Merged
merged 1 commit into from
Mar 9, 2022
Merged

Add core::hint::must_use #94723

merged 1 commit into from
Mar 9, 2022

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Mar 8, 2022

The example code in this documentation is minimized from a real-world situation in the anyhow crate where this function would have been valuable.

Having this provided by the standard library is especially useful for proc macros, even more than for macro_rules. That's because proc macro crates aren't allowed to export anything other than macros, so they couldn't make their own must_use function for their macro-generated code to call.


Rendered documentation

An identity function that causes an unused_must_use warning to be triggered if the given value is not used (returned, stored in a variable, etc) by the caller.

This is primarily intended for use in macro-generated code, in which a #[must_use] attribute either on a type or a function would not be convenient.

Example

#![feature(hint_must_use)]

use core::fmt;

pub struct Error(/* ... */);

#[macro_export]
macro_rules! make_error {
    ($($args:expr),*) => {
        core::hint::must_use({
            let error = $crate::make_error(core::format_args!($($args),*));
            error
        })
    };
}

// Implementation detail of make_error! macro.
#[doc(hidden)]
pub fn make_error(args: fmt::Arguments<'_>) -> Error {
    Error(/* ... */)
}

fn demo() -> Option<Error> {
    if true {
        // Oops, meant to write `return Some(make_error!("..."));`
        Some(make_error!("..."));
    }
    None
}

In the above example, we'd like an unused_must_use lint to apply to the value created by make_error!. However, neither #[must_use] on a struct nor #[must_use] on a function is appropriate here, so the macro expands using core::hint::must_use instead.

  • We wouldn't want #[must_use] on the struct Error because that would make the following unproblematic code trigger a warning:

    fn f(arg: &str) -> Result<(), Error>
    
    #[test]
    fn t() {
        // Assert that `f` returns error if passed an empty string.
        // A value of type `Error` is unused here but that's not a problem.
        f("").unwrap_err();
    }
  • Using #[must_use] on fn make_error can't help because the return value is used, as the right-hand side of a let statement. The let statement looks useless but is in fact necessary for ensuring that temporaries within the format_args expansion are not kept alive past the creation of the Error, as keeping them alive past that point can cause autotrait issues in async code:

    async fn f() {
        // Using `let` inside the make_error expansion causes temporaries like
        // `unsync()` to drop at the semicolon of that `let` statement, which
        // is prior to the await point. They would otherwise stay around until
        // the semicolon on *this* statement, which is after the await point,
        // and the enclosing Future would not implement Send.
        log(make_error!("look: {:p}", unsync())).await;
    }
    
    async fn log(error: Error) {/* ... */}
    
    // Returns something without a Sync impl.
    fn unsync() -> *const () {
        0 as *const ()
    }

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2022
@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 8, 2022
@Mark-Simulacrum
Copy link
Member

This seems pretty elegant, but I suppose there are a couple language-altering alternatives that spring to mind:

  • #[must_use] on macro_rules! (perhaps on each arm, or on the whole macro), perhaps with a lint or error if the macro does not produce an expression. Likely pretty complicated, not really worthwhile.
  • #[must_use] on an arbitrary block:
#[must_use]
{
    some_expr
}

I feel like the second option in particular feels somewhat nice, though it's definitely more work to implement and may have design problems I'm missing. It feels a little more "native" in some sense, and might make a slight difference in some subtle edge cases -- I'm not sure whether for example type inference will always 'see through' the identity function, whereas through just a block it might be easier to do so. But I don't have immediate cases that I can think of.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Mar 8, 2022

That said, I would be totally fine adding this as unstable (r=me), if you want to file a tracking issue for it (maybe with my options as "questions", or feel free to r? a libs-api member if you want another pair of eyes from the team proper.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2022
@dtolnay
Copy link
Member Author

dtolnay commented Mar 8, 2022

Added tracking issue: #94745.

#[must_use] on blocks would be better than this function if it's doable. Let's definitely reevaluate before stabilizing. I recorded the alternatives in the tracking issue.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Mar 8, 2022

📌 Commit b2473e9 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#94689 (Use impl substs in `#[rustc_on_unimplemented]`)
 - rust-lang#94714 (Enable `close_read_wakes_up` test on Windows)
 - rust-lang#94723 (Add core::hint::must_use)
 - rust-lang#94724 (unix: Avoid name conversions in `remove_dir_all_recursive`)
 - rust-lang#94730 (Reverted atomic_mut_ptr feature removal causing compilation break)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ff54e34 into rust-lang:master Mar 9, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 9, 2022
@dtolnay dtolnay deleted the mustuse branch March 9, 2022 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants