Skip to content
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

Enforce Copy bounds for repeat elements while considering lifetimes #95819

Merged
merged 5 commits into from
Apr 29, 2022
Merged
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
53 changes: 12 additions & 41 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,13 @@ use rustc_middle::ty::{
use rustc_span::def_id::CRATE_DEF_ID;
use rustc_span::{Span, DUMMY_SP};
use rustc_target::abi::VariantIdx;
use rustc_trait_selection::infer::InferCtxtExt as _;
use rustc_trait_selection::traits::error_reporting::InferCtxtExt as _;
use rustc_trait_selection::traits::query::type_op;
use rustc_trait_selection::traits::query::type_op::custom::scrape_region_constraints;
use rustc_trait_selection::traits::query::type_op::custom::CustomTypeOp;
use rustc_trait_selection::traits::query::type_op::{TypeOp, TypeOpOutput};
use rustc_trait_selection::traits::query::Fallible;
use rustc_trait_selection::traits::{self, ObligationCause, PredicateObligation};
use rustc_trait_selection::traits::PredicateObligation;

use rustc_const_eval::transform::{
check_consts::ConstCx, promote_consts::is_const_fn_in_array_repeat_expression,
};
use rustc_mir_dataflow::impls::MaybeInitializedPlaces;
use rustc_mir_dataflow::move_paths::MoveData;
use rustc_mir_dataflow::ResultsCursor;
Expand Down Expand Up @@ -1868,41 +1863,17 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
Operand::Move(place) => {
// Make sure that repeated elements implement `Copy`.
let span = body.source_info(location).span;
let ty = operand.ty(body, tcx);
if !self.infcx.type_is_copy_modulo_regions(self.param_env, ty, span) {
let ccx = ConstCx::new_with_param_env(tcx, body, self.param_env);
let is_const_fn =
is_const_fn_in_array_repeat_expression(&ccx, &place, &body);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that function is now unused, so you can remove it


debug!("check_rvalue: is_const_fn={:?}", is_const_fn);

let def_id = body.source.def_id().expect_local();
let obligation = traits::Obligation::new(
ObligationCause::new(
span,
self.tcx().hir().local_def_id_to_hir_id(def_id),
traits::ObligationCauseCode::RepeatElementCopy {
is_const_fn,
},
),
self.param_env,
ty::Binder::dummy(ty::TraitRef::new(
self.tcx().require_lang_item(
LangItem::Copy,
Some(self.last_span),
),
tcx.mk_substs_trait(ty, &[]),
))
.without_const()
.to_predicate(self.tcx()),
);
self.infcx.report_selection_error(
obligation.clone(),
&obligation,
&traits::SelectionError::Unimplemented,
false,
);
}
let ty = place.ty(body, tcx).ty;
let trait_ref = ty::TraitRef::new(
tcx.require_lang_item(LangItem::Copy, Some(span)),
tcx.mk_substs_trait(ty, &[]),
);

self.prove_trait_ref(
trait_ref,
Locations::Single(location),
ConstraintCategory::CopyBound,
);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2227,7 +2227,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
}
ObligationCauseCode::RepeatElementCopy { is_const_fn } => {
err.note(
"the `Copy` trait is required because the repeated element will be copied",
"the `Copy` trait is required because this value will be copied for each element of the array",
);

if is_const_fn {
Expand Down
40 changes: 40 additions & 0 deletions compiler/rustc_typeck/src/check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1292,9 +1292,49 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
return tcx.ty_error();
}

self.check_repeat_element_needs_copy_bound(element, count, element_ty);

tcx.mk_ty(ty::Array(t, count))
}

fn check_repeat_element_needs_copy_bound(
&self,
element: &hir::Expr<'_>,
count: ty::Const<'tcx>,
element_ty: Ty<'tcx>,
) {
let tcx = self.tcx;
// Actual constants as the repeat element get inserted repeatedly instead of getting copied via Copy.
match &element.kind {
hir::ExprKind::ConstBlock(..) => return,
hir::ExprKind::Path(qpath) => {
let res = self.typeck_results.borrow().qpath_res(qpath, element.hir_id);
if let Res::Def(DefKind::Const | DefKind::AssocConst | DefKind::AnonConst, _) = res
{
return;
}
}
_ => {}
}
// If someone calls a const fn, they can extract that call out into a separate constant (or a const
// block in the future), so we check that to tell them that in the diagnostic. Does not affect typeck.
let is_const_fn = match element.kind {
hir::ExprKind::Call(func, _args) => match *self.node_ty(func.hir_id).kind() {
ty::FnDef(def_id, _) => tcx.is_const_fn(def_id),
_ => false,
},
_ => false,
};

// If the length is 0, we don't create any elements, so we don't copy any. If the length is 1, we
// don't copy that one element, we move it. Only check for Copy if the length is larger.
if count.try_eval_usize(tcx, self.param_env).map_or(true, |len| len > 1) {
let lang_item = self.tcx.require_lang_item(LangItem::Copy, None);
let code = traits::ObligationCauseCode::RepeatElementCopy { is_const_fn };
self.require_type_meets(element_ty, element.span, code, lang_item);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move that entire block to its own maybe_require_copy_value (or something) function? That might make skimming easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn I love rust analyzer. Just selected the code and clicked "extract into fn". Add some lifetimes to types and remove the generated tcx argument done

}

fn check_expr_tuple(
&self,
elts: &'tcx [hir::Expr<'tcx>],
Expand Down
12 changes: 6 additions & 6 deletions src/test/ui/array-slice-vec/repeat_empty_ok.stderr
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
error[E0277]: the trait bound `Header<'_>: Copy` is not satisfied
--> $DIR/repeat_empty_ok.rs:8:19
--> $DIR/repeat_empty_ok.rs:8:20
|
LL | let headers = [Header{value: &[]}; 128];
| ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `Header<'_>`
| ^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `Header<'_>`
|
= note: the `Copy` trait is required because the repeated element will be copied
= note: the `Copy` trait is required because this value will be copied for each element of the array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only nitpick I'd have with the output would be to point at the whole array, But I think that'd be useful only in uncommon code, so it's not worth it to change it in this PR, particularly with the verbosity tradeoff:

let headers = [
    Header { value: &[] };
    128
];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had this before. I could revert the spans to point at the entire array again, but I thought it was actually neat that we were just pointing to the repeat element now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nono, what we have now it's good.

help: consider annotating `Header<'_>` with `#[derive(Copy)]`
|
LL | #[derive(Copy)]
|

error[E0277]: the trait bound `Header<'_>: Copy` is not satisfied
--> $DIR/repeat_empty_ok.rs:13:19
--> $DIR/repeat_empty_ok.rs:13:20
|
LL | let headers = [Header{value: &[0]}; 128];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `Header<'_>`
| ^^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `Header<'_>`
|
= note: the `Copy` trait is required because the repeated element will be copied
= note: the `Copy` trait is required because this value will be copied for each element of the array
help: consider annotating `Header<'_>` with `#[derive(Copy)]`
|
LL | #[derive(Copy)]
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/const-generics/issues/issue-61336-2.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error[E0277]: the trait bound `T: Copy` is not satisfied
--> $DIR/issue-61336-2.rs:6:5
--> $DIR/issue-61336-2.rs:6:6
|
LL | [x; { N }]
| ^^^^^^^^^^ the trait `Copy` is not implemented for `T`
| ^ the trait `Copy` is not implemented for `T`
|
= note: the `Copy` trait is required because the repeated element will be copied
= note: the `Copy` trait is required because this value will be copied for each element of the array
help: consider restricting type parameter `T`
|
LL | fn g<T: std::marker::Copy, const N: usize>(x: T) -> [T; N] {
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/const-generics/issues/issue-61336.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error[E0277]: the trait bound `T: Copy` is not satisfied
--> $DIR/issue-61336.rs:6:5
--> $DIR/issue-61336.rs:6:6
|
LL | [x; N]
| ^^^^^^ the trait `Copy` is not implemented for `T`
| ^ the trait `Copy` is not implemented for `T`
|
= note: the `Copy` trait is required because the repeated element will be copied
= note: the `Copy` trait is required because this value will be copied for each element of the array
help: consider restricting type parameter `T`
|
LL | fn g<T: std::marker::Copy, const N: usize>(x: T) -> [T; N] {
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/consts/const-blocks/fn-call-in-non-const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ const fn copy() -> u32 {
fn main() {
let _: [u32; 2] = [copy(); 2];
let _: [Option<Bar>; 2] = [no_copy(); 2];
//~^ ERROR the trait bound `Option<Bar>: Copy` is not satisfied
//~^ ERROR the trait bound `Bar: Copy` is not satisfied
}
14 changes: 9 additions & 5 deletions src/test/ui/consts/const-blocks/fn-call-in-non-const.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
error[E0277]: the trait bound `Option<Bar>: Copy` is not satisfied
--> $DIR/fn-call-in-non-const.rs:14:31
error[E0277]: the trait bound `Bar: Copy` is not satisfied
--> $DIR/fn-call-in-non-const.rs:14:32
|
LL | let _: [Option<Bar>; 2] = [no_copy(); 2];
| ^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `Option<Bar>`
| ^^^^^^^^^ the trait `Copy` is not implemented for `Bar`
|
= help: the trait `Copy` is implemented for `Option<T>`
= note: the `Copy` trait is required because the repeated element will be copied
= note: required because of the requirements on the impl of `Copy` for `Option<Bar>`
= note: the `Copy` trait is required because this value will be copied for each element of the array
= help: consider creating a new `const` item and initializing it with the result of the function call to be used in the repeat position, like `const VAL: Type = const_fn();` and `let x = [VAL; 42];`
= help: create an inline `const` block, see RFC #2920 <https://github.com/rust-lang/rfcs/pull/2920> for more information
help: consider annotating `Bar` with `#[derive(Copy)]`
|
LL | #[derive(Copy)]
|

error: aborting due to previous error

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/consts/const-blocks/migrate-fail.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ mod non_constants {
fn no_impl_copy_empty_value_multiple_elements() {
let x = None;
let arr: [Option<Bar>; 2] = [x; 2];
//~^ ERROR the trait bound `Option<Bar>: Copy` is not satisfied [E0277]
//~^ ERROR the trait bound `Bar: Copy` is not satisfied [E0277]
}

fn no_impl_copy_value_multiple_elements() {
let x = Some(Bar);
let arr: [Option<Bar>; 2] = [x; 2];
//~^ ERROR the trait bound `Option<Bar>: Copy` is not satisfied [E0277]
//~^ ERROR the trait bound `Bar: Copy` is not satisfied [E0277]
}
}

Expand Down
28 changes: 18 additions & 10 deletions src/test/ui/consts/const-blocks/migrate-fail.stderr
Original file line number Diff line number Diff line change
@@ -1,20 +1,28 @@
error[E0277]: the trait bound `Option<Bar>: Copy` is not satisfied
--> $DIR/migrate-fail.rs:13:37
error[E0277]: the trait bound `Bar: Copy` is not satisfied
--> $DIR/migrate-fail.rs:13:38
|
LL | let arr: [Option<Bar>; 2] = [x; 2];
| ^^^^^^ the trait `Copy` is not implemented for `Option<Bar>`
| ^ the trait `Copy` is not implemented for `Bar`
|
= note: required because of the requirements on the impl of `Copy` for `Option<Bar>`
= note: the `Copy` trait is required because this value will be copied for each element of the array
help: consider annotating `Bar` with `#[derive(Copy)]`
|
LL | #[derive(Copy)]
|
= help: the trait `Copy` is implemented for `Option<T>`
= note: the `Copy` trait is required because the repeated element will be copied

error[E0277]: the trait bound `Option<Bar>: Copy` is not satisfied
--> $DIR/migrate-fail.rs:19:37
error[E0277]: the trait bound `Bar: Copy` is not satisfied
--> $DIR/migrate-fail.rs:19:38
|
LL | let arr: [Option<Bar>; 2] = [x; 2];
| ^^^^^^ the trait `Copy` is not implemented for `Option<Bar>`
| ^ the trait `Copy` is not implemented for `Bar`
|
= note: required because of the requirements on the impl of `Copy` for `Option<Bar>`
= note: the `Copy` trait is required because this value will be copied for each element of the array
help: consider annotating `Bar` with `#[derive(Copy)]`
|
LL | #[derive(Copy)]
|
= help: the trait `Copy` is implemented for `Option<T>`
= note: the `Copy` trait is required because the repeated element will be copied

error: aborting due to 2 previous errors

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/consts/const-blocks/nll-fail.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ mod non_constants {
fn no_impl_copy_empty_value_multiple_elements() {
let x = None;
let arr: [Option<Bar>; 2] = [x; 2];
//~^ ERROR the trait bound `Option<Bar>: Copy` is not satisfied [E0277]
//~^ ERROR the trait bound `Bar: Copy` is not satisfied [E0277]
}

fn no_impl_copy_value_multiple_elements() {
let x = Some(Bar);
let arr: [Option<Bar>; 2] = [x; 2];
//~^ ERROR the trait bound `Option<Bar>: Copy` is not satisfied [E0277]
//~^ ERROR the trait bound `Bar: Copy` is not satisfied [E0277]
}
}

Expand Down
28 changes: 18 additions & 10 deletions src/test/ui/consts/const-blocks/nll-fail.stderr
Original file line number Diff line number Diff line change
@@ -1,20 +1,28 @@
error[E0277]: the trait bound `Option<Bar>: Copy` is not satisfied
--> $DIR/nll-fail.rs:12:37
error[E0277]: the trait bound `Bar: Copy` is not satisfied
--> $DIR/nll-fail.rs:12:38
|
LL | let arr: [Option<Bar>; 2] = [x; 2];
| ^^^^^^ the trait `Copy` is not implemented for `Option<Bar>`
| ^ the trait `Copy` is not implemented for `Bar`
|
= note: required because of the requirements on the impl of `Copy` for `Option<Bar>`
= note: the `Copy` trait is required because this value will be copied for each element of the array
help: consider annotating `Bar` with `#[derive(Copy)]`
|
LL | #[derive(Copy)]
|
= help: the trait `Copy` is implemented for `Option<T>`
= note: the `Copy` trait is required because the repeated element will be copied

error[E0277]: the trait bound `Option<Bar>: Copy` is not satisfied
--> $DIR/nll-fail.rs:18:37
error[E0277]: the trait bound `Bar: Copy` is not satisfied
--> $DIR/nll-fail.rs:18:38
|
LL | let arr: [Option<Bar>; 2] = [x; 2];
| ^^^^^^ the trait `Copy` is not implemented for `Option<Bar>`
| ^ the trait `Copy` is not implemented for `Bar`
|
= note: required because of the requirements on the impl of `Copy` for `Option<Bar>`
= note: the `Copy` trait is required because this value will be copied for each element of the array
help: consider annotating `Bar` with `#[derive(Copy)]`
|
LL | #[derive(Copy)]
|
= help: the trait `Copy` is implemented for `Option<T>`
= note: the `Copy` trait is required because the repeated element will be copied

error: aborting due to 2 previous errors

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/consts/const-blocks/trait-error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ struct Foo<T>(T);

fn main() {
[Foo(String::new()); 4];
//~^ ERROR the trait bound `Foo<String>: Copy` is not satisfied [E0277]
//~^ ERROR the trait bound `String: Copy` is not satisfied [E0277]
}
17 changes: 12 additions & 5 deletions src/test/ui/consts/const-blocks/trait-error.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
error[E0277]: the trait bound `Foo<String>: Copy` is not satisfied
--> $DIR/trait-error.rs:5:5
error[E0277]: the trait bound `String: Copy` is not satisfied
--> $DIR/trait-error.rs:5:6
|
LL | [Foo(String::new()); 4];
| ^^^^^^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `Foo<String>`
| ^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `String`
|
= help: the trait `Copy` is implemented for `Foo<T>`
= note: the `Copy` trait is required because the repeated element will be copied
note: required because of the requirements on the impl of `Copy` for `Foo<String>`
--> $DIR/trait-error.rs:1:10
|
LL | #[derive(Copy, Clone)]
| ^^^^
= note: the `Copy` trait is required because this value will be copied for each element of the array
= help: consider creating a new `const` item and initializing it with the result of the function call to be used in the repeat position, like `const VAL: Type = const_fn();` and `let x = [VAL; 42];`
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
= help: create an inline `const` block, see RFC #2920 <https://github.com/rust-lang/rfcs/pull/2920> for more information
= note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/consts/const-fn-in-vec.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error[E0277]: the trait bound `String: Copy` is not satisfied
--> $DIR/const-fn-in-vec.rs:4:32
--> $DIR/const-fn-in-vec.rs:4:33
|
LL | let strings: [String; 5] = [String::new(); 5];
| ^^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `String`
| ^^^^^^^^^^^^^ the trait `Copy` is not implemented for `String`
|
= note: the `Copy` trait is required because the repeated element will be copied
= note: the `Copy` trait is required because this value will be copied for each element of the array
= help: consider creating a new `const` item and initializing it with the result of the function call to be used in the repeat position, like `const VAL: Type = const_fn();` and `let x = [VAL; 42];`
= help: create an inline `const` block, see RFC #2920 <https://github.com/rust-lang/rfcs/pull/2920> for more information

Expand Down
19 changes: 19 additions & 0 deletions src/test/ui/lifetimes/copy_modulo_regions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![feature(nll)]

#[derive(Clone)]
struct Foo<'a>(fn(&'a ()) -> &'a ());

impl Copy for Foo<'static> {}

fn mk_foo<'a>() -> Foo<'a> {
println!("mk_foo");
Foo(|x| x)
}

fn foo<'a>() -> [Foo<'a>; 100] {
[mk_foo::<'a>(); 100] //~ ERROR lifetime may not live long enough
}

fn main() {
foo();
}
Loading