tinygo
tinygo copied to clipboard
tinygo-org
src/reflect: implement rawType.Name()
In good news with this merged I am able to use json :)
package main
import (
"encoding/json"
"fmt"
)
type JSONStruct struct {
Outer string
Embedded struct {
Inner string
}
}
func main() {
s := JSONStruct{}
s.Outer = "Outer"
s.Embedded.Inner = "Inner"
out, err := json.Marshal(s)
fmt.Println(string(out))
fmt.Println(err)
}
before
panic: unimplemented: (reflect.Type).Name()
error: failed to run compiled binary /tmp/tinygo2878253476/main: signal: aborted
after
{"Outer":"Outer","Embedded":{"Inner":"Inner"}}
<nil>
I am out of time tonight. @dgryski @aykevl Any pointers on how to get the name/how structs are encoded?
@Sean-Der can you please rebase this branch against dev? It should then pass the various CI builds.
I'm not sure I agree with this change. I've considered it before but rejected the idea because while it gets the encoding/json package to mostly work, it is somewhat incorrect.
Any idea when this pullrequest is accepted? Actually quite a lot depends on it e.g. the parsing of x.509 certificate. We need this to write WASM filters for Istio. Without the crypto package, which needs this fix, TinyGo is almost worthless for WASM.
Specifically, what I'm worried about with this PR is that it is not correct for named types. For unnamed types it seems to be correct (it just returns the underlying type name like int or float32) but for named types it returns the underlying type name, not the named type as it should. This could lead to misbehaving code. Panicking would in my opinion be more appropriate than returning an incorrect value as it makes it very obvious where the problem is coming from.
A partial solution might be to detect whether the type is named and panic if it is, returning the underlying type name if it is not named. But I'm afraid this won't solve the problem for serialization libraries.
@aykevl I am happy to add tests/modify how we do this to get it merged :) JSON I think matters a lot for JSON users (probably not so much for the small devices)
I ended up not using the stdlib json because of how big it made the binary size. I switched to github.com/moznion/go-json-ice
I think support for unnamed types is better than no support at all which is in line with @aykevl. I am up to drive the PR towards such direction, tests included.
Is it possible to proceed with this, even if a longer term, larger PR may happen in the future? As probably some notice, there's an increase of people trying and failing to use TinyGo with wasm, sometimes leaving the Go language entirely as a result. It would seem like "cutting of one's nose to spite their face" to not proceed tactically even if a more strategic change could happen one day.
I suspect to move this forward, we need someone with commit access to wave their hand and say "I'll help" and give whatever feedback is needed to the authors, as it seems the authors are ready.
How about it?
Is it possible to proceed with this, even if a longer term, larger PR may happen in the future?
I still don't agree with merging incorrect code. This PR, while well intentioned, will result in broken code.
A relatively simple way forward is by returning "" for non-named types and panicking for named types. You can determine the difference between the two using the documentation at the top of src/reflect/types.go. Feel free to ask if something is unclear.
@aykevl maybe close out this then? open PRs are distracting especially in reflect which is spread across so many things. I think that people knowing what won't happen is at least direction giving.
