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

Improve diagnostic for const ctors in array repeat expressions #113925

Merged
merged 1 commit into from
Nov 8, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 28 additions & 7 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1507,21 +1507,42 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
_ => {}
}
// 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 {
// If someone calls a const fn or constructs a const value, they can extract that
// 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_constable = 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,
ty::FnDef(def_id, _) if tcx.is_const_fn(def_id) => traits::IsConstable::Fn,
_ => traits::IsConstable::No,
},
_ => false,
hir::ExprKind::Path(qpath) => {
match self.typeck_results.borrow().qpath_res(&qpath, element.hir_id) {
Res::Def(DefKind::Ctor(_, CtorKind::Const), _) => traits::IsConstable::Ctor,
_ => traits::IsConstable::No,
}
}
_ => traits::IsConstable::No,
};

// 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_target_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 };
let code = traits::ObligationCauseCode::RepeatElementCopy {
is_constable,
elt_type: element_ty,
elt_span: element.span,
elt_stmt_span: self
.tcx
.hir()
.parent_iter(element.hir_id)
.find_map(|(_, node)| match node {
hir::Node::Item(it) => Some(it.span),
hir::Node::Stmt(stmt) => Some(stmt.span),
_ => None,
})
.expect("array repeat expressions must be inside an item or statement"),
};
self.require_type_meets(element_ty, element.span, code, lang_item);
}
}
Expand Down
26 changes: 23 additions & 3 deletions compiler/rustc_middle/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,14 @@ pub enum ObligationCauseCode<'tcx> {
InlineAsmSized,
/// `[expr; N]` requires `type_of(expr): Copy`.
RepeatElementCopy {
/// If element is a `const fn` we display a help message suggesting to move the
/// function call to a new `const` item while saying that `T` doesn't implement `Copy`.
is_const_fn: bool,
/// If element is a `const fn` or const ctor we display a help message suggesting
/// to move it to a new `const` item while saying that `T` doesn't implement `Copy`.
is_constable: IsConstable,
elt_type: Ty<'tcx>,
elt_span: Span,
/// Span of the statement/item in which the repeat expression occurs. We can use this to
/// place a `const` declaration before it
elt_stmt_span: Span,
},

/// Types of fields (other than the last, except for packed structs) in a struct must be sized.
Expand Down Expand Up @@ -448,6 +453,21 @@ pub enum ObligationCauseCode<'tcx> {
TypeAlias(InternedObligationCauseCode<'tcx>, Span, DefId),
}

/// Whether a value can be extracted into a const.
/// Used for diagnostics around array repeat expressions.
#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, TyEncodable, TyDecodable)]
pub enum IsConstable {
Copy link
Contributor

Choose a reason for hiding this comment

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

👮

No,
/// Call to a const fn
Fn,
/// Use of a const ctor
Ctor,
}

crate::TrivialTypeTraversalAndLiftImpls! {
IsConstable,
}

/// The 'location' at which we try to perform HIR-based wf checking.
/// This information is used to obtain an `hir::Ty`, which
/// we can walk in order to obtain precise spans for any
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use rustc_infer::infer::error_reporting::TypeErrCtxt;
use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use rustc_infer::infer::{DefineOpaqueTypes, InferOk, LateBoundRegionConversionTime};
use rustc_middle::hir::map;
use rustc_middle::traits::IsConstable;
use rustc_middle::ty::error::TypeError::{self, Sorts};
use rustc_middle::ty::{
self, suggest_arbitrary_trait_bound, suggest_constraining_type_param, AdtKind,
Expand Down Expand Up @@ -2832,20 +2833,30 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
));
}
}
ObligationCauseCode::RepeatElementCopy { is_const_fn } => {
ObligationCauseCode::RepeatElementCopy { is_constable, elt_type, elt_span, elt_stmt_span } => {
err.note(
"the `Copy` trait is required because this value will be copied for each element of the array",
);

if is_const_fn {
err.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];`",
);
let value_kind = match is_constable {
IsConstable::Fn => Some("the result of the function call"),
IsConstable::Ctor => Some("the result of the constructor"),
_ => None
};
let sm = tcx.sess.source_map();
if let Some(value_kind) = value_kind &&
let Ok(snip) = sm.span_to_snippet(elt_span)
{
let help_msg = format!(
"consider creating a new `const` item and initializing it with {value_kind} \
to be used in the repeat position");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
to be used in the repeat position");
to be used in the repeat position"
);

let indentation = sm.indentation_before(elt_stmt_span).unwrap_or_default();
err.multipart_suggestion(help_msg, vec![
(elt_stmt_span.shrink_to_lo(), format!("const ARRAY_REPEAT_VALUE: {elt_type} = {snip};\n{indentation}")),
(elt_span, "ARRAY_REPEAT_VALUE".to_string())
], Applicability::MachineApplicable);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this MachineApplicable because ARRAY_REPEAT_VALUE is unlikely to conflict unless there's multiple instances of this in the same scope, and I'm reasonably sure the HIR walking should give us a span it's safe to put a new item on. This can be changed to MaybeIncorrect if there's any weird counterexamples

}

if self.tcx.sess.is_nightly_build() && is_const_fn {
if self.tcx.sess.is_nightly_build() && matches!(is_constable, IsConstable::Fn|IsConstable::Ctor) {
err.help(
"create an inline `const` block, see RFC #2920 \
<https://github.com/rust-lang/rfcs/pull/2920> for more information",
Expand Down
6 changes: 5 additions & 1 deletion tests/ui/consts/const-blocks/fn-call-in-non-const.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@ LL | let _: [Option<Bar>; 2] = [no_copy(); 2];
|
= note: required for `Option<Bar>` to implement `Copy`
= 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)]
LL | struct Bar;
|
help: consider creating a new `const` item and initializing it with the result of the function call to be used in the repeat position
|
LL ~ const ARRAY_REPEAT_VALUE: Option<Bar> = no_copy();
LL ~ let _: [Option<Bar>; 2] = [ARRAY_REPEAT_VALUE; 2];
|

error: aborting due to previous error

Expand Down
6 changes: 5 additions & 1 deletion tests/ui/consts/const-blocks/trait-error.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,13 @@ note: required for `Foo<String>` to implement `Copy`
LL | #[derive(Copy, Clone)]
| ^^^^ unsatisfied trait bound introduced in this `derive` macro
= 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
= note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider creating a new `const` item and initializing it with the result of the function call to be used in the repeat position
|
LL ~ const ARRAY_REPEAT_VALUE: Foo<String> = Foo(String::new());
LL ~ [ARRAY_REPEAT_VALUE; 4];
|

error: aborting due to previous error

Expand Down
8 changes: 6 additions & 2 deletions tests/ui/consts/const-fn-in-vec.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
static _MAYBE_STRINGS: [Option<String>; 5] = [None; 5];
Copy link
Contributor

Choose a reason for hiding this comment

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

If we annotate this file with // run-rustfix, does the test continue to pass? If so, let's add that to protect against involuntary regressions in the suggestions. (You might have to allow some lints, potentially.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah unfortunately no, because the const uses the same name which conflicts. It might be a good idea to maybe make the identifier more unique (do we have a way of 'identifier-izing' an arbritrary string?). Something like Enum::Variant to ENUM_VARIANT might help

//~^ ERROR the trait bound `String: Copy` is not satisfied

fn main() {
// should hint to create an inline `const` block
// or to create a new `const` item
let strings: [String; 5] = [String::new(); 5];
let _strings: [String; 5] = [String::new(); 5];
//~^ ERROR the trait bound `String: Copy` is not satisfied
let _maybe_strings: [Option<String>; 5] = [None; 5];
//~^ ERROR the trait bound `String: Copy` is not satisfied
println!("{:?}", strings);
}
44 changes: 39 additions & 5 deletions tests/ui/consts/const-fn-in-vec.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,47 @@
error[E0277]: the trait bound `String: Copy` is not satisfied
--> $DIR/const-fn-in-vec.rs:4:33
--> $DIR/const-fn-in-vec.rs:1:47
|
LL | let strings: [String; 5] = [String::new(); 5];
| ^^^^^^^^^^^^^ the trait `Copy` is not implemented for `String`
LL | static _MAYBE_STRINGS: [Option<String>; 5] = [None; 5];
| ^^^^ the trait `Copy` is not implemented for `String`
|
= note: required for `Option<String>` to implement `Copy`
= 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 creating a new `const` item and initializing it with the result of the constructor to be used in the repeat position
|
LL + const ARRAY_REPEAT_VALUE: Option<String> = None;
LL ~ static _MAYBE_STRINGS: [Option<String>; 5] = [ARRAY_REPEAT_VALUE; 5];
|

error[E0277]: the trait bound `String: Copy` is not satisfied
--> $DIR/const-fn-in-vec.rs:7:34
|
LL | let _strings: [String; 5] = [String::new(); 5];
| ^^^^^^^^^^^^^ the trait `Copy` is not implemented for `String`
|
= note: the `Copy` trait is required because this value will be copied for each element of the array
= help: create an inline `const` block, see RFC #2920 <https://github.com/rust-lang/rfcs/pull/2920> for more information
help: consider creating a new `const` item and initializing it with the result of the function call to be used in the repeat position
|
LL ~ const ARRAY_REPEAT_VALUE: String = String::new();
LL ~ let _strings: [String; 5] = [ARRAY_REPEAT_VALUE; 5];
|

error[E0277]: the trait bound `String: Copy` is not satisfied
--> $DIR/const-fn-in-vec.rs:9:48
|
LL | let _maybe_strings: [Option<String>; 5] = [None; 5];
| ^^^^ the trait `Copy` is not implemented for `String`
|
= note: required for `Option<String>` to implement `Copy`
= note: the `Copy` trait is required because this value will be copied for each element of the array
= help: create an inline `const` block, see RFC #2920 <https://github.com/rust-lang/rfcs/pull/2920> for more information
help: consider creating a new `const` item and initializing it with the result of the constructor to be used in the repeat position
|
LL ~ const ARRAY_REPEAT_VALUE: Option<String> = None;
LL ~ let _maybe_strings: [Option<String>; 5] = [ARRAY_REPEAT_VALUE; 5];
|

error: aborting due to previous error
error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0277`.