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 attribute #306

Merged
merged 6 commits into from
Jun 7, 2022
Merged

Conversation

gonzaponte
Copy link
Contributor

First draft for #292.

I've included an example file at the beginning that intentionally calls every function in the interface and leaves the output unused. The file contains 40 checks. At that point, the compiler does not complain. After the last commit, 40 warnings are thrown. This file is not meant to go through, but I found it useful for demonstration purposes.

I've split the commits into 3 (somewhat arbitrary) categories. You may want to disregard some of them.

@iliekturtles
Copy link
Owner

Thanks for the PR! I kicked off the CI builds but it looks like everything is failing because of unused must_use in mustuse.rs. Surprisingly, clippy::must_use_candidate and clippy::return_self_not_must_use are allow by default. I think these should be added to the default lints:

diff --git a/src/lib.rs b/src/lib.rs
index be430b4..23bea1e 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -171,16 +171,23 @@
     unused_extern_crates,
     unused_import_braces,
     unused_qualifications,
-    unused_results
+    unused_results,
 )]
 // Clippy lints.
+#![cfg_attr(
+    feature = "cargo-clippy",
+    warn(
+        clippy::must_use_candidate,
+        clippy::return_self_not_must_use,
+    )
+)]
 #![cfg_attr(
     feature = "cargo-clippy",
     allow(
         clippy::deprecated_cfg_attr,
         clippy::excessive_precision,
         clippy::inconsistent_digit_grouping, // https://github.com/rust-lang/rust-clippy/issues/6096
-        clippy::inline_always
+        clippy::inline_always,
     )
 )]
 // Lints allowed in tests because they are unavoidable in the generic code when a type may or may

@gonzaponte
Copy link
Contributor Author

I've added a commit with the lints, but the failures are caused by RUSTFLAGS: -D warnings in the GHA config. The file mustuse.rs aimed to demonstrate that unused values throw a warning for every unused value, so with this configuration, the GHA will always fail. (By the way, the GHA raises the expected 40 errors). I've added also a revert commit removing the file so the GHA passes but you might want to checkout the previous commit and compile it locally to see the warnings. Let me know if you would like to proceed differently.

@iliekturtles
Copy link
Owner

I apologize for the delay in getting to this PR. I kicked off the build again with your latest changes and wanted to let you know it's next on my list to review and get merged.

@iliekturtles
Copy link
Owner

Could you rebase on top of master to resolve the current merge conflicts and add attributes for the few remaining spots that clippy calls out? I think we can merge after that! The diff below should cover the Clippy issues.

 src/lib.rs      | 4 ++++
 src/quantity.rs | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/src/lib.rs b/src/lib.rs
index daa591a..7a4c4ef 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -429,6 +429,7 @@ pub trait Conversion<V> {
     /// converting the given unit to the base unit for the quantity: `(value * coefficient()) +
     /// constant()`. Implementation should return the multiplicative identity (`Self::T::one()`) if
     /// no coefficient exists.
+    #[must_use = "method returns a new number and does not mutate the original value"]
     #[inline(always)]
     fn coefficient() -> Self::T {
         <Self::T as crate::num::One>::one()
@@ -439,6 +440,7 @@ pub trait Conversion<V> {
     /// constant()`. Implementation should return the additive identity (`Self::T::zero()`) if no
     /// constant exists. See [ConstantOp](enum.ConstantOp.html) documentation for details about
     /// parameter use to ensure the method optimizes correctly.
+    #[must_use = "method returns a new number and does not mutate the original value"]
     #[inline(always)]
     #[allow(unused_variables)]
     fn constant(op: ConstantOp) -> Self::T {
@@ -475,9 +477,11 @@ pub trait ConversionFactor<V>:
     + crate::num::One
 {
     /// Raises a `ConversionFactor<V>` to an integer power.
+    #[must_use = "method returns a new number and does not mutate the original value"]
     fn powi(self, e: i32) -> Self;
 
     /// Converts a `ConversionFactor<V>` into its underlying storage type.
+    #[must_use = "method returns a new number and does not mutate the original value"]
     fn value(self) -> V;
 }
 
diff --git a/src/quantity.rs b/src/quantity.rs
index dfb5bac..e9ba92a 100644
--- a/src/quantity.rs
+++ b/src/quantity.rs
@@ -166,6 +166,7 @@ macro_rules! quantity {
 
         impl Units {
             /// Unit abbreviation.
+            #[must_use = "method returns a static value"]
             #[allow(dead_code)]
             pub fn abbreviation(&self) -> &'static str {
                 match self {
@@ -176,6 +177,7 @@ macro_rules! quantity {
             }
 
             /// Unit singular description.
+            #[must_use = "method returns a static value"]
             #[allow(dead_code)]
             pub fn singular(&self) -> &'static str {
                 match self {
@@ -186,6 +188,7 @@ macro_rules! quantity {
             }
 
             /// Unit plural description.
+            #[must_use = "method returns a static value"]
             #[allow(dead_code)]
             pub fn plural(&self) -> &'static str {
                 match self {

@gonzaponte
Copy link
Contributor Author

Rebased. I've included your requested changes in the original commits.

@iliekturtles iliekturtles merged commit 5bd0c3c into iliekturtles:master Jun 7, 2022
@iliekturtles
Copy link
Owner

Thanks so much for the PR, and for being patient with my limited time. I've accepted the PR! While the rustfmt step is failing I'll push another commit that fixes the error and probably report the issue upstream.

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 this pull request may close these issues.

2 participants