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

Getting a slice or struct from a struct will get pointers to them instead of copies #378

Closed
mstoykov opened this issue Apr 4, 2022 · 1 comment

Comments

@mstoykov
Copy link
Contributor

mstoykov commented Apr 4, 2022

This was found in a roundabout way - the actual thing I observed was that a []byte argument was not able to be cast to []byte as it was reporting to be *[]uint8.

Along the way I found out byte is just alias for uint8 and that if the argument is really []byte it ... works :).

The problem actually is that when accessing the field of a go struct(reflected) it gets the address for slices and struct fields and from there we actually get *[]uint8( or *[]byte). So if later I try to cast to *[]byte - it works.

But this is unexpected.

Also, if I change that slice that is being pointed to ... I don't actually change the slice that is the field of the struct. So having it as a pointer is even more confusing. But that changes if I actually give goja a pointer to the struct - which makes sense.

The smallest test that shows what I mean:

func TestGoSliceReflectBytes(t *testing.T) {
	vm := New()
	p := struct {
		A []byte
		F func(b interface{}) // This argument can be Value and Export to be used - same result
	}{
		A: []byte{1, 2, 3, 4},
		F: func(b interface{}) {
			switch s := b.(type) {
			case []byte:
			case *[]byte: // enters here
				*s = append(*s, 5)
			default:
				t.Fatalf("wrong type %T expected []byte", s) // if the above case is removed will report `*[]uint8`
			}
		},
	}
	vm.Set("f", vm.ToValue(p)) // if this is given as a pointer the slice is changed in the struct as well

	// seems to be important that we access a field on a struct
	_, err := vm.RunString(`f.F(f.A);`)
	if err != nil {
		t.Fatal(err)
	}
	fmt.Println(p.A) // this will print 1, 2, 3, 4 - unless the struct is also given as a pointer in which case it will 1,2,3,4,5
}

After some more digging I got to these lines

goja/object_goreflect.go

Lines 167 to 169 in 90825c0

if v := o._getField(name); v.IsValid() {
return o.val.runtime.ToValue(o.getAddr(v).Interface())
}

Which gets the field and then its address before getting an interface{} and calling ToValue with that interface it.
Getting the address gets the address only for structs and slices:

goja/object_goreflect.go

Lines 158 to 163 in 90825c0

func (o *objectGoReflect) getAddr(v reflect.Value) reflect.Value {
if (v.Kind() == reflect.Struct || v.Kind() == reflect.Slice) && v.CanAddr() {
return v.Addr()
}
return v
}

Removing getting the address first - fixes my issue, but breaks a bunch of tests with panics :(.

I would argue this should not be the case - goja should not get a pointer to slices and structs when accessing fields of at struct. But I am not certain what the other consequences of this are.

@dop251
Copy link
Owner

dop251 commented Apr 4, 2022

I agree. The reason why it's done like this was that the conversion of a non-primitive struct field is effectively done through reflect.ValueOf(fieldValue.Interface()) which loses the ability to address even if fieldValue was addressable. The solution is to let ToValue() somehow know about the original reflect.Value, I'll have a look.

@dop251 dop251 closed this as completed in 9037c2b Apr 5, 2022
Gabri3l pushed a commit to mongodb-forks/goja that referenced this issue Sep 1, 2022
… than pointers to them. Closes dop251#378.

(cherry picked from commit 9037c2b)
Gabri3l added a commit to mongodb-forks/goja that referenced this issue Mar 15, 2023
* Fixed panic in newArrayFromIter when the iterator is already closed. Fixes dop251#375
* Fixed panic when parsing invalid object property keys. Fixes dop251#376.
* Fixed accidental shadowing in the else branches of type assertion
* Fixed defineProperty("length") for arrays. Improved detection of non-standard array configurations. Upgraded tc39 tests.
* Return true values of struct fields or reflect slice elements, rather than pointers to them. Closes dop251#378.
* Upgraded dependencies. Closes dop251#380.
* Implemented exponentiation expressions. Closes dop251#381.
* Enabled tests that use ** operator. Some array fixes as a result.
* Implemented nullish coalescing operator (??). Closes dop251#382.
* Implemented `{Array,String,%TypedArray%}.prototype.at` (dop251#384)e7c2872c8)
* Fixed callee expressions in optional chains. Fixes dop251#385.
* Do not use fmt.Sprintf() for plain error strings. Fixes dop251#388.
* Implemented 'copy-on-change' mechanism for inner compound values. Fixes dop251#403.
* Fixed objectGoReflect equality. See dop251#403
* Don't clear interrupt until the stack is empty (dop251#405)
* test: skip Promise based tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants