Add manual_as_slice lint

[nils-degroot]

View all comments

This pr adds the manual_as_slice lint.

closes: #7633

---

changelog: [manual_as_slice]: new lint

[llogiq]

This looks like a good starting point. I'd want some more test cases, and there is some duplication we can remove, but otherwise this seems mostly merge-worthy. I'd like to rename the lint though, it's too wordy. How about manual_as_slice? That fits very well into our naming structure. Also please squash your commits when ready.

[nils-degroot]

I like manual_as_slice, it shows the intended behavior for the lint. I'll make the changes

[llogiq]

I think you need to re-bless the tests.

[samueltardieu]

You should at least check that:

• the resulting type is a slice
• the .as_slice()/.as_mut_slice() methods exist on the original type

or restrict the original type to those coming from core/alloc.

Otherwise, you take the risk of having a type implement [..] for something else (let's say, in a domain-specific-language) without having .as_slice() available. Here is an example:

#![feature(new_range_api)]
use std::ops::Index;
use std::range::RangeBounds;

struct Count;
impl<R: RangeBounds<()>> Index<R> for Count {
    type Output = ();
    fn index(&self, _: R) -> &Self::Output {
        &()
    }
}

fn main() {
    _ = &Count[..];
}

It will suggest to use Count.as_slice() even though this method does not exist on Count.

[nils-degroot]

I'm gonna restrict is to Vec, array and slice in that case, since doing this on a string also would not work

[nils-degroot]

Is it possible to get this working on vec since its not a language item?

[samueltardieu]

Is it possible to get this working on vec since its not a language item?

Language items are items that the compiler needs when it transforms code. For example, it needs panic!() as a language item, as it will generate calls to it. alloc::vec::Vec is not in this category, as the compiler never needs to transform the code in a way that would build a vector.

However, Vec is a diagnostic item, which means that you can get methods from clippy_utils (look for "diagnostic" there) to identify whether something is a Vec or not. Most of the things Clippy needs to identify in the standard library are marked as diagnostic items, and we can mark new items if we need them, so that we can easily use them in lints.

[llogiq]

I'd like the message to be shorter. Otherwise this looks mostly good. We may want to select a different lint group though. I'll start the final comment period soon.

[Jarcho]

Ping @nils-degroot are you willing to continue working on this?

r? Jarcho

[nils-degroot]

Hey, forgot this was open. I'll handle the comments and rebase now

[github-actions]

Lintcheck changes for 009e84d

Lint	Added	Removed	Changed
clippy::manual_as_slice	204	0	0

This comment will be updated if you push new changes

[ada4a]

@rustbot label lint-nominated

View changes since this review

[rustbot]

This lint has been nominated for inclusion.

A FCP topic has been created on Zulip.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[ada4a]

Just some nits, looks great otherwise:)

View changes since this review

[nils-degroot]

Thanks for the help 🙏, I've resolved the comments. Should I resolve the dogfood comments by running clippy fix? I can't really find documentation about whats normal for this

[ada4a]

Ah, that's a good point, we should have docs for this.

In general, the dogfood comments should be resolved, yes – unless you notice a diagnostic which looks broken, in which case you should fix the lint instead.

Please make a separate commit for the dogfood fixes (a description as simple as fix dogfood would be fine), and have it come before the commit that actually implements the lint – this helps with reverting and whatnot.

[rustbot]

Some changes occurred in clippy_lints/src/doc

cc @notriddle

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ada4a]

This comment applies to the case above as well I think? So maybe move it there, and also s/shoudl/should.

Btw, is there a reason to have these wrapper structs? Why not use Box<Vec<u8>> and Box<[u8]> directly?

View changes since the review

[ada4a]

Suggested change

	// unsized slices don't have a as_slice implementations, so don't suggest the lint
	// unsized slices don't have the `.as_slice()` method, so don't suggest the lint

View changes since the review

[ada4a]

nit: I think you can expect box_collection instead of allowing, possibly the other lint as well. That's preferred because it would warn us when the expect is no longer needed

View changes since the review

[rustbot]

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