search

tinygo-org / tinygo

Go compiler for small places. Microcontrollers, WebAssembly (WASM/WASI), and command-line tools. Based on LLVM.
https://tinygo.org
Other
15.51k stars 914 forks source link

reflect: add `Value.Clear`; support `anytype->interface{}`, `Slice->(*)Array` in `Value.Convert`, fix `Copy` #4543

Closed ben-krieger closed 1 week ago

ben-krieger commented 1 month ago

Fixes #4554

This PR adds a few things to the reflect package.

  1. Add Value.Clear
  2. Convert anything to interface{}, i.e. reflect.ValueOf(42).Convert(reflect.TypeFor[interface{}]())
  3. Convert slices to arrays, i.e. reflect.ValueOf([]int{1, 2, 3}).Convert(reflect.TypeFor[[3]int]())
  4. Convert slices to array pointers... because after doing 3, I felt like I had to
  5. Fixes reflect.Copy when the source is an array

The motivation was to allow compiling the CBOR library at https://github.com/fido-device-onboard/go-fdo/tree/tinygo-server-support/cbor. However, this PR can be split into smaller ones if desired.

ben-krieger commented 1 month ago

Thanks for the thorough review @dgryski! I'll spend some time this weekend fixing it up and adding more tests.

dgryski commented 3 weeks ago

testdata/reflect.go is still failing. That's Bad because the desired output is generated by upstream Go, so that indicates incompatibilities.

ben-krieger commented 3 weeks ago

testdata/reflect.go is still failing. That's Bad because the desired output is generated by upstream Go, so that indicates incompatibilities.

Got it. So it seems that arrays in some cases have become addrable and settable, when they shouldn't. Let me see if I can figure out why...

dgryski commented 3 weeks ago

It's almost certainly the flag changes for indirect support. You probably need some RO bits too.

ben-krieger commented 3 weeks ago

It's almost certainly the flag changes for indirect support. You probably need some RO bits too.

Tried RO, but couldn't get it working, so I reverted the change that broke it. (I was also uncomfortable with that code, because I didn't understand the underlying mechanism that distinguished between arrays > 64 bits in size.) However, the test I added for reflect.Copy is still failing.

Any thoughts on the proper way to fix #4554?

ben-krieger commented 3 weeks ago

It's almost certainly the flag changes for indirect support. You probably need some RO bits too.

@dgryski I think this is ready for (hopefully the last) review. All tests are passing now and the fix for #4554 matches a lot of other code within the reflect package that check either indirect OR size > uintptr.

I don't see any more low-hanging fruit for test coverage, but let me know what you think. Thanks!

dgryski commented 3 weeks ago

One quick comment. I'll do a full review tomorrow (Monday) and then hopefully we can get this merged. Thanks for all your work on this!

dgryski commented 2 weeks ago

@aykevl Any comments?

aykevl commented 6 days ago

Took a look, looks good to me.