fix: don't pass value types by reference

[deelawn]

Addresses #1096

These changes ensure that passing non-primitive value types to a function do not result in the modification of the original value, even if the value type is represented as a pointer inside the VM.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a88e3e3) 55.70% compared to head (3da4ef1) 55.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
+ Coverage   55.70%   55.71%   +0.01%     
==========================================
  Files         421      421              
  Lines       65555    65562       +7     
==========================================
+ Hits        36515    36528      +13     
  Misses      26177    26177              
+ Partials     2863     2857       -6     

Files	Coverage Δ
gnovm/pkg/gnolang/op_call.go	58.55% <100.00%> (ø)
gnovm/pkg/gnolang/values.go	42.56% <100.00%> (+0.23%)	⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mvertes]

The solution you provide is correct to me, and I approve it 👏 .

Regarding the test case, it would be better if the code was compatible with pure Go, which is not at the moment due to the use of println which is more limited in pure Go. It is certainly fixable easily.

[zivkovicmilos]

Looks great 💯

Thank you for the fix 🙏

[thehowl]

LGTM.

requesting review by @piux2 or @jaekwon

[thehowl]

Regarding the test case, it would be better if the code was compatible with pure Go, which is not at the moment due to the use of println which is more limited in pure Go. It is certainly fixable easily.

the "weird printing" of println is present in a lot of filetests (rg -A5 '// Output:$' gnovm/tests) and I think it's a bit out of scope for this PR, so I'd say to do that elsewhere.

you're welcome to create an issue :)

[piux2]

The fix works well. Since the fix is in op_call during VM execution and involves large number copying, maybe @jaekwon can help identify if it's the best place for the fix and any other related areas that need attention.

[jaekwon]

I think it's the right place.
Stating the obvious, but .Assign() calls Copy() too, so args for function calls should also.
On the other hand I return values don't need to be Copy()'d separately unless they are likewise assigned.
So this means function chaining doesn't result in unnecessary copies for throwaway values.

op_eval.go for *CallExpr which invokes OpPrecall doesn't copy, it just evaluates.
OpPrecall doesn't do any copying, it just digs into the value stack to fetch the function.
OpCall seems like the right place to do it; after that is OpBody, and by that point the input args should have been fully prepared.

[jaekwon]

LGTM