Add manual_as_slice lint

[nils-degroot]

View all comments

This pr adds the manual_as_slice lint.

closes: #7633

---

changelog: add [manual_as_slice] 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.

[ada4a]

since doing this on a string also would not work

I think it would work, you'd just need to suggest as_str/as_str_mut instead. But this could be left as a future improvement

[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

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

[github-actions]

Lintcheck changes for dabee41

Lint	Added	Removed	Changed
clippy::manual_as_slice	201	0	0

This comment will be updated if you push new changes

[ada4a]

@rustbot label lint-nominated

View changes since this review

[ada4a]

Suggested change

	/// Using the `some_value.as_slice()` method is more explicit then using `&some_value[..]`
	/// Using the `some_value.as_slice()` method is more explicit than using `&some_value[..]`

[ada4a]

Suggested change

	#[clippy::version = "1.88.0"]
	#[clippy::version = "1.100.0"]

[ada4a]

Suggested change

	nursery,
	pedantic,

[ada4a]

nit: could extend the let-chain to make the code more concise

Suggested change

	if expr.span.from_expansion() {
	return;
	}
	if let ExprKind::AddrOf(_, mutability, borrow) = expr.kind
	if !expr.span.from_expansion()
	&& let ExprKind::AddrOf(_, mutability, borrow) = expr.kind

[ada4a]

let's make this a bit more descriptive

Suggested change

	"try",
	format!("use `{sugg_tail}` instead"),

[ada4a]

I think you could simplify this down to

Suggested change

	{
	match cx.typeck_results().expr_ty(value).kind() {
	ty::Array(_, _) | ty::Slice(_) => {},
	ty::Ref(_, t, _) if let ty::Array(_, _) | ty::Slice(_) = t.kind() => {},
	ty::Adt(adt, _) if cx.tcx.is_diagnostic_item(sym::Vec, adt.did()) => {},
	_ => return,
	}
	&& !is_slice_like(cx.typeck_results().expr_ty_adjusted(value))
	{

using clippy_utils::is_slice_like

[ada4a]

lintcheck shows that this lint currently fires on &[][..], for which [].as_slice() sounds overkill imo... consider not linting in that case?

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