panic vm.Error instead of plain string

[Berailitz]

convertVMFunctionToType panics strings if any error happens, which blocks invokers form acquiring the position where errors occur. If we could panic the raw vm.Error, the invoker will be able to report the line and column of the error. IDEs and programmers will benefit from the change. Thanks. :)

[MichaelS11]

Can you show an example?

[Berailitz]

EDIT: fix define of println.

package main

import (
    "fmt"
    "log"

    "github.com/mattn/anko/env"

    "github.com/mattn/anko/vm"
)

const script = "func hi(){x};register(hi)"

type BarkFunc func() string

type Dog struct {
    Bark BarkFunc
}

func NewDog(bark BarkFunc) *Dog {
    return &Dog{
        Bark: bark,
    }
}

func main() {
    defer func() {
        if err := recover(); err != nil {
            fmt.Printf("%#v", err)
        }
    }()
    baseEnv := env.NewEnv()
    baseEnv.Define("register", NewDog)
    baseEnv.Define("println", log.Println)
    if dogOut, err := vm.Execute(baseEnv, nil, script); err == nil {
        if dog, ok := dogOut.(*Dog); ok {
            dog.Bark()
        }
    }
}

For example, the code above will print "function run error: undefined symbol 'x'" before this commit.

$ go run main.go 
"function run error: undefined symbol 'x'"

but &vm.Error{Message:"undefined symbol 'x'", Pos:ast.Position{Line:1, Column:1}} after this commit.

$ go run main.go 
&vm.Error{Message:"undefined symbol 'x'", Pos:ast.Position{Line:1, Column:1}}

[MichaelS11]

Do you have an example where the Position is not line 1 column 1?

[Berailitz]

If you change const script to "println(0);func hi(){println(1);x};register(hi)", go run will output {Line:1, Column:12}.

Complete code is listed below:

package main

import (
    "fmt"
    "log"

    "github.com/mattn/anko/env"

    "github.com/mattn/anko/vm"
)

const script = "println(0);func hi(){println(1);x};register(hi)"

type BarkFunc func() string

type Dog struct {
    Bark BarkFunc
}

func NewDog(bark BarkFunc) *Dog {
    return &Dog{
        Bark: bark,
    }
}

func main() {
    defer func() {
        if err := recover(); err != nil {
            fmt.Printf("%#v", err)
        }
    }()
    baseEnv := env.NewEnv()
    baseEnv.Define("register", NewDog)
    baseEnv.Define("println", log.Println)
    if dogOut, err := vm.Execute(baseEnv, nil, script); err == nil {
        if dog, ok := dogOut.(*Dog); ok {
            dog.Bark()
        }
    }
}

The output after this change is:

$ go run main.go
2020/03/26 14:15:26 0
2020/03/26 14:15:26 1
&vm.Error{Message:"undefined symbol 'x'", Pos:ast.Position{Line:1, Column:12}}

[MichaelS11]

Great.

So how about changing the panic output to be formatted? Like: function run error: undefined symbol 'x' - Position: Line:1, Column:12 Also please fix the test and add a test so coverage is complete for both error paths.

[Berailitz]

The invoker controls the panic output, not the anko library. This commit exposes the raw vm.Error object to the invoker, which means invokers will acquire this error object and can do whatever they want, including changing the output.

Tests will be added later.

[MichaelS11]

My bad, thought panic took a string. Just need to wrap the error messages and return a new VM error message. I will work on it tommorrow.

[Berailitz]

Test code above checks the panicked error. This commit panics the error object since I thought IDEs may prefer structured error objects to plain error messages. However, I think it's still open to further discussion.

[Berailitz]

This commit breaks TestFunctionConversions, because it excepts to recover a string, not a vm.Error. Though I still hold the point that vm.Error should format the error message itself, since we do not panic the error object, I'll fix the code and panics a formatted string myself, for changing the Error method might break too much code.

[codecov-io]

Codecov Report

Merging #332 into master will decrease coverage by 0.05%. The diff coverage is 60%.

@@            Coverage Diff             @@
##           master     #332      +/-   ##
==========================================
- Coverage   88.24%   88.18%   -0.06%     
==========================================
  Files          15       15              
  Lines        2680     2684       +4     
==========================================
+ Hits         2365     2367       +2     
- Misses        204      206       +2     
  Partials      111      111

Impacted Files	Coverage Δ
vm/vmConvertToX.go	92.15% <60%> (-1.73%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 716730c...b157d2a. Read the comment docs.

[Berailitz]

Sorry for the reset of b157d2a and the decrease of test coverage, for I failed to find a way to test against the else branch, which, as far as I know, is supposed to be unreachable under normal conditions.

[MichaelS11]

Thank you for finding this issue.

Sorry for not being more clear. I am going to work on submiting a PR to clean up most/all of the panics in all of Anko, instead of just the one or a few.

[MichaelS11]

Please see PR https://github.com/mattn/anko/pull/333

[MichaelS11]

Should be able to close this now