Skip to content

Fix precedence of '..' range notation and some grammar inconsistencies. #20958

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

Closed
Closed
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
6 changes: 3 additions & 3 deletions src/doc/reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -3134,15 +3134,15 @@ The precedence of Rust binary operators is ordered as follows, going from
strong to weak:

```{.text .precedence}
* / %
as
* / %
+ -
<< >>
&
^
|
< > <= >=
== !=
..
== != < > <= >=
&&
||
=
Expand Down
12 changes: 12 additions & 0 deletions src/libsyntax/ast_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,24 +321,36 @@ pub fn struct_field_visibility(field: ast::StructField) -> Visibility {
/// Maps a binary operator to its precedence
pub fn operator_prec(op: ast::BinOp) -> uint {
match op {
// prefix expressions sit here with 13
// 'as' sits here with 12
BiMul | BiDiv | BiRem => 11u,
BiAdd | BiSub => 10u,
BiShl | BiShr => 9u,
BiBitAnd => 8u,
BiBitXor => 7u,
BiBitOr => 6u,
// '..' sits here with 5
BiLt | BiLe | BiGe | BiGt | BiEq | BiNe => 3u,
BiAnd => 2u,
BiOr => 1u
}
}

/// Precedence of the prefix operators '!', '-', '&', '*', '~' etc.
/// Does not include the prefix form of '..'
#[allow(non_upper_case_globals)]
pub static prefix_prec: uint = 13u;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, and somewhat pre-existing, but (a) this should be a const, not a static, and (b) it'd be nice if it followed the CAPITALIZATION convention. I found it very confusing when reading the parser -- it was hard to distinguish "fixed" precedence levels like prefix_prec from "varying" ones like min_prec. Can you rename it? Thanks.

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'll rename these constants.


/// Precedence of the `as` operator, which is a binary operator
/// not appearing in the prior table.
#[allow(non_upper_case_globals)]
pub static as_prec: uint = 12u;

// Precedence of '..', which exists as a binary operator,
// and as unary operators (prefix and postfix form)
#[allow(non_upper_case_globals)]
pub static range_prec: uint = 5u;

pub fn empty_generics() -> Generics {
Generics {
lifetimes: Vec::new(),
Expand Down
96 changes: 58 additions & 38 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ use ast::{ViewItem, ViewItem_, ViewItemExternCrate, ViewItemUse};
use ast::{ViewPath, ViewPathGlob, ViewPathList, ViewPathSimple};
use ast::{Visibility, WhereClause};
use ast;
use ast_util::{self, as_prec, ident_to_path, operator_prec};
use ast_util::{self, prefix_prec, as_prec, range_prec, ident_to_path, operator_prec};
use codemap::{self, Span, BytePos, Spanned, spanned, mk_sp};
use diagnostic;
use ext::tt::macro_parser;
Expand Down Expand Up @@ -93,7 +93,6 @@ bitflags! {
const RESTRICTION_STMT_EXPR = 0b0001,
const RESTRICTION_NO_BAR_OP = 0b0010,
const RESTRICTION_NO_STRUCT_LITERAL = 0b0100,
const RESTRICTION_NO_DOTS = 0b1000,
}
}

Expand Down Expand Up @@ -2769,34 +2768,35 @@ impl<'a> Parser<'a> {
}

/// Parse a prefix-operator expr
pub fn parse_prefix_expr(&mut self) -> P<Expr> {
/// only operators with a precedence >= min_prec will be accepted
pub fn parse_prefix_expr(&mut self, min_prec: uint) -> P<Expr> {
let lo = self.span.lo;
let hi;

let ex;
match self.token {
token::Not => {
self.bump();
let e = self.parse_prefix_expr();
let e = self.parse_prefix_expr(prefix_prec);
hi = e.span.hi;
ex = self.mk_unary(UnNot, e);
}
token::BinOp(token::Minus) => {
self.bump();
let e = self.parse_prefix_expr();
let e = self.parse_prefix_expr(prefix_prec);
hi = e.span.hi;
ex = self.mk_unary(UnNeg, e);
}
token::BinOp(token::Star) => {
self.bump();
let e = self.parse_prefix_expr();
let e = self.parse_prefix_expr(prefix_prec);
hi = e.span.hi;
ex = self.mk_unary(UnDeref, e);
}
token::BinOp(token::And) | token::AndAnd => {
self.expect_and();
let m = self.parse_mutability();
let e = self.parse_prefix_expr();
let e = self.parse_prefix_expr(prefix_prec);
hi = e.span.hi;
ex = ExprAddrOf(m, e);
}
Expand All @@ -2810,14 +2810,14 @@ impl<'a> Parser<'a> {
_ => self.obsolete(last_span, ObsoleteSyntax::OwnedExpr)
}

let e = self.parse_prefix_expr();
let e = self.parse_prefix_expr(prefix_prec);
hi = e.span.hi;
ex = self.mk_unary(UnUniq, e);
}
token::DotDot if !self.restrictions.contains(RESTRICTION_NO_DOTS) => {
token::DotDot if min_prec <= range_prec => {
// A range, closed above: `..expr`.
self.bump();
let e = self.parse_expr();
let e = self.parse_binops(range_prec + 1);
hi = e.span.hi;
ex = self.mk_range(None, Some(e));
}
Expand Down Expand Up @@ -2848,15 +2848,15 @@ impl<'a> Parser<'a> {
"perhaps you meant `box() (foo)` instead?");
self.abort_if_errors();
}
let subexpression = self.parse_prefix_expr();
let subexpression = self.parse_prefix_expr(prefix_prec);
hi = subexpression.span.hi;
ex = ExprBox(Some(place), subexpression);
return self.mk_expr(lo, hi, ex);
}
}

// Otherwise, we use the unique pointer default.
let subexpression = self.parse_prefix_expr();
let subexpression = self.parse_prefix_expr(prefix_prec);
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means that box ..5 does not parse, which I consider a bug. I'm not really clear on why we change from min_prec to prefix_prec here.

hi = subexpression.span.hi;
// FIXME (pnkfelix): After working out kinks with box
// desugaring, should be `ExprBox(None, subexpression)`
Expand All @@ -2868,10 +2868,10 @@ impl<'a> Parser<'a> {
return self.mk_expr(lo, hi, ex);
}

/// Parse an expression of binops
pub fn parse_binops(&mut self) -> P<Expr> {
let prefix_expr = self.parse_prefix_expr();
self.parse_more_binops(prefix_expr, 0)
/// Parse an expression of binops of at least min_prec precedence
pub fn parse_binops(&mut self, min_prec: uint) -> P<Expr> {
let prefix_expr = self.parse_prefix_expr(min_prec);
self.parse_more_binops(prefix_expr, min_prec)
}

/// Parse an expression of binops of at least min_prec precedence
Expand All @@ -2894,10 +2894,9 @@ impl<'a> Parser<'a> {
self.check_no_chained_comparison(&*lhs, cur_op)
}
let cur_prec = operator_prec(cur_op);
if cur_prec > min_prec {
if cur_prec >= min_prec {
self.bump();
let expr = self.parse_prefix_expr();
let rhs = self.parse_more_binops(expr, cur_prec);
let rhs = self.parse_binops(cur_prec + 1);
let lhs_span = lhs.span;
let rhs_span = rhs.span;
let binary = self.mk_binary(cur_op, lhs, rhs);
Expand All @@ -2908,19 +2907,57 @@ impl<'a> Parser<'a> {
}
}
None => {
if as_prec > min_prec && self.eat_keyword(keywords::As) {
if as_prec >= min_prec && self.eat_keyword(keywords::As) {
let rhs = self.parse_ty();
let _as = self.mk_expr(lhs.span.lo,
rhs.span.hi,
ExprCast(lhs, rhs));
self.parse_more_binops(_as, min_prec)
} else if range_prec >= min_prec
&& match lhs.node { ExprRange(_, _) => false, _ => true }
&& self.eat(&token::DotDot) {
// '..' range notation, infix or postfix form
// Note that we intentionally reject other range expressions on the lhs.
// This makes '..1..2' invalid.
// This is necessary for consistency between the prefix and postfix forms.
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means that things like a..b..c..d don't parse (at least not without parens) -- which is not necessarily bad, since that is nonsense, and what the user presumably wants is (a..b)..(c..d), but I'd still sort of expect it to just produce a left- or right-leaning tree. I'm wondering if the goal of this rule was just to prevent such constructs from parsing, or if there was another concern? (If that is the goal, I'm not necessarily opposed, I guess it's consistent with a < b < c not parsing.)

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'm not sure what exactly the original goal was. Note that a..b..c..d was already a syntax error without this pull request. This special case just preserves the non-associativity of .. while moving the implementation to parse_more_binops. It also fixes some inconsistencies: a..b.. was already invalid without this pull request, but ..a..b was valid.
I think it's a good idea to keep .. non-associative -- it's consistent with a < b < c, and means we have one less thing to worry about when thinking about the ambiguities caused by the optional expressions around ...

let opt_rhs = if self.is_at_start_of_range_notation_rhs() {
Some(self.parse_binops(range_prec + 1))
} else {
None
};
let lo = lhs.span.lo;
let hi = self.span.hi;
let range = self.mk_range(Some(lhs), opt_rhs);
let bin = self.mk_expr(lo, hi, range);
self.parse_more_binops(bin, min_prec)
} else {
lhs
}
}
}
}

fn is_at_start_of_range_notation_rhs(&self) -> bool {
if self.token.can_begin_expr() {
// parse `for i in 1.. { }` as infinite loop, not as `for i in (1..{})`.
if self.token == token::OpenDelim(token::Brace) {
return !self.restrictions.contains(RESTRICTION_NO_STRUCT_LITERAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems good.

}

// `1..*i` is ambiguous between `1..(*i)` and `(1..)*(i)`.
// We pick the `1..(*i)` interpretation.

// `r==1..&&true` is ambiguous between `r==(1..(&&true))` and `(r==(1..))&&true`.
// We pick the latter interpretation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this is right. The fact that we have to have a special case for it kind of makes me feel that my intutions here are correct, and that having a special case around ..&& feels kind of wrong. For one thing, it seems a bit odd that 1..&x is fine but 1..&&x is not?

On the other hand, I can't see a good reason to ever do ..&&x whereas your example kind of makes sense.

match self.token.to_binop() {
Some(op) => operator_prec(op) > range_prec,
None => true
}
} else {
false
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this ensures that [a]..[b] is still treated as ([a])..([b]) rather than ([a]..)[b], which is good. It's unlikely to break, but it might be worth putting a test in for that case anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely think we want tests for a few such cases.

/// Produce an error if comparison operators are chained (RFC #558).
/// We only need to check lhs, not rhs, because all comparison ops
/// have same precedence and are left-associative
Expand All @@ -2944,7 +2981,7 @@ impl<'a> Parser<'a> {
/// actually, this seems to be the main entry point for
/// parsing an arbitrary expression.
pub fn parse_assign_expr(&mut self) -> P<Expr> {
let lhs = self.parse_binops();
let lhs = self.parse_binops(0);
self.parse_assign_expr_with(lhs)
}

Expand Down Expand Up @@ -2976,23 +3013,6 @@ impl<'a> Parser<'a> {
let assign_op = self.mk_assign_op(aop, lhs, rhs);
self.mk_expr(span.lo, rhs_span.hi, assign_op)
}
// A range expression, either `expr..expr` or `expr..`.
token::DotDot if !self.restrictions.contains(RESTRICTION_NO_DOTS) => {
self.bump();

let opt_end = if self.token.can_begin_expr() {
let end = self.parse_expr_res(RESTRICTION_NO_DOTS);
Some(end)
} else {
None
};

let lo = lhs.span.lo;
let hi = self.span.hi;
let range = self.mk_range(Some(lhs), opt_end);
return self.mk_expr(lo, hi, range);
}

_ => {
lhs
}
Expand Down
16 changes: 16 additions & 0 deletions src/test/compile-fail/range-3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright 2014 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Test range syntax - syntax errors.

pub fn main() {
let r = 1..2..3;
//~^ ERROR expected one of `.`, `;`, or an operator, found `..`
}
8 changes: 8 additions & 0 deletions src/test/run-pass/ranges-precedence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,13 @@ fn main() {
assert!(x == &a[3..]);

for _i in 2+4..10-3 {}

let i = 42;
for _ in 1..i {}
for _ in 1.. { break; }

if 1..2 == 1..2 {}
if 1.. == 1.. {}
if ..1 == ..1 {}
}