uom icon indicating copy to clipboard operation
uom copied to clipboard

Add [must_use = ...] attributes where appropriate

Open iliekturtles opened this issue 2 years ago • 4 comments

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.

iliekturtles avatar Mar 24 '22 12:03 iliekturtles

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?

gonzaponte avatar Apr 07 '22 19:04 gonzaponte

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.

iliekturtles avatar Apr 07 '22 19:04 iliekturtles

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.

gonzaponte avatar Apr 08 '22 09:04 gonzaponte

Awesome, that would be great!

iliekturtles avatar Apr 08 '22 12:04 iliekturtles