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 to rustc #109842

Closed
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
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ 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)
}
}

Expand All @@ -583,7 +583,7 @@ fn link_dwarf_object<'a>(
}

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 @@ -5,6 +5,9 @@ lint_array_into_iter =
.use_explicit_into_iter_suggestion =
or use `IntoIterator::into_iter(..)` instead of `.into_iter()` to explicitly iterate by value

lint_suspicious_double_ref_op =
using `.{$call}()` on a double reference, which copies `{$ty}` instead of {$op} the inner type

lint_enum_intrinsics_mem_discriminant =
the return value of `mem::discriminant` is unspecified when called with a non-enum type
.note = the argument to `discriminant` should be a reference to an enum, but it was passed a reference to a `{$ty_param}`, which is not an enum.
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 @@ -564,6 +564,14 @@ pub struct BuiltinUnexpectedCliConfigValue {
pub value: Symbol,
}

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

// deref_into_dyn_supertrait.rs
#[derive(LintDiagnostic)]
#[diag(lint_supertrait_as_deref_target)]
Expand Down
85 changes: 65 additions & 20 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::{CloneDoubleRef, NoopMethodCallDiag};
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 All @@ -15,7 +16,6 @@ declare_lint! {
///
/// ```rust
/// # #![allow(unused)]
/// #![warn(noop_method_call)]
/// struct Foo;
/// let foo = &Foo;
/// let clone: &Foo = foo.clone();
Expand All @@ -31,18 +31,48 @@ declare_lint! {
/// calling `clone` on a `&T` where `T` does not implement clone, actually doesn't do anything
/// as references are copy. This lint detects these calls and warns the user about them.
pub NOOP_METHOD_CALL,
Allow,
Warn,
Copy link
Member

@compiler-errors compiler-errors Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this change and the associated changes to lib/compiler should be done in a separate PR.

Or at least the PR title and description need to be updated to reflect that this is also bumping NOOP_METHOD_CALL to warn.

But I would prefer the first option. Just uplifting clone_double_ref seems to me very easy to r+, having to approve both is a bit harder to disentangle (esp in case we need to unland one in the future, e.g. due to lots of false positives in some yet undiscovered case -- which I have no reason to believe may happen, but stranger things have happened before).

"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 copy 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,
"using `clone` on `&&T`"
Copy link
Member

@compiler-errors compiler-errors Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lint isn't Clone specific as implemented below? Wording, etc. should probably be adjusted?

}

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 +100,40 @@ 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,
CloneDoubleRef { call: call.ident.name, ty: expr_ty, op },
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ use rustc_span::def_id::LocalDefId;
use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_span::{BytePos, DesugaringKind, ExpnKind, MacroKind, Span, DUMMY_SP};
use rustc_target::spec::abi;
use std::ops::Deref;

use super::method_chain::CollectAllMismatches;
use super::InferCtxtPrivExt;
Expand Down Expand Up @@ -3571,7 +3570,7 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
// trait_pred `S: Sum<<Self as Iterator>::Item>` and predicate `i32: Sum<&()>`
let mut type_diffs = vec![];

if let ObligationCauseCode::ExprBindingObligation(def_id, _, _, idx) = parent_code.deref()
if let ObligationCauseCode::ExprBindingObligation(def_id, _, _, idx) = parent_code
&& let Some(node_substs) = typeck_results.node_substs_opt(call_hir_id)
&& let where_clauses = self.tcx.predicates_of(def_id).instantiate(self.tcx, node_substs)
&& let Some(where_pred) = where_clauses.predicates.get(*idx)
Expand Down
4 changes: 2 additions & 2 deletions library/alloc/src/str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ macro_rules! specialize_for_lengths {
$num => {
for s in iter {
copy_slice_and_advance!(target, sep_bytes);
let content_bytes = s.borrow().as_ref();
let content_bytes = s.as_ref();
copy_slice_and_advance!(target, content_bytes);
}
},
Expand All @@ -98,7 +98,7 @@ macro_rules! specialize_for_lengths {
// arbitrary non-zero size fallback
for s in iter {
copy_slice_and_advance!(target, sep_bytes);
let content_bytes = s.borrow().as_ref();
let content_bytes = s.as_ref();
copy_slice_and_advance!(target, content_bytes);
}
}
Expand Down
3 changes: 1 addition & 2 deletions library/core/benches/iter.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use core::borrow::Borrow;
use core::iter::*;
use core::mem;
use core::num::Wrapping;
Expand Down Expand Up @@ -428,7 +427,7 @@ fn bench_trusted_random_access_chunks(b: &mut Bencher) {
black_box(&v)
.iter()
// this shows that we're not relying on the slice::Iter specialization in Copied
.map(|b| *b.borrow())
.map(|b| *b)
.array_chunks::<{ mem::size_of::<u64>() }>()
.map(|ary| {
let d = u64::from_ne_bytes(ary);
Expand Down
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 @@ -312,7 +312,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 @@ -2,8 +2,8 @@
#![warn(clippy::explicit_deref_methods)]
#![allow(unused_variables)]
#![allow(
noop_method_call,
clippy::borrow_deref_ref,
clippy::clone_double_ref,
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 @@ -2,8 +2,8 @@
#![warn(clippy::explicit_deref_methods)]
#![allow(unused_variables)]
#![allow(
noop_method_call,
clippy::borrow_deref_ref,
clippy::clone_double_ref,
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
Loading