Improve `redundant_slicing` lint #8218

Jarcho · 2022-01-03T20:28:49Z

This can supersede #7976

changelog: Fix suggestion for redundant_slicing when re-borrowing for a method call
changelog: New lint deref_as_slicing

rust-highfive · 2022-01-03T20:28:52Z

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

camsteffen · 2022-01-06T02:01:32Z

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.

Jarcho · 2022-01-06T02:20:57Z

That is a fair point. I'd say pedantic for it, but restriction would also be reasonable.

I'd say &x[..] changing &&T to &T would still fall under redundant_slicing

camsteffen · 2022-01-06T03:42:31Z

@rust-lang/clippy thoughts? Should &vec[..] to &*vec or vec.to_slice() be pedantic or restriction?

camsteffen · 2022-01-06T03:50:51Z

I'd say &x[..] changing &&T to &T would still fall under redundant_slicing

Sounds good. That would be &&[T] to &[T].

Manishearth · 2022-01-06T17:27:27Z

@rust-lang/clippy thoughts? Should &vec[..] to &*vec or vec.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

Jarcho · 2022-01-06T20:09:02Z

#7257 kind of asked for it. Although I'm sure it was assuming letting auto-deref take care of things rather than explicitly dereferencing.

camsteffen · 2022-01-07T14:33:58Z

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.

camsteffen · 2022-01-11T19:59:23Z

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.

flip1995 · 2022-01-12T13:29:56Z

Should &vec[..] to &*vec or vec.to_slice() be pedantic or restriction?

I would say restriction. &vec[..] is a too common of a pattern for Clippy to suggest that it is bad (even for a pedantic lint), IMO.

Jarcho · 2022-01-12T20:12:40Z

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.

camsteffen · 2022-01-12T21:07:58Z

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".

Jarcho · 2022-01-12T23:11:20Z

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.

camsteffen · 2022-01-13T01:55:07Z

Fair point. How about deref_by_slicing? slice_as_deref makes me think of as.

Jarcho · 2022-01-13T03:20:59Z

That works.

clippy_lints/src/redundant_slicing.rs

camsteffen · 2022-01-15T19:28:38Z

clippy_lints/src/utils/internal_lints.rs

@@ -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.

camsteffen · 2022-02-04T21:52:12Z

tests/ui/redundant_slicing.stderr

-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 -> &T case being under redundant_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.

bors · 2022-02-10T17:33:36Z

☔ The latest upstream changes (presumably #8409) made this pull request unmergeable. Please resolve the merge conflicts.

camsteffen · 2022-02-16T22:05:16Z

Thanks!

@bors r+

bors · 2022-02-16T22:05:18Z

📌 Commit 46353f8 has been approved by camsteffen

camsteffen · 2022-02-16T22:05:55Z

@bors r-

Sorry, probably should squash some?

* Lint when slicing triggers auto-deref * Lint when slicing returns the same type as dereferencing

camsteffen · 2022-02-17T16:05:09Z

@bors r+

bors · 2022-02-17T16:05:10Z

📌 Commit 7724d67 has been approved by camsteffen

bors · 2022-02-17T16:08:48Z

⌛ Testing commit 7724d67 with merge 668b3e4...

bors · 2022-02-17T16:29:08Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 668b3e4 to master...

rust-highfive assigned camsteffen Jan 3, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 3, 2022

Jarcho force-pushed the redundant_slicing_deref branch 2 times, most recently from c242ef2 to 2c1f2ff Compare January 3, 2022 21:11

Jarcho mentioned this pull request Jan 5, 2022

redundant_slicing precedence fix #7976

Closed

Jarcho force-pushed the redundant_slicing_deref branch 4 times, most recently from bd637f2 to ac282d8 Compare January 11, 2022 19:33

Jarcho force-pushed the redundant_slicing_deref branch from 014b0e7 to 70bc0cf Compare January 13, 2022 04:07

camsteffen suggested changes Jan 15, 2022

View reviewed changes

camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 15, 2022

camsteffen reviewed Feb 4, 2022

View reviewed changes

camsteffen mentioned this pull request Feb 9, 2022

Suggest usage of array and Vec .as_slice() and .as_mut_slice() methods #8405

Closed

6 tasks

nindalf mentioned this pull request Feb 9, 2022

Suggest usage of array.as_slice() (instead of &array[..]) and array.as_mut_slice() #7633

Open

Jarcho force-pushed the redundant_slicing_deref branch 2 times, most recently from 2308856 to 46353f8 Compare February 13, 2022 23:58

Jarcho added 2 commits February 16, 2022 23:52

Improve redundant_slicing lint

faeeef3

* Lint when slicing triggers auto-deref * Lint when slicing returns the same type as dereferencing

Account for auto-borrows and precedence in redundant_slicing lint

4bdc97c

Jarcho force-pushed the redundant_slicing_deref branch from 46353f8 to 7fad2f9 Compare February 17, 2022 05:00

Jarcho added 2 commits February 17, 2022 00:02

Split off new lint deref_by_slicing from redundant_slicing

113ac6e

Move some cases from redundant_slicing to deref_by_slicing

7724d67

Jarcho force-pushed the redundant_slicing_deref branch from 7fad2f9 to 7724d67 Compare February 17, 2022 05:02

bors merged commit 668b3e4 into rust-lang:master Feb 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `redundant_slicing` lint #8218

Improve `redundant_slicing` lint #8218

Jarcho commented Jan 3, 2022 •

edited

Loading

rust-highfive commented Jan 3, 2022

camsteffen commented Jan 6, 2022

Jarcho commented Jan 6, 2022 •

edited

Loading

camsteffen commented Jan 6, 2022

camsteffen commented Jan 6, 2022

Manishearth commented Jan 6, 2022 •

edited

Loading

Jarcho commented Jan 6, 2022

camsteffen commented Jan 7, 2022

camsteffen commented Jan 11, 2022

flip1995 commented Jan 12, 2022 •

edited

Loading

Jarcho commented Jan 12, 2022

camsteffen commented Jan 12, 2022

Jarcho commented Jan 12, 2022

camsteffen commented Jan 13, 2022

Jarcho commented Jan 13, 2022

camsteffen Jan 15, 2022

camsteffen Feb 4, 2022

Jarcho Feb 4, 2022

camsteffen Feb 4, 2022

Jarcho Feb 4, 2022

camsteffen Feb 4, 2022

Jarcho Feb 6, 2022

camsteffen Feb 8, 2022

Jarcho Feb 9, 2022 •

edited

Loading

camsteffen Feb 9, 2022

Jarcho Feb 9, 2022

bors commented Feb 10, 2022

camsteffen commented Feb 16, 2022

bors commented Feb 16, 2022

camsteffen commented Feb 16, 2022

camsteffen commented Feb 17, 2022

bors commented Feb 17, 2022

bors commented Feb 17, 2022

bors commented Feb 17, 2022

Improve redundant_slicing lint #8218

Improve redundant_slicing lint #8218

Conversation

Jarcho commented Jan 3, 2022 • edited Loading

rust-highfive commented Jan 3, 2022

camsteffen commented Jan 6, 2022

Jarcho commented Jan 6, 2022 • edited Loading

camsteffen commented Jan 6, 2022

camsteffen commented Jan 6, 2022

Manishearth commented Jan 6, 2022 • edited Loading

Jarcho commented Jan 6, 2022

camsteffen commented Jan 7, 2022

camsteffen commented Jan 11, 2022

flip1995 commented Jan 12, 2022 • edited Loading

Jarcho commented Jan 12, 2022

camsteffen commented Jan 12, 2022

Jarcho commented Jan 12, 2022

camsteffen commented Jan 13, 2022

Jarcho commented Jan 13, 2022

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

Jarcho Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 10, 2022

camsteffen commented Feb 16, 2022

bors commented Feb 16, 2022

camsteffen commented Feb 16, 2022

camsteffen commented Feb 17, 2022

bors commented Feb 17, 2022

bors commented Feb 17, 2022

bors commented Feb 17, 2022

Improve `redundant_slicing` lint #8218

Improve `redundant_slicing` lint #8218

Jarcho commented Jan 3, 2022 •

edited

Loading

Jarcho commented Jan 6, 2022 •

edited

Loading

Manishearth commented Jan 6, 2022 •

edited

Loading

flip1995 commented Jan 12, 2022 •

edited

Loading

Jarcho Feb 9, 2022 •

edited

Loading