new lints around`#[must_use]` #4560

llogiq · 2019-09-20T16:12:26Z

changelog: Add must_use_candidate lint, add must-use-unit lint, add double_must_use lint

The first one checks if an public function or method has no mutable argument and mutates no non-local data and lints if it has no #[must_use] attribute. It will skip inner functions, because those are usually highly local and the attribute doesn't have as much benefit there.

The second lints #[must_use] attributes on functions and methods that return unit. Those attributes are likely a remnant from a refactoring that removed the return value.

The last one lints for #[must_use] attributrs without text on functions that return a type which is already marked #[must_use]. This has no auto-suggestion, because while it would be easy to do, there may be value in writing a detailed text for the attribute instead.

This fixes #4526

phansch

Just an initial review, I think this could use a second pair of eyes, too :)

Oh, and also a util/dev fmt was missing

tests/ui/must_use_unit.rs

clippy_lints/src/functions.rs

phansch · 2019-09-22T09:55:53Z

clippy_lints/src/functions.rs

+        if let hir::ImplItemKind::Method(..) = item.node {
+            self.depth = self.depth.saturating_sub(1);
+        }
+    }
 }

 impl<'a, 'tcx> Functions {
    fn check_arg_number(self, cx: &LateContext<'_, '_>, decl: &hir::FnDecl, span: Span) {
        // Remove the function body from the span. We can't use `SourceMap::def_span` because the


I think this comment can now be removed or should be moved to the doc comment of function_header_span?

README.md

phansch · 2019-09-22T10:02:56Z

tests/ui/must_use_pure.stderr

+LL | pub fn pure(i: u8) -> u8 {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^ help: add the attribute: `#[must_use] pub fn pure(i: u8) -> u8`
+   |
+   = note: `-D clippy::pure-without-must-use` implied by `-D warnings`


It would be nice to have more pure function examples that trigger the lint. Does it trigger in pure trait methods, for example?

llogiq · 2019-09-22T12:15:41Z

The error seems unrelated? Perhaps a Rust PR that changed the error output.

phansch · 2019-09-22T12:26:59Z

The error seems unrelated? Perhaps a Rust PR that changed the error output.

Yup, that was fixed by #4565

llogiq · 2019-09-23T07:15:06Z

I adressed your comments, @phansch. Thank you for the thorough review.

flip1995

Impl LGTM. As @phansch said, a few more tests would be great. I think the code path of mutates_static() == true is never hit by the tests?

clippy_lints/src/functions.rs

llogiq · 2019-09-23T10:58:50Z

Added the tests and fixed the doc nit.

clippy_lints/src/functions.rs

CHANGELOG.md

clippy_lints/src/functions.rs

Centril · 2019-09-23T23:09:37Z

I think this PR advocates a rather strange definition of "pure" which is not referentially transparent nor even deterministic. Looking at the diff it seems to have caused a non-trivial amount of false positives, e.g. involving file opening and such. Focusing on mutable types is also not enough, you need to consider side-effects though functions or basic things like println!(). Also, this seems like an analysis that is best done on MIR rather than HIR and should be more like the const fn analyses of the compiler.

llogiq · 2019-09-24T06:02:00Z

Purity is – perhaps even in FP circles – a nebulous concept. Some people argue that it's about freedom from side effects (yet there is no such thing – any program running on a computer will burn cycles as a side effect, for example), others say they need to be referentially transparent or even deterministic. This lint takes a pragmatic approach. If a function does not mutate a static or its arguments, you'll likely want to call it for its result. I personally don't think file opening is a side effect I'd call a function for if I didn't want to do something with the file.

Also I agree that the lint triggers on a lot of function. Can you point to some you consider false positives?

ghost · 2019-09-24T06:31:58Z

This lint shouldn't apply to functions returning Result as you get a rustc warning if you don't use the result anyway.

ghost · 2019-09-24T06:37:07Z

Iterator as well. Maybe there is a way to look up if a struct is must_use.

clippy_dev/src/fmt.rs

clippy_dev/src/lib.rs

Centril · 2019-09-24T11:13:43Z

Purity is – perhaps even in FP circles – a nebulous concept.

See https://pdfs.semanticscholar.org/32ff/28cb5b5c990fe7bdca510040ea22f5c13316.pdf for a well defined definition.

Some people argue that it's about freedom from side effects (yet there is no such thing – any program running on a computer will burn cycles as a side effect, for example),

When reasoning about computer programs we do so preferably considering an idealized abstract machine in which some things (e.g. heat from burning cycles or bit flips due to radiation) are not observable. Otherwise it would be impossible to do all sorts of other reasonings as well (e.g. about soundness).

[...] others say they need to be referentially transparent or even deterministic.

Referentially transparent is a common notion, but a basic property a pure function needs to satisfy is that given f : A → B then ∀ x, y: A. x ≡ y → f(x) ≡ f(y). This is not an operational definition, but is a sufficient yardstick to decide that the functions I've highlighted above are not pure.

This lint takes a pragmatic approach. If a function does not mutate a static or its arguments, you'll likely want to call it for its result.

That's fine; I would suggest renaming the lint to something more suggestive and to extend the "known problems" to note more false positives. In my view, a better, but still pragmatic solution would be to do this on MIR.

I personally don't think file opening is a side effect I'd call a function for if I didn't want to do something with the file.

Merely opening a file is not an interesting side-effect (yet it might cause a problem if the file was already open), but reading it means you are reading the state of the world and that can result in indeterministic results.

llogiq · 2019-09-24T17:55:07Z

@Centril I see what you mean now. I'll best implement @mikerite's suggestion and rename the lint to must_use_candidate. It's already a pedantic lint, so false positives are somewhat acceptable.

llogiq · 2019-10-11T15:19:16Z

@phansch I've added your test case as-is and also added in_external_macro checks to must_use_candidate and double_must_use. Unfortunately, CI is broken again, we need yet another rustup.

phansch · 2019-10-12T06:15:41Z

isn't that missing a #[must_use] annotation for foo()

oh, yes! 😅

Also why would we want to quell the lint in macros?

The test is just there to check that we don't lint occurences in external macros. It should still lint for non-external macros.

phansch

Left two comments and I believe there are also two open comments from @sinkuu that are only visible in the changes tab?

Apart from that it looks good to me 👍

clippy_lints/src/functions.rs

llogiq · 2019-10-13T20:06:26Z

Let's see what CI thinks about it. I may need yet another rebase though.

`must_use_unit` lints unit-returning functions with a `#[must_use]` attribute, suggesting to remove it. `double_must_use` lints functions with a plain `#[must_use]` attribute, but which return a type which is already `#[must_use]`, so the attribute has no benefit. `must_use_candidate` is a pedantic lint that lints functions and methods that return some non-unit type that is not already `#[must_use]` and suggests to add the annotation.

llogiq · 2019-10-14T16:23:46Z

@phansch r?

phansch · 2019-10-14T16:57:04Z

@bors r+ nice!

bors · 2019-10-14T16:57:06Z

📌 Commit cc62260 has been approved by phansch

new lints around`#[must_use]` changelog: Add `must_use_candidate` lint, add `must-use-unit` lint, add `double_must_use` lint The first one checks if an public function or method has no mutable argument and mutates no non-local data and lints if it has no `#[must_use]` attribute. It will skip inner functions, because those are usually highly local and the attribute doesn't have as much benefit there. The second lints `#[must_use]` attributes on functions and methods that return unit. Those attributes are likely a remnant from a refactoring that removed the return value. The last one lints for `#[must_use]` attributrs without text on functions that return a type which is already marked `#[must_use]`. This has no auto-suggestion, because while it would be easy to do, there may be value in writing a detailed text for the attribute instead. This fixes #4526

phansch · 2019-10-14T16:59:06Z

@bors retry (yielding priority to #4663)

new lints around`#[must_use]` changelog: Add `must_use_candidate` lint, add `must-use-unit` lint, add `double_must_use` lint The first one checks if an public function or method has no mutable argument and mutates no non-local data and lints if it has no `#[must_use]` attribute. It will skip inner functions, because those are usually highly local and the attribute doesn't have as much benefit there. The second lints `#[must_use]` attributes on functions and methods that return unit. Those attributes are likely a remnant from a refactoring that removed the return value. The last one lints for `#[must_use]` attributrs without text on functions that return a type which is already marked `#[must_use]`. This has no auto-suggestion, because while it would be easy to do, there may be value in writing a detailed text for the attribute instead. This fixes #4526

phansch · 2019-10-14T17:00:44Z

@bors retry

bors · 2019-10-14T17:33:34Z

⌛ Testing commit cc62260 with merge 8fae2dd...

new lints around`#[must_use]` changelog: Add `must_use_candidate` lint, add `must-use-unit` lint, add `double_must_use` lint The first one checks if an public function or method has no mutable argument and mutates no non-local data and lints if it has no `#[must_use]` attribute. It will skip inner functions, because those are usually highly local and the attribute doesn't have as much benefit there. The second lints `#[must_use]` attributes on functions and methods that return unit. Those attributes are likely a remnant from a refactoring that removed the return value. The last one lints for `#[must_use]` attributrs without text on functions that return a type which is already marked `#[must_use]`. This has no auto-suggestion, because while it would be easy to do, there may be value in writing a detailed text for the attribute instead. This fixes #4526

bors · 2019-10-14T18:03:56Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing 8fae2dd to master...

llogiq · 2019-10-14T19:11:25Z

Thanks, @phansch!

llogiq force-pushed the must-use-pure branch 8 times, most recently from 845f4af to a59abf3 Compare September 22, 2019 07:17

phansch reviewed Sep 22, 2019

View reviewed changes

llogiq force-pushed the must-use-pure branch from a59abf3 to a3912fe Compare September 23, 2019 07:05

llogiq force-pushed the must-use-pure branch from a3912fe to 134fac1 Compare September 23, 2019 07:15

flip1995 reviewed Sep 23, 2019

View reviewed changes

clippy_lints/src/functions.rs Outdated Show resolved Hide resolved

llogiq force-pushed the must-use-pure branch from 134fac1 to 6145cf8 Compare September 23, 2019 10:58

llogiq force-pushed the must-use-pure branch from 6145cf8 to 901ecfd Compare September 23, 2019 11:00

sinkuu reviewed Sep 23, 2019

View reviewed changes

clippy_lints/src/functions.rs Outdated Show resolved Hide resolved

phansch reviewed Sep 23, 2019

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

Centril reviewed Sep 23, 2019

View reviewed changes

clippy_lints/src/functions.rs Outdated Show resolved Hide resolved

clippy_lints/src/functions.rs Outdated Show resolved Hide resolved

Centril reviewed Sep 24, 2019

View reviewed changes

clippy_dev/src/fmt.rs Outdated Show resolved Hide resolved

clippy_dev/src/lib.rs Outdated Show resolved Hide resolved

phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 25, 2019

llogiq changed the title ~~new lint for missing #[must_use] on pure fns~~ new lint for missing #[must_use] on suitable fns Sep 25, 2019

phansch reviewed Oct 12, 2019

View reviewed changes

clippy_lints/src/functions.rs Show resolved Hide resolved

clippy_lints/src/functions.rs Show resolved Hide resolved

llogiq force-pushed the must-use-pure branch from 2fae14c to 7fa7587 Compare October 13, 2019 20:01

llogiq force-pushed the must-use-pure branch 3 times, most recently from b9785b3 to 80f342b Compare October 14, 2019 07:29

llogiq force-pushed the must-use-pure branch from 80f342b to cc62260 Compare October 14, 2019 10:11

This comment has been minimized.

Sign in to view

bors merged commit cc62260 into master Oct 14, 2019

This was referenced Oct 14, 2019

New lint: Recommend using ptr::eq when possible #4596

Closed

Add a lint for unused self #4619

Merged

New Lint: push same item to Vec (#4078) #4647

Closed

Update clippy for latest rustc changes #4650

Merged

llogiq deleted the must-use-pure branch October 14, 2019 19:11

flip1995 mentioned this pull request Nov 6, 2019

must_use_candidate is going mad (false positives?) #4779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new lints around`#[must_use]` #4560

new lints around`#[must_use]` #4560

llogiq commented Sep 20, 2019 •

edited

Loading

phansch left a comment

phansch Sep 22, 2019

phansch Sep 22, 2019

llogiq commented Sep 22, 2019

phansch commented Sep 22, 2019

llogiq commented Sep 23, 2019

flip1995 left a comment

llogiq commented Sep 23, 2019

Centril commented Sep 23, 2019

llogiq commented Sep 24, 2019

ghost commented Sep 24, 2019

ghost commented Sep 24, 2019 •

edited by ghost

Loading

Centril commented Sep 24, 2019

llogiq commented Sep 24, 2019

llogiq commented Oct 11, 2019 •

edited

Loading

phansch commented Oct 12, 2019

phansch left a comment •

edited

Loading

llogiq commented Oct 13, 2019

llogiq commented Oct 14, 2019

phansch commented Oct 14, 2019

bors commented Oct 14, 2019

This comment has been minimized.

phansch commented Oct 14, 2019

This comment has been minimized.

phansch commented Oct 14, 2019

bors commented Oct 14, 2019

bors commented Oct 14, 2019

llogiq commented Oct 14, 2019

new lints around#[must_use] #4560

new lints around#[must_use] #4560

Conversation

llogiq commented Sep 20, 2019 • edited Loading

phansch left a comment

Choose a reason for hiding this comment

phansch Sep 22, 2019

Choose a reason for hiding this comment

phansch Sep 22, 2019

Choose a reason for hiding this comment

llogiq commented Sep 22, 2019

phansch commented Sep 22, 2019

llogiq commented Sep 23, 2019

flip1995 left a comment

Choose a reason for hiding this comment

llogiq commented Sep 23, 2019

Centril commented Sep 23, 2019

llogiq commented Sep 24, 2019

ghost commented Sep 24, 2019

ghost commented Sep 24, 2019 • edited by ghost Loading

Centril commented Sep 24, 2019

llogiq commented Sep 24, 2019

llogiq commented Oct 11, 2019 • edited Loading

phansch commented Oct 12, 2019

phansch left a comment • edited Loading

Choose a reason for hiding this comment

llogiq commented Oct 13, 2019

llogiq commented Oct 14, 2019

phansch commented Oct 14, 2019

bors commented Oct 14, 2019

This comment has been minimized.

phansch commented Oct 14, 2019

This comment has been minimized.

phansch commented Oct 14, 2019

bors commented Oct 14, 2019

bors commented Oct 14, 2019

llogiq commented Oct 14, 2019

new lints around`#[must_use]` #4560

new lints around`#[must_use]` #4560

llogiq commented Sep 20, 2019 •

edited

Loading

ghost commented Sep 24, 2019 •

edited by ghost

Loading

llogiq commented Oct 11, 2019 •

edited

Loading

phansch left a comment •

edited

Loading