Skip to content

Add a new lint to check for defining of read-only arrays on stack #12854

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 10 commits into from
Closed
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5435,6 +5435,7 @@ Released 2018-09-13
[`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
[`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
[`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
[`let_arr_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_arr_const
[`let_underscore_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_drop
[`let_underscore_future`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_future
[`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock
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 @@ -260,6 +260,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::len_zero::COMPARISON_TO_EMPTY_INFO,
crate::len_zero::LEN_WITHOUT_IS_EMPTY_INFO,
crate::len_zero::LEN_ZERO_INFO,
crate::let_arr_const::LET_ARR_CONST_INFO,
crate::let_if_seq::USELESS_LET_IF_SEQ_INFO,
crate::let_underscore::LET_UNDERSCORE_FUTURE_INFO,
crate::let_underscore::LET_UNDERSCORE_LOCK_INFO,
Expand Down
110 changes: 110 additions & 0 deletions clippy_lints/src/let_arr_const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::{implements_trait, is_type_lang_item};
use clippy_utils::visitors::is_const_evaluatable;
use hir::{BindingMode, ExprKind, LangItem, PatKind, Stmt, StmtKind};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_lint_pass;

declare_clippy_lint! {
/// ### What it does
/// Checks for defining of read-only arrays on stack.
///
/// ### Why is this bad?
/// `let array` puts array on the stack. As as intended, the compiler will
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `let array` puts array on the stack. As as intended, the compiler will
/// `let array` puts the array on the stack. As directed, the compiler will

/// initialize the array directly on the stack, which might make the
/// generated binary bigger and slower.
///
/// A read-only array should be defined as a `static` item or used a trick
/// to made it into `.rodata` section of the compiled program. The use of
/// the trick (`*&<array literal>`) will make rustc static-promote the
/// array literal. Which means that the array now lives in the read-only
/// section of the generated binary. The downside of the trick is that
/// it is non-ergonomic and may not be very clear to readers of the code.
///
/// ### Known problems
///
/// ### Example
/// ```rust,ignore
/// let a: [u32; N] = [1, 3, 5, ...];
/// ```
///
/// Use instead:
/// ```rust,ignore
/// let a: [u32; N] = *&[1, 3, 5, ...];
/// // or
/// static A: [u32; N] = [1, 3, 5, ...];
/// ```
#[clippy::version = "1.80.0"]
pub LET_ARR_CONST,
perf,
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should be (at most) a pedantic lint, at least for such a small threshold of 16 bytes. It seems like a micro optimization in cases like this one where it's just two string slices.

It would also have a fair number of false positives when MaybeUninit is involved. E.g.

struct Buffer([MaybeUninit<u8>; 40]);
impl Buffer {
  pub fn new() -> Self {
    let bytes = [MaybeUninit::uninit(); 40];
    Self(bytes)
  }
}

I don't think a static would really help here. There's no initialization code involved (taken from the itoa crate)

Copy link
Contributor Author

@tesuji tesuji May 27, 2024

Choose a reason for hiding this comment

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

Thanks for letting me know. I haven't considered this case. I would try to exclude this in code.

And for threshold of 16 bytes, I would try to come up with a micro benchmark.

"declare a read-only array on stack"
}

declare_lint_pass!(LetArrConst => [LET_ARR_CONST]);

// FIXME: See also LARGE_CONST_ARRAYS, LARGE_STACK_ARRAYS.
impl<'tcx> LateLintPass<'tcx> for LetArrConst {
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
// let pat: ty = init;
if let StmtKind::Let(local) = stmt.kind
&& let PatKind::Binding(BindingMode::NONE, _, _name, None) = local.pat.kind
&& let Some(init) = local.init
&& !init.span.from_expansion()
{
// LLVM optimizes the load of 16 byte as a single `mov`.
// Bigger values make more `mov` instructions generated.
// While changing code as this lint suggests, it becomes
// a single load (`lea`) of an address in `.rodata`.
const STACK_THRESHOLD: u64 = 16;
// lint only `if size_of(init) > STACK_THRESHOLD`
let ty = cx.typeck_results().expr_ty(init);
if let Ok(layout) = cx.tcx.layout_of(cx.param_env.and(ty))
&& let size = layout.layout.size()
&& size.bytes() <= STACK_THRESHOLD
{
return;
}

let mut applicability = Applicability::MachineApplicable;
let lang_items = cx.tcx.lang_items();
let Some(copy_id) = lang_items.copy_trait() else {
return;
};

// if init is [<Copy type>; N]
if let ExprKind::Repeat(..) = init.kind {
// `init` optimized as the same code.
return;
}

// if init is [1, 2, 3, ...]
let ExprKind::Array(items @ [ref expr, ..]) = init.kind else {
return;
};

let ty = cx.typeck_results().expr_ty(expr);
// skip MaybeUnint and ManuallyDrop types
if is_type_lang_item(cx, ty, LangItem::MaybeUninit) || is_type_lang_item(cx, ty, LangItem::ManuallyDrop) {
return;
}

if implements_trait(cx, ty, copy_id, &[]) && items.iter().all(|expr| is_const_evaluatable(cx, expr)) {
let msg = "using `static` to push the array to read-only section of program or try";
let snippet = snippet_with_applicability(cx, init.span, "_", &mut applicability);
let sugg = format!("*&{snippet}");
span_lint_and_sugg(
cx,
LET_ARR_CONST,
init.span,
"declaring a read-only array on the stack",
msg,
sugg,
applicability,
);
}
}
}
}
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ mod large_stack_arrays;
mod large_stack_frames;
mod legacy_numeric_constants;
mod len_zero;
mod let_arr_const;
mod let_if_seq;
mod let_underscore;
mod let_with_type_underscore;
Expand Down Expand Up @@ -1165,6 +1166,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
..Default::default()
})
});
store.register_late_pass(|_| Box::new(let_arr_const::LetArrConst));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
24 changes: 12 additions & 12 deletions clippy_lints/src/methods/option_as_ref_deref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@ pub(super) fn check(
is_mut: bool,
msrv: &Msrv,
) {
static DEREF_ALIASES: [&[&str]; 7] = [
&paths::CSTRING_AS_C_STR,
&paths::OS_STRING_AS_OS_STR,
&paths::PATH_BUF_AS_PATH,
&paths::STRING_AS_STR,
&paths::STRING_AS_MUT_STR,
&paths::VEC_AS_SLICE,
&paths::VEC_AS_MUT_SLICE,
];

if !msrv.meets(msrvs::OPTION_AS_DEREF) {
return;
}
Expand All @@ -31,24 +41,14 @@ pub(super) fn check(
return;
}

let deref_aliases: [&[&str]; 7] = [
&paths::CSTRING_AS_C_STR,
&paths::OS_STRING_AS_OS_STR,
&paths::PATH_BUF_AS_PATH,
&paths::STRING_AS_STR,
&paths::STRING_AS_MUT_STR,
&paths::VEC_AS_SLICE,
&paths::VEC_AS_MUT_SLICE,
];

let is_deref = match map_arg.kind {
hir::ExprKind::Path(ref expr_qpath) => {
cx.qpath_res(expr_qpath, map_arg.hir_id)
.opt_def_id()
.map_or(false, |fun_def_id| {
cx.tcx.is_diagnostic_item(sym::deref_method, fun_def_id)
|| cx.tcx.is_diagnostic_item(sym::deref_mut_method, fun_def_id)
|| deref_aliases.iter().any(|path| match_def_path(cx, fun_def_id, path))
|| DEREF_ALIASES.iter().any(|path| match_def_path(cx, fun_def_id, path))
})
},
hir::ExprKind::Closure(&hir::Closure { body, .. }) => {
Expand All @@ -69,7 +69,7 @@ pub(super) fn check(
let method_did = cx.typeck_results().type_dependent_def_id(closure_expr.hir_id).unwrap();
cx.tcx.is_diagnostic_item(sym::deref_method, method_did)
|| cx.tcx.is_diagnostic_item(sym::deref_mut_method, method_did)
|| deref_aliases.iter().any(|path| match_def_path(cx, method_did, path))
|| DEREF_ALIASES.iter().any(|path| match_def_path(cx, method_did, path))
} else {
false
}
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/reference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ impl EarlyLintPass for DerefAddrOf {
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
if let ExprKind::Unary(UnOp::Deref, ref deref_target) = e.kind
&& let ExprKind::AddrOf(_, ref mutability, ref addrof_target) = without_parens(deref_target).kind
// NOTE(tesuji): without it, this lints conflicts with `let-arr-const` lint.
&& !matches!(addrof_target.kind, ExprKind::Array(_) | ExprKind::Repeat(..))
&& deref_target.span.eq_ctxt(e.span)
&& !addrof_target.span.from_expansion()
{
Expand Down
2 changes: 1 addition & 1 deletion lintcheck/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ fn clippy_project_root() -> &'static Path {

#[test]
fn lintcheck_test() {
let args = [
let args = *&[
"run",
"--target-dir",
"lintcheck/target",
Expand Down
3 changes: 2 additions & 1 deletion tests/ui-toml/disallowed_names_append/disallowed_names.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#[warn(clippy::disallowed_names)]
#![warn(clippy::disallowed_names)]
#![allow(clippy::let_arr_const)]

fn main() {
// `foo` is part of the default configuration
Expand Down
4 changes: 2 additions & 2 deletions tests/ui-toml/disallowed_names_append/disallowed_names.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: use of a disallowed/placeholder name `foo`
--> tests/ui-toml/disallowed_names_append/disallowed_names.rs:5:9
--> tests/ui-toml/disallowed_names_append/disallowed_names.rs:6:9
|
LL | let foo = "bar";
| ^^^
Expand All @@ -8,7 +8,7 @@ LL | let foo = "bar";
= help: to override `-D warnings` add `#[allow(clippy::disallowed_names)]`

error: use of a disallowed/placeholder name `ducks`
--> tests/ui-toml/disallowed_names_append/disallowed_names.rs:7:9
--> tests/ui-toml/disallowed_names_append/disallowed_names.rs:8:9
|
LL | let ducks = ["quack", "quack"];
| ^^^^^
Expand Down
3 changes: 2 additions & 1 deletion tests/ui-toml/disallowed_names_replace/disallowed_names.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#[warn(clippy::disallowed_names)]
#![warn(clippy::disallowed_names)]
#![allow(clippy::let_arr_const)]

fn main() {
// `foo` is part of the default configuration
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: use of a disallowed/placeholder name `ducks`
--> tests/ui-toml/disallowed_names_replace/disallowed_names.rs:7:9
--> tests/ui-toml/disallowed_names_replace/disallowed_names.rs:8:9
|
LL | let ducks = ["quack", "quack"];
| ^^^^^
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/large_futures/large_futures.fixed
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::large_futures)]
#![allow(clippy::let_arr_const)]

fn main() {}

Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/large_futures/large_futures.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::large_futures)]
#![allow(clippy::let_arr_const)]

fn main() {}

Expand Down
2 changes: 1 addition & 1 deletion tests/ui-toml/large_futures/large_futures.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: large future with a size of 1026 bytes
--> tests/ui-toml/large_futures/large_futures.rs:18:5
--> tests/ui-toml/large_futures/large_futures.rs:19:5
|
LL | should_warn().await;
| ^^^^^^^^^^^^^ help: consider `Box::pin` on it: `Box::pin(should_warn())`
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/too_large_for_stack/useless_vec.fixed
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::useless_vec)]
#![allow(clippy::let_arr_const)]

fn main() {
let x = [0u8; 500];
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/too_large_for_stack/useless_vec.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::useless_vec)]
#![allow(clippy::let_arr_const)]

fn main() {
let x = vec![0u8; 500];
Expand Down
2 changes: 1 addition & 1 deletion tests/ui-toml/too_large_for_stack/useless_vec.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: useless use of `vec!`
--> tests/ui-toml/too_large_for_stack/useless_vec.rs:4:13
--> tests/ui-toml/too_large_for_stack/useless_vec.rs:5:13
|
LL | let x = vec![0u8; 500];
| ^^^^^^^^^^^^^^ help: you can use an array directly: `[0u8; 500]`
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-12253.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#[allow(overflowing_literals, unconditional_panic, clippy::no_effect)]
#[allow(overflowing_literals, unconditional_panic, clippy::no_effect, clippy::let_arr_const)]
fn main() {
let arr: [i32; 5] = [0; 5];
arr[0xfffffe7ffffffffffffffffffffffff];
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-5389.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![allow(clippy::explicit_counter_loop)]
#![allow(clippy::explicit_counter_loop, clippy::let_arr_const)]

fn main() {
let v = vec![1, 2, 3];
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/default_numeric_fallback_f64.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
clippy::branches_sharing_code,
clippy::match_single_binding,
clippy::let_unit_value,
clippy::let_with_type_underscore
clippy::let_with_type_underscore,
clippy::let_arr_const
)]

extern crate proc_macros;
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/default_numeric_fallback_f64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
clippy::branches_sharing_code,
clippy::match_single_binding,
clippy::let_unit_value,
clippy::let_with_type_underscore
clippy::let_with_type_underscore,
clippy::let_arr_const
)]

extern crate proc_macros;
Expand Down
Loading
Loading