Skip to content

fix [large_stack_arrays] linting in vec macro #12624

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

Merged
merged 2 commits into from
Apr 27, 2024
Merged
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
6 changes: 2 additions & 4 deletions clippy_lints/src/format_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::is_diag_trait_item;
use clippy_utils::macros::{
find_format_arg_expr, find_format_args, format_arg_removal_span, format_placeholder_format_span, is_assert_macro,
is_format_macro, is_panic, root_macro_call, root_macro_call_first_node, FormatParamUsage, MacroCall,
is_format_macro, is_panic, matching_root_macro_call, root_macro_call_first_node, FormatParamUsage, MacroCall,
};
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::{implements_trait, is_type_lang_item};
Expand Down Expand Up @@ -271,9 +271,7 @@ impl<'a, 'tcx> FormatArgsExpr<'a, 'tcx> {
let mut suggest_format = |spec| {
let message = format!("for the {spec} to apply consider using `format!()`");

if let Some(mac_call) = root_macro_call(arg_span)
&& self.cx.tcx.is_diagnostic_item(sym::format_args_macro, mac_call.def_id)
{
if let Some(mac_call) = matching_root_macro_call(self.cx, arg_span, sym::format_args_macro) {
diag.span_suggestion(
self.cx.sess().source_map().span_until_char(mac_call.span, '!'),
message,
Expand Down
65 changes: 55 additions & 10 deletions clippy_lints/src/large_stack_arrays.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::is_from_proc_macro;
use clippy_utils::macros::macro_backtrace;
use clippy_utils::source::snippet;
use rustc_hir::{Expr, ExprKind, Item, ItemKind, Node};
use rustc_hir::{ArrayLen, Expr, ExprKind, Item, ItemKind, Node};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::ty::{self, ConstKind};
use rustc_session::impl_lint_pass;
use rustc_span::{sym, Span};

declare_clippy_lint! {
/// ### What it does
Expand All @@ -25,20 +28,41 @@ declare_clippy_lint! {

pub struct LargeStackArrays {
maximum_allowed_size: u128,
prev_vec_macro_callsite: Option<Span>,
}

impl LargeStackArrays {
#[must_use]
pub fn new(maximum_allowed_size: u128) -> Self {
Self { maximum_allowed_size }
Self {
maximum_allowed_size,
prev_vec_macro_callsite: None,
}
}

/// Check if the given span of an expr is already in a `vec!` call.
fn is_from_vec_macro(&mut self, cx: &LateContext<'_>, span: Span) -> bool {
// First, we check if this is span is within the last encountered `vec!` macro's root callsite.
self.prev_vec_macro_callsite
.is_some_and(|vec_mac| vec_mac.contains(span))
|| {
// Then, we try backtracking the macro expansions, to see if there's a `vec!` macro,
// and update the `prev_vec_macro_callsite`.
let res = macro_backtrace(span).any(|mac| cx.tcx.is_diagnostic_item(sym::vec_macro, mac.def_id));
if res {
self.prev_vec_macro_callsite = Some(span.source_callsite());
}
res
}
}
}

impl_lint_pass!(LargeStackArrays => [LARGE_STACK_ARRAYS]);

impl<'tcx> LateLintPass<'tcx> for LargeStackArrays {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if let ExprKind::Repeat(_, _) | ExprKind::Array(_) = expr.kind
&& !self.is_from_vec_macro(cx, expr.span)
&& let ty::Array(element_type, cst) = cx.typeck_results().expr_ty(expr).kind()
&& let ConstKind::Value(ty::ValTree::Leaf(element_count)) = cst.kind()
&& let Ok(element_count) = element_count.try_to_target_usize(cx.tcx)
Expand All @@ -54,20 +78,41 @@ impl<'tcx> LateLintPass<'tcx> for LargeStackArrays {
})
&& self.maximum_allowed_size < u128::from(element_count) * u128::from(element_size)
{
span_lint_and_help(
span_lint_and_then(
cx,
LARGE_STACK_ARRAYS,
expr.span,
format!(
"allocating a local array larger than {} bytes",
self.maximum_allowed_size
),
None,
format!(
"consider allocating on the heap with `vec!{}.into_boxed_slice()`",
snippet(cx, expr.span, "[...]")
),
|diag| {
if !might_be_expanded(cx, expr) {
diag.help(format!(
"consider allocating on the heap with `vec!{}.into_boxed_slice()`",
snippet(cx, expr.span, "[...]")
));
}
},
);
}
}
}

/// Only giving help messages if the expr does not contains macro expanded codes.
fn might_be_expanded<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
/// Check if the span of `ArrayLen` of a repeat expression is within the expr's span,
/// if not, meaning this repeat expr is definitely from some proc-macro.
///
/// This is a fail-safe to a case where even the `is_from_proc_macro` is unable to determain the
/// correct result.
fn repeat_expr_might_be_expanded<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
let ExprKind::Repeat(_, ArrayLen::Body(anon_const)) = expr.kind else {
return false;
};
let len_span = cx.tcx.def_span(anon_const.def_id);
!expr.span.contains(len_span)
}
Comment on lines +104 to +115
Copy link
Member Author

Choose a reason for hiding this comment

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

therefore I have to use this hacky way to check, but I don't know if this can always work or just coincidentally succeed in my specific test case.

Copy link
Member

Choose a reason for hiding this comment

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

I think this hack is good enough. quote_spanned is sadly pretty bad for rust tools, we just have to hope that users of quote_spanned understand what they're doing


expr.span.from_expansion() || is_from_proc_macro(cx, expr) || repeat_expr_might_be_expanded(cx, expr)
}
5 changes: 2 additions & 3 deletions clippy_lints/src/manual_assert.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use crate::rustc_lint::LintContext;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::macros::root_macro_call;
use clippy_utils::macros::{is_panic, root_macro_call};
use clippy_utils::{is_else_clause, is_parent_stmt, peel_blocks_with_stmt, span_extract_comment, sugg};
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::declare_lint_pass;
use rustc_span::sym;

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -42,7 +41,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAssert {
&& !expr.span.from_expansion()
&& let then = peel_blocks_with_stmt(then)
&& let Some(macro_call) = root_macro_call(then.span)
&& cx.tcx.item_name(macro_call.def_id) == sym::panic
&& is_panic(cx, macro_call.def_id)
&& !cx.tcx.sess.source_map().is_multiline(cond.span)
&& let Ok(panic_snippet) = cx.sess().source_map().span_to_snippet(macro_call.span)
&& let Some(panic_snippet) = panic_snippet.strip_suffix(')')
Expand Down
15 changes: 2 additions & 13 deletions clippy_lints/src/manual_is_ascii_check.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::macros::root_macro_call;
use clippy_utils::macros::matching_root_macro_call;
use clippy_utils::sugg::Sugg;
use clippy_utils::{higher, in_constant};
use rustc_ast::ast::RangeLimits;
Expand All @@ -9,7 +9,6 @@ use rustc_errors::Applicability;
use rustc_hir::{BorrowKind, Expr, ExprKind, PatKind, RangeEnd};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
use rustc_span::def_id::DefId;
use rustc_span::{sym, Span};

declare_clippy_lint! {
Expand Down Expand Up @@ -97,9 +96,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualIsAsciiCheck {
return;
}

if let Some(macro_call) = root_macro_call(expr.span)
&& is_matches_macro(cx, macro_call.def_id)
{
if let Some(macro_call) = matching_root_macro_call(cx, expr.span, sym::matches_macro) {
if let ExprKind::Match(recv, [arm, ..], _) = expr.kind {
let range = check_pat(&arm.pat.kind);
check_is_ascii(cx, macro_call.span, recv, &range);
Expand Down Expand Up @@ -187,11 +184,3 @@ fn check_range(start: &Expr<'_>, end: &Expr<'_>) -> CharRange {
CharRange::Otherwise
}
}

fn is_matches_macro(cx: &LateContext<'_>, macro_def_id: DefId) -> bool {
if let Some(name) = cx.tcx.get_diagnostic_name(macro_def_id) {
return sym::matches_macro == name;
}

false
}
5 changes: 2 additions & 3 deletions clippy_lints/src/methods/filter_map.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::macros::{is_panic, root_macro_call};
use clippy_utils::macros::{is_panic, matching_root_macro_call, root_macro_call};
use clippy_utils::source::{indent_of, reindent_multiline, snippet};
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{higher, is_trait_method, path_to_local_id, peel_blocks, SpanlessEq};
Expand Down Expand Up @@ -247,8 +247,7 @@ impl<'tcx> OffendingFilterExpr<'tcx> {
} else {
None
}
} else if let Some(macro_call) = root_macro_call(expr.span)
&& cx.tcx.get_diagnostic_name(macro_call.def_id) == Some(sym::matches_macro)
} else if matching_root_macro_call(cx, expr.span, sym::matches_macro).is_some()
// we know for a fact that the wildcard pattern is the second arm
&& let ExprKind::Match(scrutinee, [arm, _], _) = expr.kind
&& path_to_local_id(scrutinee, filter_param_id)
Expand Down
5 changes: 2 additions & 3 deletions clippy_lints/src/repeat_vec_with_capacity.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use clippy_utils::consts::{constant, Constant};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::higher::VecArgs;
use clippy_utils::macros::root_macro_call;
use clippy_utils::macros::matching_root_macro_call;
use clippy_utils::source::snippet;
use clippy_utils::{expr_or_init, fn_def_id, match_def_path, paths};
use rustc_errors::Applicability;
Expand Down Expand Up @@ -65,8 +65,7 @@ fn emit_lint(cx: &LateContext<'_>, span: Span, kind: &str, note: &'static str, s

/// Checks `vec![Vec::with_capacity(x); n]`
fn check_vec_macro(cx: &LateContext<'_>, expr: &Expr<'_>) {
if let Some(mac_call) = root_macro_call(expr.span)
&& cx.tcx.is_diagnostic_item(sym::vec_macro, mac_call.def_id)
if matching_root_macro_call(cx, expr.span, sym::vec_macro).is_some()
&& let Some(VecArgs::Repeat(repeat_expr, len_expr)) = VecArgs::hir(cx, expr)
&& fn_def_id(cx, repeat_expr).is_some_and(|did| match_def_path(cx, did, &paths::VEC_WITH_CAPACITY))
&& !len_expr.span.from_expansion()
Expand Down
6 changes: 2 additions & 4 deletions clippy_lints/src/slow_vector_initialization.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::macros::root_macro_call;
use clippy_utils::macros::matching_root_macro_call;
use clippy_utils::sugg::Sugg;
use clippy_utils::{
get_enclosing_block, is_expr_path_def_path, is_integer_literal, is_path_diagnostic_item, path_to_local,
Expand Down Expand Up @@ -145,9 +145,7 @@ impl SlowVectorInit {
// Generally don't warn if the vec initializer comes from an expansion, except for the vec! macro.
// This lets us still warn on `vec![]`, while ignoring other kinds of macros that may output an
// empty vec
if expr.span.from_expansion()
&& root_macro_call(expr.span).map(|m| m.def_id) != cx.tcx.get_diagnostic_item(sym::vec_macro)
{
if expr.span.from_expansion() && matching_root_macro_call(cx, expr.span, sym::vec_macro).is_none() {
return None;
}

Expand Down
10 changes: 10 additions & 0 deletions clippy_utils/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,20 @@ pub fn macro_backtrace(span: Span) -> impl Iterator<Item = MacroCall> {

/// If the macro backtrace of `span` has a macro call at the root expansion
/// (i.e. not a nested macro call), returns `Some` with the `MacroCall`
///
/// If you only want to check whether the root macro has a specific name,
/// consider using [`matching_root_macro_call`] instead.
pub fn root_macro_call(span: Span) -> Option<MacroCall> {
macro_backtrace(span).last()
}

/// A combination of [`root_macro_call`] and
/// [`is_diagnostic_item`](rustc_middle::ty::TyCtxt::is_diagnostic_item) that returns a `MacroCall`
/// at the root expansion if only it matches the given name.
pub fn matching_root_macro_call(cx: &LateContext<'_>, span: Span, name: Symbol) -> Option<MacroCall> {
root_macro_call(span).filter(|mc| cx.tcx.is_diagnostic_item(name, mc.def_id))
}

/// Like [`root_macro_call`], but only returns `Some` if `node` is the "first node"
/// produced by the macro call, as in [`first_node_in_macro`].
pub fn root_macro_call_first_node(cx: &LateContext<'_>, node: &impl HirNode) -> Option<MacroCall> {
Expand Down
17 changes: 17 additions & 0 deletions tests/ui/auxiliary/proc_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use proc_macro::token_stream::IntoIter;
use proc_macro::Delimiter::{self, Brace, Parenthesis};
use proc_macro::Spacing::{self, Alone, Joint};
use proc_macro::{Group, Ident, Literal, Punct, Span, TokenStream, TokenTree as TT};
use syn::spanned::Spanned;

type Result<T> = core::result::Result<T, TokenStream>;

Expand Down Expand Up @@ -124,6 +125,22 @@ fn write_with_span(s: Span, mut input: IntoIter, out: &mut TokenStream) -> Resul
Ok(())
}

/// Takes an array repeat expression such as `[0_u32; 2]`, and return the tokens with 10 times the
/// original size, which turns to `[0_u32; 20]`.
#[proc_macro]
pub fn make_it_big(input: TokenStream) -> TokenStream {
let mut expr_repeat = syn::parse_macro_input!(input as syn::ExprRepeat);
let len_span = expr_repeat.len.span();
if let syn::Expr::Lit(expr_lit) = &mut *expr_repeat.len {
if let syn::Lit::Int(lit_int) = &expr_lit.lit {
let orig_val = lit_int.base10_parse::<usize>().expect("not a valid length parameter");
let new_val = orig_val.saturating_mul(10);
expr_lit.lit = syn::parse_quote_spanned!( len_span => #new_val);
}
}
quote::quote!(#expr_repeat).into()
}

/// Within the item this attribute is attached to, an `inline!` macro is available which expands the
/// contained tokens as though they came from a macro expansion.
///
Expand Down
48 changes: 48 additions & 0 deletions tests/ui/large_stack_arrays.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
//@aux-build:proc_macros.rs
#![warn(clippy::large_stack_arrays)]
#![allow(clippy::large_enum_variant)]

extern crate proc_macros;

#[derive(Clone, Copy)]
struct S {
pub data: [u64; 32],
Expand Down Expand Up @@ -55,3 +58,48 @@ fn main() {
[(); 20_000_000],
);
}

#[allow(clippy::useless_vec)]
fn issue_12586() {
macro_rules! dummy {
($n:expr) => {
$n
};
// Weird rule to test help messages.
($a:expr => $b:expr) => {
[$a, $b, $a, $b]
//~^ ERROR: allocating a local array larger than 512000 bytes
};
($id:ident; $n:literal) => {
dummy!(::std::vec![$id;$n])
};
($($id:expr),+ $(,)?) => {
::std::vec![$($id),*]
}
}
macro_rules! create_then_move {
($id:ident; $n:literal) => {{
let _x_ = [$id; $n];
//~^ ERROR: allocating a local array larger than 512000 bytes
_x_
}};
}

let x = [0u32; 50_000];
let y = vec![x, x, x, x, x];
let y = vec![dummy![x, x, x, x, x]];
let y = vec![dummy![[x, x, x, x, x]]];
let y = dummy![x, x, x, x, x];
let y = [x, x, dummy!(x), x, x];
//~^ ERROR: allocating a local array larger than 512000 bytes
let y = dummy![x => x];
let y = dummy![x;5];
let y = dummy!(vec![dummy![x, x, x, x, x]]);
let y = dummy![[x, x, x, x, x]];
//~^ ERROR: allocating a local array larger than 512000 bytes

let y = proc_macros::make_it_big!([x; 1]);
//~^ ERROR: allocating a local array larger than 512000 bytes
Comment on lines +101 to +102
Copy link
Member Author

Choose a reason for hiding this comment

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

Well... in this test case, the expr being checked is [x; 1], but I guess since the span of 1 was used in quote_spanned(?), for some reason even is_from_proc_macro(cx, expr) fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, runing lintcheck has no differences unfortunately, both gives 4 occurrences lol

Copy link
Member

Choose a reason for hiding this comment

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

I would guess that those occurrences are the result of incorrect quote_spanned usage. Let's get this version merged for now.

let y = vec![proc_macros::make_it_big!([x; 10])];
let y = vec![create_then_move![x; 5]; 5];
}
Loading