Skip to content
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.

Sign up for GitHub

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

Fix same_functions_in_if_condition FP #8673

Merged
merged 1 commit into from
Apr 11, 2022
Merged

Fix same_functions_in_if_condition FP #8673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 22 additions & 15 deletions clippy_utils/src/hir_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::consts::{constant_context, constant_simple};
use crate::consts::constant_simple;
use crate::source::snippet_opt;
use rustc_ast::ast::InlineAsmTemplatePiece;
use rustc_data_structures::fx::FxHasher;
Expand All @@ -16,15 +16,14 @@ use rustc_span::Symbol;
use std::hash::{Hash, Hasher};

/// Type used to check whether two ast are the same. This is different from the
/// operator
/// `==` on ast types as this operator would compare true equality with ID and
/// span.
/// operator `==` on ast types as this operator would compare true equality with
/// ID and span.
///
/// Note that some expressions kinds are not considered but could be added.
pub struct SpanlessEq<'a, 'tcx> {
/// Context used to evaluate constant expressions.
cx: &'a LateContext<'tcx>,
maybe_typeck_results: Option<&'tcx TypeckResults<'tcx>>,
maybe_typeck_results: Option<(&'tcx TypeckResults<'tcx>, &'tcx TypeckResults<'tcx>)>,
allow_side_effects: bool,
expr_fallback: Option<Box<dyn FnMut(&Expr<'_>, &Expr<'_>) -> bool + 'a>>,
}
Expand All @@ -33,7 +32,7 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
pub fn new(cx: &'a LateContext<'tcx>) -> Self {
Self {
cx,
maybe_typeck_results: cx.maybe_typeck_results(),
maybe_typeck_results: cx.maybe_typeck_results().map(|x| (x, x)),
allow_side_effects: true,
expr_fallback: None,
}
Expand Down Expand Up @@ -102,9 +101,9 @@ impl HirEqInterExpr<'_, '_, '_> {
(&StmtKind::Local(l), &StmtKind::Local(r)) => {
// This additional check ensures that the type of the locals are equivalent even if the init
// expression or type have some inferred parts.
if let Some(typeck) = self.inner.maybe_typeck_results {
let l_ty = typeck.pat_ty(l.pat);
let r_ty = typeck.pat_ty(r.pat);
if let Some((typeck_lhs, typeck_rhs)) = self.inner.maybe_typeck_results {
let l_ty = typeck_lhs.pat_ty(l.pat);
let r_ty = typeck_rhs.pat_ty(r.pat);
if l_ty != r_ty {
return false;
}
Expand Down Expand Up @@ -182,9 +181,17 @@ impl HirEqInterExpr<'_, '_, '_> {
}

pub fn eq_body(&mut self, left: BodyId, right: BodyId) -> bool {
let cx = self.inner.cx;
let eval_const = |body| constant_context(cx, cx.tcx.typeck_body(body)).expr(&cx.tcx.hir().body(body).value);
eval_const(left) == eval_const(right)
// swap out TypeckResults when hashing a body
let old_maybe_typeck_results = self.inner.maybe_typeck_results.replace((
self.inner.cx.tcx.typeck_body(left),
self.inner.cx.tcx.typeck_body(right),
));
let res = self.eq_expr(
&self.inner.cx.tcx.hir().body(left).value,
&self.inner.cx.tcx.hir().body(right).value,
);
self.inner.maybe_typeck_results = old_maybe_typeck_results;
res
}

#[allow(clippy::similar_names)]
Expand All @@ -193,10 +200,10 @@ impl HirEqInterExpr<'_, '_, '_> {
return false;
}

if let Some(typeck_results) = self.inner.maybe_typeck_results {
if let Some((typeck_lhs, typeck_rhs)) = self.inner.maybe_typeck_results {
if let (Some(l), Some(r)) = (
constant_simple(self.inner.cx, typeck_results, left),
constant_simple(self.inner.cx, typeck_results, right),
constant_simple(self.inner.cx, typeck_lhs, left),
constant_simple(self.inner.cx, typeck_rhs, right),
) {
if l == r {
return true;
Expand Down
19 changes: 19 additions & 0 deletions tests/ui/same_functions_in_if_condition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![feature(adt_const_params)]
#![allow(incomplete_features)]
#![warn(clippy::same_functions_in_if_condition)]
#![allow(clippy::ifs_same_cond)] // This warning is different from `ifs_same_cond`.
#![allow(clippy::if_same_then_else, clippy::comparison_chain)] // all empty blocks
Expand Down Expand Up @@ -87,4 +89,21 @@ fn main() {
"linux"
};
println!("{}", os);

#[derive(PartialEq, Eq)]
enum E {
A,
B,
}
fn generic<const P: E>() -> bool {
match P {
E::A => true,
E::B => false,
}
}
if generic::<{ E::A }>() {
println!("A");
} else if generic::<{ E::B }>() {
println!("B");
}
}
24 changes: 12 additions & 12 deletions tests/ui/same_functions_in_if_condition.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,72 +1,72 @@
error: this `if` has the same function call as a previous `if`
--> $DIR/same_functions_in_if_condition.rs:29:15
--> $DIR/same_functions_in_if_condition.rs:31:15
|
LL | } else if function() {
| ^^^^^^^^^^
|
= note: `-D clippy::same-functions-in-if-condition` implied by `-D warnings`
note: same as this
--> $DIR/same_functions_in_if_condition.rs:28:8
--> $DIR/same_functions_in_if_condition.rs:30:8
|
LL | if function() {
| ^^^^^^^^^^

error: this `if` has the same function call as a previous `if`
--> $DIR/same_functions_in_if_condition.rs:34:15
--> $DIR/same_functions_in_if_condition.rs:36:15
|
LL | } else if fn_arg(a) {
| ^^^^^^^^^
|
note: same as this
--> $DIR/same_functions_in_if_condition.rs:33:8
--> $DIR/same_functions_in_if_condition.rs:35:8
|
LL | if fn_arg(a) {
| ^^^^^^^^^

error: this `if` has the same function call as a previous `if`
--> $DIR/same_functions_in_if_condition.rs:39:15
--> $DIR/same_functions_in_if_condition.rs:41:15
|
LL | } else if obj.method() {
| ^^^^^^^^^^^^
|
note: same as this
--> $DIR/same_functions_in_if_condition.rs:38:8
--> $DIR/same_functions_in_if_condition.rs:40:8
|
LL | if obj.method() {
| ^^^^^^^^^^^^

error: this `if` has the same function call as a previous `if`
--> $DIR/same_functions_in_if_condition.rs:44:15
--> $DIR/same_functions_in_if_condition.rs:46:15
|
LL | } else if obj.method_arg(a) {
| ^^^^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/same_functions_in_if_condition.rs:43:8
--> $DIR/same_functions_in_if_condition.rs:45:8
|
LL | if obj.method_arg(a) {
| ^^^^^^^^^^^^^^^^^

error: this `if` has the same function call as a previous `if`
--> $DIR/same_functions_in_if_condition.rs:51:15
--> $DIR/same_functions_in_if_condition.rs:53:15
|
LL | } else if v.pop() == None {
| ^^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/same_functions_in_if_condition.rs:49:8
--> $DIR/same_functions_in_if_condition.rs:51:8
|
LL | if v.pop() == None {
| ^^^^^^^^^^^^^^^

error: this `if` has the same function call as a previous `if`
--> $DIR/same_functions_in_if_condition.rs:56:15
--> $DIR/same_functions_in_if_condition.rs:58:15
|
LL | } else if v.len() == 42 {
| ^^^^^^^^^^^^^
|
note: same as this
--> $DIR/same_functions_in_if_condition.rs:54:8
--> $DIR/same_functions_in_if_condition.rs:56:8
|
LL | if v.len() == 42 {
| ^^^^^^^^^^^^^
Expand Down