Auto merge of #10098 - lukaslueg:size_of_ref, r=Jarcho

Add size_of_ref lint

This addresses #9995, which is likely raising a valid point about `std::mem::size_of_val()`: It's [very easy to use double-references as the argument](apache/datafusion#4371 (comment)), which the function will happily accept and give back the size of _the reference_, not the size of the value _behind_ the reference. In the worst case, if the value matches the programmer's expectation, this seems to work, while in fact, everything will go horribly wrong e.g. on a different platform.

The size of a `&T` is independent of what `T` is, and people might want to use `std::mem::size_of_val()` to actually get the size of _any_ reference (e.g. via `&&()`). I would rather suggest that this is always bad behavior, though ([instead](https://doc.rust-lang.org/reference/type-layout.html#pointers-and-references-layout), [and](https://doc.rust-lang.org/stable/std/primitive.usize.html#associatedconstant.BITS)). I, therefore, put this lint into `correctness`.

Since the problem is usually easily fixed by removing extra `&`, I went light on suggesting code.

---

changelog: New lint: [`size_of_ref`]
[#10098](#10098)
<!-- changelog_checked -->

Author: bors

CHANGELOG.md

@@ -4547,6 +4547,7 @@ Released 2018-09-13
 [`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match
 [`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
 [`size_of_in_element_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_in_element_count
+[`size_of_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_ref
 [`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
 [`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
 [`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive

clippy_lints/src/declared_lints.rs

@@ -537,6 +537,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::single_char_lifetime_names::SINGLE_CHAR_LIFETIME_NAMES_INFO,
     crate::single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS_INFO,
     crate::size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT_INFO,
+    crate::size_of_ref::SIZE_OF_REF_INFO,
     crate::slow_vector_initialization::SLOW_VECTOR_INITIALIZATION_INFO,
     crate::std_instead_of_core::ALLOC_INSTEAD_OF_CORE_INFO,
     crate::std_instead_of_core::STD_INSTEAD_OF_ALLOC_INFO,

clippy_lints/src/lib.rs

@@ -265,6 +265,7 @@ mod shadow;
 mod single_char_lifetime_names;
 mod single_component_path_imports;
 mod size_of_in_element_count;
+mod size_of_ref;
 mod slow_vector_initialization;
 mod std_instead_of_core;
 mod strings;
@@ -906,6 +907,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(semicolon_block::SemicolonBlock));
     store.register_late_pass(|_| Box::new(fn_null_check::FnNullCheck));
     store.register_late_pass(|_| Box::new(permissions_set_readonly_false::PermissionsSetReadonlyFalse));
+    store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/size_of_ref.rs

@@ -0,0 +1,73 @@
+use clippy_utils::{diagnostics::span_lint_and_help, path_def_id, ty::peel_mid_ty_refs};
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Checks for calls to `std::mem::size_of_val()` where the argument is
+    /// a reference to a reference.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// Calling `size_of_val()` with a reference to a reference as the argument
+    /// yields the size of the reference-type, not the size of the value behind
+    /// the reference.
+    ///
+    /// ### Example
+    /// ```rust
+    /// struct Foo {
+    ///     buffer: [u8],
+    /// }
+    ///
+    /// impl Foo {
+    ///     fn size(&self) -> usize {
+    ///         // Note that `&self` as an argument is a `&&Foo`: Because `self`
+    ///         // is already a reference, `&self` is a double-reference.
+    ///         // The return value of `size_of_val()` therefor is the
+    ///         // size of the reference-type, not the size of `self`.
+    ///         std::mem::size_of_val(&self)
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// struct Foo {
+    ///     buffer: [u8],
+    /// }
+    ///
+    /// impl Foo {
+    ///     fn size(&self) -> usize {
+    ///         // Correct
+    ///         std::mem::size_of_val(self)
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.67.0"]
+    pub SIZE_OF_REF,
+    suspicious,
+    "Argument to `std::mem::size_of_val()` is a double-reference, which is almost certainly unintended"
+}
+declare_lint_pass!(SizeOfRef => [SIZE_OF_REF]);
+
+impl LateLintPass<'_> for SizeOfRef {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) {
+        if let ExprKind::Call(path, [arg]) = expr.kind
+            && let Some(def_id) = path_def_id(cx, path)
+            && cx.tcx.is_diagnostic_item(sym::mem_size_of_val, def_id)
+            && let arg_ty = cx.typeck_results().expr_ty(arg)
+            && peel_mid_ty_refs(arg_ty).1 > 1
+        {
+            span_lint_and_help(
+                cx,
+                SIZE_OF_REF,
+                expr.span,
+                "argument to `std::mem::size_of_val()` is a reference to a reference",
+                None,
+                "dereference the argument to `std::mem::size_of_val()` to get the size of the value instead of the size of the reference-type",
+            );
+        }
+    }
+}

tests/ui/size_of_ref.rs

@@ -0,0 +1,27 @@
+#![allow(unused)]
+#![warn(clippy::size_of_ref)]
+
+use std::mem::size_of_val;
+
+fn main() {
+    let x = 5;
+    let y = &x;
+
+    size_of_val(&x); // no lint
+    size_of_val(y); // no lint
+
+    size_of_val(&&x);
+    size_of_val(&y);
+}
+
+struct S {
+    field: u32,
+    data: Vec<u8>,
+}
+
+impl S {
+    /// Get size of object including `self`, in bytes.
+    pub fn size(&self) -> usize {
+        std::mem::size_of_val(&self) + (std::mem::size_of::<u8>() * self.data.capacity())
+    }
+}

tests/ui/size_of_ref.stderr

@@ -0,0 +1,27 @@
+error: argument to `std::mem::size_of_val()` is a reference to a reference
+  --> $DIR/size_of_ref.rs:13:5
+   |
+LL |     size_of_val(&&x);
+   |     ^^^^^^^^^^^^^^^^
+   |
+   = help: dereference the argument to `std::mem::size_of_val()` to get the size of the value instead of the size of the reference-type
+   = note: `-D clippy::size-of-ref` implied by `-D warnings`
+
+error: argument to `std::mem::size_of_val()` is a reference to a reference
+  --> $DIR/size_of_ref.rs:14:5
+   |
+LL |     size_of_val(&y);
+   |     ^^^^^^^^^^^^^^^
+   |
+   = help: dereference the argument to `std::mem::size_of_val()` to get the size of the value instead of the size of the reference-type
+
+error: argument to `std::mem::size_of_val()` is a reference to a reference
+  --> $DIR/size_of_ref.rs:25:9
+   |
+LL |         std::mem::size_of_val(&self) + (std::mem::size_of::<u8>() * self.data.capacity())
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: dereference the argument to `std::mem::size_of_val()` to get the size of the value instead of the size of the reference-type
+
+error: aborting due to 3 previous errors
+