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

new Class().method() has incorrect precedence #273

Closed
jcdickinson opened this issue Mar 9, 2020 · 9 comments
Closed

new Class().method() has incorrect precedence #273

jcdickinson opened this issue Mar 9, 2020 · 9 comments
Labels
bug Something isn't working parser Issues surrounding the parser
Milestone

Comments

@jcdickinson
Copy link
Contributor

The precedence in new Class().method() isn't correct. This expression is parsed as new (Class().method()), which is incorrect, the correct parse tree would be equivalent to (new Class()).method().

A real-world example is new Date().getTime().

@jasonwilliams
Copy link
Member

jasonwilliams commented Mar 9, 2020

Tokens for new Date().getTime()

[boa\src\lib.rs:26] &tokens = [
    Token {
        data: Keyword(
            New,
        ),
        pos: Position {        
            column_number: 1,  
            line_number: 1,    
        },
    },
    Token {
        data: Identifier(      
            "Date",
        ),
        pos: Position {        
            column_number: 5,  
            line_number: 1,    
        },
    },
    Token {
        data: Punctuator(      
            OpenParen,
        ),
        pos: Position {        
            column_number: 9,  
            line_number: 1,    
        },
    },
    Token {
        data: Punctuator(      
            CloseParen,        
        ),
        pos: Position {       
            column_number: 10,
            line_number: 1,   
        },
    },
    Token {
        data: Punctuator(     
            Dot,
        ),
        pos: Position {       
            column_number: 11,
            line_number: 1,   
        },
    },
    Token {
        data: Identifier(     
            "getTime",        
        ),
        pos: Position {       
            column_number: 12,
            line_number: 1,   
        },
    },
    Token {
        data: Punctuator(     
            OpenParen,        
        ),
        pos: Position {       
            column_number: 19,
            line_number: 1,
        },
    },
    Token {
        data: Punctuator(
            CloseParen,
        ),
        pos: Position {
            column_number: 20,
            line_number: 1,
        },
    },
    Token {
        data: Punctuator(
            Semicolon,
        ),
        pos: Position {
            column_number: 21,
            line_number: 1,
        },
    },
]

Tokens looks fine

Parser output

[boa\src\lib.rs:27] &expr = Expr {
    def: Block(
        [
            Expr {
                def: Construct(
                    Expr {
                        def: GetConstField(        
                            Expr {
                                def: Call(
                                    Expr {
                                        def: Local(
                                            "Date",
                                        ),
                                    },
                                    [],
                                ),
                            },
                            "getTime",
                        ),
                    },    
                    [],   
                ),        
            },
        ],
    ),
}

Yeah looks like there'll need to be some work here:
https://github.com/jasonwilliams/boa/blob/master/boa/src/syntax/parser/mod.rs#L232-L240

@jasonwilliams
Copy link
Member

Upon deeper inspection the issue is around here: https://github.com/jasonwilliams/boa/blob/master/boa/src/syntax/parser/mod.rs#L688-L721

Normally it just carries on, which is why it continues parsing until the method.
There needs to be some signal to stop after the first call.

@jasonwilliams
Copy link
Member

if we stopped after here https://github.com/jasonwilliams/boa/blob/master/boa/src/syntax/parser/mod.rs#L719 it would break other things such as function chaining

@jasonwilliams
Copy link
Member

Might be worth making the parser more spec compliant https://tc39.es/ecma262/#sec-left-hand-side-expressions

@jasonwilliams
Copy link
Member

jasonwilliams commented Mar 10, 2020

@jasonwilliams jasonwilliams added this to the v0.7.0 - New Parser milestone Mar 18, 2020
@jasonwilliams jasonwilliams added the parser Issues surrounding the parser label Mar 31, 2020
jasonwilliams added a commit that referenced this issue Mar 31, 2020
New parser!
Plus loads of tidy up in various places.

Co-authored-by: Jason Williams <jwilliams720@bloomberg.net>
Co-authored-by: HalidOdat <halidodat@gmail.com>
Co-authored-by: Iban Eguia <iban.eguia@cern.ch>
Co-authored-by: Iban Eguia <razican@protonmail.ch>
@jasonwilliams
Copy link
Member

jasonwilliams commented Apr 13, 2020

new output since 0.7

StatementList(
    [
        Call(
            GetConstField(
                New(
                    Call(
                        Local(     
                            "Date",
                        ),
                        [],        
                    ),
                ),
                "getTime",
            ),
            [],
        ),
    ],
)

The precedence looks fixed here
@HalidOdat @Razican ?

@Razican
Copy link
Member

Razican commented Apr 13, 2020

The precedence looks fixed here
@HalidOdat @Razican ?

It looks good, yes! Might be worth adding a test for this?

@HalidOdat
Copy link
Member

It looks good, yes! Might be worth adding a test for this?

Are you adding the test, or should I?

@Razican Razican added the bug Something isn't working label Apr 13, 2020
@jasonwilliams
Copy link
Member

im doing it now

jasonwilliams added a commit that referenced this issue Apr 13, 2020
Razican pushed a commit that referenced this issue Apr 13, 2020
@Razican Razican added this to the v0.8.0 milestone Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser Issues surrounding the parser
Projects
None yet
Development

No branches or pull requests

4 participants