Auto merge of #11780 - Jacherr:vec-allocator-nolint, r=xFrednet

bors · bors · commit 6be0f7414d95 · 2023-11-09T23:33:46.000Z
Disable `vec_box` when using different allocators Fixes #7114 This PR disables the `vec_box` lint when the `Box` and `Vec` use different allocators (but not when they use the same - custom - allocator). For example - `Vec<Box<i32, DummyAllocator>>` will disable the lint, and `Vec<Box<i32, DummyAllocator>, DummyAllocator>` will not disable the lint. In addition, the applicability of this lint has been changed to `Unspecified` due to the automatic fixes potentially breaking code such as the following: ```rs fn foo() -> Vec<Box<i32>> { // -> Vec<i32> vec![Box::new(1)] } ``` It should be noted that the `if_chain->let-chains` fix has also been applied to this lint, so the diff does contain many changes. changelog: disable `vec_box` lint when using nonstandard allocators
diff --git a/clippy_lints/src/types/vec_box.rs b/clippy_lints/src/types/vec_box.rs
@@ -1,7 +1,7 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::last_path_segment;
+use clippy_utils::paths::ALLOCATOR_GLOBAL;
 use clippy_utils::source::snippet;
-use if_chain::if_chain;
+use clippy_utils::{last_path_segment, match_def_path};
 use rustc_errors::Applicability;
 use rustc_hir::def_id::DefId;
 use rustc_hir::{self as hir, GenericArg, QPath, TyKind};
@@ -21,43 +21,57 @@ pub(super) fn check(
     box_size_threshold: u64,
 ) -> bool {
     if cx.tcx.is_diagnostic_item(sym::Vec, def_id) {
-        if_chain! {
+        if let Some(last) = last_path_segment(qpath).args
             // Get the _ part of Vec<_>
-            if let Some(last) = last_path_segment(qpath).args;
-            if let Some(ty) = last.args.iter().find_map(|arg| match arg {
-                GenericArg::Type(ty) => Some(ty),
-                _ => None,
-            });
+            && let Some(GenericArg::Type(ty)) = last.args.first()
+            // extract allocator from the Vec for later
+            && let vec_alloc_ty = last.args.get(1)
             // ty is now _ at this point
-            if let TyKind::Path(ref ty_qpath) = ty.kind;
-            let res = cx.qpath_res(ty_qpath, ty.hir_id);
-            if let Some(def_id) = res.opt_def_id();
-            if Some(def_id) == cx.tcx.lang_items().owned_box();
+            && let TyKind::Path(ref ty_qpath) = ty.kind
+            && let res = cx.qpath_res(ty_qpath, ty.hir_id)
+            && let Some(def_id) = res.opt_def_id()
+            && Some(def_id) == cx.tcx.lang_items().owned_box()
             // At this point, we know ty is Box<T>, now get T
-            if let Some(last) = last_path_segment(ty_qpath).args;
-            if let Some(boxed_ty) = last.args.iter().find_map(|arg| match arg {
-                GenericArg::Type(ty) => Some(ty),
-                _ => None,
-            });
-            let ty_ty = hir_ty_to_ty(cx.tcx, boxed_ty);
-            if !ty_ty.has_escaping_bound_vars();
-            if ty_ty.is_sized(cx.tcx, cx.param_env);
-            if let Ok(ty_ty_size) = cx.layout_of(ty_ty).map(|l| l.size.bytes());
-            if ty_ty_size < box_size_threshold;
-            then {
-                span_lint_and_sugg(
-                    cx,
-                    VEC_BOX,
-                    hir_ty.span,
-                    "`Vec<T>` is already on the heap, the boxing is unnecessary",
-                    "try",
-                    format!("Vec<{}>", snippet(cx, boxed_ty.span, "..")),
-                    Applicability::MachineApplicable,
-                );
-                true
-            } else {
-                false
+            && let Some(last) = last_path_segment(ty_qpath).args
+            && let Some(GenericArg::Type(boxed_ty)) = last.args.first()
+            // extract allocator from the Box for later
+            && let boxed_alloc_ty = last.args.get(1)
+            && let ty_ty = hir_ty_to_ty(cx.tcx, boxed_ty)
+            && !ty_ty.has_escaping_bound_vars()
+            && ty_ty.is_sized(cx.tcx, cx.param_env)
+            && let Ok(ty_ty_size) = cx.layout_of(ty_ty).map(|l| l.size.bytes())
+            && ty_ty_size < box_size_threshold
+            // https://github.com/rust-lang/rust-clippy/issues/7114
+            && match (vec_alloc_ty, boxed_alloc_ty) {
+                (None, None) => true,
+                // this is in the event that we have something like
+                // Vec<_, Global>, in which case is equivalent to
+                // Vec<_>
+                (None, Some(GenericArg::Type(inner))) | (Some(GenericArg::Type(inner)), None) => {
+                    if let TyKind::Path(path) = inner.kind
+                        && let Some(did) = cx.qpath_res(&path, inner.hir_id).opt_def_id() {
+                        match_def_path(cx, did, &ALLOCATOR_GLOBAL)
+                    } else {
+                        false
+                    }
+                },
+                (Some(GenericArg::Type(l)), Some(GenericArg::Type(r))) =>
+                    hir_ty_to_ty(cx.tcx, l) == hir_ty_to_ty(cx.tcx, r),
+                _ => false
             }
+        {
+            span_lint_and_sugg(
+                cx,
+                VEC_BOX,
+                hir_ty.span,
+                "`Vec<T>` is already on the heap, the boxing is unnecessary",
+                "try",
+                format!("Vec<{}>", snippet(cx, boxed_ty.span, "..")),
+                Applicability::Unspecified,
+            );
+            true
+        } else {
+            false
         }
     } else {
         false
diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs
@@ -99,3 +99,4 @@ pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"];
 pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"];
 #[expect(clippy::invalid_paths)] // not sure why it thinks this, it works so
 pub const BOOL_THEN: [&str; 4] = ["core", "bool", "<impl bool>", "then"];
+pub const ALLOCATOR_GLOBAL: [&str; 3] = ["alloc", "alloc", "Global"];
diff --git a/tests/ui/vec_box_sized.fixed b/tests/ui/vec_box_sized.fixed
diff --git a/tests/ui/vec_box_sized.rs b/tests/ui/vec_box_sized.rs
@@ -1,12 +1,28 @@
+//@no-rustfix
+
 #![allow(dead_code)]
+#![feature(allocator_api)]
+
+use std::alloc::{AllocError, Allocator, Layout};
+use std::ptr::NonNull;
 
 struct SizedStruct(i32);
 struct UnsizedStruct([i32]);
 struct BigStruct([i32; 10000]);
 
+struct DummyAllocator;
+unsafe impl Allocator for DummyAllocator {
+    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {
+        todo!()
+    }
+    unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
+        todo!()
+    }
+}
+
 /// The following should trigger the lint
 mod should_trigger {
-    use super::SizedStruct;
+    use super::{DummyAllocator, SizedStruct};
     const C: Vec<Box<i32>> = Vec::new();
     static S: Vec<Box<i32>> = Vec::new();
 
@@ -16,11 +32,21 @@ mod should_trigger {
 
     struct A(Vec<Box<SizedStruct>>);
     struct B(Vec<Vec<Box<(u32)>>>);
+
+    fn allocator_global_defined_vec() -> Vec<Box<i32>, std::alloc::Global> {
+        Vec::new()
+    }
+    fn allocator_global_defined_box() -> Vec<Box<i32, std::alloc::Global>> {
+        Vec::new()
+    }
+    fn allocator_match() -> Vec<Box<i32, DummyAllocator>, DummyAllocator> {
+        Vec::new_in(DummyAllocator)
+    }
 }
 
 /// The following should not trigger the lint
 mod should_not_trigger {
-    use super::{BigStruct, UnsizedStruct};
+    use super::{BigStruct, DummyAllocator, UnsizedStruct};
 
     struct C(Vec<Box<UnsizedStruct>>);
     struct D(Vec<Box<BigStruct>>);
@@ -33,6 +59,13 @@ mod should_not_trigger {
         // Regression test for #3720. This was causing an ICE.
         inner: Vec<Box<T>>,
     }
+
+    fn allocator_mismatch() -> Vec<Box<i32, DummyAllocator>> {
+        Vec::new()
+    }
+    fn allocator_mismatch_2() -> Vec<Box<i32>, DummyAllocator> {
+        Vec::new_in(DummyAllocator)
+    }
 }
 
 mod inner_mod {
diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr
@@ -1,5 +1,5 @@
 error: `Vec<T>` is already on the heap, the boxing is unnecessary
-  --> $DIR/vec_box_sized.rs:10:14
+  --> $DIR/vec_box_sized.rs:26:14
    |
 LL |     const C: Vec<Box<i32>> = Vec::new();
    |              ^^^^^^^^^^^^^ help: try: `Vec<i32>`
@@ -8,34 +8,52 @@ LL |     const C: Vec<Box<i32>> = Vec::new();
    = help: to override `-D warnings` add `#[allow(clippy::vec_box)]`
 
 error: `Vec<T>` is already on the heap, the boxing is unnecessary
-  --> $DIR/vec_box_sized.rs:11:15
+  --> $DIR/vec_box_sized.rs:27:15
    |
 LL |     static S: Vec<Box<i32>> = Vec::new();
    |               ^^^^^^^^^^^^^ help: try: `Vec<i32>`
 
 error: `Vec<T>` is already on the heap, the boxing is unnecessary
-  --> $DIR/vec_box_sized.rs:14:21
+  --> $DIR/vec_box_sized.rs:30:21
    |
 LL |         sized_type: Vec<Box<SizedStruct>>,
    |                     ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<SizedStruct>`
 
 error: `Vec<T>` is already on the heap, the boxing is unnecessary
-  --> $DIR/vec_box_sized.rs:17:14
+  --> $DIR/vec_box_sized.rs:33:14
    |
 LL |     struct A(Vec<Box<SizedStruct>>);
    |              ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<SizedStruct>`
 
 error: `Vec<T>` is already on the heap, the boxing is unnecessary
-  --> $DIR/vec_box_sized.rs:18:18
+  --> $DIR/vec_box_sized.rs:34:18
    |
 LL |     struct B(Vec<Vec<Box<(u32)>>>);
    |                  ^^^^^^^^^^^^^^^ help: try: `Vec<u32>`
 
 error: `Vec<T>` is already on the heap, the boxing is unnecessary
-  --> $DIR/vec_box_sized.rs:46:23
+  --> $DIR/vec_box_sized.rs:36:42
+   |
+LL |     fn allocator_global_defined_vec() -> Vec<Box<i32>, std::alloc::Global> {
+   |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<i32>`
+
+error: `Vec<T>` is already on the heap, the boxing is unnecessary
+  --> $DIR/vec_box_sized.rs:39:42
+   |
+LL |     fn allocator_global_defined_box() -> Vec<Box<i32, std::alloc::Global>> {
+   |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<i32>`
+
+error: `Vec<T>` is already on the heap, the boxing is unnecessary
+  --> $DIR/vec_box_sized.rs:42:29
+   |
+LL |     fn allocator_match() -> Vec<Box<i32, DummyAllocator>, DummyAllocator> {
+   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec<i32>`
+
+error: `Vec<T>` is already on the heap, the boxing is unnecessary
+  --> $DIR/vec_box_sized.rs:79:23
    |
 LL |         pub fn f() -> Vec<Box<S>> {
    |                       ^^^^^^^^^^^ help: try: `Vec<S>`
 
-error: aborting due to 6 previous errors
+error: aborting due to 9 previous errors
 
