Conversation
This is a big reimplementation that simplifies the compiler a lot. Instead of storing the method set in metadata and lowering the type asserts as a whole program pass, this change just puts the list of methods in the type code (and a separate global for the interface type).
This fully implements AssignableTo, fixing a number of bugs in the previous implementation. In particular, it now supports AssignableTo for interface types.
|
|
||
| // Run TinyGo-specific optimization passes. | ||
| OptimizeStringToBytes(mod) | ||
| OptimizeReflectImplements(mod) |
There was a problem hiding this comment.
So what happens with type asserts that are statically known to be true or false? Optimization pushed for future work?
There was a problem hiding this comment.
This is specifically for the reflect Implements method, which is used in some important code but not really used that much in total (so I don't think optimizing it will provide much benefit). The main reason this "optimization" existed was to get the encoding/json package to work.
But yeah, good point about other (normal Go style) interface type asserts. I can do a check how common that is and whether optimizing that case is beneficial. That is something that should work well as a package level optimization though.
|
@dgryski any progress on this? This is causing issues for me. I would like to use tinygo to generate WASM bindings, but the lack of a JSON encoder isn't great. See reteps/dockerfmt#25. |
|
Is the dependency on #4375 a hard one? Could this be implemented without it? |
Based on the design from tinygo-org#4376 by aykevl. Fixes tinygo-org#4277, fixes tinygo-org#3580. Co-authored-by: Ayke van Laethem <aykevanlaethem@gmail.com>
Based on the design from tinygo-org#4376 by aykevl. Fixes tinygo-org#4277, fixes tinygo-org#3580. Co-authored-by: Ayke van Laethem <aykevanlaethem@gmail.com>
|
Closing since #5304 is just a better version of this PR. |
* reflect: implement method-set based AssignableTo and Implements Based on the design from #4376 by aykevl. Fixes #4277, fixes #3580. Co-authored-by: Ayke van Laethem <aykevanlaethem@gmail.com> * builder: update expected binary sizes for reflect changes * Make interface checks similar to invoke, allowing typeImplementsMethodSet and method info to be dropped when reflect is not present * Add more tests that BigGo reflect tests * Even more pruning * Add go/token and net/url to passing tests * Prune even further, I am less happy with this, though * Update size test now that we are smaller * Skip some tests * elide method lists * format, oops * fix tests * Add a panic, pull out constant to keep in sync * Add debug info * Remove code that was leftover from a previous refactor --------- Co-authored-by: Ayke van Laethem <aykevanlaethem@gmail.com>
5 participants
This fully implements AssignableTo, fixing a number of bugs in the previous implementation. In particular, it now supports AssignableTo for interface types.
Depends on #4375