gobuffalo / plush

The powerful template system that Go needs
MIT License
895 stars 57 forks source link

Fix uninitialized variables #171

Closed Mido-sys closed 1 year ago

Mido-sys commented 1 year ago

What is being done in this PR?

Fix panic issue

What are the main choices made to get to this solution?

using reflect to check if the holding value is nil

List the manual test cases you've covered before sending this PR:

Added a new test Test_Let_Ident_NotInitialized under variables_test.go

sio4 commented 1 year ago

Hi @Mido-sys,

First of all, thank you for reporting this issue and the PR. I really appreciate it!

Your solution on the original PR to use reflect to check nil value could be one option, but I would like to fix the real issue. I am not sure if I can explain clearly in my poor Yonglish, but let me point out the real issues.

In the method parseProgram(), the first line of the for loop on tokens is getting stmt from parser.parseStatement(). The method returns ast.Statement, which is an interface. We should use a method that returns an interface extra carefully, and the original reason for this issue came from it. In this case, we should be prepared that the returned value could be a nil but also could be a non-nil with a nil value. (So your direction could be fine if we cannot control the problematic method)

However, for this issue, the problematic method is our own and we can control it. Currently, method parseStatement() has no safety guard for returning non-nil with nil value, so the first thing we can do is add a nil-checker for the return value. (see the first part of the following patch.) The second thing we can do is to prevent nil return from the called methods in the switch. Currently, the method parseLetStatement() returns nil for two cases, but we don't need to return nil here, we can just return an empty structure since we already set the syntax error in the parser.errors by expectPeek() method.

With this patch, your test will not get panic. Could you please take a look at this patch and let me know your concern if any?

--- a/parser/parser.go
+++ b/parser/parser.go
@@ -156,9 +156,14 @@ func (p *parser) noPrefixParseFnError(t token.Type) {
 }

 func (p *parser) parseStatement() ast.Statement {
+       // NOTE: It returns interface. Prevent possiblity of returning a non-nil
+       // with nil value.
        switch p.curToken.Type {
        case token.LET:
                l := p.parseLetStatement()
+               if l == nil {
+                       return nil
+               }
                return l
        case token.S_START:
                p.nextToken()
@@ -192,14 +197,15 @@ func (p *parser) parseReturnStatement(t string) *ast.ReturnStatement {
 func (p *parser) parseLetStatement() *ast.LetStatement {
        stmt := &ast.LetStatement{TokenAble: ast.TokenAble{Token: p.curToken}}

+       // NOTE: do not return nil but return empty structure!
        if !p.expectPeek(token.IDENT) {
-               return nil
+               return stmt
        }

        stmt.Name = &ast.Identifier{TokenAble: ast.TokenAble{Token: p.curToken}, Value: p.curToken.Literal}

        if !p.expectPeek(token.ASSIGN) {
-               return nil
+               return stmt
        }

        p.nextToken()