From 39ae7441edf8213360520a9292ea18c0dae1bf77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 23 Sep 2019 12:46:52 -0700 Subject: [PATCH] review comments --- src/libsyntax/ast.rs | 23 ++-- src/libsyntax/parse/parser/expr.rs | 4 +- src/libsyntax/parse/parser/path.rs | 117 +++++++++++------- .../const-expression-parameter.rs | 45 +++---- .../const-expression-parameter.stderr | 99 +++++++++++---- .../disallowed-positions.rs | 5 +- .../disallowed-positions.stderr | 22 +--- 7 files changed, 200 insertions(+), 115 deletions(-) diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index 87b1f8451a0ed..3c893436a9357 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -1255,17 +1255,26 @@ pub enum ExprKind { } impl ExprKind { - /// Whether this expression can appear in a contst argument without being surrounded by braces. + /// Whether this expression can appear applied to a `const` parameter without being surrounded + /// by braces. /// /// Only used in error recovery. - pub(crate) fn is_valid_const_on_its_own(&self) -> bool { + crate fn is_valid_const_on_its_own(&self) -> bool { + fn is_const_lit(kind: &ExprKind) -> bool { + // These are the only literals that can be negated as a bare `const` argument. + match kind { + ExprKind::Lit(Lit { node: LitKind::Int(..), ..}) | + ExprKind::Lit(Lit { node: LitKind::Float(..), ..}) | + ExprKind::Lit(Lit { node: LitKind::FloatUnsuffixed(..), ..}) => true, + _ => false, + } + } + match self { - ExprKind::Tup(_) | - ExprKind::Lit(_) | - ExprKind::Type(..) | - ExprKind::Path(..) | - ExprKind::Unary(..) | + ExprKind::Lit(_) | // `foo::<42>()` ExprKind::Err => true, + // `foo::<-42>()` + ExprKind::Unary(UnOp::Neg, expr) if is_const_lit(&expr.node) => true, _ => false, } } diff --git a/src/libsyntax/parse/parser/expr.rs b/src/libsyntax/parse/parser/expr.rs index b21ac4ecb7ea2..14483fdfb052d 100644 --- a/src/libsyntax/parse/parser/expr.rs +++ b/src/libsyntax/parse/parser/expr.rs @@ -355,7 +355,9 @@ impl<'a> Parser<'a> { (self.restrictions.contains(Restrictions::STMT_EXPR) && !classify::expr_requires_semi_to_be_stmt(e)) || (self.restrictions.contains(Restrictions::CONST_EXPR_RECOVERY) && - (self.token == token::Lt || self.token == token::Gt || self.token == token::Comma)) + // `<` is necessary here to avoid cases like `foo::< 1 < 3 >()` where we'll fallback + // to a regular parse error without recovery or suggestions. + [token::Lt, token::Gt, token::Comma].contains(&self.token.kind)) } fn is_at_start_of_range_notation_rhs(&self) -> bool { diff --git a/src/libsyntax/parse/parser/path.rs b/src/libsyntax/parse/parser/path.rs index 05233d3440adb..ac153f66fc385 100644 --- a/src/libsyntax/parse/parser/path.rs +++ b/src/libsyntax/parse/parser/path.rs @@ -1,8 +1,10 @@ -use super::{Parser, PResult, Restrictions, TokenType}; +use super::{P, Parser, PResult, Restrictions, TokenType}; use crate::{maybe_whole, ThinVec}; -use crate::ast::{self, QSelf, Path, PathSegment, Ident, ParenthesizedArgs, AngleBracketedArgs}; -use crate::ast::{AnonConst, GenericArg, AssocTyConstraint, AssocTyConstraintKind, BlockCheckMode}; +use crate::ast::{ + self, AngleBracketedArgs, AnonConst, AssocTyConstraint, AssocTyConstraintKind, BlockCheckMode, + Expr, GenericArg, Ident, ParenthesizedArgs, Path, PathSegment, QSelf, +}; use crate::parse::token::{self, Token}; use crate::source_map::{Span, BytePos}; use crate::symbol::kw; @@ -412,54 +414,37 @@ impl<'a> Parser<'a> { span, }); assoc_ty_constraints.push(span); + } else if [ + token::Not, + token::OpenDelim(token::Paren), + ].contains(&self.token.kind) && self.look_ahead(1, |t| t.is_lit() || t.is_bool_lit()) { + // Parse bad `const` argument. `!` is only allowed here to go through + // `recover_bare_const_expr` for better diagnostics when encountering + // `foo::()`. `(` is allowed for the case `foo::<(1, 2, 3)>()`. + + // This can't possibly be a valid const arg, it is likely missing braces. + let value = AnonConst { + id: ast::DUMMY_NODE_ID, + value: self.recover_bare_const_expr()?, + }; + args.push(GenericArg::Const(value)); + misplaced_assoc_ty_constraints.append(&mut assoc_ty_constraints); } else if self.check_const_arg() { - // Parse const argument. + // Parse `const` argument. + + // `const` arguments that don't require surrunding braces would have a length of + // one token, so anything that *isn't* surrounded by braces and is not + // immediately followed by `,` or `>` is not a valid `const` argument. let invalid = self.look_ahead(1, |t| t != &token::Lt && t != &token::Comma); + let expr = if let token::OpenDelim(token::Brace) = self.token.kind { + // Parse `const` argument surrounded by braces. self.parse_block_expr( None, self.token.span, BlockCheckMode::Default, ThinVec::new() )? } else if invalid { // This can't possibly be a valid const arg, it is likely missing braces. - let snapshot = self.clone(); - match self.parse_expr_res(Restrictions::CONST_EXPR_RECOVERY, None) { - Ok(expr) => { - if self.token == token::Comma || self.token == token::Gt { - // We parsed the whole const argument successfully without braces. - if !expr.node.is_valid_const_on_its_own() { - // But it wasn't a literal, so we emit a custom error and - // suggest the appropriate code. - let msg = - "complex const arguments must be surrounded by braces"; - let appl = Applicability::MachineApplicable; - self.span_fatal(expr.span, msg) - .multipart_suggestion( - "surround this const argument in braces", - vec![ - (expr.span.shrink_to_lo(), "{ ".to_string()), - (expr.span.shrink_to_hi(), " }".to_string()), - ], - appl, - ) - .emit(); - } - expr - } else { - // We parsed *some* expression, but it isn't the whole argument - // so we can't ensure it was a const argument with missing braces. - // Roll-back and emit a regular parser error. - mem::replace(self, snapshot); - self.parse_literal_maybe_minus()? - } - } - Err(mut err) => { - // We couldn't parse an expression successfully. - // Roll-back, hide the error and emit a regular parser error. - err.cancel(); - mem::replace(self, snapshot); - self.parse_literal_maybe_minus()? - } - } + self.recover_bare_const_expr()? } else if self.token.is_ident() { // FIXME(const_generics): to distinguish between idents for types and consts, // we should introduce a GenericArg::Ident in the AST and distinguish when @@ -512,4 +497,50 @@ impl<'a> Parser<'a> { Ok((args, constraints)) } + + fn recover_bare_const_expr(&mut self) -> PResult<'a, P> { + let snapshot = self.clone(); + debug!("recover_bare_const_expr {:?}", self.token); + match self.parse_expr_res(Restrictions::CONST_EXPR_RECOVERY, None) { + Ok(expr) => { + debug!("recover_bare_const_expr expr {:?} {:?}", expr, expr.node); + if let token::Comma | token::Gt = self.token.kind { + // We parsed the whole const argument successfully without braces. + debug!("recover_bare_const_expr ok"); + if !expr.node.is_valid_const_on_its_own() { + // But it wasn't a literal, so we emit a custom error and + // suggest the appropriate code. `foo::<-1>()` is valid but gets parsed + // here, so we need to gate the error only for invalid cases. + self.span_fatal( + expr.span, + "complex const arguments must be surrounded by braces", + ).multipart_suggestion( + "surround this const argument in braces", + vec![ + (expr.span.shrink_to_lo(), "{ ".to_string()), + (expr.span.shrink_to_hi(), " }".to_string()), + ], + Applicability::MachineApplicable, + ).emit(); + } + Ok(expr) + } else { + debug!("recover_bare_const_expr not"); + // We parsed *some* expression, but it isn't the whole argument + // so we can't ensure it was a const argument with missing braces. + // Roll-back and emit a regular parser error. + mem::replace(self, snapshot); + self.parse_literal_maybe_minus() + } + } + Err(mut err) => { + debug!("recover_bare_const_expr err"); + // We couldn't parse an expression successfully. + // Roll-back, hide the error and emit a regular parser error. + err.cancel(); + mem::replace(self, snapshot); + self.parse_literal_maybe_minus() + } + } + } } diff --git a/src/test/ui/const-generics/const-expression-parameter.rs b/src/test/ui/const-generics/const-expression-parameter.rs index 80ee4a0808543..105bcdc29d135 100644 --- a/src/test/ui/const-generics/const-expression-parameter.rs +++ b/src/test/ui/const-generics/const-expression-parameter.rs @@ -1,39 +1,40 @@ #![feature(const_generics)] //~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash -fn i32_identity() -> i32 { - 5 -} +fn foo() -> i32 { 5 } -fn foo_a() { - i32_identity::<-1>(); // ok -} +fn baz() { } -fn foo_b() { - i32_identity::<1 + 2>(); //~ ERROR complex const arguments must be surrounded by braces -} +fn bar() {} -fn foo_c() { - i32_identity::< -1 >(); // ok -} +fn bat() {} -fn foo_d() { - i32_identity::<1 + 2, 3 + 4>(); +fn main() { + foo::<-1>(); // ok + foo::<1 + 2>(); //~ ERROR complex const arguments must be surrounded by braces + foo::< -1 >(); // ok + foo::<1 + 2, 3 + 4>(); //~^ ERROR complex const arguments must be surrounded by braces //~| ERROR complex const arguments must be surrounded by braces //~| ERROR wrong number of const arguments: expected 1, found 2 -} + foo::<5>(); // ok -fn baz() -> i32 { - 42 -} - -fn foo_e() { + baz::<-1, -2>(); // ok baz::<1 + 2, 3 + 4>(); //~^ ERROR complex const arguments must be surrounded by braces //~| ERROR complex const arguments must be surrounded by braces + baz::< -1 , 2 >(); // ok + baz::< -1 , "2" >(); //~ ERROR mismatched types + + bat::<(1, 2, 3)>(); //~ ERROR complex const arguments must be surrounded by braces + bat::<(1, 2)>(); + //~^ ERROR complex const arguments must be surrounded by braces + //~| ERROR mismatched types + + bar::(); // ok + bar::(); //~ ERROR complex const arguments must be surrounded by braces } -fn main() { - i32_identity::<5>(); // ok +fn parse_err_1() { + bar::< 3 < 4 >(); //~ ERROR expected one of `,` or `>`, found `<` } diff --git a/src/test/ui/const-generics/const-expression-parameter.stderr b/src/test/ui/const-generics/const-expression-parameter.stderr index 856176aa5d894..e968f195212f0 100644 --- a/src/test/ui/const-generics/const-expression-parameter.stderr +++ b/src/test/ui/const-generics/const-expression-parameter.stderr @@ -1,35 +1,35 @@ error: complex const arguments must be surrounded by braces - --> $DIR/const-expression-parameter.rs:13:20 + --> $DIR/const-expression-parameter.rs:14:11 | -LL | i32_identity::<1 + 2>(); - | ^^^^^ +LL | foo::<1 + 2>(); + | ^^^^^ help: surround this const argument in braces | -LL | i32_identity::<{ 1 + 2 }>(); - | ^ ^ +LL | foo::<{ 1 + 2 }>(); + | ^ ^ error: complex const arguments must be surrounded by braces - --> $DIR/const-expression-parameter.rs:21:20 + --> $DIR/const-expression-parameter.rs:16:11 | -LL | i32_identity::<1 + 2, 3 + 4>(); - | ^^^^^ +LL | foo::<1 + 2, 3 + 4>(); + | ^^^^^ help: surround this const argument in braces | -LL | i32_identity::<{ 1 + 2 }, 3 + 4>(); - | ^ ^ +LL | foo::<{ 1 + 2 }, 3 + 4>(); + | ^ ^ error: complex const arguments must be surrounded by braces - --> $DIR/const-expression-parameter.rs:21:27 + --> $DIR/const-expression-parameter.rs:16:18 | -LL | i32_identity::<1 + 2, 3 + 4>(); - | ^^^^^ +LL | foo::<1 + 2, 3 + 4>(); + | ^^^^^ help: surround this const argument in braces | -LL | i32_identity::<1 + 2, { 3 + 4 }>(); - | ^ ^ +LL | foo::<1 + 2, { 3 + 4 }>(); + | ^ ^ error: complex const arguments must be surrounded by braces - --> $DIR/const-expression-parameter.rs:32:11 + --> $DIR/const-expression-parameter.rs:23:11 | LL | baz::<1 + 2, 3 + 4>(); | ^^^^^ @@ -39,7 +39,7 @@ LL | baz::<{ 1 + 2 }, 3 + 4>(); | ^ ^ error: complex const arguments must be surrounded by braces - --> $DIR/const-expression-parameter.rs:32:18 + --> $DIR/const-expression-parameter.rs:23:18 | LL | baz::<1 + 2, 3 + 4>(); | ^^^^^ @@ -48,6 +48,42 @@ help: surround this const argument in braces LL | baz::<1 + 2, { 3 + 4 }>(); | ^ ^ +error: complex const arguments must be surrounded by braces + --> $DIR/const-expression-parameter.rs:29:11 + | +LL | bat::<(1, 2, 3)>(); + | ^^^^^^^^^ +help: surround this const argument in braces + | +LL | bat::<{ (1, 2, 3) }>(); + | ^ ^ + +error: complex const arguments must be surrounded by braces + --> $DIR/const-expression-parameter.rs:30:11 + | +LL | bat::<(1, 2)>(); + | ^^^^^^ +help: surround this const argument in braces + | +LL | bat::<{ (1, 2) }>(); + | ^ ^ + +error: complex const arguments must be surrounded by braces + --> $DIR/const-expression-parameter.rs:35:11 + | +LL | bar::(); + | ^^^^^^ +help: surround this const argument in braces + | +LL | bar::<{ !false }>(); + | ^ ^ + +error: expected one of `,` or `>`, found `<` + --> $DIR/const-expression-parameter.rs:39:14 + | +LL | bar::< 3 < 4 >(); + | ^ expected one of `,` or `>` here + warning: the feature `const_generics` is incomplete and may cause the compiler to crash --> $DIR/const-expression-parameter.rs:1:12 | @@ -57,11 +93,30 @@ LL | #![feature(const_generics)] = note: `#[warn(incomplete_features)]` on by default error[E0107]: wrong number of const arguments: expected 1, found 2 - --> $DIR/const-expression-parameter.rs:21:27 + --> $DIR/const-expression-parameter.rs:16:18 + | +LL | foo::<1 + 2, 3 + 4>(); + | ^^^^^ unexpected const argument + +error[E0308]: mismatched types + --> $DIR/const-expression-parameter.rs:27:17 + | +LL | baz::< -1 , "2" >(); + | ^^^ expected i32, found reference + | + = note: expected type `i32` + found type `&'static str` + +error[E0308]: mismatched types + --> $DIR/const-expression-parameter.rs:30:11 + | +LL | bat::<(1, 2)>(); + | ^^^^^^ expected a tuple with 3 elements, found one with 2 elements | -LL | i32_identity::<1 + 2, 3 + 4>(); - | ^^^^^ unexpected const argument + = note: expected type `(i32, i32, i32)` + found type `(i32, i32)` -error: aborting due to 6 previous errors +error: aborting due to 12 previous errors -For more information about this error, try `rustc --explain E0107`. +Some errors have detailed explanations: E0107, E0308. +For more information about an error, try `rustc --explain E0107`. diff --git a/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs b/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs index b074ea22e8c86..0c5d4d4820ca3 100644 --- a/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs +++ b/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.rs @@ -239,7 +239,6 @@ fn inside_const_generic_arguments() { // admit non-IDENT expressions in const generic arguments. if A::< - true && let 1 = 1 //~ ERROR complex const arguments must be surrounded by braces - >::O == 5 {} //~ ERROR chained comparison operators require parentheses - //~^ ERROR expected one of `,`, `.`, `>`, `?`, or an operator, found `{` + true && let 1 = 1 //~ ERROR expected one of `,` or `>`, found `&&` + >::O == 5 {} //~ ERROR chained comparison operators require parentheses } diff --git a/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr b/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr index 31709838c5e03..329a9a5487fc0 100644 --- a/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr +++ b/src/test/ui/rfc-2497-if-let-chains/disallowed-positions.stderr @@ -4,23 +4,11 @@ error: chained comparison operators require parentheses LL | >::O == 5 {} | ^^^^^^^^^ -error: complex const arguments must be surrounded by braces - --> $DIR/disallowed-positions.rs:242:9 +error: expected one of `,` or `>`, found `&&` + --> $DIR/disallowed-positions.rs:242:14 | -LL | / true && let 1 = 1 -LL | | >::O == 5 {} - | |_____________^ -help: surround this const argument in braces - | -LL | { true && let 1 = 1 -LL | >::O == 5 } {} - | - -error: expected one of `,`, `.`, `>`, `?`, or an operator, found `{` - --> $DIR/disallowed-positions.rs:243:15 - | -LL | >::O == 5 {} - | ^ expected one of `,`, `.`, `>`, `?`, or an operator here +LL | true && let 1 = 1 + | ^^ expected one of `,` or `>` here error: `let` expressions are not supported here --> $DIR/disallowed-positions.rs:32:9 @@ -1001,7 +989,7 @@ error[E0019]: constant contains unimplemented expression type LL | true && let 1 = 1 | ^ -error: aborting due to 111 previous errors +error: aborting due to 110 previous errors Some errors have detailed explanations: E0019, E0277, E0308, E0600, E0614. For more information about an error, try `rustc --explain E0019`.