New lint: Suggests using Cell<T: Copy> instead of RefCell<T: Copy> - #17522
New lint: Suggests using Cell<T: Copy> instead of RefCell<T: Copy>#17522qdot3 wants to merge 8 commits into
Cell<T: Copy> instead of RefCell<T: Copy>#17522Conversation
rustbot
added
the
S-waiting-on-community-reviews
|
Thanks for the pull request, and welcome! You should hear from one of our reviewers after this PR gets at least 2 reviews from the community. 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 (
|
rustbot
added
needs-fcp
rustbot
removed
the
S-waiting-on-review
rustbot
added
the
S-waiting-on-review
|
☔ The latest upstream changes (possibly #17499) made this pull request unmergeable. Please resolve the merge conflicts. |
| // Because this may be required to be `RefCell` | ||
| let mut app = Applicability::MaybeIncorrect; |
There was a problem hiding this comment.
Seems like a bit of an unclear comment. Can you clarify this comment?
| let mut app = Applicability::MaybeIncorrect; | ||
| let sugg = { | ||
| let (init, _) = snippet_with_context(cx, span, stmt.span.ctxt(), "..", &mut app); | ||
| init.into_owned().replace("RefCell", "Cell") |
There was a problem hiding this comment.
this .replace("RefCell", "Cell") seems suspicious.
How about RefCell<InnerRefCell> where InnerRefCell is a struct with Copy?
| snippet(cx, init.span, "").contains("RefCell"), | ||
| let_stmt | ||
| .ty | ||
| .is_some_and(|ty| snippet(cx, ty.span, "").contains("RefCell")), |
There was a problem hiding this comment.
Is the blanket String .contains here safe, as in does this have false-positives? Can't we do this via matching instead as in the outer if-let?
| @@ -1,3 +1,4 @@ | |||
| #![allow(clippy::refcell_cell)] | |||
There was a problem hiding this comment.
can this be expect? (also applies to the other instances)
The reason behind this is that this way we "GC" these lints when they don't apply.
There was a problem hiding this comment.
instead of an lint, should this be a change to that lint to not lint in this case?
There was a problem hiding this comment.
should we not instead add an exeption to those lints to not lint?
They seem a bit conflicting...
| /// ### Why is this bad? | ||
| /// `RefCell` avoids cloning at the cost of additional memory usage and | ||
| /// instructions, which isn't worth it for `Copy` types. | ||
| /// |
There was a problem hiding this comment.
there are some Known problems regarding trait bounds as noted in the tests, can we document them here?
Also, we need to note that while on avarage this is a good idea, there might be better options..
There is no runtime cost to using
Cell<T>, however if one is using it to wrap larger (Copy) structs, it might be worthwhile to instead wrap individual fields inCell<T>since each write is a full copy of the struct
https://manishearth.github.io/blog/2015/05/27/wrapper-types-in-rust-choosing-your-guarantees/
3 participants
Implementation of #17380
This lint suggests using
Cellinstead ofRefCellforCopytypes.RefCellavoids cloning, but it introduces additional memory overhead and runtime checks, which are not worthwhile forCopytypes.Scope
letstatements (e.g.,let _ = RefCell::new(1))Out of scope
letstatements