-
Notifications
You must be signed in to change notification settings - Fork 565
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
Type conversion support between a type parameter and basic types. #1235
Conversation
The test failures seem to be due to a bug in minified variable allocation logic, which is not directly related to this PR, but somehow triggered by it. I'll look into it and send a fix. |
Since GopherJS doesn't generate instantiation-specific code for generic functions and types, we have to do conversion at runtime. This commit creates basic infrastructure for such conversion, and uses int64 and uint64 types as a first demonstration of concept. Each type `T` provides a `T.convertFrom` method, which accepts a value of any compatible type `Q` and returns a corresponding value of type `T`. This conversion will be less efficient compared to non-generic code, since we have to branch based on the input type every time we do the conversion, but this is the price for more compact code.
Unsafe pointers are not well supported by GopherJS and whatever support exists is not well documented. Implementing this conversion will require a great deal of great reverse engineering, and I'm not sure anyone will ever need this. So for now, attempting such a conversion will simply throw an "unimplemented" exception.
In general, this conversion is almost a no-op, except that previously *js.Object instances were not considered a wrapped type (which is incorrect) and were not properly wrapped when converting to an interface. Other than that, GopherJS's representation for an interface value is actually its concrete value. Testing the conversion is a bit tricky, since we can't use reflect because it relies on conversion to `any` interface being correct - something we can't take for granted. To make testing possible, I've refactored the tests a bit and use low-level JS manipulation to inspect values that result from conversion.
Two tests temporarily regressed, since try use type conversions that we don't yet support. Previously they passed mostly by a chance - doing no conversion happened to be good enough.
716051c
to
ac16ade
Compare
Also added support for type conversion to interfaces, since without it many test cases were regressing. They used to work just by the virtue that most of the times conversion to an interface at runtime is actually a no-op, but the original version of this PR explicitly threw an error for unsupported conversions. #1237 should've addressed another class of errors that CI detected previously. |
Eh, apparently something isn't quite right with reflection. I'll look into it tomorrow. |
ac16ade
to
0b0642e
Compare
@flimzy okay, all seems to be fixed now. I made some significant changes since your approval though, would you mind giving another look? |
0b0642e
to
039acdd
Compare
Since gopherjs doesn't generate unique code per generic type instantiation, we won't know the real type of a typeparam until run time. This means that type conversion between a typeparam types and any other type also has to shift into runtime. We do this by adding a static method
convertFrom()
to each type constructor. That way, when the exact type is now known at compile time, the compiler can emit codeT.convertFrom(expr)
regardless of what typeT
is.This PR adds some basic infrastructure for type conversion and implements support for converting to most basic types: strings, ints, floats, booleans, complex. Later PRs will add support for complex types, which are, well, more complicated to handle.
Updates #1013.