Improve `redundant_slicing` lint by Jarcho · Pull Request #8218 · rust-lang/rust...
source link: https://github.com/rust-lang/rust-clippy/pull/8218
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
r? @camsteffen
(rust-highfive has picked a reviewer for you, use r? to override)
Linting "slice instead of deref" is more picky than linting "slice has no effect", so I think this should be a new lint. Named deref_by_slicing perhaps?
And then I'm not sure if it should be pedantic or restriction. I think some people may actually prefer the slicing operator since it makes the type of the expression more apparent. There's also a third option - to use .as_slice() (if available). So people may prefer these three options in any order and it's not clear to me that any is more idiomatic than another.
@rust-lang/clippy thoughts? Should &vec[..] to &*vec or vec.to_slice() be pedantic or restriction?
I'd say
&x[..]changing&&Tto&Twould still fall underredundant_slicing
Sounds good. That would be &&[T] to &[T].
@rust-lang/clippy thoughts? Should
&vec[..]to&*vecorvec.to_slice()be pedantic or restriction?
restriction I think. Pedantic is kinda fine but I think we need to have a reason for it
If it's going to be a restriction lint I suspect we should wait for people to ask us for it
I'm also thinking restriction - feels opinionated/controversial. But I do think having the lint would be good, especially since the implementation easily fits in with redundant_slicing.
force-pushed the redundant_slicing_deref
branch
4 times, most recently
from
bd637f2
to
ac282d8
2 months ago
What do you think about the name full_slicing? This terminology appears in slice docs. IIUC the lint pretty much bans any usage of [..].
Also don't forget to add a test case where the sliced expression is &&Vec<_>. This is an exceptionally picky case, but I think it's consistent to include it.
Between the two lints almost all slicing with RangeFull would be banned. There might be the odd type that doesn't implement Deref, or just return itself. There's none in std at least.
The problem with the full_slicing name is it doesn't mention only triggering when it overlaps with a Deref impl. The notable example being the slice type itself.
WRT naming, I think it's fine to assume the convention: index operator with a range is called "slicing" and it returns a slice.
I think redundant_slicing represents a subset of full_slicing conceptually, and when that happens it's not necessary to force the lint definitions to be mutually exclusive. Otherwise in this case you end up with "bans full slicing except when its a noop".
There's still the case where the type has full slicing that changes the type, but doesn't impl Deref for whatever reason, or where slicing has a different type than the deref. I don't think either case is particularly common, but those are cases where the name would be incorrect.
Fair point. How about deref_by_slicing? slice_as_deref makes me think of as.
clippy_lints/src/redundant_slicing.rs
Outdated Show resolved
@@ -520,7 +520,7 @@ fn extract_clippy_version_value(cx: &LateContext<'_>, item: &'_ Item<'_>) -> Opt
if_chain! {
// Identify attribute
if let ast::AttrKind::Normal(ref attr_kind, _) = &attr.kind;
if let [tool_name, attr_name] = &attr_kind.path.segments[..];
if let [tool_name, attr_name] = &*attr_kind.path.segments;
I'd prefer not to "fix" all these since it's a restriction lint. Please also revert any such changes in the tests unless you think it is improving the quality of the test.
LL | let _ = &(&v[..])[..]; // Outer borrow is redundant
| ^^^^^^^^^^^^^ help: use the original value instead: `(&v[..])`
LL | let _ = &(&*v)[..]; // Outer borrow is redundant
| ^^^^^^^^^^ help: use the original value instead: `(&*v)`
It's been a while since I looked at this PR. I believe if a * is suggested, it should be a deref_by_slicing case?
The suggested change is from &(&*v)[..] -> (&*v) where v is a Vec<_>. The lint isn't suggesting to add a dereference here, it was already there to start with.
Apparrently I can't tell the difference between the commit diff and the rust output diff 
. But what about the error at the bottom of this file (same question)?
I'll assume you're referring to &slice_ref[..]. slice_ref is a &&[_]. Full slicing is the same as just removing a reference in this case.
Below that is
error: redundant slicing of the whole range
--> $DIR/redundant_slicing.rs:45:13
|
LL | let _ = (&bytes[..]).read_to_end(&mut vec![]).unwrap();
| ^^^^^^^^^^^^ help: reborrow the original value instead: `(&*bytes)`
If anything the previous one is much more of a deref-by-slicing. This is very much a slice-to-get-a-new-value-so-we-don't-modify-the-current-one (really flows of the tongue there).
I think leaving redundant_slicing to only handle cases where it's doing nothing and can be fully removed is fine, but deref_by_slicing is not really a good spot to put the remaining cases. That would mean having a name for a new lint (as precise as the name earlier is, I don't want to fight for longest lint name).
You could call it slice-to-deref-to-get-a-new-borrow-of... You can put a lot of new names on it but ultimately it just changes &[u8] to [u8] before the borrow operator applies. I don't see how it is not a deref. The suggestion literally replaces the slicing with a deref operator which has the exact same effect. If I enabled deref_by_slicing in my code, I'd consider it a bug if this case were not included.
I think leaving redundant_slicing to only handle cases where it's doing nothing and can be fully removed is fine
That is the most important thing since it is warn-by-default.
We're talking about two slightly different things here. I'm talking about the entire expression &x[..] not just x[..]. x[..] is just *x.index(..) so it will almost always be a deref. In this case the deref part is the only effect being used, so I guess that fits under deref_by_slicing.
I'm not sure why you don't take issue with the &&T -> &T case being under redundant_slicing.
I'm not sure why you don't take issue with the
&&T->&Tcase being underredundant_slicing.
Honestly I was just focusing on the other case. Yeah that one probably should be treated in the same way.
I'll move that all to deref_as_slice then. I still think the re-borrowing case is different enough to warrant it's own lint, but that can be dealt with at another time.
The latest upstream changes (presumably #8409) made this pull request unmergeable. Please resolve the merge conflicts.
force-pushed the redundant_slicing_deref
branch
2 times, most recently
from
2308856
to
46353f8
12 days ago
Thanks!
@bors r+
Commit 46353f8 has been approved by camsteffen
@bors r-
Sorry, probably should squash some?
@bors r+
Commit 7724d67 has been approved by camsteffen
Testing commit Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK