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

Allow sane conversions for As*Map* and As*Array* conversions #11351

Merged
merged 1 commit into from Nov 14, 2022

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Nov 14, 2022

Fixes #11348
Fixes #11347

This PR implements deep type conversion for the As*Map and As*Array methods on AnyOutput.

For example, you can now call AsIntArrayMapOutput on an AnyOutput with underlying value map[string]interface{}{"zero": []int{0}, "one": []int{1}}.

I choose to implement this with reflection (as opposed to in the templating system) because I didn't want to handle the combinatorial explosion of outer and inner types. This solution works for arbitrary nesting of arbitrary types.

@iwahbe iwahbe self-assigned this Nov 14, 2022
@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2022-11-14)

Features

  • [sdk/go] Allow sane conversions for As*Map* and As*Array* conversions.
    #11351

Copy link
Contributor

@dixler dixler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iwahbe iwahbe force-pushed the iwahbe/11348/go-easier-as-conversions branch from 1afe0af to 4616eb7 Compare November 14, 2022 21:27
Update generated code
@iwahbe iwahbe force-pushed the iwahbe/11348/go-easier-as-conversions branch from 4616eb7 to 5a419f1 Compare November 14, 2022 21:28
@iwahbe
Copy link
Member Author

iwahbe commented Nov 14, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Build succeeded:

@bors bors bot merged commit 16afb72 into master Nov 14, 2022
@pulumi-bot pulumi-bot deleted the iwahbe/11348/go-easier-as-conversions branch November 14, 2022 22:27
Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elegant solution!

Comment on lines +1252 to +1258
// coerceTypeConversion assigns src to dst, performing deep type coercion as necessary.
func coerceTypeConversion(src interface{}, dst reflect.Type) (interface{}, error) {
makeError := func(src, dst reflect.Value) error {
return fmt.Errorf("expected value of type %s, not %s", dst.Type(), src.Type())
}
var coerce func(reflect.Value, reflect.Value) error
coerce = func(src, dst reflect.Value) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good recursive algorithm 👍

Contravariance implemented in ApplyT is extremely general.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a query, can any not be used in place of interface{}? Looks cleaner, but I'm guessing it is for backward compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're just using go's built in reflect.Type fmt.Stringer implementation. I was going for an error message that was similar to what we had before (i.(newType)). I would assume that the go dev team prefers interface {} over any for backwards compatibility, but you'd need to ask them to be sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Graham-Beer we could use any here if we updated go.mod to require 1.18 and above. I believe we'll be doing that soon anyhow as 1.17 is no longer supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants