Skip to content

Commit

Permalink
Add lint for using a type with a destructor in a zero-length repeat expr
Browse files Browse the repository at this point in the history
Currently, writing a zero-length array repeat expression (e.g.
`[String::new(); 0]`) will cause the initializer value to be leaked.
See rust-lang#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<T>`), 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.
  • Loading branch information
Aaron1011 committed Jul 28, 2020
1 parent 9be8ffc commit d6c60a6
Show file tree
Hide file tree
Showing 4 changed files with 155 additions and 2 deletions.
35 changes: 33 additions & 2 deletions src/librustc_mir/borrow_check/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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 _;
Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -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();

}
);
}
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/librustc_session/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -623,6 +629,7 @@ declare_lint_pass! {
UNSAFE_OP_IN_UNSAFE_FN,
INCOMPLETE_INCLUDE,
CENUM_IMPL_DROP_CAST,
ZERO_REPEAT_WITH_DROP,
]
}

Expand Down
36 changes: 36 additions & 0 deletions src/test/ui/lint/lint-zero-repeat-with-drop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
#![feature(const_generics)]
#![allow(incomplete_features)]
#![deny(zero_repeat_with_drop)]

const ZERO: usize = 1 * 0;

const fn make_val<T>(val: T) -> T { val }

struct NoDropOrCopy;
struct WithDropGlue(String);

fn foo<T, V: Copy, F: Fn() -> 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::<T>; 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() {}
79 changes: 79 additions & 0 deletions src/test/ui/lint/lint-zero-repeat-with-drop.stderr
Original file line number Diff line number Diff line change
@@ -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<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:18:13
|
LL | let _ = [None::<T>; 0];
| ^^^^^^^^^^^^^^ help: consider using an empty array expression: `[]`
|
= note: the value used here has type `std::option::Option<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: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

0 comments on commit d6c60a6

Please sign in to comment.