From cd80c1bb5540fa2610a8f3d8a25560d1f0981400 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Tue, 15 Mar 2016 13:02:59 -0400 Subject: [PATCH 1/4] test errors for malformed inclusive ranges --- src/test/compile-fail/impossible_range.rs | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 src/test/compile-fail/impossible_range.rs diff --git a/src/test/compile-fail/impossible_range.rs b/src/test/compile-fail/impossible_range.rs new file mode 100644 index 0000000000000..b3b26bd73b452 --- /dev/null +++ b/src/test/compile-fail/impossible_range.rs @@ -0,0 +1,27 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Make sure that invalid ranges generate an error during HIR lowering, not an ICE + +#![feature(inclusive_range_syntax)] + +pub fn main() { + ..; + 0..; + ..1; + 0..1; + + ...; //~ERROR inclusive range with no end + 0...; //~ERROR unexpected token + ...1; + 0...1; +} + + From 9799cacba3042420cc7b49d555289241cf0456e1 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Tue, 15 Mar 2016 13:03:42 -0400 Subject: [PATCH 2/4] fatal error instead of ICE for impossible range during HIR lowering End-less ranges (`a...`) don't parse but bad syntax extensions could conceivably produce them. Unbounded ranges (`...`) do parse and are caught here. The other panics in HIR lowering are all for unexpanded macros, which cannot be constructed by bad syntax extensions. --- src/librustc/session/mod.rs | 4 ++++ src/librustc_front/lowering.rs | 9 ++++++++- src/libsyntax/ast.rs | 5 +++++ src/test/parse-fail/range_inclusive.rs | 5 +++-- 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs index b198eda181208..a02f82745279f 100644 --- a/src/librustc/session/mod.rs +++ b/src/librustc/session/mod.rs @@ -336,6 +336,10 @@ impl NodeIdAssigner for Session { fn peek_node_id(&self) -> NodeId { self.next_node_id.get().checked_add(1).unwrap() } + + fn diagnostic(&self) -> &errors::Handler { + self.diagnostic() + } } fn split_msg_into_multilines(msg: &str) -> Option { diff --git a/src/librustc_front/lowering.rs b/src/librustc_front/lowering.rs index 8aac6356f9d3b..a0466de999acf 100644 --- a/src/librustc_front/lowering.rs +++ b/src/librustc_front/lowering.rs @@ -68,6 +68,7 @@ use std::collections::HashMap; use std::iter; use syntax::ast::*; use syntax::attr::{ThinAttributes, ThinAttributesExt}; +use syntax::errors::Handler; use syntax::ext::mtwt; use syntax::ptr::P; use syntax::codemap::{respan, Spanned, Span}; @@ -140,6 +141,11 @@ impl<'a, 'hir> LoweringContext<'a> { result } } + + // panics if this LoweringContext's NodeIdAssigner is not a Session + fn diagnostic(&self) -> &Handler { + self.id_assigner.diagnostic() + } } // Utility fn for setting and unsetting the cached id. @@ -1289,7 +1295,8 @@ pub fn lower_expr(lctx: &LoweringContext, e: &Expr) -> P { make_struct(lctx, e, &["RangeInclusive", "NonEmpty"], &[("start", e1), ("end", e2)]), - _ => panic!("impossible range in AST"), + _ => panic!(lctx.diagnostic().span_fatal(e.span, + "inclusive range with no end")) } }); } diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs index ac1a07d1cb5f5..e096aa9902476 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -19,6 +19,7 @@ pub use self::PathParameters::*; use attr::ThinAttributes; use codemap::{Span, Spanned, DUMMY_SP, ExpnId}; use abi::Abi; +use errors; use ext::base; use ext::tt::macro_parser; use parse::token::InternedString; @@ -344,6 +345,10 @@ pub const DUMMY_NODE_ID: NodeId = !0; pub trait NodeIdAssigner { fn next_node_id(&self) -> NodeId; fn peek_node_id(&self) -> NodeId; + + fn diagnostic(&self) -> &errors::Handler { + panic!("this ID assigner cannot emit diagnostics") + } } /// The AST represents all type param bounds as types. diff --git a/src/test/parse-fail/range_inclusive.rs b/src/test/parse-fail/range_inclusive.rs index 5fd6f1834e02d..be2a63a07bbd6 100644 --- a/src/test/parse-fail/range_inclusive.rs +++ b/src/test/parse-fail/range_inclusive.rs @@ -13,6 +13,7 @@ #![feature(inclusive_range_syntax, inclusive_range)] pub fn main() { - for _ in 1... {} -} //~ ERROR expected one of + for _ in 1... {} //~ERROR inclusive range with no end + //~^HELP 28237 +} From 9f899a66591c1a4bc68b51fc6d1f207d2d49a087 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Wed, 16 Mar 2016 21:35:36 -0400 Subject: [PATCH 3/4] error during parsing for malformed inclusive range Now it is impossible for `...` or `a...` to reach HIR lowering without a rogue syntax extension in play. --- src/libsyntax/parse/parser.rs | 67 +++++++++++++---------- src/test/compile-fail/impossible_range.rs | 4 +- 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index a1adc99055f08..deda0c5905179 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2072,8 +2072,15 @@ impl<'a> Parser<'a> { start: Option>, end: Option>, limits: RangeLimits) - -> ast::ExprKind { - ExprKind::Range(start, end, limits) + -> PResult<'a, ast::ExprKind> { + if end.is_none() && limits == RangeLimits::Closed { + Err(self.span_fatal_help(self.span, + "inclusive range with no end", + "currently, inclusive ranges must be bounded at the end \ + (`...b` or `a...b`) -- see tracking issue #28237")) + } else { + Ok(ExprKind::Range(start, end, limits)) + } } pub fn mk_field(&mut self, expr: P, ident: ast::SpannedIdent) -> ast::ExprKind { @@ -2961,12 +2968,12 @@ impl<'a> Parser<'a> { lhs = self.mk_expr(lhs_span.lo, rhs.span.hi, ExprKind::Type(lhs, rhs), None); continue - } else if op == AssocOp::DotDot { - // If we didn’t have to handle `x..`, it would be pretty easy to generalise - // it to the Fixity::None code. + } else if op == AssocOp::DotDot || op == AssocOp::DotDotDot { + // If we didn’t have to handle `x..`/`x...`, it would be pretty easy to + // generalise it to the Fixity::None code. // - // We have 2 alternatives here: `x..y` and `x..` The other two variants are - // handled with `parse_prefix_range_expr` call above. + // We have 2 alternatives here: `x..y`/`x...y` and `x..`/`x...` The other + // two variants are handled with `parse_prefix_range_expr` call above. let rhs = if self.is_at_start_of_range_notation_rhs() { let rhs = self.parse_assoc_expr_with(op.precedence() + 1, LhsExpr::NotYetParsed); @@ -2985,7 +2992,13 @@ impl<'a> Parser<'a> { } else { cur_op_span }); - let r = self.mk_range(Some(lhs), rhs, RangeLimits::HalfOpen); + let limits = if op == AssocOp::DotDot { + RangeLimits::HalfOpen + } else { + RangeLimits::Closed + }; + + let r = try!(self.mk_range(Some(lhs), rhs, limits)); lhs = self.mk_expr(lhs_span.lo, rhs_span.hi, r, None); break } @@ -3003,8 +3016,7 @@ impl<'a> Parser<'a> { this.parse_assoc_expr_with(op.precedence() + 1, LhsExpr::NotYetParsed) }), - // the only operator handled here is `...` (the other non-associative operators are - // special-cased above) + // no operators are currently handled here Fixity::None => self.with_res( restrictions - Restrictions::RESTRICTION_STMT_EXPR, |this| { @@ -3045,13 +3057,8 @@ impl<'a> Parser<'a> { let aopexpr = self.mk_assign_op(codemap::respan(cur_op_span, aop), lhs, rhs); self.mk_expr(lhs_span.lo, rhs_span.hi, aopexpr, None) } - AssocOp::DotDotDot => { - let (lhs_span, rhs_span) = (lhs.span, rhs.span); - let r = self.mk_range(Some(lhs), Some(rhs), RangeLimits::Closed); - self.mk_expr(lhs_span.lo, rhs_span.hi, r, None) - } - AssocOp::As | AssocOp::Colon | AssocOp::DotDot => { - self.bug("As, Colon or DotDot branch reached") + AssocOp::As | AssocOp::Colon | AssocOp::DotDot | AssocOp::DotDotDot => { + self.bug("As, Colon, DotDot or DotDotDot branch reached") } }; @@ -3095,21 +3102,23 @@ impl<'a> Parser<'a> { // RHS must be parsed with more associativity than the dots. let next_prec = AssocOp::from_token(&tok).unwrap().precedence() + 1; Some(self.parse_assoc_expr_with(next_prec, - LhsExpr::NotYetParsed) - .map(|x|{ - hi = x.span.hi; - x - })?) + LhsExpr::NotYetParsed) + .map(|x|{ + hi = x.span.hi; + x + })?) } else { None }; - let r = self.mk_range(None, - opt_end, - if tok == token::DotDot { - RangeLimits::HalfOpen - } else { - RangeLimits::Closed - }); + let limits = if tok == token::DotDot { + RangeLimits::HalfOpen + } else { + RangeLimits::Closed + }; + + let r = try!(self.mk_range(None, + opt_end, + limits)); Ok(self.mk_expr(lo, hi, r, attrs)) } diff --git a/src/test/compile-fail/impossible_range.rs b/src/test/compile-fail/impossible_range.rs index b3b26bd73b452..169ba44224079 100644 --- a/src/test/compile-fail/impossible_range.rs +++ b/src/test/compile-fail/impossible_range.rs @@ -19,7 +19,9 @@ pub fn main() { 0..1; ...; //~ERROR inclusive range with no end - 0...; //~ERROR unexpected token + //~^HELP 28237 + 0...; //~ERROR inclusive range with no end + //~^HELP 28237 ...1; 0...1; } From 861644f2af5421f5aa55d4e7fddfc8dba54bcb70 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Fri, 18 Mar 2016 19:04:43 -0400 Subject: [PATCH 4/4] address nits --- src/librustc_front/lowering.rs | 2 +- src/libsyntax/parse/parser.rs | 7 ++++--- src/test/compile-fail/impossible_range.rs | 4 ++-- src/test/parse-fail/range_inclusive.rs | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/librustc_front/lowering.rs b/src/librustc_front/lowering.rs index a0466de999acf..be1841794512b 100644 --- a/src/librustc_front/lowering.rs +++ b/src/librustc_front/lowering.rs @@ -142,7 +142,7 @@ impl<'a, 'hir> LoweringContext<'a> { } } - // panics if this LoweringContext's NodeIdAssigner is not a Session + // Panics if this LoweringContext's NodeIdAssigner is not able to emit diagnostics. fn diagnostic(&self) -> &Handler { self.id_assigner.diagnostic() } diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index deda0c5905179..4782d3fb3b97c 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -2076,8 +2076,8 @@ impl<'a> Parser<'a> { if end.is_none() && limits == RangeLimits::Closed { Err(self.span_fatal_help(self.span, "inclusive range with no end", - "currently, inclusive ranges must be bounded at the end \ - (`...b` or `a...b`) -- see tracking issue #28237")) + "inclusive ranges must be bounded at the end \ + (`...b` or `a...b`)")) } else { Ok(ExprKind::Range(start, end, limits)) } @@ -3016,7 +3016,8 @@ impl<'a> Parser<'a> { this.parse_assoc_expr_with(op.precedence() + 1, LhsExpr::NotYetParsed) }), - // no operators are currently handled here + // We currently have no non-associative operators that are not handled above by + // the special cases. The code is here only for future convenience. Fixity::None => self.with_res( restrictions - Restrictions::RESTRICTION_STMT_EXPR, |this| { diff --git a/src/test/compile-fail/impossible_range.rs b/src/test/compile-fail/impossible_range.rs index 169ba44224079..94e048fed655e 100644 --- a/src/test/compile-fail/impossible_range.rs +++ b/src/test/compile-fail/impossible_range.rs @@ -19,9 +19,9 @@ pub fn main() { 0..1; ...; //~ERROR inclusive range with no end - //~^HELP 28237 + //~^HELP bounded at the end 0...; //~ERROR inclusive range with no end - //~^HELP 28237 + //~^HELP bounded at the end ...1; 0...1; } diff --git a/src/test/parse-fail/range_inclusive.rs b/src/test/parse-fail/range_inclusive.rs index be2a63a07bbd6..ce97372c66845 100644 --- a/src/test/parse-fail/range_inclusive.rs +++ b/src/test/parse-fail/range_inclusive.rs @@ -14,6 +14,6 @@ pub fn main() { for _ in 1... {} //~ERROR inclusive range with no end - //~^HELP 28237 + //~^HELP bounded at the end }