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

#[must_use] is missing on many methods #1609

Closed
0xForerunner opened this issue Feb 10, 2023 · 4 comments · Fixed by #1678
Closed

#[must_use] is missing on many methods #1609

0xForerunner opened this issue Feb 10, 2023 · 4 comments · Fixed by #1678
Assignees
Milestone

Comments

@0xForerunner
Copy link

#[must_use] tells the compiler that the output of a particular function must be used, and that not using it is a bug. A perfect example of where this belongs is:

    // This is a great place for #[must_use]
    pub fn saturating_sub(self, other: Self) -> Self {
        Self(self.0.saturating_sub(other.0))
    }

This rust code is currently valid and gives no warnings

let a = Uint256::one();
let mut b = Uint256::from(2u128);
b += a;
b.saturating_sub(a);
println("{}", b.to_string());

A user might wrongly assume that saturating_sub is modifying b, when it is not. (Ask me how I know haha). This fix should be pretty easy to implement!

@0xForerunner
Copy link
Author

We wasted 8 hours searching for a bug caused by this in my code base today haha. Sad times.

@webmaster128
Copy link
Member

Good idea. The standard library does this too:

Bildschirm­foto 2023-03-02 um 11 34 01

Do you mind adding such a block to the relevant methods in a PR?

        #[must_use = "this returns the result of the operation, \
                      without modifying the original"]

@webmaster128
Copy link
Member

Same for Timestamp::plus_seconds, ::plus_nanos, ::minus_seconds, ::minus_nanos

@webmaster128
Copy link
Member

Done in #1678 on the 1.2 branch

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 a pull request may close this issue.

3 participants