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

[sdk/go] Add RegisterInputType and register built-in types #7928

Merged
merged 2 commits into from
Sep 16, 2021

Conversation

justinvp
Copy link
Member

@justinvp justinvp commented Sep 8, 2021

This change adds a RegisterInputType function (similar to the existing RegisterOutputType) that is used to register an input interface's type and its associated input type, and adds registrations for the built-in input types.

This will be used when copying inputs to an args struct for multi-lang components (separate PR). When a field is typed as the input interface (e.g. StringMapInput) and doesn't need to be an Output, we can use this to lookup the non-Output type that implements the interface (e.g. StringMap) so it can be instantiated.

A subsequent change will update the Go SDK codegen to produce input type registrations for a provider's input types.

Part of #7848
Part of #7434


Note: I was originally planning to include a new InputType interface:

type InputType interface {
    Input

    InputType() reflect.Type
}

Which could be implemented on an input type like:

// InputType returns the input interface type of this Input (StringMapInput).
func (StringMap) InputType() reflect.Type {
	return reflect.TypeOf((*StringMapInput)(nil)).Elem()
}

And registered like:

fun init() {
	RegisterInputType(StringMap{})
}

And the implementation of RegisterInputType could call InputType() to get the interface type and store it in the table.

There are two problems with this:

  1. We have concrete types that are associated with the different interfaces, e.g. the StringInput and StringPtrInput interfaces are both associated with String -- we'd have to change the interface to return an array of types.
  2. This would require adding a new method to every input type, which could bloat to our providers and programs.

The only real benefit of the interface is that it makes the registration call site simpler, but since you aren't generally writing these registrations by hand anyway, we can avoid the above problems by not having a new interface at all.

One benefit to not using an interface -- in the immediate term, users can add temporary input registrations of relevant input types from other providers in their component provider before we've updated the codegen of those providers to include the registrations.

@t0yv0
Copy link
Member

t0yv0 commented Sep 9, 2021

This is introducing a liability to call Register* on all input types which will take a bit of work to percolate to all examples and programs out there, so I'd really like to understand the motivation but I'm not sure I do from this description?

Is it that you are in a context that:

  1. The code has identified a type reflect.Type like StringMapInput
  2. There is no value of this type
  3. The code needs to generate an "empty" value of this type

What's confusing me is that "This will be used when copying inputs to an args struct for multi-lang components (separate PR)" make it sound like the code has an instance of the StringMapInput?

@t0yv0
Copy link
Member

t0yv0 commented Sep 9, 2021

The implementation LGTM.

@justinvp
Copy link
Member Author

justinvp commented Sep 9, 2021

Is it that you are in a context that:

  1. The code has identified a type reflect.Type like StringMapInput
  2. There is no value of this type
  3. The code needs to generate an "empty" value of this type

What's confusing me is that "This will be used when copying inputs to an args struct for multi-lang components (separate PR)" make it sound like the code has an instance of the StringMapInput?

Yes, that's it. Sorry the motivation wasn't clear from the description. Here's an example:

Imagine a component has the following args struct:

type StaticPageArgs struct {
	Tags pulumi.StringMapInput `pulumi:"tags"`
}

The component provider's implementation of Construct for this component will typically do this:

	args := &StaticPageArgs{}
	if err := inputs.CopyTo(args); err != nil {
		return nil, err
	}

inputs.CopyTo is a method we provide for copying the raw inputs from the Construct RPC into a component's args struct.

Inside the implementation of CopyTo, as we loop through the fields of the args struct, when we see StringMapInput, today, we create an Output instance. But that's not always the desired behavior.

Imagine the actual values for tags is {{"foo": "bar"}, {"hello": "<unknown-sentinel-value>"}} and the property dependencies for tags has some dependencies. We don't have enough information to know if those dependencies are only associated with certain values within the map, which forces us to create an output instance for the whole Tags property with those dependencies included. And since there's a nested value that's an unknown, we lift that to the top-level Output so it gets marked as unknown. Imagine then that Tags is set on a child custom resource of the component (like a Bucket). This leads to undesirable previews. The whole tags property shows up as unknown in the preview, when ideally, (if the property dependencies were only for the "hello" value) we'd only have to create an Output that's unknown for the "hello" value, such that the preview would show the "foo" value, and only show unknown for "hello".

Now, with output values, we'll have the information we need to know that the dependencies are only associated with the "hello" value. So when we see StringMapInput, we don't want to create an Output, we want to create an empty value, but there's currently no way to know that we should be creating a StringMap when all we have is StringMapInput. The registration is what gives us that mapping. If you can think of a way to create a non-Output empty value for a StringMapInput without requiring registrations, I'd love to know.

This is introducing a liability to call Register* on all input types which will take a bit of work to percolate to all examples and programs out there

If the registration doesn't exist, we fall back to the current behavior of creating the top-level Output for StringMapInput. So it's not a huge problem that it takes a while to percolate. I don't think any examples or programs would need to be changed at all -- only our provider Go SDKs. And even if it takes time to roll the change out to all of those, very few people are using multi-lang components that care about this right now. And even if a particular provider hasn't been updated yet to include input type registrations, and you want this improved behavior, you can add a registration for the types you care about inside your component provider temporarily until the provider SDK itself includes them.

@t0yv0
Copy link
Member

t0yv0 commented Sep 9, 2021

Thanks for that you convince me it's safe to ship.

@t0yv0
Copy link
Member

t0yv0 commented Sep 9, 2021

I thought about it some and yeah your approach makes sense.

The formulation that makes the most sense to me is that we need to construct a serializer/deserializer for an interface input type like StringMapInput. The serializer has to demonstrate turnaround properties to be considered reliable.

Now in Go we are immediately in deep water because the deserializer part of any interface needs to make a choice on the concrete type to use to instantiate the interface.

What can make this morally correct without having the entire type pointer of the concrete type spelled out in the serialized form, is that Input interfaces are not really open interfaces but are morally unions over statically known set of types T and Output[T].

Where these assumptions may be violated is if more than one concrete type inhabits an interface. We've briefly touched on representing Input[Resource] and Input[CustomResource] where the subtyping lattice makes things complicated.. perhaps bad example if those types are not serializable or serializable specially. I can't come up with a more real example though.

And.. if we're using reflection so reflection needs to know about these type relations, a type-map is reasonable. Languages where I seen this work well rely on type-indexed maps supported by the language in some form. In Haskell typeclasses are essentially threading type-indexed maps as extra parameters. In Scala implicits rely on type-indexed resolution. It's just a matter of some taste what exactly we hang into these maps.

@t0yv0
Copy link
Member

t0yv0 commented Sep 9, 2021

func RegisterInputType(interfaceType reflect.Type, input Input) {

So this approach would be in trouble IFF there is an example where more than 1 concrete type

type FooInput interface { Input }
type a struct {}
var _ FooInput = &a{} 
type b struct {}
var _ FooInput = &b{} 

RegisterInputType(reflect.TypeOf((*FooInput)(nil)).Elem(), &a{})
// can't register &b{} because it would panic, but ok

// then what is the issue? the issue is that

func checkSerializerTurnAround(t *testing.T, x FooInput) {
  ...
}

checkSerializerTurnAround(t, &b{})

// This will actually try to deserialize into &a{}

@justinvp
Copy link
Member Author

Input[Resource] and Input[CustomResource] where the subtyping lattice makes things complicated.. perhaps bad example if those types are not serializable or serializable specially

Right, these are handled specially (as resource references).

Another oddity is the AssetOrArchive, Asset, and Archive interfaces along with their associated Input interfaces. These are also handled specially.

So this approach would be in trouble IFF there is an example where more than 1 concrete type

That's right. In practice, I can't think of any examples where this is the case. Someone would have to be doing something really off the rails.

This change adds a `RegisterInputType` function (similar to the existing `RegisterOutputType`) that is used to register an input interface's type and its associated input type, and adds registrations for the built-in input types.

This will be used when copying inputs to an args struct for multi-lang components. When a field is typed as the input interface (e.g. `StringMapInput`) and doesn't need to be an `Output`, we can use this to lookup the non-Output type that implements the interface (e.g. `StringMap`) so it can be instantiated.

A subsequent change will update the Go SDK codegen to produce input type registrations for a provider's input types.
@justinvp justinvp merged commit 1a4f36e into master Sep 16, 2021
@pulumi-bot pulumi-bot deleted the justin/outputvalues_registerinputs branch September 16, 2021 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants