Skip to content

new lint null_pointer_optimization #10960

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

Closed
wants to merge 2 commits into from
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5120,6 +5120,7 @@ Released 2018-09-13
[`nonsensical_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonsensical_open_options
[`nonstandard_macro_braces`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonstandard_macro_braces
[`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
[`null_pointer_optimization`]: https://rust-lang.github.io/rust-clippy/master/index.html#null_pointer_optimization
[`obfuscated_if_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#obfuscated_if_else
[`octal_escapes`]: https://rust-lang.github.io/rust-clippy/master/index.html#octal_escapes
[`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect
Expand Down
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ Suppress lints whenever the suggested change would cause breakage for other crat
* [`rc_mutex`](https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex)
* [`unnecessary_box_returns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns)
* [`single_call_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#single_call_fn)
* [`null_pointer_optimization`](https://rust-lang.github.io/rust-clippy/master/index.html#null_pointer_optimization)


## `msrv`
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::types::BORROWED_BOX_INFO,
crate::types::BOX_COLLECTION_INFO,
crate::types::LINKEDLIST_INFO,
crate::types::NULL_POINTER_OPTIMIZATION_INFO,
crate::types::OPTION_OPTION_INFO,
crate::types::RC_BUFFER_INFO,
crate::types::RC_MUTEX_INFO,
Expand Down
66 changes: 56 additions & 10 deletions clippy_lints/src/types/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod borrowed_box;
mod box_collection;
mod linked_list;
mod null_pointer_optimization;
mod option_option;
mod rc_buffer;
mod rc_mutex;
Expand Down Expand Up @@ -303,13 +304,50 @@ declare_clippy_lint! {
"usage of `Rc<Mutex<T>>`"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for `C<Option<T>>` where `C` is a type that has
/// [null pointer optimization](https://doc.rust-lang.org/core/option/#representation).
///
/// Note: There are some cases where `C<Option<T>>` is necessary, like getting a
/// `&mut Option<T>` from a `Box<Option<T>>`. This requires ownership of the `Box`, so
/// if you have a reference this is not possible to do.
///
/// ### Why is this bad?
/// It's slower, as `Option` can use `null` as `None`, instead of adding another layer of
/// indirection.
///
/// ### Example
/// ```rust
/// struct MyWrapperType<T>(Box<Option<T>>);
/// ```
/// Use instead:
/// ```rust
/// struct MyWrapperType<T>(Option<Box<T>>);
/// ```
#[clippy::version = "1.73.0"]
pub NULL_POINTER_OPTIMIZATION,
perf,
"checks for `C<Option<T>>` where `C` is a type that has null pointer optimization"
}
pub struct Types {
vec_box_size_threshold: u64,
type_complexity_threshold: u64,
avoid_breaking_exported_api: bool,
}

impl_lint_pass!(Types => [BOX_COLLECTION, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]);
impl_lint_pass!(Types => [
BOX_COLLECTION,
VEC_BOX,
OPTION_OPTION,
LINKEDLIST,
BORROWED_BOX,
REDUNDANT_ALLOCATION,
RC_BUFFER,
RC_MUTEX,
TYPE_COMPLEXITY,
NULL_POINTER_OPTIMIZATION,
]);

impl<'tcx> LateLintPass<'tcx> for Types {
fn check_fn(
Expand Down Expand Up @@ -349,10 +387,11 @@ impl<'tcx> LateLintPass<'tcx> for Types {
let is_exported = cx.effective_visibilities.is_exported(item.owner_id.def_id);

match item.kind {
ItemKind::Static(ty, _, _) | ItemKind::Const(ty, _) => self.check_ty(
ItemKind::Static(ty, _, _) | ItemKind::Const(ty, _) | ItemKind::TyAlias(ty, _) => self.check_ty(
cx,
ty,
CheckTyContext {
is_in_ty_alias: matches!(item.kind, ItemKind::TyAlias(..)),
is_exported,
..CheckTyContext::default()
},
Expand Down Expand Up @@ -476,7 +515,10 @@ impl Types {
return;
}

if !context.is_nested_call && type_complexity::check(cx, hir_ty, self.type_complexity_threshold) {
if !context.is_nested_call
&& !context.is_in_ty_alias
&& type_complexity::check(cx, hir_ty, self.type_complexity_threshold)
{
return;
}

Expand All @@ -492,13 +534,16 @@ impl Types {
// in `clippy_lints::utils::conf.rs`

let mut triggered = false;
triggered |= box_collection::check(cx, hir_ty, qpath, def_id);
triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id);
triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id);
triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold);
triggered |= option_option::check(cx, hir_ty, qpath, def_id);
triggered |= linked_list::check(cx, hir_ty, def_id);
triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id);
if !context.is_in_ty_alias {
triggered |= box_collection::check(cx, hir_ty, qpath, def_id);
triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id);
triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id);
triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold);
triggered |= option_option::check(cx, hir_ty, qpath, def_id);
triggered |= linked_list::check(cx, hir_ty, def_id);
triggered |= rc_mutex::check(cx, hir_ty, qpath, def_id);
}
triggered |= null_pointer_optimization::check(cx, hir_ty, qpath, res);

if triggered {
return;
Expand Down Expand Up @@ -580,6 +625,7 @@ impl Types {
#[allow(clippy::struct_excessive_bools)]
#[derive(Clone, Copy, Default)]
struct CheckTyContext {
is_in_ty_alias: bool,
is_in_trait_impl: bool,
/// `true` for types on local variables.
is_local: bool,
Expand Down
35 changes: 35 additions & 0 deletions clippy_lints/src/types/null_pointer_optimization.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
use super::NULL_POINTER_OPTIMIZATION;
use clippy_utils::diagnostics::span_lint_and_help;
use clippy_utils::{is_lang_item_or_ctor, last_path_segment, match_def_path, paths};
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{GenericArg, LangItem, QPath, Ty, TyKind};
use rustc_lint::LateContext;

pub(super) fn check(cx: &LateContext<'_>, ty: &Ty<'_>, qpath: &QPath<'_>, res: Res) -> bool {
if let Res::Def(DefKind::Struct, def_id) = res
&& (is_lang_item_or_ctor(cx, def_id, LangItem::OwnedBox) || match_def_path(cx, def_id, &paths::PTR_NON_NULL))
&& let Some(args) = last_path_segment(qpath).args
&& let GenericArg::Type(option_ty) = args.args[0]
&& let TyKind::Path(option_qpath) = option_ty.kind
&& let res = cx.qpath_res(&option_qpath, option_ty.hir_id)
&& let Res::Def(.., def_id) = res
&& is_lang_item_or_ctor(cx, def_id, LangItem::Option)
{
let outer_ty = last_path_segment(qpath).ident.name;
span_lint_and_help(
cx,
NULL_POINTER_OPTIMIZATION,
ty.span,
&format!("usage of `{outer_ty}<Option<T>>`"),
None,
&format!(
"consider using `Option<{outer_ty}<T>>` instead, as it will grant better performance. For more information, see\n\
https://doc.rust-lang.org/core/option/#representation",
),
);

return true;
}

false
}
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ define_Conf! {
/// arithmetic-side-effects-allowed-unary = ["SomeType", "AnotherType"]
/// ```
(arithmetic_side_effects_allowed_unary: rustc_data_structures::fx::FxHashSet<String> = <_>::default()),
/// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX, UNNECESSARY_BOX_RETURNS, SINGLE_CALL_FN.
/// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UNUSED_SELF, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX, UNNECESSARY_BOX_RETURNS, SINGLE_CALL_FN, NULL_POINTER_OPTIMIZATION.
///
/// Suppress lints whenever the suggested change would cause breakage for other crates.
(avoid_breaking_exported_api: bool = true),
Expand Down
32 changes: 32 additions & 0 deletions tests/ui/null_pointer_optimization.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#![allow(clippy::boxed_local, unused)]
#![warn(clippy::null_pointer_optimization)]
#![no_main]

use std::marker::PhantomData;
use std::ptr::NonNull;

type A<T> = Box<Option<T>>; //~ ERROR: usage of `Box<Option<T>>`

fn a<T>(a: Box<Option<T>>) {} //~ ERROR: usage of `Box<Option<T>>`

fn a_ty_alias<T>(a: A<T>) {}

fn b(b: String) {}

fn c<T>(c: NonNull<Option<T>>) {} //~ ERROR: usage of `NonNull<Option<T>>`

fn g<T>(e: Option<Box<Option<T>>>) {} //~ ERROR: usage of `Box<Option<T>>`

struct H<T>(Box<Option<T>>); //~ ERROR: usage of `Box<Option<T>>`

enum I<T> {
I(Box<Option<T>>), //~ ERROR: usage of `Box<Option<T>>`
}

struct D<T>(Option<T>);

fn d<T>(d: D<T>) {}

fn e<T>(e: Option<NonNull<T>>) {}

fn f<T>(e: Option<Box<T>>) {}
57 changes: 57 additions & 0 deletions tests/ui/null_pointer_optimization.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
error: usage of `Box<Option<T>>`
--> $DIR/null_pointer_optimization.rs:8:13
|
LL | type A<T> = Box<Option<T>>;
| ^^^^^^^^^^^^^^
|
= help: consider using `Option<Box<T>>` instead, as it will grant better performance. For more information, see
https://doc.rust-lang.org/core/option/#representation
= note: `-D clippy::null-pointer-optimization` implied by `-D warnings`

error: usage of `Box<Option<T>>`
--> $DIR/null_pointer_optimization.rs:10:12
|
LL | fn a<T>(a: Box<Option<T>>) {}
| ^^^^^^^^^^^^^^
|
= help: consider using `Option<Box<T>>` instead, as it will grant better performance. For more information, see
https://doc.rust-lang.org/core/option/#representation

error: usage of `NonNull<Option<T>>`
--> $DIR/null_pointer_optimization.rs:16:12
|
LL | fn c<T>(c: NonNull<Option<T>>) {}
| ^^^^^^^^^^^^^^^^^^
|
= help: consider using `Option<NonNull<T>>` instead, as it will grant better performance. For more information, see
https://doc.rust-lang.org/core/option/#representation

error: usage of `Box<Option<T>>`
--> $DIR/null_pointer_optimization.rs:18:19
|
LL | fn g<T>(e: Option<Box<Option<T>>>) {}
| ^^^^^^^^^^^^^^
|
= help: consider using `Option<Box<T>>` instead, as it will grant better performance. For more information, see
https://doc.rust-lang.org/core/option/#representation

error: usage of `Box<Option<T>>`
--> $DIR/null_pointer_optimization.rs:20:13
|
LL | struct H<T>(Box<Option<T>>);
| ^^^^^^^^^^^^^^
|
= help: consider using `Option<Box<T>>` instead, as it will grant better performance. For more information, see
https://doc.rust-lang.org/core/option/#representation

error: usage of `Box<Option<T>>`
--> $DIR/null_pointer_optimization.rs:23:7
|
LL | I(Box<Option<T>>),
| ^^^^^^^^^^^^^^
|
= help: consider using `Option<Box<T>>` instead, as it will grant better performance. For more information, see
https://doc.rust-lang.org/core/option/#representation

error: aborting due to 6 previous errors