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 [must_use = ...] attributes where appropriate #292

Open
iliekturtles opened this issue Mar 24, 2022 · 4 comments
Open

Add [must_use = ...] attributes where appropriate #292

iliekturtles opened this issue Mar 24, 2022 · 4 comments

Comments

@iliekturtles
Copy link
Owner

Since uom was first created the [must_use = ...] was stabilized in Version 1.27.0 (2018-06-21)! Review all uom functions and add the attribute if necessary.

@gonzaponte
Copy link
Contributor

I was looking into this and noticed that the must_use attribute doesn't make a huge difference because the #![warn(unused_results)] lint is already in place. The diff for a given function is the following:

without must_use

warning: unused result of type ...
[...]
note: the lint level is defined here
   --> src/lib.rs:174:5
    |
174 |     unused_results
    |     ^^^^^^^^^^^^^^

vs with must_use = <message>

warning: unused return value of ... that must be used
[...]
    = note: `#[warn(unused_must_use)]` on by default
    = note: <message>

What would be the preferred solution, removing the lint + adding must_use everywhere, keeping both, or forgetting about must_use altogether?

@iliekturtles
Copy link
Owner Author

Thanks for taking a look at this! The reason for the must_use attributes isn't for function calls within uom but external crates calling uom's functions.

For example the si example compiles and runs without errors:

~/Source/uom (master *$)$ cargo run --example si
   Compiling uom v0.32.0 (~\Source\uom)
    Finished dev [unoptimized + debuginfo] target(s) in 1.29s
     Running `target\debug\examples\si.exe`
15 m + 9.999999 cm = 15.1 m
...

However adding a call to abs without capturing the return value and a must_use attribute to abs and re-running produces a warning:

diff --git a/examples/si.rs b/examples/si.rs
index 7a5bd0f..1bd48f2 100644
--- a/examples/si.rs
+++ b/examples/si.rs
@@ -19,6 +19,8 @@ fn main() {
     let cm = Length::format_args(centimeter, Abbreviation);
     let s = Time::format_args(second, Abbreviation);
 
+    l1.abs();
+
     // Print results of simple formulas using different output units.
     println!("{} + {} = {}", m.with(l1), cm.with(l2), m.with(l1 + l2));
     println!(
diff --git a/src/system.rs b/src/system.rs
index 6324553..1ed1ee1 100644
--- a/src/system.rs
+++ b/src/system.rs
@@ -718,6 +718,7 @@ macro_rules! system {
             /// Computes the absolute value of `self`. Returns `NAN` if the quantity is
             /// `NAN`.
             #[inline(always)]
+            #[must_use = "method returns a new number and does not mutate the original value"] 
             pub fn abs(self) -> Self
             where
                 V: $crate::num::Signed,
~/Source/uom (master *$)$ cargo run --example si
   Compiling uom v0.32.0 (~\Source\uom)
warning: unused return value of `Quantity::<D, U, V>::abs` that must be used
  --> examples\si.rs:22:5
   |
22 |     l1.abs();
   |     ^^^^^^^^^
   |
   = note: `#[warn(unused_must_use)]` on by default
   = note: method returns a new number and does not mutate the original value

warning: `uom` (example "si") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 37.43s
     Running `target\debug\examples\si.exe`
15 m + 9.999999 cm = 15.1 m
...

Essentially we're looking to review all public functions and determine if a must_use attribute should be added. Most functions are just thin wrappers on top of a function from the std library and the inner function can be reviewed to see if it has a must_use attribute.

@gonzaponte
Copy link
Contributor

Ah, right, thanks!

I can dedicate some time to this, but scattered over a few weeks. If that's an acceptable time scale, I'll do it.

@iliekturtles
Copy link
Owner Author

Awesome, that would be great!

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

No branches or pull requests

2 participants