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

Add check for [T;N]/usize mismatch in astconv #80538

Merged
merged 1 commit into from
Jan 5, 2021
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
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3745,6 +3745,7 @@ version = "0.0.0"
dependencies = [
"rustc_ast",
"rustc_data_structures",
"rustc_feature",
"rustc_index",
"rustc_macros",
"rustc_serialize",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ doctest = false

[dependencies]
rustc_target = { path = "../rustc_target" }
rustc_feature = { path = "../rustc_feature" }
rustc_macros = { path = "../rustc_macros" }
rustc_data_structures = { path = "../rustc_data_structures" }
rustc_index = { path = "../rustc_index" }
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,14 @@ impl GenericArg<'_> {
GenericArg::Const(_) => "const",
}
}

pub fn to_ord(&self, feats: &rustc_feature::Features) -> ast::ParamKindOrd {
match self {
GenericArg::Lifetime(_) => ast::ParamKindOrd::Lifetime,
GenericArg::Type(_) => ast::ParamKindOrd::Type,
GenericArg::Const(_) => ast::ParamKindOrd::Const { unordered: feats.const_generics },
}
}
}

#[derive(Debug, HashStable_Generic)]
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,15 @@ impl GenericParamDefKind {
GenericParamDefKind::Const => "constant",
}
}
pub fn to_ord(&self, tcx: TyCtxt<'_>) -> ast::ParamKindOrd {
match self {
GenericParamDefKind::Lifetime => ast::ParamKindOrd::Lifetime,
GenericParamDefKind::Type { .. } => ast::ParamKindOrd::Type,
GenericParamDefKind::Const => {
ast::ParamKindOrd::Const { unordered: tcx.features().const_generics }
}
}
}
}

#[derive(Clone, Debug, TyEncodable, TyDecodable, HashStable)]
Expand Down
92 changes: 51 additions & 41 deletions compiler/rustc_typeck/src/astconv/generics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use rustc_hir::GenericArg;
use rustc_middle::ty::{
self, subst, subst::SubstsRef, GenericParamDef, GenericParamDefKind, Ty, TyCtxt,
};
use rustc_session::{lint::builtin::LATE_BOUND_LIFETIME_ARGUMENTS, Session};
use rustc_session::lint::builtin::LATE_BOUND_LIFETIME_ARGUMENTS;
use rustc_span::{symbol::kw, MultiSpan, Span};

use smallvec::SmallVec;
Expand All @@ -20,62 +20,72 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
/// Report an error that a generic argument did not match the generic parameter that was
/// expected.
fn generic_arg_mismatch_err(
sess: &Session,
tcx: TyCtxt<'_>,
arg: &GenericArg<'_>,
kind: &'static str,
param: &GenericParamDef,
possible_ordering_error: bool,
help: Option<&str>,
) {
let sess = tcx.sess;
let mut err = struct_span_err!(
sess,
arg.span(),
E0747,
"{} provided when a {} was expected",
arg.descr(),
kind,
param.kind.descr(),
);

let unordered = sess.features_untracked().const_generics;
let kind_ord = match kind {
"lifetime" => ParamKindOrd::Lifetime,
"type" => ParamKindOrd::Type,
"constant" => ParamKindOrd::Const { unordered },
// It's more concise to match on the string representation, though it means
// the match is non-exhaustive.
_ => bug!("invalid generic parameter kind {}", kind),
};

if let ParamKindOrd::Const { .. } = kind_ord {
if let GenericParamDefKind::Const { .. } = param.kind {
if let GenericArg::Type(hir::Ty { kind: hir::TyKind::Infer, .. }) = arg {
err.help("const arguments cannot yet be inferred with `_`");
}
}

let arg_ord = match arg {
GenericArg::Lifetime(_) => ParamKindOrd::Lifetime,
GenericArg::Type(_) => ParamKindOrd::Type,
GenericArg::Const(_) => ParamKindOrd::Const { unordered },
};

if matches!(arg, GenericArg::Type(hir::Ty { kind: hir::TyKind::Path { .. }, .. }))
&& matches!(kind_ord, ParamKindOrd::Const { .. })
{
let suggestions = vec![
(arg.span().shrink_to_lo(), String::from("{ ")),
(arg.span().shrink_to_hi(), String::from(" }")),
];
err.multipart_suggestion(
"if this generic argument was intended as a const parameter, \
// Specific suggestion set for diagnostics
match (arg, &param.kind) {
(
GenericArg::Type(hir::Ty { kind: hir::TyKind::Path { .. }, .. }),
GenericParamDefKind::Const { .. },
) => {
let suggestions = vec![
(arg.span().shrink_to_lo(), String::from("{ ")),
(arg.span().shrink_to_hi(), String::from(" }")),
];
err.multipart_suggestion(
"if this generic argument was intended as a const parameter, \
try surrounding it with braces:",
suggestions,
Applicability::MaybeIncorrect,
);
suggestions,
Applicability::MaybeIncorrect,
);
}
(
GenericArg::Type(hir::Ty { kind: hir::TyKind::Array(_, len), .. }),
GenericParamDefKind::Const { .. },
) if tcx.type_of(param.def_id) == tcx.types.usize => {
let snippet = sess.source_map().span_to_snippet(tcx.hir().span(len.hir_id));
if let Ok(snippet) = snippet {
err.span_suggestion(
arg.span(),
"array type provided where a `usize` was expected, try",
format!("{{ {} }}", snippet),
Applicability::MaybeIncorrect,
);
}
}
_ => {}
}

let kind_ord = param.kind.to_ord(tcx);
let arg_ord = arg.to_ord(&tcx.features());

// This note is only true when generic parameters are strictly ordered by their kind.
if possible_ordering_error && kind_ord.cmp(&arg_ord) != core::cmp::Ordering::Equal {
let (first, last) =
if kind_ord < arg_ord { (kind, arg.descr()) } else { (arg.descr(), kind) };
let (first, last) = if kind_ord < arg_ord {
(param.kind.descr(), arg.descr())
} else {
(arg.descr(), param.kind.descr())
};
err.note(&format!("{} arguments must be provided before {} arguments", first, last));
if let Some(help) = help {
err.help(help);
Expand Down Expand Up @@ -203,7 +213,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
// We expected a lifetime argument, but got a type or const
// argument. That means we're inferring the lifetimes.
substs.push(ctx.inferred_kind(None, param, infer_args));
force_infer_lt = Some(arg);
force_infer_lt = Some((arg, param));
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that this loop is a lot hotter than I expected so storing arg with param here is quite costly?

Other than that i really don't know waht could be causing the perf regression.

Copy link
Contributor Author

@JulianKnodt JulianKnodt Jan 8, 2021

Choose a reason for hiding this comment

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

I agree that this would be the most likely cause, it could be that the type for param for deeply-nested-async stuff is huge and thus takes up a lot of space on the stack, so even if it's not hot it might just be causing a lot more cache misses

edit: these are refs, so the size shouldn't matter, ignore above

Would it be good to change this to an index instead? I don't remember where the param was coming from at the moment

Copy link
Member

Choose a reason for hiding this comment

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

It seems possible that this increases the size of force_infer_lt enough that it becomes stored on the stack instead of a register, I guess, but it's pretty odd that it would cause any problems (still just two pointers vs one). An index would still be a usize so should be equally costly I'd expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a print to the inner body, and during compilation it appears that the body is hit at least 1.5M times, so I guess that's fairly hot?

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean during the compilation of the compiler itself? so during ./x.py build --stage 2 or just while building std?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was running ./x.py test src/test/ui, but it got to compiling stage 1 artifacts and I quit out

params.next();
}
(GenericArg::Lifetime(_), _, ExplicitLateBound::Yes) => {
Expand All @@ -213,7 +223,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
// ignore it.
args.next();
}
(_, kind, _) => {
(_, _, _) => {
// We expected one kind of parameter, but the user provided
// another. This is an error. However, if we already know that
// the arguments don't match up with the parameters, we won't issue
Expand Down Expand Up @@ -256,9 +266,9 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
param_types_present.dedup();

Self::generic_arg_mismatch_err(
tcx.sess,
tcx,
arg,
kind.descr(),
param,
!args_iter.clone().is_sorted_by_key(|arg| match arg {
GenericArg::Lifetime(_) => ParamKindOrd::Lifetime,
GenericArg::Type(_) => ParamKindOrd::Type,
Expand Down Expand Up @@ -315,9 +325,9 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
{
let kind = arg.descr();
assert_eq!(kind, "lifetime");
let provided =
let (provided_arg, param) =
force_infer_lt.expect("lifetimes ought to have been inferred");
Self::generic_arg_mismatch_err(tcx.sess, provided, kind, false, None);
Self::generic_arg_mismatch_err(tcx, provided_arg, param, false, None);
}

break;
Expand Down
10 changes: 10 additions & 0 deletions src/test/ui/const-generics/suggest_const_for_array.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#![crate_type = "lib"]

fn example<const N: usize>() {}

fn other() {
example::<[usize; 3]>();
//~^ ERROR type provided when a const
example::<[usize; 4+5]>();
//~^ ERROR type provided when a const
}
15 changes: 15 additions & 0 deletions src/test/ui/const-generics/suggest_const_for_array.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0747]: type provided when a constant was expected
--> $DIR/suggest_const_for_array.rs:6:13
|
LL | example::<[usize; 3]>();
| ^^^^^^^^^^ help: array type provided where a `usize` was expected, try: `{ 3 }`

error[E0747]: type provided when a constant was expected
--> $DIR/suggest_const_for_array.rs:8:13
|
LL | example::<[usize; 4+5]>();
| ^^^^^^^^^^^^ help: array type provided where a `usize` was expected, try: `{ 4+5 }`

error: aborting due to 2 previous errors

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