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.

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

uplift clippy::clone_double_ref as suspicious_double_ref_op #110955

Merged
merged 3 commits into from
May 2, 2023
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
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
8 changes: 8 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ 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 ->
*[should_not_happen] [{$op}]
[deref] dereferencing
[borrow] borrowing
[clone] cloning
} 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 => "borrow",
sym::noop_method_clone => "clone",
sym::noop_method_deref => "deref",
_ => 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: 1 addition & 0 deletions library/core/tests/clone.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#[test]
#[cfg_attr(not(bootstrap), allow(suspicious_double_ref_op))]
fn test_borrowed_clone() {
let x = 5;
let y: &i32 = &x;
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ pub(crate) fn print_generic_bounds<'a, 'tcx: 'a>(
display_fn(move |f| {
let mut bounds_dup = FxHashSet::default();

for (i, bound) in bounds.iter().filter(|b| bounds_dup.insert(b.clone())).enumerate() {
for (i, bound) in bounds.iter().filter(|b| bounds_dup.insert(*b)).enumerate() {
if i > 0 {
f.write_str(" + ")?;
}
Expand Down
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