Skip to content

Commit

Permalink
uplift clippy::clone_double_ref as suspicious_double_ref_op
Browse files Browse the repository at this point in the history
  • Loading branch information
fee1-dead committed Apr 28, 2023
1 parent 1a6ae3d commit 5c99175
Show file tree
Hide file tree
Showing 19 changed files with 230 additions and 199 deletions.
7 changes: 3 additions & 4 deletions compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ use regex::Regex;
use tempfile::Builder as TempFileBuilder;

use itertools::Itertools;
use std::borrow::Borrow;
use std::cell::OnceCell;
use std::collections::BTreeSet;
use std::ffi::OsString;
Expand Down Expand Up @@ -573,17 +572,17 @@ fn link_dwarf_object<'a>(

impl<Relocations> ThorinSession<Relocations> {
fn alloc_mmap(&self, data: Mmap) -> &Mmap {
(*self.arena_mmap.alloc(data)).borrow()
&*self.arena_mmap.alloc(data)
}
}

impl<Relocations> thorin::Session<Relocations> for ThorinSession<Relocations> {
fn alloc_data(&self, data: Vec<u8>) -> &[u8] {
(*self.arena_data.alloc(data)).borrow()
&*self.arena_data.alloc(data)
}

fn alloc_relocation(&self, data: Relocations) -> &Relocations {
(*self.arena_relocations.alloc(data)).borrow()
&*self.arena_relocations.alloc(data)
}

fn read_input(&self, path: &Path) -> std::io::Result<&[u8]> {
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ lint_deprecated_lint_name =
lint_renamed_or_removed_lint = {$msg}
.suggestion = use the new name
lint_suspicious_double_ref_op =
using `.{$call}()` on a double reference, which returns `{$ty}` instead of {$op} the inner type
lint_unknown_lint =
unknown lint: `{$name}`
.suggestion = did you mean
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1150,6 +1150,14 @@ pub struct NoopMethodCallDiag<'a> {
pub label: Span,
}

#[derive(LintDiagnostic)]
#[diag(lint_suspicious_double_ref_op)]
pub struct SuspiciousDoubleRefDiag<'a> {
pub call: Symbol,
pub ty: Ty<'a>,
pub op: &'static str,
}

// pass_by_value.rs
#[derive(LintDiagnostic)]
#[diag(lint_pass_by_value)]
Expand Down
81 changes: 63 additions & 18 deletions compiler/rustc_lint/src/noop_method_call.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use crate::context::LintContext;
use crate::lints::NoopMethodCallDiag;
use crate::lints::{NoopMethodCallDiag, SuspiciousDoubleRefDiag};
use crate::LateContext;
use crate::LateLintPass;
use rustc_hir::def::DefKind;
use rustc_hir::{Expr, ExprKind};
use rustc_middle::ty;
use rustc_middle::ty::adjustment::Adjust;
use rustc_span::symbol::sym;

declare_lint! {
Expand Down Expand Up @@ -35,14 +36,44 @@ declare_lint! {
"detects the use of well-known noop methods"
}

declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL]);
declare_lint! {
/// The `suspicious_double_ref_op` lint checks for usage of `.clone()`/`.borrow()`/`.deref()`
/// on an `&&T` when `T: !Deref/Borrow/Clone`, which means the call will return the inner `&T`,
/// instead of performing the operation on the underlying `T` and can be confusing.
///
/// ### Example
///
/// ```rust
/// # #![allow(unused)]
/// struct Foo;
/// let foo = &&Foo;
/// let clone: &Foo = foo.clone();
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Since `Foo` doesn't implement `Clone`, running `.clone()` only dereferences the double
/// reference, instead of cloning the inner type which should be what was intended.
pub SUSPICIOUS_DOUBLE_REF_OP,
Warn,
"suspicious call of trait method on `&&T`"
}

declare_lint_pass!(NoopMethodCall => [NOOP_METHOD_CALL, SUSPICIOUS_DOUBLE_REF_OP]);

impl<'tcx> LateLintPass<'tcx> for NoopMethodCall {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
// We only care about method calls.
let ExprKind::MethodCall(call, receiver, ..) = &expr.kind else {
return
let ExprKind::MethodCall(call, receiver, _, call_span) = &expr.kind else {
return;
};

if call_span.from_expansion() {
return;
}

// We only care about method calls corresponding to the `Clone`, `Deref` and `Borrow`
// traits and ignore any other method call.
let did = match cx.typeck_results().type_dependent_def(expr.hir_id) {
Expand Down Expand Up @@ -70,25 +101,39 @@ impl<'tcx> LateLintPass<'tcx> for NoopMethodCall {
};
// (Re)check that it implements the noop diagnostic.
let Some(name) = cx.tcx.get_diagnostic_name(i.def_id()) else { return };
if !matches!(
name,
sym::noop_method_borrow | sym::noop_method_clone | sym::noop_method_deref
) {
return;
}

let op = match name {
sym::noop_method_borrow => "borrowing",
sym::noop_method_clone => "cloning",
sym::noop_method_deref => "dereferencing",
_ => return,
};

let receiver_ty = cx.typeck_results().expr_ty(receiver);
let expr_ty = cx.typeck_results().expr_ty_adjusted(expr);
if receiver_ty != expr_ty {
// This lint will only trigger if the receiver type and resulting expression \
// type are the same, implying that the method call is unnecessary.
let arg_adjustments = cx.typeck_results().expr_adjustments(receiver);

// If there is any user defined auto-deref step, then we don't want to warn.
// https://github.com/rust-lang/rust-clippy/issues/9272
if arg_adjustments.iter().any(|adj| matches!(adj.kind, Adjust::Deref(Some(_)))) {
return;
}

let expr_span = expr.span;
let span = expr_span.with_lo(receiver.span.hi());
cx.emit_spanned_lint(
NOOP_METHOD_CALL,
span,
NoopMethodCallDiag { method: call.ident.name, receiver_ty, label: span },
);

if receiver_ty == expr_ty {
cx.emit_spanned_lint(
NOOP_METHOD_CALL,
span,
NoopMethodCallDiag { method: call.ident.name, receiver_ty, label: span },
);
} else {
cx.emit_spanned_lint(
SUSPICIOUS_DOUBLE_REF_OP,
span,
SuspiciousDoubleRefDiag { call: call.ident.name, ty: expr_ty, op },
)
}
}
}
1 change: 0 additions & 1 deletion src/tools/clippy/clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::methods::CHARS_NEXT_CMP_INFO,
crate::methods::CLEAR_WITH_DRAIN_INFO,
crate::methods::CLONED_INSTEAD_OF_COPIED_INFO,
crate::methods::CLONE_DOUBLE_REF_INFO,
crate::methods::CLONE_ON_COPY_INFO,
crate::methods::CLONE_ON_REF_PTR_INFO,
crate::methods::COLLAPSIBLE_STR_REPLACE_INFO,
Expand Down
40 changes: 2 additions & 38 deletions src/tools/clippy/clippy_lints/src/methods/clone_on_copy.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::get_parent_node;
use clippy_utils::source::snippet_with_context;
use clippy_utils::sugg;
use clippy_utils::ty::is_copy;
use rustc_errors::Applicability;
use rustc_hir::{BindingAnnotation, ByRef, Expr, ExprKind, MatchSource, Node, PatKind, QPath};
use rustc_lint::LateContext;
use rustc_middle::ty::{self, adjustment::Adjust, print::with_forced_trimmed_paths};
use rustc_span::symbol::{sym, Symbol};

use super::CLONE_DOUBLE_REF;
use super::CLONE_ON_COPY;

/// Checks for the `CLONE_ON_COPY` lint.
Expand Down Expand Up @@ -42,41 +40,7 @@ pub(super) fn check(

let ty = cx.typeck_results().expr_ty(expr);
if let ty::Ref(_, inner, _) = arg_ty.kind() {
if let ty::Ref(_, innermost, _) = inner.kind() {
span_lint_and_then(
cx,
CLONE_DOUBLE_REF,
expr.span,
&with_forced_trimmed_paths!(format!(
"using `clone` on a double-reference; \
this will copy the reference of type `{ty}` instead of cloning the inner type"
)),
|diag| {
if let Some(snip) = sugg::Sugg::hir_opt(cx, arg) {
let mut ty = innermost;
let mut n = 0;
while let ty::Ref(_, inner, _) = ty.kind() {
ty = inner;
n += 1;
}
let refs = "&".repeat(n + 1);
let derefs = "*".repeat(n);
let explicit = with_forced_trimmed_paths!(format!("<{refs}{ty}>::clone({snip})"));
diag.span_suggestion(
expr.span,
"try dereferencing it",
with_forced_trimmed_paths!(format!("{refs}({derefs}{}).clone()", snip.deref())),
Applicability::MaybeIncorrect,
);
diag.span_suggestion(
expr.span,
"or try being explicit if you are sure, that you want to clone a reference",
explicit,
Applicability::MaybeIncorrect,
);
}
},
);
if let ty::Ref(..) = inner.kind() {
return; // don't report clone_on_copy
}
}
Expand Down
24 changes: 0 additions & 24 deletions src/tools/clippy/clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -984,29 +984,6 @@ declare_clippy_lint! {
"using 'clone' on a ref-counted pointer"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `.clone()` on an `&&T`.
///
/// ### Why is this bad?
/// Cloning an `&&T` copies the inner `&T`, instead of
/// cloning the underlying `T`.
///
/// ### Example
/// ```rust
/// fn main() {
/// let x = vec![1];
/// let y = &&x;
/// let z = y.clone();
/// println!("{:p} {:p}", *y, z); // prints out the same pointer
/// }
/// ```
#[clippy::version = "pre 1.29.0"]
pub CLONE_DOUBLE_REF,
correctness,
"using `clone` on `&&T`"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for usage of `.to_string()` on an `&&T` where
Expand Down Expand Up @@ -3258,7 +3235,6 @@ impl_lint_pass!(Methods => [
CHARS_LAST_CMP,
CLONE_ON_COPY,
CLONE_ON_REF_PTR,
CLONE_DOUBLE_REF,
COLLAPSIBLE_STR_REPLACE,
ITER_OVEREAGER_CLONED,
CLONED_INSTEAD_OF_COPIED,
Expand Down
1 change: 1 addition & 0 deletions src/tools/clippy/clippy_lints/src/renamed_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
("clippy::stutter", "clippy::module_name_repetitions"),
("clippy::to_string_in_display", "clippy::recursive_format_impl"),
("clippy::zero_width_space", "clippy::invisible_characters"),
("clippy::clone_double_ref", "suspicious_double_ref_op"),
("clippy::drop_bounds", "drop_bounds"),
("clippy::for_loop_over_option", "for_loops_over_fallibles"),
("clippy::for_loop_over_result", "for_loops_over_fallibles"),
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/explicit_deref_methods.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#![allow(unused_variables)]
#![allow(
clippy::borrow_deref_ref,
clippy::clone_double_ref,
suspicious_double_ref_op,
clippy::explicit_auto_deref,
clippy::needless_borrow,
clippy::uninlined_format_args
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/tests/ui/explicit_deref_methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#![allow(unused_variables)]
#![allow(
clippy::borrow_deref_ref,
clippy::clone_double_ref,
suspicious_double_ref_op,
clippy::explicit_auto_deref,
clippy::needless_borrow,
clippy::uninlined_format_args
Expand Down
2 changes: 2 additions & 0 deletions src/tools/clippy/tests/ui/rename.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#![allow(enum_intrinsics_non_enums)]
#![allow(non_fmt_panics)]
#![allow(named_arguments_used_positionally)]
#![allow(suspicious_double_ref_op)]
#![allow(temporary_cstring_as_ptr)]
#![allow(unknown_lints)]
#![allow(unused_labels)]
Expand Down Expand Up @@ -67,6 +68,7 @@
#![warn(clippy::module_name_repetitions)]
#![warn(clippy::recursive_format_impl)]
#![warn(clippy::invisible_characters)]
#![warn(suspicious_double_ref_op)]
#![warn(drop_bounds)]
#![warn(for_loops_over_fallibles)]
#![warn(for_loops_over_fallibles)]
Expand Down
2 changes: 2 additions & 0 deletions src/tools/clippy/tests/ui/rename.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#![allow(enum_intrinsics_non_enums)]
#![allow(non_fmt_panics)]
#![allow(named_arguments_used_positionally)]
#![allow(suspicious_double_ref_op)]
#![allow(temporary_cstring_as_ptr)]
#![allow(unknown_lints)]
#![allow(unused_labels)]
Expand Down Expand Up @@ -67,6 +68,7 @@
#![warn(clippy::stutter)]
#![warn(clippy::to_string_in_display)]
#![warn(clippy::zero_width_space)]
#![warn(clippy::clone_double_ref)]
#![warn(clippy::drop_bounds)]
#![warn(clippy::for_loop_over_option)]
#![warn(clippy::for_loop_over_result)]
Expand Down
Loading

0 comments on commit 5c99175

Please sign in to comment.