`if` syntax with `then` causes interpreter panic

[64kramsystem]

Using if with then causes the interpreter to panic:

Goby 0.1.6 😶 😈 👻
» if true then
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x74ece5]

goroutine 1 [running]:
github.com/goby-lang/goby/compiler/ast.(*BlockStatement).KeepLastValue(0xc42015af80)
  /path/to/go/src/github.com/goby-lang/goby/compiler/ast/statements.go:215 +0x65
github.com/goby-lang/goby/compiler/parser.(*Parser).parseConditionalExpression(0xc42011a1b0, 0x0)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/flow_control_parsing.go:120 +0x15e
github.com/goby-lang/goby/compiler/parser.(*Parser).parseConditionalExpressions(0xc42011a1b0, 0xc4201722d0, 0x0, 0x0)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/flow_control_parsing.go:106 +0x40
github.com/goby-lang/goby/compiler/parser.(*Parser).parseIfExpression(0xc42011a1b0, 0xbc7688, 0xc42011d770)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/flow_control_parsing.go:92 +0xac
github.com/goby-lang/goby/compiler/parser.(*Parser).(github.com/goby-lang/goby/compiler/parser.parseIfExpression)-fm(0xb3c220, 0xc420172060)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/parser.go:143 +0x2a
github.com/goby-lang/goby/compiler/parser.(*Parser).parseExpression(0xc42011a1b0, 0x2, 0x8, 0x18)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/expression_parsing.go:133 +0xbd
github.com/goby-lang/goby/compiler/parser.(*Parser).parseExpressionStatement(0xc42011a1b0, 0x4)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/statement_parsing.go:271 +0x146
github.com/goby-lang/goby/compiler/parser.(*Parser).parseStatement(0xc42011a1b0, 0x10c9a40, 0x1)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/statement_parsing.go:28 +0x8a
github.com/goby-lang/goby/compiler/parser.(*Parser).ParseProgram(0xc42011a1b0, 0xc, 0xc42016e780)
  /path/to/go/src/github.com/goby-lang/goby/compiler/parser/parser.go:193 +0xe8
github.com/goby-lang/goby/igb.StartIgb(0xbc6b68, 0x5)
  /path/to/go/src/github.com/goby-lang/goby/igb/repl.go:160 +0x4d1
main.main()
  /path/to/go/src/github.com/goby-lang/goby/goby.go:29 +0x954

[hachi8833]

I got the same. Now I noticed that token.Then can only be found once in parser/flow_control_parsing.go and just for case syntax.

            case 9
            when 0, 1, 2, 3, 4, 5
              0
            when 6, 7, 7 + 1, 7 + 2 then
              1
            when 10, 11, 12
              2
            end

Then looks optional and I just wonder if we can drop the keyword then.

[st0012]

@hachi8833 Yes we can/should, plz help me drop it

[amohamedali]

Hi I have pushed a PR for this issue. As suggested it drops the support of the then keyword. But I still need an insight on how to test it. It kind of feels awkward to test something that isn't handled by the language.

Anyway now when you use the then keyword it does something like this.

goby ➤ ./goby -i
Goby 0.1.6 😜 😚 
» if true then
» end
#» UndefinedMethodError: Undefined Method 'then' for <Instance of: Object>

[hachi8833]

Congrats for the first PR!

Actually, I created almost the same PR locally 💦

The only difference is a test just to make sure then is dropped as following:

// vm/evaluation_test.go
func TestUnusedKeywordFail(t *testing.T) {
    testsFail := []errorTestCase{
        {`
        if true then puts 1 end
        `, "UndefinedMethodError: Undefined Method 'then' for <Instance of: Object>", 1},
        {`
        for
        `, "UndefinedMethodError: Undefined Method 'for' for <Instance of: Object>", 1},
    }

    for i, tt := range testsFail {
        v := initTestVM()
        evaluated := v.testEval(t, tt.input, getFilename())
        checkErrorMsg(t, i, evaluated, tt.expected)
        v.checkCFP(t, i, 1)
        v.checkSP(t, i, 1)
    }
}

Any suggestions? @st0012

[st0012]

I think the then's case is fine. But for is a keyword, giving the undefined method error might cause confusion.

[hachi8833]

The test code for for is unnecessary as you pointed out 😓

[amohamedali]

Ok I will add this test to the PR so. thank you guys :smile:

[st0012]

This has been fixed by @amohamedali in #551