Add check_self_items setting for needless_pass_by_ref_mut lint

[GuillaumeGomez]

Fixes #12589.
Fixes #9591.

This adds a check_self_items setting to allow the needless_pass_by_ref_mut lint to check for &mut self items.

changelog: Add check_self_items setting for needless_pass_by_ref_mut lint

[rustbot]

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[GuillaumeGomez]

Assigning someone else as I think they're still busy at the moment.

r? @llogiq

[klensy]

Should fix #9591 too?

[GuillaumeGomez]

Ah indeed.

[Manishearth]

it's unclear to me that a config is actually the solution here. we should first try to better understand the problem space.

[llogiq]

Since I have a full plate already and he seems to have the PR well in hand, back to

@bors r? @Manishearth

[klensy]

it's unclear to me that a config is actually the solution here. we should first try to better understand the problem space.

For start, you can read linked issues, #9591, #10900 (comment)

[Manishearth]

Yes, I'm not convinced that that is a case not better served with targeted allows.

For it to be a config I'd need to see a strong reason why this needs to be a codebase-wide decision.

[klensy]

Yes, I'm not convinced that that is a case not better served with targeted allows.

Targeted allows - what you mean? If i currently want to lint &mut self - there is no way to do this. This lint more useful for analysis of code (and for analysis i want to see more suggestions, including &mut selfs), not for automatically fixing all suggested things. Besides, this lint is in nursery.

[Manishearth]

Ok, but if this lint were to unconditionally lint &mut self, what is the reason for someone to wish to disable that aspect of the check across the codebase? I understand wanting to do it for a specific type where there are special semantic needs, but those cases are fixed with a very warranted allow.

Basically what is the benefit of a config over making this available all the time?

Sure, this lint is in nursery, but why do work we may decide to undo at some point? For the lint to get out of nursery these discussions need to happen at some point, and there's not a huge benefit of waiting on this one.

[GuillaumeGomez]

Ok, but if this lint were to unconditionally lint &mut self, what is the reason for someone to wish to disable that aspect of the check across the codebase? I understand wanting to do it for a specific type where there are special semantic needs, but those cases are fixed with a very warranted allow.

It's the other way around: this lint never lints &mut self and this PR adds a config to allow &mut self to be linted.

[Manishearth]

Yes, I understand: why does this have to be a config and not unconditional behavior?

My comment was talking about that hypothetical and comparing it with this PR.

[GuillaumeGomez]

Thanks for clarifying. I was able to find my original answer about it here. Basically, you might want to keep &mut self to ensure that even though self is not used mutably, you want to restrain its access. Maybe it's too far fetched though. I don't have a strong opinion about it nor do I have a use case where I actually need this use case.

[Manishearth]

Yes, why is that use case not adequately satisfied by allow? What is the reason to want to disable that for the entire crate? That's when you have a config.

And I don't think it's concrete enough.

[GuillaumeGomez]

In short, I was trying to limit the scope of the lint as much as possible. That's the only backing to it. Like I said, we could enable it by default and instead have this config to disable this behaviour.

[Manishearth]

Yeah in general config options expand the scope of a lint and need to have a strong reason

[GuillaumeGomez]

Just to confirm: following your logic, if the config would be to disable the check on self, wouldn't it be the opposite of an expansion of a lint scope? Or did I misunderstood what you meant?

[Manishearth]

No. I'm saying having a config in the first place, regardless of what it does, expands the scope of a lint because it's effectively multiple lints now. We should avoid that unless both lints can be justified. I can't see a strong justification for the "no-lint-self" version of this lint.

[GuillaumeGomez]

Makes sense. Gonna update this lint to check for self as well instead then.

[GuillaumeGomez]

Closing in favor of #12693.