-
Notifications
You must be signed in to change notification settings - Fork 1
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
Parser add var decl #55
Conversation
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
=========================================
+ Coverage 63.61% 65.1% +1.48%
=========================================
Files 19 19
Lines 1528 1576 +48
=========================================
+ Hits 972 1026 +54
+ Misses 507 502 -5
+ Partials 49 48 -1
Continue to review full report at Codecov.
|
parser/parser.go
Outdated
) | ||
|
||
func init() { | ||
unaryParsers = map[token.Type]parserfn{ | ||
token.Minus: parseUnary, | ||
token.Plus: parseUnary, | ||
} | ||
literalParsers = map[token.Type]parserfn{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiago4orion I moved everything to init for the sake of consistency and also because I wrote a bug before because a variable that could be initialized outside init used the unaryParsers variable but the variable was an empty map at the time since the initialization was not on the order that the variables appeared on the code. If everything was able to be initialized outside init it would be good, but if one needs to be inside init it seems better to all the other ones to be there as well.
Value Node | ||
} | ||
|
||
VarDecls []VarDecl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
ast/node.go
Outdated
@@ -76,6 +83,8 @@ const ( | |||
NodeUnaryExpr | |||
NodeMemberExpr | |||
NodeCallExpr | |||
NodeVarDecl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(var a = 1) { console.log("hello"); }
VM99:1 Uncaught SyntaxError: Unexpected token var
simple var decl is not permitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ast/node.go
Outdated
@@ -76,6 +83,8 @@ const ( | |||
NodeUnaryExpr | |||
NodeMemberExpr | |||
NodeCallExpr | |||
NodeVarDecl | |||
NodeVarDecls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(var a = 1, b = 2, c =3) console.log("ok")
VM85:1 Uncaught SyntaxError: Unexpected token var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't tried to use var statements as expressions, they are being accepted right now as a bug ? I just added the var parser on the node parser, I did not realized that the parseNode function is the expression parser (it is the main entrypoint no ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my other comment. Sorry for not being clear.
parser/parser.go
Outdated
@@ -126,10 +125,10 @@ func (p *Parser) parseNode() (n ast.Node, eof bool, err error) { | |||
// parsers should not leave tokens not processed | |||
// in the lookahead buffer. | |||
if len(p.lookahead) != 0 { | |||
panic(fmt.Sprintf( | |||
return nil, false, fmt.Errorf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not panic? (just asking for the motivation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the error the a test was giving was a lot harder to understand with panic, after using a error return the error was much easier to track. Since this is a bug inside the parser panic may be better but I forgot the code like this.
parser/parser.go
Outdated
return _parseVarDecls(p) | ||
} | ||
|
||
func _parseVarDecls(p *Parser) (ast.VarDecls, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o.o
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I wanted to recursively build the vardecls from other vardecls but to use the function on the parserfn maps it needs to return an ast.Node. If I allow VarDecls to be built from ast.Node I would allow all sort of odd stuff to happen like var decls with other things inside (function declarations for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm complaining mostly about the name hehe
parser/parser.go
Outdated
|
||
func _parseVarDecls(p *Parser) (ast.VarDecls, error) { | ||
|
||
if !p.scry(2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why lookahead of 2?
var a; // no lookahead
var a = 1; // no lookahead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I had just "var". I dropped it since it is not necessary anymore. Now I need at least two tokens to see if it is:
- identifier + semicolon
- identifier + assignment (=)
How could this be written differently ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ident := p.next()
if ident.Type() != token.Ident {
return nil, p.errorf(ident, "expected IDENT")
}
tok := p.next()
if tok.Type() == token.Semicolon {
return ast.NewVarDecls(ast.NewVarDecl(ident.Value, ast.NewUndefined())), nil
}
if tok.Type != token.Assign {
return nil, fmt.Errorf("parser: var decl: expected assignment token [=] got [%s]", tok)
}
// parse assignment ...
fail: true, | ||
}, | ||
{ | ||
name: "InvalidMemberAccess", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this test be together with memberexpr tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, they are just as about member expression as they are about variable initialization. Actually the objective of these tests is to check that if the parser of the member access (the one that already has been tested isolated) fails the parser for variable statements checks that error and handles it properly, so it seemed like here would be the best place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you are saying that this just test if errors are being checked ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not understood some things =/
ast/node.go
Outdated
@@ -76,6 +83,8 @@ const ( | |||
NodeUnaryExpr | |||
NodeMemberExpr | |||
NodeCallExpr | |||
NodeVarDecl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the point
ast/node.go
Outdated
@@ -76,6 +83,8 @@ const ( | |||
NodeUnaryExpr | |||
NodeMemberExpr | |||
NodeCallExpr | |||
NodeVarDecl | |||
NodeVarDecls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't tried to use var statements as expressions, they are being accepted right now as a bug ? I just added the var parser on the node parser, I did not realized that the parseNode function is the expression parser (it is the main entrypoint no ?)
parser/parser.go
Outdated
@@ -126,10 +125,10 @@ func (p *Parser) parseNode() (n ast.Node, eof bool, err error) { | |||
// parsers should not leave tokens not processed | |||
// in the lookahead buffer. | |||
if len(p.lookahead) != 0 { | |||
panic(fmt.Sprintf( | |||
return nil, false, fmt.Errorf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the error the a test was giving was a lot harder to understand with panic, after using a error return the error was much easier to track. Since this is a bug inside the parser panic may be better but I forgot the code like this.
parser/parser.go
Outdated
return _parseVarDecls(p) | ||
} | ||
|
||
func _parseVarDecls(p *Parser) (ast.VarDecls, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I wanted to recursively build the vardecls from other vardecls but to use the function on the parserfn maps it needs to return an ast.Node. If I allow VarDecls to be built from ast.Node I would allow all sort of odd stuff to happen like var decls with other things inside (function declarations for example).
parser/parser.go
Outdated
|
||
func _parseVarDecls(p *Parser) (ast.VarDecls, error) { | ||
|
||
if !p.scry(2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I had just "var". I dropped it since it is not necessary anymore. Now I need at least two tokens to see if it is:
- identifier + semicolon
- identifier + assignment (=)
How could this be written differently ?
fail: true, | ||
}, | ||
{ | ||
name: "InvalidMemberAccess", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, they are just as about member expression as they are about variable initialization. Actually the objective of these tests is to check that if the parser of the member access (the one that already has been tested isolated) fails the parser for variable statements checks that error and handles it properly, so it seemed like here would be the best place.
parser/parser.go
Outdated
return nil, fmt.Errorf("parser: var decl: invalid token[%s] expected comma", possibleSemiColon) | ||
} | ||
|
||
vars, err := _parseVarDecls(p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to be able to do this recursion, it does not expect a "var" token and it can't return an ast.Node (not exactly can't but it don't seem like a good idea to me). The name is kinda lame but I was unable to think on something better until now. I thought about varAssigment since it parsers "a = value" not expecting the "var", but I imagine that the kind of node produced must be different since there is a difference on Javascript between declaring multiple variables starting with "var" and without (reference errors if the variable does not exist and it is not initialized).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spec call this section of declaration as VariableDeclarationList
. What about parseDeclList
?
Closes #53