Skip to content

Add slice_as_bytes lint #10984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5205,6 +5205,7 @@ Released 2018-09-13
[`size_of_in_element_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_in_element_count
[`size_of_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_ref
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
[`slice_as_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#slice_as_bytes
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
[`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::SINGLE_CHAR_ADD_STR_INFO,
crate::methods::SINGLE_CHAR_PATTERN_INFO,
crate::methods::SKIP_WHILE_NEXT_INFO,
crate::methods::SLICE_AS_BYTES_INFO,
crate::methods::STABLE_SORT_PRIMITIVE_INFO,
crate::methods::STRING_EXTEND_CHARS_INFO,
crate::methods::SUSPICIOUS_COMMAND_ARG_SPACE_INFO,
Expand Down
25 changes: 25 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ mod single_char_insert_string;
mod single_char_pattern;
mod single_char_push_string;
mod skip_while_next;
mod slice_as_bytes;
mod stable_sort_primitive;
mod str_splitn;
mod string_extend_chars;
Expand Down Expand Up @@ -3316,6 +3317,26 @@ declare_clippy_lint! {
"checks for usage of `Iterator::fold` with a type that implements `Try`"
}

declare_clippy_lint! {
/// Checks for string slices immediantly followed by `as_bytes`.
/// ### Why is this bad?
/// It involves doing an unnecessary UTF-8 alignment check which is less efficient, and can cause a panic.
/// ### Example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A ### Known problems section is needed with the note that the panic may be required for the code's correctness.

/// ```rust
/// let s = "Lorem ipsum";
/// s[1..5].as_bytes();
/// ```
/// Use instead:
/// ```rust
/// let s = "Lorem ipsum";
/// &s.as_bytes()[1..5];
/// ```
#[clippy::version = "1.72.0"]
pub SLICE_AS_BYTES,
perf,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the behaviour change and the small perf gain, this should not be linting by default. pedantic would be a better category.

"slicing a string and immediately calling as_bytes is less efficient and can lead to panics"
}

pub struct Methods {
avoid_breaking_exported_api: bool,
msrv: Msrv,
Expand Down Expand Up @@ -3448,6 +3469,7 @@ impl_lint_pass!(Methods => [
UNNECESSARY_LITERAL_UNWRAP,
DRAIN_COLLECT,
MANUAL_TRY_FOLD,
SLICE_AS_BYTES,
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -3656,6 +3678,9 @@ impl Methods {
("arg", [arg]) => {
suspicious_command_arg_space::check(cx, recv, arg, span);
}
("as_bytes",[]) => {
slice_as_bytes::check(cx, expr, recv);
}
("as_deref" | "as_deref_mut", []) => {
needless_option_as_deref::check(cx, expr, recv, name);
},
Expand Down
33 changes: 33 additions & 0 deletions clippy_lints/src/methods/slice_as_bytes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
use clippy_utils::{diagnostics::span_lint_and_sugg, source::snippet_with_applicability, ty::is_type_lang_item};
use rustc_errors::Applicability;
use rustc_hir::{is_range_literal, Expr, ExprKind, LangItem};
use rustc_lint::LateContext;

use super::SLICE_AS_BYTES;

pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>) {
if let ExprKind::Index(indexed, index) = recv.kind {
if is_range_literal(index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let-chains can be used here. No need for nested if statements.

let ty = cx.typeck_results().expr_ty(indexed).peel_refs();
let is_str = ty.is_str();
let is_string = is_type_lang_item(cx, ty, LangItem::String);
if is_str || is_string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be `ty.is_str() || is_type_lang_item(..)``. They can both just be referred to as a string in the message.

let mut applicability = Applicability::MachineApplicable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be MaybeIncorrect. Same justification as the category change.

let stringish = snippet_with_applicability(cx, indexed.span, "..", &mut applicability);
let range = snippet_with_applicability(cx, index.span, "..", &mut applicability);
let type_name = if is_str { "str" } else { "String" };
span_lint_and_sugg(
cx,
SLICE_AS_BYTES,
expr.span,
&(format!(
"slicing a {type_name} before calling `as_bytes` results in needless UTF-8 alignment checks, and has the possiblity of panicking"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The justification for the lint should not be in the message, only a short description of what is being linted. The rest of it should just be in the documentation, or if needed, as an additional note.

)),
"try",
format!("&{stringish}.as_bytes()[{range}]"),
applicability,
);
}
}
}
}
1 change: 1 addition & 0 deletions tests/ui/bytes_nth.fixed
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//@run-rustfix

#![allow(clippy::unnecessary_operation)]
#![allow(clippy::slice_as_bytes)]
#![warn(clippy::bytes_nth)]

fn main() {
Expand Down
1 change: 1 addition & 0 deletions tests/ui/bytes_nth.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//@run-rustfix

#![allow(clippy::unnecessary_operation)]
#![allow(clippy::slice_as_bytes)]
#![warn(clippy::bytes_nth)]

fn main() {
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/bytes_nth.stderr
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
error: called `.bytes().nth()` on a `String`
--> $DIR/bytes_nth.rs:8:13
--> $DIR/bytes_nth.rs:9:13
|
LL | let _ = s.bytes().nth(3);
| ^^^^^^^^^^^^^^^^ help: try: `s.as_bytes().get(3).copied()`
|
= note: `-D clippy::bytes-nth` implied by `-D warnings`

error: called `.bytes().nth().unwrap()` on a `String`
--> $DIR/bytes_nth.rs:9:14
--> $DIR/bytes_nth.rs:10:14
|
LL | let _ = &s.bytes().nth(3).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `s.as_bytes()[3]`

error: called `.bytes().nth()` on a `str`
--> $DIR/bytes_nth.rs:10:13
--> $DIR/bytes_nth.rs:11:13
|
LL | let _ = s[..].bytes().nth(3);
| ^^^^^^^^^^^^^^^^^^^^ help: try: `s[..].as_bytes().get(3).copied()`
Expand Down
35 changes: 35 additions & 0 deletions tests/ui/slice_as_bytes.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//@run-rustfix
#![allow(unused)]
#![warn(clippy::slice_as_bytes)]

use std::ops::{Index, Range};

struct Foo;

struct Bar;

impl Bar {
fn as_bytes(&self) -> &[u8] {
&[0, 1, 2, 3]
}
}

impl Index<Range<usize>> for Foo {
type Output = Bar;

fn index(&self, _: Range<usize>) -> &Self::Output {
&Bar
}
}

fn main() {
let s = "Lorem ipsum";
let string: String = "dolor sit amet".to_owned();

let bytes = &s.as_bytes()[1..5];
let bytes = &string.as_bytes()[1..];
let bytes = &"consectetur adipiscing".as_bytes()[..=5];

let f = Foo;
let bytes = f[0..4].as_bytes();
}
35 changes: 35 additions & 0 deletions tests/ui/slice_as_bytes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//@run-rustfix
#![allow(unused)]
#![warn(clippy::slice_as_bytes)]

use std::ops::{Index, Range};

struct Foo;

struct Bar;

impl Bar {
fn as_bytes(&self) -> &[u8] {
&[0, 1, 2, 3]
}
}

impl Index<Range<usize>> for Foo {
type Output = Bar;

fn index(&self, _: Range<usize>) -> &Self::Output {
&Bar
}
}

fn main() {
let s = "Lorem ipsum";
let string: String = "dolor sit amet".to_owned();

let bytes = s[1..5].as_bytes();
let bytes = string[1..].as_bytes();
let bytes = "consectetur adipiscing"[..=5].as_bytes();

let f = Foo;
let bytes = f[0..4].as_bytes();
}
22 changes: 22 additions & 0 deletions tests/ui/slice_as_bytes.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error: slicing a str before calling `as_bytes` results in needless UTF-8 alignment checks, and has the possiblity of panicking
--> $DIR/slice_as_bytes.rs:29:17
|
LL | let bytes = s[1..5].as_bytes();
| ^^^^^^^^^^^^^^^^^^ help: try: `&s.as_bytes()[1..5]`
|
= note: `-D clippy::slice-as-bytes` implied by `-D warnings`

error: slicing a String before calling `as_bytes` results in needless UTF-8 alignment checks, and has the possiblity of panicking
--> $DIR/slice_as_bytes.rs:30:17
|
LL | let bytes = string[1..].as_bytes();
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `&string.as_bytes()[1..]`

error: slicing a str before calling `as_bytes` results in needless UTF-8 alignment checks, and has the possiblity of panicking
--> $DIR/slice_as_bytes.rs:31:17
|
LL | let bytes = "consectetur adipiscing"[..=5].as_bytes();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `&"consectetur adipiscing".as_bytes()[..=5]`

error: aborting due to 3 previous errors