Skip to content

Fix false-positve of unnecessary-operation lints #12839

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 4 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
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 @@ -51,6 +51,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
13 changes: 12 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,15 @@ 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());
}
13 changes: 12 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,15 @@ fn main() {
'label: {
break 'label
};
let () = const {
[42, 55][get_usize()];
};
}

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

const fn foo() {
[42, 55][get_usize()];
}
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