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
Merged
Show file tree
Hide file tree
Changes from 6 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
29 changes: 19 additions & 10 deletions src/librustc_ast/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,18 @@ impl GenericArg {
pub struct AngleBracketedArgs {
/// The overall span.
pub span: Span,
/// The arguments for this path segment.
pub args: Vec<GenericArg>,
/// Constraints on associated types, if any.
/// E.g., `Foo<A = Bar, B: Baz>`.
pub constraints: Vec<AssocTyConstraint>,
/// The comma separated parts in the `<...>`.
pub args: Vec<AngleBracketedArg>,
}

/// Either an argument for a parameter e.g., `'a`, `Vec<u8>`, `0`,
/// or a constraint on an associated item, e.g., `Item = String` or `Item: Bound`.
#[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
pub enum AngleBracketedArg {
/// 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?

}

impl Into<Option<P<GenericArgs>>> for AngleBracketedArgs {
Expand Down Expand Up @@ -248,11 +255,13 @@ pub struct ParenthesizedArgs {

impl ParenthesizedArgs {
pub fn as_angle_bracketed_args(&self) -> AngleBracketedArgs {
AngleBracketedArgs {
span: self.span,
args: self.inputs.iter().cloned().map(GenericArg::Type).collect(),
constraints: vec![],
}
let args = self
.inputs
.iter()
.cloned()
.map(|input| AngleBracketedArg::Arg(GenericArg::Type(input)))
.collect();
AngleBracketedArgs { span: self.span, args }
}
}

Expand Down
8 changes: 5 additions & 3 deletions src/librustc_ast/mut_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,9 +546,11 @@ pub fn noop_visit_angle_bracketed_parameter_data<T: MutVisitor>(
data: &mut AngleBracketedArgs,
vis: &mut T,
) {
let AngleBracketedArgs { args, constraints, span } = data;
visit_vec(args, |arg| vis.visit_generic_arg(arg));
visit_vec(constraints, |constraint| vis.visit_ty_constraint(constraint));
let AngleBracketedArgs { args, span } = data;
visit_vec(args, |arg| match arg {
AngleBracketedArg::Arg(arg) => vis.visit_generic_arg(arg),
AngleBracketedArg::Constraint(constraint) => vis.visit_ty_constraint(constraint),
});
vis.visit_span(span);
}

Expand Down
8 changes: 6 additions & 2 deletions src/librustc_ast/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,8 +464,12 @@ where
{
match *generic_args {
GenericArgs::AngleBracketed(ref data) => {
walk_list!(visitor, visit_generic_arg, &data.args);
walk_list!(visitor, visit_assoc_ty_constraint, &data.constraints);
for arg in &data.args {
match arg {
AngleBracketedArg::Arg(a) => visitor.visit_generic_arg(a),
AngleBracketedArg::Constraint(c) => visitor.visit_assoc_ty_constraint(c),
}
}
}
GenericArgs::Parenthesized(ref data) => {
walk_list!(visitor, visit_ty, &data.inputs);
Expand Down
1 change: 1 addition & 0 deletions src/librustc_ast_lowering/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#![feature(crate_visibility_modifier)]
#![feature(marker_trait_attr)]
#![feature(specialization)]
#![feature(or_patterns)]
#![recursion_limit = "256"]

use rustc_ast::ast;
Expand Down
35 changes: 20 additions & 15 deletions src/librustc_ast_lowering/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,22 +366,27 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
param_mode: ParamMode,
mut itctx: ImplTraitContext<'_, 'hir>,
) -> (GenericArgsCtor<'hir>, bool) {
let &AngleBracketedArgs { ref args, ref constraints, .. } = data;
let has_non_lt_args = args.iter().any(|arg| match arg {
ast::GenericArg::Lifetime(_) => false,
ast::GenericArg::Type(_) => true,
ast::GenericArg::Const(_) => true,
let has_non_lt_args = data.args.iter().any(|arg| match arg {
AngleBracketedArg::Arg(ast::GenericArg::Lifetime(_))
| AngleBracketedArg::Constraint(_) => false,
AngleBracketedArg::Arg(ast::GenericArg::Type(_) | ast::GenericArg::Const(_)) => true,
});
(
GenericArgsCtor {
args: args.iter().map(|a| self.lower_generic_arg(a, itctx.reborrow())).collect(),
bindings: self.arena.alloc_from_iter(
constraints.iter().map(|b| self.lower_assoc_ty_constraint(b, itctx.reborrow())),
),
parenthesized: false,
},
!has_non_lt_args && param_mode == ParamMode::Optional,
)
let args = data
.args
.iter()
.filter_map(|arg| match arg {
AngleBracketedArg::Arg(arg) => Some(self.lower_generic_arg(arg, itctx.reborrow())),
AngleBracketedArg::Constraint(_) => None,
})
.collect();
let bindings = self.arena.alloc_from_iter(data.args.iter().filter_map(|arg| match arg {
Centril marked this conversation as resolved.
Show resolved Hide resolved
AngleBracketedArg::Constraint(c) => {
Some(self.lower_assoc_ty_constraint(c, itctx.reborrow()))
}
AngleBracketedArg::Arg(_) => None,
}));
let ctor = GenericArgsCtor { args, bindings, parenthesized: false };
(ctor, !has_non_lt_args && param_mode == ParamMode::Optional)
}

fn lower_parenthesized_parameter_data(
Expand Down
52 changes: 41 additions & 11 deletions src/librustc_ast_passes/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,33 @@ impl<'a> AstValidator<'a> {
.emit();
}
}

/// Enforce generic args coming before constraints in `<...>` of a path segment.
fn check_generic_args_before_constraints(&self, data: &AngleBracketedArgs) {
// Early exit in case it's partitioned as it should be.
if data.args.iter().is_partitioned(|arg| matches!(arg, AngleBracketedArg::Arg(_))) {
return;
}
// Find all generic argument coming after the first constraint...
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(_)) => {
}
Centril marked this conversation as resolved.
Show resolved Hide resolved
}
}
// ...and then error:
self.err_handler()
.struct_span_err(
misplaced_args,
"generic arguments must come before the first constraint",
)
.span_label(first.unwrap(), "the first constraint is provided here")
.emit();
}
}

/// Checks that generic parameters are in the correct order,
Expand Down Expand Up @@ -1008,17 +1035,20 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
fn visit_generic_args(&mut self, _: Span, generic_args: &'a GenericArgs) {
match *generic_args {
GenericArgs::AngleBracketed(ref data) => {
walk_list!(self, visit_generic_arg, &data.args);

// Type bindings such as `Item = impl Debug` in `Iterator<Item = Debug>`
// are allowed to contain nested `impl Trait`.
self.with_impl_trait(None, |this| {
walk_list!(
this,
visit_assoc_ty_constraint_from_generic_args,
&data.constraints
);
});
self.check_generic_args_before_constraints(data);

for arg in &data.args {
match arg {
AngleBracketedArg::Arg(arg) => self.visit_generic_arg(arg),
// Type bindings such as `Item = impl Debug` in `Iterator<Item = Debug>`
// are allowed to contain nested `impl Trait`.
AngleBracketedArg::Constraint(constraint) => {
self.with_impl_trait(None, |this| {
this.visit_assoc_ty_constraint_from_generic_args(constraint);
});
}
}
}
}
GenericArgs::Parenthesized(ref data) => {
walk_list!(self, visit_ty, &data.inputs);
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_ast_passes/lib.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
#![feature(bindings_after_at)]
//! The `rustc_ast_passes` crate contains passes which validate the AST in `syntax`
//! parsed by `rustc_parse` and then lowered, after the passes in this crate,
//! by `rustc_ast_lowering`.
//!
//! The crate also contains other misc AST visitors, e.g. `node_count` and `show_span`.

#![feature(bindings_after_at)]
#![feature(iter_is_partitioned)]

pub mod ast_validation;
pub mod feature_gate;
pub mod node_count;
Expand Down
41 changes: 17 additions & 24 deletions src/librustc_ast_pretty/pprust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -796,31 +796,10 @@ impl<'a> PrintState<'a> for State<'a> {
match *args {
ast::GenericArgs::AngleBracketed(ref data) => {
self.s.word("<");

self.commasep(Inconsistent, &data.args, |s, generic_arg| {
s.print_generic_arg(generic_arg)
self.commasep(Inconsistent, &data.args, |s, arg| match arg {
ast::AngleBracketedArg::Arg(a) => s.print_generic_arg(a),
ast::AngleBracketedArg::Constraint(c) => s.print_assoc_constraint(c),
});

let mut comma = !data.args.is_empty();

for constraint in data.constraints.iter() {
if comma {
self.word_space(",")
}
self.print_ident(constraint.ident);
self.s.space();
match constraint.kind {
ast::AssocTyConstraintKind::Equality { ref ty } => {
self.word_space("=");
self.print_type(ty);
}
ast::AssocTyConstraintKind::Bound { ref bounds } => {
self.print_type_bounds(":", &*bounds);
}
}
comma = true;
}

self.s.word(">")
}

Expand Down Expand Up @@ -891,6 +870,20 @@ impl<'a> State<'a> {
}
}

fn print_assoc_constraint(&mut self, constraint: &ast::AssocTyConstraint) {
self.print_ident(constraint.ident);
self.s.space();
match &constraint.kind {
ast::AssocTyConstraintKind::Equality { ty } => {
self.word_space("=");
self.print_type(ty);
}
ast::AssocTyConstraintKind::Bound { bounds } => {
self.print_type_bounds(":", &*bounds);
}
}
}

crate fn print_generic_arg(&mut self, generic_arg: &GenericArg) {
match generic_arg {
GenericArg::Lifetime(lt) => self.print_lifetime(*lt),
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_expand/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ impl<'a> ExtCtxt<'a> {
idents.into_iter().map(|ident| ast::PathSegment::from_ident(ident.with_span_pos(span))),
);
let args = if !args.is_empty() {
ast::AngleBracketedArgs { args, constraints: Vec::new(), span }.into()
let args = args.into_iter().map(ast::AngleBracketedArg::Arg).collect();
ast::AngleBracketedArgs { args, span }.into()
} else {
None
};
Expand Down
16 changes: 9 additions & 7 deletions src/librustc_interface/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -634,17 +634,19 @@ impl<'a, 'b> ReplaceBodyWithLoop<'a, 'b> {
match seg.args.as_ref().map(|generic_arg| &**generic_arg) {
None => false,
Some(&ast::GenericArgs::AngleBracketed(ref data)) => {
let types = data.args.iter().filter_map(|arg| match arg {
ast::GenericArg::Type(ty) => Some(ty),
_ => None,
});
any_involves_impl_trait(types)
|| data.constraints.iter().any(|c| match c.kind {
data.args.iter().any(|arg| match arg {
ast::AngleBracketedArg::Arg(arg) => match arg {
ast::GenericArg::Type(ty) => involves_impl_trait(ty),
ast::GenericArg::Lifetime(_)
| ast::GenericArg::Const(_) => false,
},
ast::AngleBracketedArg::Constraint(c) => match c.kind {
ast::AssocTyConstraintKind::Bound { .. } => true,
ast::AssocTyConstraintKind::Equality { ref ty } => {
involves_impl_trait(ty)
}
})
},
})
}
Some(&ast::GenericArgs::Parenthesized(ref data)) => {
any_involves_impl_trait(data.inputs.iter())
Expand Down
Loading