From 0eb9f41a07ae7087d7f9ce557cbfcaa2b173c95a Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 24 May 2024 08:43:44 +0700 Subject: [PATCH 1/7] make it more readable by faster early exitting --- clippy_lints/src/no_effect.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 87f886b1128..dc017fa6634 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -258,10 +258,10 @@ fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { fn check_unnecessary_operation(cx: &LateContext<'_>, stmt: &Stmt<'_>) { if let StmtKind::Semi(expr) = stmt.kind + && !in_external_macro(cx.sess(), stmt.span) && let ctxt = stmt.span.ctxt() && expr.span.ctxt() == ctxt && let Some(reduced) = reduce_expression(cx, expr) - && !in_external_macro(cx.sess(), stmt.span) && reduced.iter().all(|e| e.span.ctxt() == ctxt) { if let ExprKind::Index(..) = &expr.kind { From aff0e6d6d3aec4ce29af95a4dcf615650f1c5e4d Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 24 May 2024 09:24:54 +0700 Subject: [PATCH 2/7] add uitest for indexing in unnecessary const items --- tests/ui/unnecessary_operation.fixed | 13 ++++++++++++- tests/ui/unnecessary_operation.rs | 13 ++++++++++++- tests/ui/unnecessary_operation.stderr | 20 +++++++++++++++++++- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/tests/ui/unnecessary_operation.fixed b/tests/ui/unnecessary_operation.fixed index 11761c6c90e..e54491ab023 100644 --- a/tests/ui/unnecessary_operation.fixed +++ b/tests/ui/unnecessary_operation.fixed @@ -43,7 +43,7 @@ fn get_number() -> i32 { 0 } -fn get_usize() -> usize { +const fn get_usize() -> usize { 0 } fn get_struct() -> Struct { @@ -113,4 +113,15 @@ fn main() { 'label: { break 'label }; + let () = const { + assert!([42, 55].len() > get_usize()); + }; +} + +const _: () = { + assert!([42, 55].len() > get_usize()); +}; + +const fn foo() { + assert!([42, 55].len() > get_usize()); } diff --git a/tests/ui/unnecessary_operation.rs b/tests/ui/unnecessary_operation.rs index de0081289ac..679d303f3ad 100644 --- a/tests/ui/unnecessary_operation.rs +++ b/tests/ui/unnecessary_operation.rs @@ -43,7 +43,7 @@ fn get_number() -> i32 { 0 } -fn get_usize() -> usize { +const fn get_usize() -> usize { 0 } fn get_struct() -> Struct { @@ -117,4 +117,15 @@ fn main() { 'label: { break 'label }; + let () = const { + [42, 55][get_usize()]; + }; +} + +const _: () = { + [42, 55][get_usize()]; +}; + +const fn foo() { + [42, 55][get_usize()]; } diff --git a/tests/ui/unnecessary_operation.stderr b/tests/ui/unnecessary_operation.stderr index 27be5e6f4b9..3c031d006d4 100644 --- a/tests/ui/unnecessary_operation.stderr +++ b/tests/ui/unnecessary_operation.stderr @@ -119,5 +119,23 @@ LL | | s: String::from("blah"), LL | | }; | |______^ help: statement can be reduced to: `String::from("blah");` -error: aborting due to 19 previous errors +error: unnecessary operation + --> tests/ui/unnecessary_operation.rs:121:9 + | +LL | [42, 55][get_usize()]; + | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());` + +error: unnecessary operation + --> tests/ui/unnecessary_operation.rs:126:5 + | +LL | [42, 55][get_usize()]; + | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());` + +error: unnecessary operation + --> tests/ui/unnecessary_operation.rs:130:5 + | +LL | [42, 55][get_usize()]; + | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());` + +error: aborting due to 22 previous errors From b161dc659c87df29cc9738a49105124ab30638e8 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 24 May 2024 09:54:42 +0700 Subject: [PATCH 3/7] add utils is_inside_always_const_context --- clippy_lints/src/default_numeric_fallback.rs | 2 ++ clippy_utils/src/lib.rs | 31 ++++++++++++++++---- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/default_numeric_fallback.rs b/clippy_lints/src/default_numeric_fallback.rs index fbc4ede37b1..51252f59db4 100644 --- a/clippy_lints/src/default_numeric_fallback.rs +++ b/clippy_lints/src/default_numeric_fallback.rs @@ -50,6 +50,8 @@ declare_lint_pass!(DefaultNumericFallback => [DEFAULT_NUMERIC_FALLBACK]); impl<'tcx> LateLintPass<'tcx> for DefaultNumericFallback { fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) { let hir = cx.tcx.hir(); + // NOTE: this is different from `clippy_utils::is_inside_always_const_context`. + // Inline const supports type inference. let is_parent_const = matches!( hir.body_const_context(hir.body_owner_def_id(body.id())), Some(ConstContext::Const { inline: false } | ConstContext::Static(_)) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index bd0911d5268..8185a523f1f 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -100,10 +100,10 @@ use rustc_hir::hir_id::{HirIdMap, HirIdSet}; use rustc_hir::intravisit::{walk_expr, FnKind, Visitor}; use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk}; use rustc_hir::{ - self as hir, def, Arm, ArrayLen, BindingMode, Block, BlockCheckMode, Body, ByRef, Closure, Destination, Expr, - ExprField, ExprKind, FnDecl, FnRetTy, GenericArgs, HirId, Impl, ImplItem, ImplItemKind, ImplItemRef, Item, - ItemKind, LangItem, LetStmt, MatchSource, Mutability, Node, OwnerId, Param, Pat, PatKind, Path, PathSegment, - PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitItemRef, TraitRef, TyKind, UnOp, + self as hir, def, Arm, ArrayLen, BindingMode, Block, BlockCheckMode, Body, ByRef, Closure, ConstContext, + Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy, GenericArgs, HirId, Impl, ImplItem, ImplItemKind, + ImplItemRef, Item, ItemKind, LangItem, LetStmt, MatchSource, Mutability, Node, OwnerId, Param, Pat, PatKind, Path, + PathSegment, PrimTy, QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitItemRef, TraitRef, TyKind, UnOp, }; use rustc_lexer::{tokenize, TokenKind}; use rustc_lint::{LateContext, Level, Lint, LintContext}; @@ -208,7 +208,10 @@ pub fn local_is_initialized(cx: &LateContext<'_>, local: HirId) -> bool { false } -/// Returns `true` if the given `NodeId` is inside a constant context +/// Returns `true` if the given `HirId` is inside a constant context. +/// +/// This is the same as `is_inside_always_const_context`, but also includes +/// `const fn`. /// /// # Example /// @@ -221,6 +224,24 @@ pub fn in_constant(cx: &LateContext<'_>, id: HirId) -> bool { cx.tcx.hir().is_inside_const_context(id) } +/// Returns `true` if the given `HirId` is inside an always constant context. +/// +/// This context includes: +/// * const/static items +/// * const blocks (or inline consts) +/// * associated constants +pub fn is_inside_always_const_context(tcx: TyCtxt<'_>, hir_id: HirId) -> bool { + use ConstContext::{Const, ConstFn, Static}; + let hir = tcx.hir(); + let Some(ctx) = hir.body_const_context(hir.enclosing_body_owner(hir_id)) else { + return false; + }; + match ctx { + ConstFn => false, + Static(_) | Const { inline: _ } => true, + } +} + /// Checks if a `Res` refers to a constructor of a `LangItem` /// For example, use this to check whether a function call or a pattern is `Some(..)`. pub fn is_res_lang_ctor(cx: &LateContext<'_>, res: Res, lang_item: LangItem) -> bool { From 2c61b4557688aca993f59d2170673e86c4df9101 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 24 May 2024 03:25:58 +0000 Subject: [PATCH 4/7] do not lint on indexing inside const contexts --- clippy_lints/src/no_effect.rs | 7 ++++++- tests/ui/unnecessary_operation.fixed | 4 ++-- tests/ui/unnecessary_operation.stderr | 14 +------------- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index dc017fa6634..139c33d3f4a 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -1,7 +1,9 @@ use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then}; use clippy_utils::source::snippet_opt; use clippy_utils::ty::has_drop; -use clippy_utils::{any_parent_is_automatically_derived, is_lint_allowed, path_to_local, peel_blocks}; +use clippy_utils::{ + any_parent_is_automatically_derived, is_inside_always_const_context, is_lint_allowed, path_to_local, peel_blocks, +}; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::{ @@ -265,6 +267,9 @@ fn check_unnecessary_operation(cx: &LateContext<'_>, stmt: &Stmt<'_>) { && reduced.iter().all(|e| e.span.ctxt() == ctxt) { if let ExprKind::Index(..) = &expr.kind { + if is_inside_always_const_context(cx.tcx, expr.hir_id) { + return; + } let snippet = if let (Some(arr), Some(func)) = (snippet_opt(cx, reduced[0].span), snippet_opt(cx, reduced[1].span)) { format!("assert!({}.len() > {});", &arr, &func) diff --git a/tests/ui/unnecessary_operation.fixed b/tests/ui/unnecessary_operation.fixed index e54491ab023..5a3c49626a1 100644 --- a/tests/ui/unnecessary_operation.fixed +++ b/tests/ui/unnecessary_operation.fixed @@ -114,12 +114,12 @@ fn main() { break 'label }; let () = const { - assert!([42, 55].len() > get_usize()); + [42, 55][get_usize()]; }; } const _: () = { - assert!([42, 55].len() > get_usize()); + [42, 55][get_usize()]; }; const fn foo() { diff --git a/tests/ui/unnecessary_operation.stderr b/tests/ui/unnecessary_operation.stderr index 3c031d006d4..036a9a44bba 100644 --- a/tests/ui/unnecessary_operation.stderr +++ b/tests/ui/unnecessary_operation.stderr @@ -119,23 +119,11 @@ LL | | s: String::from("blah"), LL | | }; | |______^ help: statement can be reduced to: `String::from("blah");` -error: unnecessary operation - --> tests/ui/unnecessary_operation.rs:121:9 - | -LL | [42, 55][get_usize()]; - | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());` - -error: unnecessary operation - --> tests/ui/unnecessary_operation.rs:126:5 - | -LL | [42, 55][get_usize()]; - | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());` - error: unnecessary operation --> tests/ui/unnecessary_operation.rs:130:5 | LL | [42, 55][get_usize()]; | ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());` -error: aborting due to 22 previous errors +error: aborting due to 20 previous errors From e18b27aa2a37fb7597c16f72a55f26bde6badf51 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 24 May 2024 10:37:19 +0700 Subject: [PATCH 5/7] add uitest for issue 12816 --- tests/ui/assertions_on_constants.rs | 6 ++++++ tests/ui/assertions_on_constants.stderr | 18 +++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/ui/assertions_on_constants.rs b/tests/ui/assertions_on_constants.rs index 1309ae45d0a..2e0fee08a59 100644 --- a/tests/ui/assertions_on_constants.rs +++ b/tests/ui/assertions_on_constants.rs @@ -53,3 +53,9 @@ fn main() { const N: usize = 1024; const _: () = assert!(N.is_power_of_two()); } + +#[allow(clippy::eq_op)] +const _: () = { + assert!(true); + assert!(8 == (7 + 1)); +}; diff --git a/tests/ui/assertions_on_constants.stderr b/tests/ui/assertions_on_constants.stderr index 00f117c9492..3a64f1892ad 100644 --- a/tests/ui/assertions_on_constants.stderr +++ b/tests/ui/assertions_on_constants.stderr @@ -80,5 +80,21 @@ LL | const _: () = assert!(true); | = help: remove it -error: aborting due to 10 previous errors +error: `assert!(true)` will be optimized out by the compiler + --> tests/ui/assertions_on_constants.rs:59:5 + | +LL | assert!(true); + | ^^^^^^^^^^^^^ + | + = help: remove it + +error: `assert!(true)` will be optimized out by the compiler + --> tests/ui/assertions_on_constants.rs:60:5 + | +LL | assert!(8 == (7 + 1)); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove it + +error: aborting due to 12 previous errors From ac600282a0cc35bd99b3e3152a655991d3c1dfea Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 24 May 2024 10:51:15 +0700 Subject: [PATCH 6/7] ignore `assertions-on-constants` in const contexts --- clippy_lints/src/assertions_on_constants.rs | 5 ++++ tests/ui/assertions_on_constants.rs | 1 - tests/ui/assertions_on_constants.stderr | 26 +-------------------- 3 files changed, 6 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/assertions_on_constants.rs b/clippy_lints/src/assertions_on_constants.rs index 2003dd1fb0e..863c25aba86 100644 --- a/clippy_lints/src/assertions_on_constants.rs +++ b/clippy_lints/src/assertions_on_constants.rs @@ -1,5 +1,6 @@ use clippy_utils::consts::{constant_with_source, Constant, ConstantSource}; use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::is_inside_always_const_context; use clippy_utils::macros::{find_assert_args, root_macro_call_first_node, PanicExpn}; use rustc_hir::{Expr, Item, ItemKind, Node}; use rustc_lint::{LateContext, LateLintPass}; @@ -31,6 +32,10 @@ declare_lint_pass!(AssertionsOnConstants => [ASSERTIONS_ON_CONSTANTS]); impl<'tcx> LateLintPass<'tcx> for AssertionsOnConstants { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { + if is_inside_always_const_context(cx.tcx, e.hir_id) { + return; + } + let Some(macro_call) = root_macro_call_first_node(cx, e) else { return; }; diff --git a/tests/ui/assertions_on_constants.rs b/tests/ui/assertions_on_constants.rs index 2e0fee08a59..c3574b0b466 100644 --- a/tests/ui/assertions_on_constants.rs +++ b/tests/ui/assertions_on_constants.rs @@ -47,7 +47,6 @@ fn main() { assert!(!CFG_FLAG); const _: () = assert!(true); - //~^ ERROR: `assert!(true)` will be optimized out by the compiler // Don't lint if the value is dependent on a defined constant: const N: usize = 1024; diff --git a/tests/ui/assertions_on_constants.stderr b/tests/ui/assertions_on_constants.stderr index 3a64f1892ad..aa4868af5a8 100644 --- a/tests/ui/assertions_on_constants.stderr +++ b/tests/ui/assertions_on_constants.stderr @@ -72,29 +72,5 @@ LL | debug_assert!(true); | = help: remove it -error: `assert!(true)` will be optimized out by the compiler - --> tests/ui/assertions_on_constants.rs:49:19 - | -LL | const _: () = assert!(true); - | ^^^^^^^^^^^^^ - | - = help: remove it - -error: `assert!(true)` will be optimized out by the compiler - --> tests/ui/assertions_on_constants.rs:59:5 - | -LL | assert!(true); - | ^^^^^^^^^^^^^ - | - = help: remove it - -error: `assert!(true)` will be optimized out by the compiler - --> tests/ui/assertions_on_constants.rs:60:5 - | -LL | assert!(8 == (7 + 1)); - | ^^^^^^^^^^^^^^^^^^^^^ - | - = help: remove it - -error: aborting due to 12 previous errors +error: aborting due to 9 previous errors From a0234b4e8b8c58a8726ec64828c7144c910faf67 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Sat, 8 Jun 2024 09:39:17 +0000 Subject: [PATCH 7/7] Ignore non ExprKind::{Path,Lit) inside const context - remove now dead code in ASSERTIONS_ON_CONSTANTS cc #11966 - Partially revert "ignore `assertions-on-constants` in const contexts" This reverts commit c7074de420a2192fb40d3f2194a20dd0d1b65cc6. --- clippy_lints/src/assertions_on_constants.rs | 23 +++++++----------- tests/ui/assertions_on_constants.rs | 8 +++++-- tests/ui/assertions_on_constants.stderr | 26 ++++++++++++++++++++- tests/ui/unnecessary_operation.fixed | 1 + tests/ui/unnecessary_operation.rs | 1 + 5 files changed, 42 insertions(+), 17 deletions(-) diff --git a/clippy_lints/src/assertions_on_constants.rs b/clippy_lints/src/assertions_on_constants.rs index 863c25aba86..ed4cdce8cb8 100644 --- a/clippy_lints/src/assertions_on_constants.rs +++ b/clippy_lints/src/assertions_on_constants.rs @@ -1,8 +1,8 @@ -use clippy_utils::consts::{constant_with_source, Constant, ConstantSource}; +use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::is_inside_always_const_context; use clippy_utils::macros::{find_assert_args, root_macro_call_first_node, PanicExpn}; -use rustc_hir::{Expr, Item, ItemKind, Node}; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -32,10 +32,6 @@ declare_lint_pass!(AssertionsOnConstants => [ASSERTIONS_ON_CONSTANTS]); impl<'tcx> LateLintPass<'tcx> for AssertionsOnConstants { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { - if is_inside_always_const_context(cx.tcx, e.hir_id) { - return; - } - let Some(macro_call) = root_macro_call_first_node(cx, e) else { return; }; @@ -47,17 +43,16 @@ impl<'tcx> LateLintPass<'tcx> for AssertionsOnConstants { let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn) else { return; }; - let Some((Constant::Bool(val), source)) = constant_with_source(cx, cx.typeck_results(), condition) else { + let Some(Constant::Bool(val)) = constant(cx, cx.typeck_results(), condition) else { return; }; - if let ConstantSource::Constant = source - && let Node::Item(Item { - kind: ItemKind::Const(..), - .. - }) = cx.tcx.parent_hir_node(e.hir_id) - { - return; + + match condition.kind { + ExprKind::Path(..) | ExprKind::Lit(_) => {}, + _ if is_inside_always_const_context(cx.tcx, e.hir_id) => return, + _ => {}, } + if val { span_lint_and_help( cx, diff --git a/tests/ui/assertions_on_constants.rs b/tests/ui/assertions_on_constants.rs index c3574b0b466..957154e60de 100644 --- a/tests/ui/assertions_on_constants.rs +++ b/tests/ui/assertions_on_constants.rs @@ -1,4 +1,4 @@ -#![allow(non_fmt_panics, clippy::needless_bool)] +#![allow(non_fmt_panics, clippy::needless_bool, clippy::eq_op)] macro_rules! assert_const { ($len:expr) => { @@ -47,14 +47,18 @@ fn main() { assert!(!CFG_FLAG); const _: () = assert!(true); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler + + assert!(8 == (7 + 1)); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler // Don't lint if the value is dependent on a defined constant: const N: usize = 1024; const _: () = assert!(N.is_power_of_two()); } -#[allow(clippy::eq_op)] const _: () = { assert!(true); + //~^ ERROR: `assert!(true)` will be optimized out by the compiler assert!(8 == (7 + 1)); }; diff --git a/tests/ui/assertions_on_constants.stderr b/tests/ui/assertions_on_constants.stderr index aa4868af5a8..e164a999c43 100644 --- a/tests/ui/assertions_on_constants.stderr +++ b/tests/ui/assertions_on_constants.stderr @@ -72,5 +72,29 @@ LL | debug_assert!(true); | = help: remove it -error: aborting due to 9 previous errors +error: `assert!(true)` will be optimized out by the compiler + --> tests/ui/assertions_on_constants.rs:49:19 + | +LL | const _: () = assert!(true); + | ^^^^^^^^^^^^^ + | + = help: remove it + +error: `assert!(true)` will be optimized out by the compiler + --> tests/ui/assertions_on_constants.rs:52:5 + | +LL | assert!(8 == (7 + 1)); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove it + +error: `assert!(true)` will be optimized out by the compiler + --> tests/ui/assertions_on_constants.rs:61:5 + | +LL | assert!(true); + | ^^^^^^^^^^^^^ + | + = help: remove it + +error: aborting due to 12 previous errors diff --git a/tests/ui/unnecessary_operation.fixed b/tests/ui/unnecessary_operation.fixed index 5a3c49626a1..006f123cbcd 100644 --- a/tests/ui/unnecessary_operation.fixed +++ b/tests/ui/unnecessary_operation.fixed @@ -124,4 +124,5 @@ const _: () = { const fn foo() { assert!([42, 55].len() > get_usize()); + //~^ ERROR: unnecessary operation } diff --git a/tests/ui/unnecessary_operation.rs b/tests/ui/unnecessary_operation.rs index 679d303f3ad..b4067c74074 100644 --- a/tests/ui/unnecessary_operation.rs +++ b/tests/ui/unnecessary_operation.rs @@ -128,4 +128,5 @@ const _: () = { const fn foo() { [42, 55][get_usize()]; + //~^ ERROR: unnecessary operation }