traefik / yaegi

Yaegi is Another Elegant Go Interpreter
https://pkg.go.dev/github.com/traefik/yaegi
Apache License 2.0
6.78k stars 341 forks source link

Cannot assign to variable in imported package #1623

Closed theclapp closed 2 months ago

theclapp commented 2 months ago

The following program sample.go triggers an unexpected result

package main

import (
    "fmt"
    "os"
    "reflect"

    "github.com/traefik/yaegi/interp"
)

func main() {
    var F float64

    i := interp.New(interp.Options{})
    if err := i.Use(interp.Exports{
        "pkg/pkg": map[string]reflect.Value{
            "F": reflect.ValueOf(&F).Elem(),
        },
    }); err != nil {
        panic(err)
    }
    i.ImportUsed()

    res, err := i.Eval("pkg.F++; pkg.F")
    if err != nil {
        panic(err)
    }
    if res.Interface().(float64) != 1.0 {
        fmt.Printf("F++: expected 1.0, got %v\n", res.Interface())
    }
    res, err = i.Eval("pkg.F = 4.0; pkg.F")
    if err != nil {
        fmt.Printf("F = 4.0: err: %v\n", err)
        os.Exit(1)
    }
    if res.Interface().(float64) != 4.0 {
        fmt.Printf("F = 4.0: Expected 4.0; got %v\n", res.Interface())
        os.Exit(1)
    }
}

Expected result

$ go run ./sample.go
// no output expected

Got

$ go run ./sample.go
F = 4.0: err: 1:28: cannot assign to %!s(float64=1) (float64 constant)

Yaegi Version

3fbebb3

Additional Notes

In test-case form: add this to the end of interp/interp_eval_test.go:

func TestIssue1623(t *testing.T) {
    var F float64
    Pf := &F
    var ii int
    var s string = "foo"

    i := interp.New(interp.Options{})
    if err := i.Use(interp.Exports{
        "pkg/pkg": map[string]reflect.Value{
            "F":  reflect.ValueOf(&F).Elem(),
            "Pf": reflect.ValueOf(&Pf).Elem(),
            "II": reflect.ValueOf(&ii).Elem(),
            "S":  reflect.ValueOf(&s).Elem(),
        },
    }); err != nil {
        t.Fatal(err)
    }
    i.ImportUsed()

    runTests(t, i, []testCase{
        // {desc: "F++", src: "pkg.F++; pkg.F", res: "1"},
        // {desc: "*Pf = 2", src: "*pkg.Pf = 2; *pkg.Pf", res: "2"},
        // {desc: "check F", src: "pkg.F", res: "2"},
        // {desc: "*(&F) = 3", src: "*(&pkg.F) = 3; pkg.F", res: "3"},
        {desc: "F = 4", src: "pkg.F = 4.0; pkg.F", res: "4"}, // breaks

        // {desc: "II++", src: "pkg.II++; pkg.II", res: "1"},
        {desc: "II = 2", src: "pkg.II = 2; pkg.II", res: "2"}, // breaks

        {desc: `S = "bar"`, src: `pkg.S = "bar"; pkg.S`, res: "bar"}, // breaks
    })
}
theclapp commented 2 months ago

This appears to've broken in v0.14.3. It works in v0.14.2.

theclapp commented 2 months ago

I don't know if this is the right fix, but it's a fix:

% git diff interp
diff --git a/interp/cfg.go b/interp/cfg.go
index 39133a4c..c7a00eef 100644
--- a/interp/cfg.go
+++ b/interp/cfg.go
@@ -651,7 +651,7 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string
                                var sym *symbol
                                var level int

-                               if dest.rval.IsValid() && isConstType(dest.typ) {
+                               if dest.rval.IsValid() && !dest.rval.CanSet() && isConstType(dest.typ) {
                                        err = n.cfgErrorf("cannot assign to %s (%s constant)", dest.rval, dest.typ.str)
                                        break
                                }
mvertes commented 2 months ago

I confirm the issue and I think that your fix is the right solution. Well done!

Feel free to submit a pull request with your proposed fix and test, so you can be properly credited. Thanks.

theclapp commented 2 months ago

Thanks, will do!

theclapp commented 2 months ago

Just a heads-up to any interested observers that this problem is not entirely fixed. I'm getting various weird panics in interpreted code when I do it. I'm still trying to nail down a test case. When I figure it out I'll open a new bug.

The way to prevent the panic / allow the update is to either create the variable as a pointer to begin with

// native Go
var V = new(float32)

// interpreted code where V has been imported via `interp.ImportUsed`.
*V += 5
math.Sin(*V)

or do a *& dance whenever you access it:

// native Go
var V float32

// Interpreted code
*&V += 5
math.Sin(*&V)

As a hint to the dev team, should you be interested in looking into this without a concrete reproducer, when I tried to update a struct I got

panic: reflect: call of reflect.Value.SetFloat on zero Value

This might not actually be related to the problem, but I suspect that it is.