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

Move arg/constraint partition check to validation & improve recovery #70261

Merged
merged 7 commits into from
Mar 28, 2020

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Mar 22, 2020

  • In the first commit, we move the check rejecting e.g., <'a, Item = u8, String> from the parser into AST validation.
  • We then use this to improve the code for parsing generic arguments.
  • And we add recovery for e.g., <Item = > (missing), <Item = 42> (constant), and <Item = 'a> (lifetime).

This is also preparatory work for supporting #70256.

r? @varkor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2020
@Centril Centril added the F-const_generics `#![feature(const_generics)]` label Mar 22, 2020
@rust-highfive

This comment has been minimized.

/// Argument for a generic parameter.
Arg(GenericArg),
/// Constraint for an associated item.
Constraint(AssocTyConstraint),
Copy link
Contributor

@petrochenkov petrochenkov Mar 22, 2020

Choose a reason for hiding this comment

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

At AST/syntax level this should be a variant of GenericArg, IMO.
It would make everything simpler and would help to avoid all those "angle_args" and the terminology duplication.

Only after lowering to HIR the semantic difference between ty/const/lt args and constraints becomes important, before that they are just components of a single list with different grammars.

Copy link
Contributor Author

@Centril Centril Mar 23, 2020

Choose a reason for hiding this comment

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

Putting naming aside, I'm not sure this would simplify things, at least not in terms of parsing (maybe some of the visitors, but that doesn't seem notable). In particular, I would like to have one way of parsing Lifetime | ConstArg | Type and then use that to the right of ident = . So I think the ideal grammar here would be:

TermArg = Lifetime | ConstArg | Type ;
GenericArg = TermArg | Ident "=" TermArg | Ident ":" Bounds ;

It's true that we don't have associated lifetimes, but supporting Foo<Bar = CONST> means we're one variant out. At least in the parser, I would like to have this 3-variant enum equivalent to TermArg so that it can be reused for AssocItem = TermArg for recovery purposes and to support #70256. That could also be achieved with unreachable!() for ::Constraint in your setup, but that doesn't seem ideal?

@bors

This comment has been minimized.

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

Overall, this looks good. I just have a bit of feedback about the diagnostics, and a couple of nits with the code.

src/librustc_ast_lowering/path.rs Show resolved Hide resolved
src/librustc_ast_lowering/path.rs Outdated Show resolved Hide resolved
src/librustc_ast_passes/ast_validation.rs Show resolved Hide resolved
src/test/ui/suggestions/suggest-move-types.stderr Outdated Show resolved Hide resolved
src/test/ui/parser/recover-assoc-const-constraint.stderr Outdated Show resolved Hide resolved
--> $DIR/recover-assoc-eq-missing-term.rs:3:11
|
LL | bar::<Item = >();
| ^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the right span is immediately the right of the =?

help: remove the `=` if `Item` is a type
|
LL | bar::<Item >();
| --
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the whitespace too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did the best I could, almost there, but not quite. Span hackery is deeply mysterious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't go overboard with span wrangling. It's finicky and ICE prone. We can rely on rustfmt for the last 20%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, @estebank I think that's good advice :)

@Centril
Copy link
Contributor Author

Centril commented Mar 27, 2020

@varkor Addressed the comments. :)

| | | this associated type binding should be moved after the generic parameters
| | this associated type binding should be moved after the generic parameters
| this associated type binding should be moved after the generic parameters
| ---- ^ ^ ^ ^^ ^^ ^^
Copy link
Member

Choose a reason for hiding this comment

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

Do we not still want the label here?

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 thought you wanted to avoid the long label repeated on each generic argument. I'll add a shorter one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a shorter label now.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant I thought we should have one long label (the original one), but the spans it should be associated with are all the spans of the generic parameters that need to be reordered. That way we don't have many duplicate lines, but we still point all the generic parameters out, with a helpful message.

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 don't think that works out visually. If we have A, B = u8, C, D = u8, E, then you would get a span underlining D as well, and that wouldn't really visually separate D from the pack.

Also, I don't suspect that the common case will have more than 1 constraint and one generic argument, so I think the diagnostic as is optimizes well for the common case.

Copy link
Member

Choose a reason for hiding this comment

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

cc @estebank: this seems like a good place to be able to use MultiSpan with span_label.

Copy link
Contributor

Choose a reason for hiding this comment

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

For these kind of cases I usually end up giving a label to the last of the spans in a MultiSpan, but we could encode that for span_label<S: Into<MultiSpan>>(&self, span: S, label: &str) in the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably add a label along these lines

error: generic arguments must come before the first constraint
  --> $DIR/suggest-move-types.rs:48:71
   |
LL | struct Bl<'a, 'b, 'c, T, U, V, M: ThreeWithLifetime<A=(), B=(), C=(), T, U, V, 'a, 'b, 'c>> {
   |                                                     ----              ^  ^  ^  ^^  ^^  ^^ the constraint must come after these generic parameters
   |                                                     |
   |                                                     the first constraint is provided here

Also, can we not catch all of A, B and C, like we used to?

Copy link
Member

Choose a reason for hiding this comment

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

@estebank: this is exactly the sort of thing I was thinking 👍

Also, can we not catch all of A, B and C, like we used to?

I suppose we can, but @Centril thought that it's only the first one that's really relevant for repositioning.

@Centril
Copy link
Contributor Author

Centril commented Mar 27, 2020

Per private discussions on Discord, @bors r=varkor

@bors
Copy link
Contributor

bors commented Mar 27, 2020

📌 Commit 42c5cfd has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2020
Comment on lines +650 to +669
let mut misplaced_args = Vec::new();
let mut first = None;
for arg in &data.args {
match (arg, first) {
(AngleBracketedArg::Arg(a), Some(_)) => misplaced_args.push(a.span()),
(AngleBracketedArg::Constraint(c), None) => first = Some(c.span),
(AngleBracketedArg::Arg(_), None) | (AngleBracketedArg::Constraint(_), Some(_)) => {
}
}
}
// ...and then error:
self.err_handler()
.struct_span_err(
misplaced_args.clone(),
"generic arguments must come before the first constraint",
)
.span_label(first.unwrap(), "the first constraint is provided here")
.span_labels(misplaced_args, "generic argument")
.emit();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about?

Suggested change
let mut misplaced_args = Vec::new();
let mut first = None;
for arg in &data.args {
match (arg, first) {
(AngleBracketedArg::Arg(a), Some(_)) => misplaced_args.push(a.span()),
(AngleBracketedArg::Constraint(c), None) => first = Some(c.span),
(AngleBracketedArg::Arg(_), None) | (AngleBracketedArg::Constraint(_), Some(_)) => {
}
}
}
// ...and then error:
self.err_handler()
.struct_span_err(
misplaced_args.clone(),
"generic arguments must come before the first constraint",
)
.span_label(first.unwrap(), "the first constraint is provided here")
.span_labels(misplaced_args, "generic argument")
.emit();
}
let constraint_spans = data.args.iter().filter_map(|arg| match arg {
AngleBracketedArg::Constraint(c) => Some(c.span),
_ => None,
}).collect::<Vec<_>>();
let arg_spans = data.args.iter().filter_map(|arg| match arg {
AngleBracketedArg::Arg(a) => Some(a.span()),
_ => None,
}).collect::<Vec<_>>();
let constraint_len = constraint_spans.len();
// ...and then error:
self.err_handler()
.struct_span_err(
arg_spans.clone(),
"generic arguments must come before the first constraint",
)
.span_labels(constraint_spans, &format!(
"the constraint{} {} provided here",
pluralize!(constraint_len),
if constraint_len == 1 {
"is"
} else {
"are"
}
))
.span_labels(arg_spans, "generic argument")
.emit();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we don't provide a suggestion, but we could 😇

@varkor
Copy link
Member

varkor commented Mar 28, 2020

@estebank: happy for you to r- if you'd like these changes to be made now. (Though I can't speak for @Centril 😅)

@bors
Copy link
Contributor

bors commented Mar 28, 2020

⌛ Testing commit 42c5cfd with merge b9d5ee5...

@bors
Copy link
Contributor

bors commented Mar 28, 2020

☀️ Test successful - checks-azure
Approved by: varkor
Pushing b9d5ee5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2020
@bors bors merged commit b9d5ee5 into rust-lang:master Mar 28, 2020
@Centril Centril deleted the angle-args-partition branch March 28, 2020 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants