add must use annotation to functions that have no mutable args or statics #145
Conversation
There was a problem hiding this comment.
After discussing this PR with @edmorley, I remove my approve. Adding must_use makes sense, but we need to go further than just slapping annotations on anything where static analysis can figure out the value needs to be used.
We most likely want to add messages to most of them too.
| pub fn build(self) -> BuildResult { | ||
| BuildResult(InnerBuildResult::Pass { | ||
| launch: self.launch, | ||
| store: self.store, | ||
| }) | ||
| } | ||
|
|
||
| #[must_use] |
There was a problem hiding this comment.
I don't think #[must_use] should be used on this function and the one below given they do mutate the original struct - seems like false positives. (Unless we want to enforce the chaining function style of using these builders - which may actually be what we want, but worth discussing.)
Fixing this lint is definitely one where we need to vet each case and not just add the annotations where clippy suggested without checking :-)
|
I found some guidelines on when to use |
|
Should we close this PR and continue discussion in the issue itself, or create a new one? This PR will not be merged anytime soon and with a very different changeset, correct? |
|
I think we should close this for now, and discuss in the issue what strategy we wish to use here before opening a new PR. |
https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate
Fixes #57