Fix uninitialized variables

[Mido-sys]

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]

Hi @Mido-sys,
I am sorry for my late code review for this PR. I am not able to make enough time nowadays. I reviewed your PR today, and I found that the direction to solve this issue should be changed. I would like to revert this PR and will explain more details in a separate comment.

[sio4]

I would like to prevent using reflect in this loop. Too much reflection, and actually this is not necessary. I will leave another comment for the solution, based on the real issue.

[Mido-sys]

@sio4 , Yes, there's another way to update the parseLetStatement() function

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

to return stmt instead of nil. However, we will have to update all the parser functions to not return nil as calling stmt.String() will panic if any of the parser functions return nil. I made the decision to use reflection as it's the safest route considering how heavily plush relies on interfaces{} and reflections.

[sio4]

Correct. also, we can fix parseStatement(), and that could be the best. (See my another comment) With your PR, the for loop on tokens will reflect all statements returned and it is expensive and not necessary.

[sio4]

please add a line before function start and end

[sio4]

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()