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

Wrapping expr in curly braces changes the operator precedence #28777

Closed
rprichard opened this Issue Sep 30, 2015 · 8 comments

Comments

Projects
None yet
8 participants
@rprichard
Copy link
Contributor

rprichard commented Sep 30, 2015

This program prints different values depending upon whether the v1 or v2 initializer is surrounded by curly braces:

fn main() {
    let v1 = { 1 + {2} * {3} }; // 9, indicating a parse of (1 + 2) * 3
    let v2 =   1 + {2} * {3}  ; // 7, indicating a parse of 1 + (2 * 3)
    println!("{}", v1);
    println!("{}", v2);
}

I think it should print 7 for both lines. parser-lalr -v accepts this program and consistently parses 1 + (2 * 3) for both lines.

|   |   |   |   |   |   |   (ExprBlock
|   |   |   |   |   |   |   |   (ExprBinary
|   |   |   |   |   |   |   |   |   BiAdd
|   |   |   |   |   |   |   |   |   (ExprLit
|   |   |   |   |   |   |   |   |   |   (LitInteger
|   |   |   |   |   |   |   |   |   |   |   1
|   |   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   (ExprBinary
|   |   |   |   |   |   |   |   |   |   BiMul
|   |   |   |   |   |   |   |   |   |   (ExprBlock
|   |   |   |   |   |   |   |   |   |   |   (ExprLit
|   |   |   |   |   |   |   |   |   |   |   |   (LitInteger
|   |   |   |   |   |   |   |   |   |   |   |   |   2
|   |   |   |   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   |   (ExprBlock
|   |   |   |   |   |   |   |   |   |   |   (ExprLit
|   |   |   |   |   |   |   |   |   |   |   |   (LitInteger
|   |   |   |   |   |   |   |   |   |   |   |   |   3
|   |   |   |   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   |   )
|   |   |   |   |   |   |   )

@rprichard rprichard changed the title Operator precedence problem with `nonblock_expr` Wrapping expr in curly braces changes the operator precedence Oct 1, 2015

@steveklabnik steveklabnik added the A-lang label Oct 4, 2015

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Oct 4, 2015

/cc @rust-lang/lang , I'm assuming this is not intended behavior.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Oct 6, 2015

Um, I agree.

triage: P-medium

@aaronkeen

This comment has been minimized.

Copy link
Contributor

aaronkeen commented Dec 3, 2015

In looking at this issue, the incorrect parse (the initializer for v1 in the example, though this occurs in an assignment as well) stems from the path to parse_assoc_expr_with from parse_block_expr (passing through, of note, parse_stmt_). In parse_stmt_, parse_expr_res is invoked with RESTRICTION_STMT_EXPR. This ultimately results in the expr_is_complete check (in parse_assoc_expr_with) succeeding for '{2}' which prevents matching the '*' token and, thus, building the expected right subtree.

I am new to this codebase (and, for the most part, this language), so I am sill looking into the 'expr_is_complete' aspect of this portion of the parser.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Dec 3, 2015

uh wow; I was going to ask if this is related to #15300 , but this seems much worse than that to my eye...

@aaronkeen

This comment has been minimized.

Copy link
Contributor

aaronkeen commented Dec 4, 2015

As the comment in parser.rs alludes to in regards to semi-statements, this issue is related to #29071.

The outermost braces trigger a flag to check for expressions that do not require a semicolon to in order to be considered statements (e.g., if). If such an expression is found, then the parsing of associative expressions is terminated before consuming the next operator. This occurs in the original examples after the '{2}' because the block does not require a semicolon to be considered a statement.

For instance, the following illustrates the same parsing issue as the original example.

fn main() {
    let v1 = { if 1 > 2 {2} else {3} + 5 };   // does not parse, unexpected token at +
    let v2 =   if 1 > 2 {2} else {3} + 5 ;    // parses and evaluates as expected
}

Removing the addition operator in the first expression results in a parse but with a typing error (to an extent further illustrating that the semi-statement is being considered a statement here; the same is true of the original binding for v1, but falling out of the recursion grabs the '* {3}').

fn main() {
    let v1 = { if 1 > 2 {2} else {3} 5 };   // parses, but type error
}
main.rs:2:26: 2:27 error: mismatched types:
 expected `()`,
    found `_`
(expected (),
    found integral variable) [E0308]
main.rs:2     let v1 = { if 1 > 2 {2} else {3}  5 };
                                   ^
main.rs:2:26: 2:27 help: run `rustc --explain E0308` to see a detailed explanation
main.rs:2:35: 2:36 error: mismatched types:
 expected `()`,
    found `_`
(expected (),
    found integral variable) [E0308]
main.rs:2     let v1 = { if 1 > 2 {2} else {3}  5 };
                                            ^
main.rs:2:35: 2:36 help: run `rustc --explain E0308` to see a detailed explanation
error: aborting due to 2 previous errors

I see that this all really goes back to #28379 (from which this issue was split) wherein @nikomatsakis states that a statement expression cannot start a binary expression when in statement position. The restriction flag makes it such that in '{ 1 + {2} * 3 }' it looks like '{2}' is in statement position for the subexpression '{2} * 3' (but it would not be considered as such if the original expression were '{ 1 + {2} + 3 }' because it appears as the right-operand of the first + instead of the left-operand of the *).

If the intent is that the start of a binary expression cannot be a statement-expression, but subsequent subexpressions can be, then it may be sufficient to remove the restriction when parsing the rhs (i.e., after an operator is accepted). Doing so in the original example given corrects the parsing (precedence) issue. The tests are still running to check for introduced failures.

@aaronkeen

This comment has been minimized.

Copy link
Contributor

aaronkeen commented Dec 4, 2015

The proposed change results in parse-fail/assoc-oddities-3.rs successfully parsing. It is unclear to me why it shouldn't. Back to looking through the intended grammar.

fn that_odd_parse() {
    // see assoc-oddities-1 for explanation
    x + if c { a } else { b }[n]; //~ ERROR expected one of
}

aaronkeen added a commit to aaronkeen/rust that referenced this issue Dec 14, 2015

Corrects issue rust-lang#28777 by removing, once a binary operator is…
… found, the

RESTRICTION_STMT_EXPR restriction to allow subsequent expressions to
contain braces.

rust-lang#28777
@aaronkeen

This comment has been minimized.

Copy link
Contributor

aaronkeen commented Dec 14, 2015

parse-fail/assoc-oddities-3.rs is successfully parsed by grammar/parser-lalr as well, so the proposed modification is consistent in this regard.

Manishearth added a commit to Manishearth/rust that referenced this issue Dec 17, 2015

Rollup merge of rust-lang#30375 - aaronkeen:issue_28777, r=eddyb
RESTRICTION_STMT_EXPR restriction to allow subsequent expressions to
contain braces.

rust-lang#28777

bors added a commit that referenced this issue Dec 30, 2015

Auto merge of #30375 - aaronkeen:issue_28777, r=eddyb
RESTRICTION_STMT_EXPR restriction to allow subsequent expressions to
contain braces.

#28777

bors added a commit that referenced this issue Dec 30, 2015

Auto merge of #30375 - aaronkeen:issue_28777, r=eddyb
RESTRICTION_STMT_EXPR restriction to allow subsequent expressions to
contain braces.

#28777
@jseyfried

This comment has been minimized.

Copy link
Contributor

jseyfried commented Feb 10, 2016

@eddyb This can be closed -- #30375 fixed it and added a test.

@eddyb eddyb closed this Feb 10, 2016

@steveklabnik steveklabnik added the T-lang label Mar 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.