-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Implement a lint for RefCell<impl Copy> #13388
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Centri3 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
6b8697b
to
674d9e4
Compare
674d9e4
to
cf87d93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can refactor check_local
into check_expr
, and check_field_def
into check_item
? In specific, we're missing cases like: takes_t(RefCell::new(0))
and fn f(ref_cell: RefCell<u32>)
I will also say, the types
module exists. I think we should move this there
return; | ||
} | ||
|
||
if implements_trait(cx, inner_ty, copy_def_id, &[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check is_from_proc_macro
right here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably also be better as a long let
chain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the span.from_expansion
calls earlier not catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, procedural macros can use any arbitrary span which also means its input tokens. Those won't be counted as from expansion so it'll still lint.
return; | ||
} | ||
|
||
let field_ty = rustc_hir_analysis::lower_ty(cx.tcx, field_def.ty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be type_of
(maybe only possible if check_item
is used instead?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type_of
? I don't see this mentioned on https://doc.rust-lang.org/clippy/development/type_checking.html or https://doc.rust-lang.org/clippy/development/trait_checking.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests/ui/copy_refcell.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll suggest some tests:
- Public field (shouldn't lint)
- Public function (parameters and return type)
- Generic code/generic
T
(shouldn't lint) - Macro code
- External code (
auxiliary::proc_macros
) - Type aliases (probably shouldn't lint)
|
||
fn check_local(&mut self, cx: &LateContext<'tcx>, local_def: &'tcx LetStmt<'tcx>) { | ||
// Don't want to lint macro expansions or desugaring. | ||
if local_def.span.from_expansion() || !matches!(local_def.source, LocalSource::Normal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from_expansion
includes desugaring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I remove the matches!
? Or just remove the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the matches!
is unnecessary
return; | ||
} | ||
|
||
let inner_ty = generics.type_at(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could have a check for if this is explicitly Copy
and suggesting to remove the Copy
bound, but that's just a bonus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdym "explicitly Copy", or even removing the Copy bound? I don't see why you would want to remove the Copy implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct A<T: Copy>(RefCell<T>);
This could suggest removing the Copy
bound for T
, which admittedly shouldn't be done on struct definitions anyway as convention. But this applies to functions too. Whether this is useful or not is up to you.
Sounds good, I'll look into the individual comments later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I specifically used local definitions and field defs to avoid false lints, as taking a RefCell directly as a function parameter should be quite rare and linting could lead to a private function taking the argument from a public function having no choice but to allow
/expect
.
I do not know what you are talking about with "the types module", I haven't read that in the documentation.
return; | ||
} | ||
|
||
let inner_ty = generics.type_at(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdym "explicitly Copy", or even removing the Copy bound? I don't see why you would want to remove the Copy implementation.
return; | ||
} | ||
|
||
if implements_trait(cx, inner_ty, copy_def_id, &[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the span.from_expansion
calls earlier not catch this?
return; | ||
} | ||
|
||
let field_ty = rustc_hir_analysis::lower_ty(cx.tcx, field_def.ty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type_of
? I don't see this mentioned on https://doc.rust-lang.org/clippy/development/type_checking.html or https://doc.rust-lang.org/clippy/development/trait_checking.html.
|
||
fn check_local(&mut self, cx: &LateContext<'tcx>, local_def: &'tcx LetStmt<'tcx>) { | ||
// Don't want to lint macro expansions or desugaring. | ||
if local_def.span.from_expansion() || !matches!(local_def.source, LocalSource::Normal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I remove the matches!
? Or just remove the comment?
cf87d93
to
cb7c54d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll still have the same FPs anyway, for example:
let a = RefCell::new(0u32);
i_take_refcell(a);
This can be combated by using predicates_of
+ for_each_local_use_after_expr
, but that's beyond the scope of this. Typically these FPs are fine with pedantic/restriction lints
I'm fairly sure your lint still catches let a = publicly_returns_refcell_copy()
. This should be a test, and also, preventing public functions returning RefCell
would then require specifically hardcoding the RefCell
methods.
By the types module, I mean clippy_lints::types
which is a collection of these lints under one pass.
return; | ||
} | ||
|
||
let field_ty = rustc_hir_analysis::lower_ty(cx.tcx, field_def.ty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return; | ||
} | ||
|
||
if implements_trait(cx, inner_ty, copy_def_id, &[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, procedural macros can use any arbitrary span which also means its input tokens. Those won't be counted as from expansion so it'll still lint.
return; | ||
} | ||
|
||
let inner_ty = generics.type_at(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct A<T: Copy>(RefCell<T>);
This could suggest removing the Copy
bound for T
, which admittedly shouldn't be done on struct definitions anyway as convention. But this applies to functions too. Whether this is useful or not is up to you.
|
||
fn check_local(&mut self, cx: &LateContext<'tcx>, local_def: &'tcx LetStmt<'tcx>) { | ||
// Don't want to lint macro expansions or desugaring. | ||
if local_def.span.from_expansion() || !matches!(local_def.source, LocalSource::Normal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the matches!
is unnecessary
☔ The latest upstream changes (presumably #13322) made this pull request unmergeable. Please resolve the merge conflicts. |
Add lint for unnecessary lifetime bounded &str return Closes #305. Currently implemented with a pretty strong limitation that it can only see the most basic implicit return, but this should be fixable by something with more time and brain energy than me. Cavets from #13388 apply, as I have not had a review on my clippy lints yet so am pretty new to this. ``` changelog: [`unnecessary_literal_bound`]: Add lint for unnecessary lifetime bounded &str return. ```
Closes #1 (!)
As the title says. Adds a lint which checks for
RefCell<impl Copy>
as usage can be replaced withCell
, ifT
is small enough (user configurable).As this is my first clippy lint, I'm not super firm on stuff like:
LateLintPass
checks usedso let me know if I've done something wrong and I'll get it fixed up.
changelog: [
refcell_copy
]: Added lint to detect usage ofRefCell<T>
whereT
isCopy
.