javascript-decorators icon indicating copy to clipboard operation
javascript-decorators copied to clipboard

Shift-reduce conflicts in grammar

Open anba opened this issue 10 years ago • 5 comments
trafficstars

The current grammar has multiple shift-reduce conflicts.

  1. Decorator needs to use LeftHandSideExpression instead of AssignmentExpression, otherwise there is a conflict between MultiplicativeExpression and GeneratorMethod. Example: { @ F * G () {} }.

  2. CoverMemberExpressionSquareBracketsAndComputedPropertyName isn't sufficient to handle bracket member expression and computed property names + formal parameters. Example: { @ D ["m"] () {} }. Parse as @ D + ["m"]() {} or as @ D["m"]() + {} ?

I'd propose to simply disallow bracket member expressions in decorator expressions:

anba avatar Apr 03 '15 15:04 anba

+1000 for the first part. It would also fix the second part, as long as it's parsed as Descriptor MethodDefinition*

The current behavior is surprising, and that would definitely fix the ambiguity with annotating generators.

:-1: for the proposed fix for the second part. The least surprising behavior would be it parsing as @ D + ["m"] () {}. There's no ambiguity if it's parsed as @ LeftHandSideExpression MethodDefinition, and it's parsed this way.

dead-claudia avatar Jun 19 '15 10:06 dead-claudia

:+1: on 1

:-1: on 2. I suspect that bracket notation is going to be used way more than before, given it’s currently the only way to use symbols.

yuchi avatar Jun 20 '15 09:06 yuchi

There's no ambiguity if it's parsed as @ LeftHandSideExpression MethodDefinition, and it's parsed this way.

Here's a simplified version of the current grammar, when you process it with bison you can easily identify the shift/reduce conflicts with the coverMemberExpressionSquareBracketsAndComputedPropertyName approach.

%start classElement

%error-verbose

%token ID

%%

classElement : decorator methodDefinition
             | methodDefinition
             ;

decorator : '@' leftHandSideExpression ;

methodDefinition : propertyName '(' formalParameters ')' '{' '}' ;

propertyName : coverMemberExpressionSquareBracketsAndComputedPropertyName ;

formalParameters : /* empty */
                 | formalsList
                 ;

formalsList : formalParameter
            | formalsList ',' formalParameter
            ;

formalParameter : bindingIdentifier ;

expression : assignmentExpression ;

assignmentExpression : leftHandSideExpression ;

leftHandSideExpression : callExpression
                       | memberExpression
                       ;

callExpression : memberExpression arguments ;

arguments : '(' ')'
          | '(' argumentsList ')'
          ;

argumentsList : assignmentExpression
              | argumentsList ',' assignmentExpression
              ;

memberExpression : primaryExpression
                 | memberExpression coverMemberExpressionSquareBracketsAndComputedPropertyName
                 ;

coverMemberExpressionSquareBracketsAndComputedPropertyName : '[' expression ']' ;

primaryExpression : identifierReference ;

identifierReference : ID ;

bindingIdentifier : ID ;

%%

anba avatar Jun 20 '15 11:06 anba

FYI: Just fixed (1) in https://github.com/wycats/javascript-decorators/pull/34

jeffmo avatar Sep 02 '15 03:09 jeffmo

The grammar adds CoverMemberExpressionSquareBracketsAndComputedPropertyName to MemberExpression, which is the avenue NewExpression can take that includes terminal bracket notation. However, LHS is inclusive of CallExpression as well, and CallExpression may also have terminal bracket notation (item 4):

CallExpression[Yield] :
    MemberExpression[?Yield] Arguments[?Yield]
    SuperCall[?Yield]
    CallExpression[?Yield] Arguments[?Yield]
    CallExpression[?Yield] [ Expression[In, ?Yield] ]
    CallExpression[?Yield] . IdentifierName
    CallExpression[?Yield]  TemplateLiteral[?Yield]

Wouldn't the grammar need to use CoverMemberExpressionSquareBracketsAndComputedPropertyName here as well?

bathos avatar Nov 13 '15 15:11 bathos