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.

Sign up for GitHub

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

Fix Fetch and OpDeref #467

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Fix Fetch and OpDeref #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af628d2
Fix OpDeref allowing to deref pointer of interfaces
a-peyrard Nov 14, 2023
1678baf
Fix deref interface in Fetch
a-peyrard Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions expr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1204,6 +1204,21 @@ func TestExpr_fetch_from_func(t *testing.T) {
assert.Contains(t, err.Error(), "cannot fetch Value from func()")
}

func TestExpr_fetch_from_interface(t *testing.T) {
type FooBar struct {
Value string
}
foobar := &FooBar{"waldo"}
var foobarAny any = foobar
var foobarPtrAny any = &foobarAny

res, err := expr.Eval("foo.Value", map[string]any{
"foo": foobarPtrAny,
})
assert.NoError(t, err)
assert.Equal(t, "waldo", res)
}

func TestExpr_map_default_values(t *testing.T) {
env := map[string]any{
"foo": map[string]string{},
Expand Down
63 changes: 63 additions & 0 deletions test/deref/deref_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,66 @@ func TestDeref_multiple_pointers(t *testing.T) {
require.Equal(t, 44, output)
})
}

func TestDeref_pointer_of_interface(t *testing.T) {
v := 42
a := &v
b := any(a)
c := any(&b)
t.Run("returned as is", func(t *testing.T) {
output, err := expr.Eval(`c`, map[string]any{
"c": c,
})
require.NoError(t, err)
require.Equal(t, c, output)
require.IsType(t, (*interface{})(nil), output)
})
t.Run("+ works", func(t *testing.T) {
output, err := expr.Eval(`c+2`, map[string]any{
"c": c,
})
require.NoError(t, err)
require.Equal(t, 44, output)
})
}

func TestDeref_nil(t *testing.T) {
var b *int = nil
c := &b
t.Run("returned as is", func(t *testing.T) {
output, err := expr.Eval(`c`, map[string]any{
"c": c,
})
require.NoError(t, err)
require.Equal(t, c, output)
require.IsType(t, (**int)(nil), output)
})
t.Run("== nil works", func(t *testing.T) {
output, err := expr.Eval(`c == nil`, map[string]any{
"c": c,
})
require.NoError(t, err)
require.Equal(t, true, output)
})
}

func TestDeref_nil_in_pointer_of_interface(t *testing.T) {
var a *int32 = nil
b := any(a)
c := any(&b)
t.Run("returned as is", func(t *testing.T) {
output, err := expr.Eval(`c`, map[string]any{
"c": c,
})
require.NoError(t, err)
require.Equal(t, c, output)
require.IsType(t, (*interface{})(nil), output)
})
t.Run("== nil works", func(t *testing.T) {
output, err := expr.Eval(`c == nil`, map[string]any{
"c": c,
})
require.NoError(t, err)
require.Equal(t, true, output)
})
}
32 changes: 12 additions & 20 deletions vm/runtime/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ import (
"reflect"
)

func deref(kind reflect.Kind, value reflect.Value) (reflect.Kind, reflect.Value) {
for kind == reflect.Ptr || kind == reflect.Interface {
value = value.Elem()
kind = value.Kind()
}
return kind, value
}

func Fetch(from, i any) any {
v := reflect.ValueOf(from)
kind := v.Kind()
Expand All @@ -28,10 +36,8 @@ func Fetch(from, i any) any {
// Structs, maps, and slices can be access through a pointer or through
// a value, when they are accessed through a pointer we don't want to
// copy them to a value.
if kind == reflect.Ptr {
v = reflect.Indirect(v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we going to loose reflect.Indirect here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is ooriginal change: 6f0c855

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are still doing the "indirect" in the deref function as we are calling Elem() of the value is a pointer.
Which should be doing the same as the Indirect function, which is also doing Elem() behind the scene:

func Indirect(v Value) Value {
	if v.Kind() != Pointer {
		return v
	}
	return v.Elem()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran the benchrmak, it seems we are in the same order of magnitude:
before:

Benchmark_largeStructAccess-12           1000000               134.4 ns/op

after:

Benchmark_largeStructAccess-12           1000000               132.2 ns/op

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! let me check it a bit more as runtime changes should be done very carefully.

kind = v.Kind()
}
// De-reference everything if necessary (interface and pointers)
kind, v = deref(kind, v)

// TODO: We can create separate opcodes for each of the cases below to make
// the little bit faster.
Expand Down Expand Up @@ -145,27 +151,13 @@ func Deref(i any) any {

v := reflect.ValueOf(i)

if v.Kind() == reflect.Interface {
for v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface {
if v.IsNil() {
return i
return nil
}
v = v.Elem()
}

loop:
for v.Kind() == reflect.Ptr {
if v.IsNil() {
return i
}
indirect := reflect.Indirect(v)
switch indirect.Kind() {
case reflect.Struct, reflect.Map, reflect.Array, reflect.Slice:
break loop
default:
v = v.Elem()
}
}

if v.IsValid() {
return v.Interface()
}
Expand Down
Loading