From d6c60a6fad07e8a62ef358b0a44edeb284b4734d Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 27 Jul 2020 21:38:13 -0400 Subject: [PATCH] Add lint for using a type with a destructor in a zero-length repeat expr Currently, writing a zero-length array repeat expression (e.g. `[String::new(); 0]`) will cause the initializer value to be leaked. See #74836 for more details There are three ways that we could potentially handle this case: 1. Run the destructor immediately after 'constructing' the zero-length array. 2. Run the destructor when the initializer value would otherwise be dropped (i.e. at the end of the lexical scope) 3. Keep the current behavior and continue to leak to the initializer Note that the 'obvious' choice (running the destructor when the array is dropped) does not work, since a zero-length array does not actually store the value we want to drop. All of these options seem likely to be surprising and inconsistent to users. Since any fully monomorphic constant can be used as the repeat length, this has the potential to cause confusing 'action at a distance' (e.g. changing a `const` from 0 to 1 results in drop order changing). Regardless of which option we decide on, we should tell users to use an empty array (`[]`) instead. This commit adds a new lint ZERO_REPEAT_WITH_DROP, which fires when a type that (potentially) has a destructor is used in a zero-length array repeat expression. If a type has no drop glue, we skip linting, since dropping it has no user-visible side effects. If we do not know if a type has drop glue (e.g. `Option`), we lint anyway, since some choice of generic arguments could trigger the strange drop behavior. If the length const expression is not fully monomorphic (e.g. contains a generic parameter), the compiler requires the type used in the repeat expression to be `Copy`. Therefore, we don't need to lint in this case, since a `Copy` type can never have drop glue. --- .../borrow_check/type_check/mod.rs | 35 +++++++- src/librustc_session/lint/builtin.rs | 7 ++ .../ui/lint/lint-zero-repeat-with-drop.rs | 36 +++++++++ .../ui/lint/lint-zero-repeat-with-drop.stderr | 79 +++++++++++++++++++ 4 files changed, 155 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/lint/lint-zero-repeat-with-drop.rs create mode 100644 src/test/ui/lint/lint-zero-repeat-with-drop.stderr diff --git a/src/librustc_mir/borrow_check/type_check/mod.rs b/src/librustc_mir/borrow_check/type_check/mod.rs index bc5c144cd742c..f25d6deaad7ec 100644 --- a/src/librustc_mir/borrow_check/type_check/mod.rs +++ b/src/librustc_mir/borrow_check/type_check/mod.rs @@ -7,7 +7,7 @@ use either::Either; use rustc_data_structures::frozen::Frozen; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; -use rustc_errors::struct_span_err; +use rustc_errors::{struct_span_err, Applicability}; use rustc_hir as hir; use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_hir::lang_items::{CoerceUnsizedTraitLangItem, CopyTraitLangItem, SizedTraitLangItem}; @@ -30,6 +30,7 @@ use rustc_middle::ty::{ self, CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations, RegionVid, ToPredicate, Ty, TyCtxt, UserType, UserTypeAnnotationIndex, WithConstness, }; +use rustc_session::lint::builtin::ZERO_REPEAT_WITH_DROP; use rustc_span::{Span, DUMMY_SP}; use rustc_target::abi::VariantIdx; use rustc_trait_selection::infer::InferCtxtExt as _; @@ -1993,7 +1994,8 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { // than 1. // If the length is larger than 1, the repeat expression will need to copy the // element, so we require the `Copy` trait. - if len.try_eval_usize(tcx, self.param_env).map_or(true, |len| len > 1) { + let len_val = len.try_eval_usize(tcx, self.param_env); + if len_val.map_or(true, |len| len > 1) { if let Operand::Move(_) = operand { // While this is located in `nll::typeck` this error is not an NLL error, it's // a required check to make sure that repeated elements implement `Copy`. @@ -2038,6 +2040,35 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { ); } } + } else if let Some(0) = len_val { + let ty = operand.ty(body, tcx); + if ty.needs_drop(tcx, self.param_env) { + let source_info = body.source_info(location); + let lint_root = match &body.source_scopes[source_info.scope].local_data { + ClearCrossCrate::Set(data) => data.lint_root, + _ => tcx.hir().as_local_hir_id(self.mir_def_id), + }; + + tcx.struct_span_lint_hir( + ZERO_REPEAT_WITH_DROP, + lint_root, + source_info.span, + |lint| { + lint + .build("used a type with a destructor in a zero-length repeat expression") + .note(&format!("the value used here has type `{}`, which may have a destructor", ty)) + .note("a length of zero is used, which will cause the value to be dropped in a strange location") + .span_suggestion( + source_info.span, + "consider using an empty array expression", + "[]".to_string(), + Applicability::MaybeIncorrect + ) + .emit(); + + } + ); + } } } diff --git a/src/librustc_session/lint/builtin.rs b/src/librustc_session/lint/builtin.rs index aa2a133952f8f..401046a34edfd 100644 --- a/src/librustc_session/lint/builtin.rs +++ b/src/librustc_session/lint/builtin.rs @@ -549,6 +549,12 @@ declare_lint! { }; } +declare_lint! { + pub ZERO_REPEAT_WITH_DROP, + Warn, + "detects using a type with a destructor in an zero-length array repeat expression" +} + declare_lint_pass! { /// Does nothing as a lint pass, but registers some `Lint`s /// that are used by other parts of the compiler. @@ -623,6 +629,7 @@ declare_lint_pass! { UNSAFE_OP_IN_UNSAFE_FN, INCOMPLETE_INCLUDE, CENUM_IMPL_DROP_CAST, + ZERO_REPEAT_WITH_DROP, ] } diff --git a/src/test/ui/lint/lint-zero-repeat-with-drop.rs b/src/test/ui/lint/lint-zero-repeat-with-drop.rs new file mode 100644 index 0000000000000..25664edd0d667 --- /dev/null +++ b/src/test/ui/lint/lint-zero-repeat-with-drop.rs @@ -0,0 +1,36 @@ +#![feature(const_generics)] +#![allow(incomplete_features)] +#![deny(zero_repeat_with_drop)] + +const ZERO: usize = 1 * 0; + +const fn make_val(val: T) -> T { val } + +struct NoDropOrCopy; +struct WithDropGlue(String); + +fn foo T, const N: usize>(not_copy: F, copy: V) { + // All of these should triger the lint + let _ = [not_copy(); 0]; //~ ERROR used a type + let _ = [not_copy(); 1 - 1]; //~ ERROR used a type + let _ = [not_copy(); ZERO]; //~ ERROR used a type + let _ = [Some(not_copy()); 0]; //~ ERROR used a type + let _ = [None::; 0]; //~ ERROR used a type + let _ = [make_val(not_copy()); 0]; //~ ERROR used a type + let _ = [String::new(); 0]; //~ ERROR used a type + let _ = [WithDropGlue(String::new()); 0]; //~ ERROR used a type + + // None of these should trigger the lint + let _ = [copy; 0]; + let _ = [Some(copy); 0]; + let _ = [NoDropOrCopy; 0]; + let _ = [not_copy(); 1]; + let _ = [copy; N]; +} + +fn allow_it() { + #[allow(zero_repeat_with_drop)] + let _ = [WithDropGlue(String::new()); 1 - 1]; +} + +fn main() {} diff --git a/src/test/ui/lint/lint-zero-repeat-with-drop.stderr b/src/test/ui/lint/lint-zero-repeat-with-drop.stderr new file mode 100644 index 0000000000000..326da1556bcdb --- /dev/null +++ b/src/test/ui/lint/lint-zero-repeat-with-drop.stderr @@ -0,0 +1,79 @@ +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:14:13 + | +LL | let _ = [not_copy(); 0]; + | ^^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | +note: the lint level is defined here + --> $DIR/lint-zero-repeat-with-drop.rs:3:9 + | +LL | #![deny(zero_repeat_with_drop)] + | ^^^^^^^^^^^^^^^^^^^^^ + = note: the value used here has type `T`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:15:13 + | +LL | let _ = [not_copy(); 1 - 1]; + | ^^^^^^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | + = note: the value used here has type `T`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:16:13 + | +LL | let _ = [not_copy(); ZERO]; + | ^^^^^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | + = note: the value used here has type `T`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:17:13 + | +LL | let _ = [Some(not_copy()); 0]; + | ^^^^^^^^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | + = note: the value used here has type `std::option::Option`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:18:13 + | +LL | let _ = [None::; 0]; + | ^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | + = note: the value used here has type `std::option::Option`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:19:13 + | +LL | let _ = [make_val(not_copy()); 0]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | + = note: the value used here has type `T`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:20:13 + | +LL | let _ = [String::new(); 0]; + | ^^^^^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | + = note: the value used here has type `std::string::String`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: used a type with a destructor in a zero-length repeat expression + --> $DIR/lint-zero-repeat-with-drop.rs:21:13 + | +LL | let _ = [WithDropGlue(String::new()); 0]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]` + | + = note: the value used here has type `WithDropGlue`, which may have a destructor + = note: a length of zero is used, which will cause the value to be dropped in a strange location + +error: aborting due to 8 previous errors +