Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot assign to variable in imported package #1623

Closed
theclapp opened this issue Apr 22, 2024 · 5 comments · Fixed by #1628
Closed

Cannot assign to variable in imported package #1623

theclapp opened this issue Apr 22, 2024 · 5 comments · Fixed by #1628
Assignees
Labels
bug Something isn't working
Milestone

Comments

@theclapp
Copy link
Contributor

theclapp commented Apr 22, 2024

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
Copy link
Contributor Author

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

@theclapp
Copy link
Contributor Author

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
                                }

@ldez ldez added the bug Something isn't working label Apr 22, 2024
@mvertes
Copy link
Member

mvertes commented Apr 26, 2024

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
Copy link
Contributor Author

Thanks, will do!

theclapp added a commit to theclapp/yaegi that referenced this issue Apr 30, 2024
If you (*interp.Interpreter).Use a variable, you should be able to
assign to it.

Fixes traefik#1623.
traefiker pushed a commit that referenced this issue Apr 30, 2024
If you `(*interp.Interpreter).Use` a variable, you should be able to assign to it.

Fixes #1623
@traefiker traefiker added this to the v0.16.x milestone Apr 30, 2024
@theclapp
Copy link
Contributor Author

theclapp commented May 9, 2024

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.

Bai-Yingjie pushed a commit to godevsig/yaegi that referenced this issue Jul 15, 2024
If you `(*interp.Interpreter).Use` a variable, you should be able to assign to it.

Fixes traefik#1623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants