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

Clarify the documentation of the unnecessary_mut_passed lint #5433

Closed
koivunej opened this issue Apr 7, 2020 · 4 comments · Fixed by #5639
Closed

Clarify the documentation of the unnecessary_mut_passed lint #5433

koivunej opened this issue Apr 7, 2020 · 4 comments · Fixed by #5639
Labels
A-documentation Area: Adding or improving documentation good-first-issue These issues are a good way to get started with Clippy

Comments

@koivunej
Copy link
Contributor

koivunej commented Apr 7, 2020

I was expecting $topic lint to be triggered by the code example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7ece6381245f5db875b78fff154c1169 but it was not. I was initially thinking that the lint with async was buggy, but was surprised that the non-async version wasn't triggering one either.

A free function with unnecessary &mut when & would do doesn't trigger the warning either: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7af42ceef61f5cec396f7bbad9ec0f10

Playground clippy version: 0.0.212 (2020-04-03 7907abe)
My local clippy version: clippy 0.0.212 (4ee1206 2020-02-01)

The only open related issue is the older #353 but I sort of remember this lint working in the past.

@flip1995
Copy link
Member

flip1995 commented Apr 7, 2020

This lint doesn't check if the param is used mutably inside the function, but if a mutable reference is passed, when the function signature only takes an immutable reference. IMO the lint works as expected: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7af42ceef61f5cec396f7bbad9ec0f10

Documentation

Detects giving a mutable reference to a function that only requires an immutable reference.

Should be reworded to

-Detects giving a mutable reference to a function that only requires an immutable reference.
+Detects passing a mutable reference to a function that only requires an immutable reference.

for clearity, though.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy A-documentation Area: Adding or improving documentation labels Apr 7, 2020
@flip1995 flip1995 changed the title Unnecessary mut lint not triggered Clarify the documentation of the unnecessary_mut_passed lint Apr 7, 2020
@koivunej
Copy link
Contributor Author

koivunej commented Apr 8, 2020

Took me a while to understand how the link you posted triggers the link. Just to clarify, it doesn't, but this does with difference to my second playlink (7af42c) which is the same you linked to:

-fn foo(faa: &mut Foo) -> usize {
+fn foo(faa: &Foo) -> usize {

(If I am somehow failing to use play.rust-lang.org to get the lint on 7af42c please let me know, but I see no warnings when running or when using Tools > Clippy.)

The function is still called as foo(&mut Foo(0)) which triggers the lint in question.

I wonder if this should be changed to a "feature request" then, but I have no idea how to start implementing such a lint as I described with the original description.

@flip1995
Copy link
Member

flip1995 commented Apr 8, 2020

Oh, I forgot to press share on playground and just posted the same playground as you. Sorry about that!

Your change is what I had in mind.

I wonder if this should be changed to a "feature request" then

This could be a new lint: Lint for useless &mut in function parameters, when a shared reference is enough. Can you open a new issue for this?

@nickrtorres
Copy link
Contributor

I can take this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Adding or improving documentation good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants