Skip to content

Don't lint assertions_on_constants on any const assertions #12840

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 7 commits into from
Jul 1, 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
20 changes: 10 additions & 10 deletions clippy_lints/src/assertions_on_constants.rs
Original file line number Diff line number Diff line change
@@ -1,7 +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;
Expand Down Expand Up @@ -42,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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/default_numeric_fallback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(_))
Expand Down
9 changes: 7 additions & 2 deletions clippy_lints/src/no_effect.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand Down Expand Up @@ -258,13 +260,16 @@ 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 {
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)
Expand Down
31 changes: 26 additions & 5 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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
///
Expand All @@ -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 {
Expand Down
11 changes: 10 additions & 1 deletion tests/ui/assertions_on_constants.rs
Original file line number Diff line number Diff line change
@@ -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) => {
Expand Down Expand Up @@ -49,7 +49,16 @@ fn main() {
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());
}

const _: () = {
assert!(true);
//~^ ERROR: `assert!(true)` will be optimized out by the compiler
assert!(8 == (7 + 1));
};
18 changes: 17 additions & 1 deletion tests/ui/assertions_on_constants.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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: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

14 changes: 13 additions & 1 deletion tests/ui/unnecessary_operation.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ fn get_number() -> i32 {
0
}

fn get_usize() -> usize {
const fn get_usize() -> usize {
0
}
fn get_struct() -> Struct {
Expand Down Expand Up @@ -113,4 +113,16 @@ fn main() {
'label: {
break 'label
};
let () = const {
[42, 55][get_usize()];
};
}

const _: () = {
[42, 55][get_usize()];
};

const fn foo() {
assert!([42, 55].len() > get_usize());
//~^ ERROR: unnecessary operation
}
14 changes: 13 additions & 1 deletion tests/ui/unnecessary_operation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ fn get_number() -> i32 {
0
}

fn get_usize() -> usize {
const fn get_usize() -> usize {
0
}
fn get_struct() -> Struct {
Expand Down Expand Up @@ -117,4 +117,16 @@ fn main() {
'label: {
break 'label
};
let () = const {
[42, 55][get_usize()];
};
}

const _: () = {
[42, 55][get_usize()];
};

const fn foo() {
[42, 55][get_usize()];
//~^ ERROR: unnecessary operation
}
8 changes: 7 additions & 1 deletion tests/ui/unnecessary_operation.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -119,5 +119,11 @@ 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:130:5
|
LL | [42, 55][get_usize()];
| ^^^^^^^^^^^^^^^^^^^^^^ help: statement can be written as: `assert!([42, 55].len() > get_usize());`

error: aborting due to 20 previous errors