Implement a lint for RefCell<impl Copy> #13388

GnomedDev · 2024-09-12T08:16:14Z

Closes #1 (!)

As the title says. Adds a lint which checks for RefCell<impl Copy> as usage can be replaced with Cell, if T is small enough (user configurable).

As this is my first clippy lint, I'm not super firm on stuff like:

lint group
lint message wording
the LateLintPass checks used

so let me know if I've done something wrong and I'll get it fixed up.

changelog: [refcell_copy]: Added lint to detect usage of RefCell<T> where T is Copy.

rustbot · 2024-09-12T08:16:20Z

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

clippy_config/src/conf.rs

clippy_lints/src/copy_refcell.rs

Centri3

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

Centri3 · 2024-09-16T12:42:39Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        if implements_trait(cx, inner_ty, copy_def_id, &[]) {


We should check is_from_proc_macro right here

This would probably also be better as a long let chain

Do the span.from_expansion calls earlier not catch this?

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.

clippy_lints/src/copy_refcell.rs

Centri3 · 2024-09-16T13:07:08Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        let field_ty = rustc_hir_analysis::lower_ty(cx.tcx, field_def.ty);


This could be type_of (maybe only possible if check_item is used instead?)

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.

tcx.type_of

clippy_lints/src/copy_refcell.rs

Centri3 · 2024-09-16T13:34:43Z

tests/ui/copy_refcell.rs

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)

Centri3 · 2024-09-16T13:36:13Z

clippy_lints/src/copy_refcell.rs

+
+    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) {


from_expansion includes desugaring

Shall I remove the matches!? Or just remove the comment?

Yeah the matches! is unnecessary

Centri3 · 2024-09-16T13:40:48Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        let inner_ty = generics.type_at(0);


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

Wdym "explicitly Copy", or even removing the Copy bound? I don't see why you would want to remove the Copy implementation.

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.

GnomedDev · 2024-09-16T15:41:49Z

Sounds good, I'll look into the individual comments later.

GnomedDev

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.

GnomedDev · 2024-09-17T12:12:50Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        let inner_ty = generics.type_at(0);


Wdym "explicitly Copy", or even removing the Copy bound? I don't see why you would want to remove the Copy implementation.

GnomedDev · 2024-09-17T12:12:59Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        if implements_trait(cx, inner_ty, copy_def_id, &[]) {


Do the span.from_expansion calls earlier not catch this?

clippy_lints/src/copy_refcell.rs

GnomedDev · 2024-09-17T12:16:11Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        let field_ty = rustc_hir_analysis::lower_ty(cx.tcx, field_def.ty);


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.

GnomedDev · 2024-09-17T12:16:32Z

clippy_lints/src/copy_refcell.rs

+
+    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) {


Shall I remove the matches!? Or just remove the comment?

clippy_lints/src/copy_refcell.rs

Centri3

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.

Centri3 · 2024-09-19T12:57:38Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        let field_ty = rustc_hir_analysis::lower_ty(cx.tcx, field_def.ty);


tcx.type_of

Centri3 · 2024-09-19T12:58:46Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        if implements_trait(cx, inner_ty, copy_def_id, &[]) {


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.

Centri3 · 2024-09-19T12:59:58Z

clippy_lints/src/copy_refcell.rs

+            return;
+        }
+
+        let inner_ty = generics.type_at(0);


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.

Centri3 · 2024-09-19T13:00:31Z

clippy_lints/src/copy_refcell.rs

+
+    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) {


Yeah the matches! is unnecessary

rustbot assigned Centri3 Sep 12, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 12, 2024

GnomedDev force-pushed the refcell-copy branch from 6b8697b to 674d9e4 Compare September 12, 2024 08:29

GnomedDev mentioned this pull request Sep 13, 2024

Add lint for unnecessary lifetime bounded &str return #13395

Open

lukaslueg reviewed Sep 16, 2024

View reviewed changes

clippy_config/src/conf.rs Outdated Show resolved Hide resolved

lukaslueg reviewed Sep 16, 2024

View reviewed changes

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

GnomedDev force-pushed the refcell-copy branch from 674d9e4 to cf87d93 Compare September 16, 2024 10:55

GnomedDev requested a review from lukaslueg September 16, 2024 10:56

Centri3 reviewed Sep 16, 2024

View reviewed changes

GnomedDev commented Sep 17, 2024

View reviewed changes

GnomedDev added 2 commits September 17, 2024 13:39

Implement a lint for RefCell<impl Copy>

618b6b4

Bless/fix unrelated tests

cb7c54d

GnomedDev force-pushed the refcell-copy branch from cf87d93 to cb7c54d Compare September 17, 2024 12:42

Centri3 reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a lint for RefCell<impl Copy> #13388

Implement a lint for RefCell<impl Copy> #13388

GnomedDev commented Sep 12, 2024

rustbot commented Sep 12, 2024

Centri3 left a comment

Centri3 Sep 16, 2024

Centri3 Sep 16, 2024

GnomedDev Sep 17, 2024

Centri3 Sep 19, 2024

Centri3 Sep 16, 2024

GnomedDev Sep 17, 2024

Centri3 Sep 19, 2024

Centri3 Sep 16, 2024

Centri3 Sep 16, 2024

GnomedDev Sep 17, 2024

Centri3 Sep 19, 2024

Centri3 Sep 16, 2024

GnomedDev Sep 17, 2024

Centri3 Sep 19, 2024

GnomedDev commented Sep 16, 2024

GnomedDev left a comment

GnomedDev Sep 17, 2024

GnomedDev Sep 17, 2024

GnomedDev Sep 17, 2024

GnomedDev Sep 17, 2024

Centri3 left a comment

Centri3 Sep 19, 2024

Centri3 Sep 19, 2024

Centri3 Sep 19, 2024

Centri3 Sep 19, 2024

Implement a lint for RefCell<impl Copy> #13388

Are you sure you want to change the base?

Implement a lint for RefCell<impl Copy> #13388

Conversation

GnomedDev commented Sep 12, 2024

rustbot commented Sep 12, 2024

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GnomedDev commented Sep 16, 2024

GnomedDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment