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 3 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
28 changes: 28 additions & 0 deletions compiler/rustc_typeck/src/check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1292,6 +1292,34 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
return tcx.ty_error();
}

let is_const = match &element.kind {
hir::ExprKind::ConstBlock(..) => true,
hir::ExprKind::Path(qpath) => {
let res = self.typeck_results.borrow().qpath_res(qpath, element.hir_id);
matches!(
res,
Res::Def(DefKind::Const | DefKind::AssocConst | DefKind::AnonConst, _)
Copy link
Contributor

@lcnr lcnr Apr 12, 2022

Choose a reason for hiding this comment

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

you may want to add Res::Err here as well, to avoid Copy bounds if the repeat length uses an invalid path

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

)
}
_ => false,
};

if !is_const {
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 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.

I don't object to this code, but I'm a bit confused what's going on here -- it seems like, in the MIR type check, we enforce that T: Copy unless we can statically see that the length is either 0 or 1 (which makes sense). But here, we have this if !is_const check that says (iiuc) "if this is not a constant value being repeated, require T: Copy". Why doesn't the MIR check have a similar "if not a constant value" thing going on? Or does it, and I just missed it? Perhaps it gets lowered differently in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mir has this check somewhat subtly. It only checks Operand::Move, ignoring Operand::Copy and Operand::Constant

Tho I realize now that I should probably review whether things become Operand::Copy while ignoring lifetimes

}
}
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


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

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();
}
14 changes: 14 additions & 0 deletions src/test/ui/lifetimes/copy_modulo_regions.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: lifetime may not live long enough
--> $DIR/copy_modulo_regions.rs:14:5
|
LL | fn foo<'a>() -> [Foo<'a>; 100] {
| -- lifetime `'a` defined here
LL | [mk_foo::<'a>(); 100]
| ^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'a` must outlive `'static`
|
= note: requirement occurs because of the type `Foo<'_>`, which makes the generic argument `'_` invariant
= note: the struct `Foo<'a>` is invariant over the parameter `'a`
= help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

error: aborting due to previous error

Loading