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

[codegen/go] Remove ResourcePtr input/output types #8449

Merged
merged 3 commits into from
Nov 23, 2021
Merged

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Nov 18, 2021

These changes remove the Ptr variants of input/ouptut types for
resources. A TPtr input or output is normally generated for T if T
is present in an optional(input(T)) or optional(output(T)) and if
the Go representation for T is not nilable. The generation of Ptr
variants for resource types breaks the latter rule: the canonical
representation of a resource type named Foo is a pointer to a struct
type named Foo (i.e. *Foo). Foo itself is not a resource, as it
does not implement the Go Resource interface. Because this
representation already accommodates nil to indicate the lack of a
value, we need not generate FooPtr{Input,Output} types.

Besides being unnecessary, the implementation of Ptr types for
resources was incorrect. Rather than using **Foo as their element
type, these types use *Foo--identical to the element type used for
the normal input/output types. Furthermore, the generated code for
at least FooOutput.ToFooPtrOutputWithContext and FooPtrOutput.Elem
was incorrect, making these types virtually unusable in practice.

Finally, these Ptr types should never appear on input/output
properties in practice, as the logic we use to generate input and output
type references never generates them for `optional({input,output}(T)).
Instead, it generates references to the standard input/output types.

Though this is technically a breaking change--it changes the set of
exported types for any package that defines resources--I believe that in
practice it will be invisible to users for the reasons stated above.
These types are not usable, and were never referenced.

This is preparatory work for #7943.

@pgavlin pgavlin requested review from t0yv0 and iwahbe November 18, 2021 16:56
@pgavlin
Copy link
Member Author

pgavlin commented Nov 18, 2021

cc @mikhailshilkov : this will definitely have some effect on SDK sizes, though I haven't yet measured it.

@pgavlin pgavlin requested a review from a team November 18, 2021 16:57
@pgavlin
Copy link
Member Author

pgavlin commented Nov 18, 2021

These changes also fix the element types for FooArray and FooMap where Foo is a resource type. Prior to these changes, the element types were []Foo and map[string]Foo; they are now []*Foo and map[string]*Foo. The prior element type prevented these input/output/args types from being usable in practice.

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 6582e74

@github-actions
Copy link

Diff for pulumi-random with merge commit 6582e74

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 6582e74

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 6582e74

@github-actions
Copy link

Diff for pulumi-azure with merge commit 6582e74

@github-actions
Copy link

Diff for pulumi-aws with merge commit 6582e74

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit 6582e74

@github-actions
Copy link

Diff for pulumi-random with merge commit 925b05a

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 925b05a

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 925b05a

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 925b05a

@github-actions
Copy link

Diff for pulumi-azure with merge commit 925b05a

@github-actions
Copy link

Diff for pulumi-aws with merge commit 925b05a

@mikhailshilkov
Copy link
Member

@pgavlin I don't see much changes in https://github.com/pulumi-bot/pulumi-azure-native/compare/c62ea8d3cd6..828691f7e76
Am I jumping the gun or looking at a wrong thing?

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit 925b05a

@pgavlin
Copy link
Member Author

pgavlin commented Nov 18, 2021

@pgavlin I don't see much changes in https://github.com/pulumi-bot/pulumi-azure-native/compare/c62ea8d3cd6..828691f7e76
Am I jumping the gun or looking at a wrong thing?

Ah, I think there's not much of a change there b/c azure-native already disables these types by leaving the GenerateResourceContainerTypes option on the Go language config unset.

@pgavlin
Copy link
Member Author

pgavlin commented Nov 18, 2021

...which makes me even more confident that we should take this change, as it further limits the potential set of breaks.

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 8bc655f

@github-actions
Copy link

Diff for pulumi-random with merge commit 8bc655f

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 8bc655f

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 8bc655f

@github-actions
Copy link

Diff for pulumi-azure with merge commit 8bc655f

@github-actions
Copy link

Diff for pulumi-aws with merge commit 8bc655f

@iwahbe
Copy link
Member

iwahbe commented Nov 18, 2021

@pgavlin, Will this change leave Ptr types for non-resource types? It looks like it does. I'm wondering if there is any "Ptr" handling code we can remove with this change.

@pgavlin
Copy link
Member Author

pgavlin commented Nov 22, 2021

cc @stack72 as well

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 6dcd3ed

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 6dcd3ed

@github-actions
Copy link

Diff for pulumi-azure with merge commit 6dcd3ed

@github-actions
Copy link

Diff for pulumi-aws with merge commit 6dcd3ed

@t0yv0
Copy link
Member

t0yv0 commented Nov 22, 2021

I've filed follow up as #8477

New information: the scope of the remaining problem is thankfully a lot smaller than I thought. It affects URN property only, and is not related to Map/Array helpers. The problem is present on master and remains unchanged with this PR.

@github-actions
Copy link

Diff for pulumi-random with merge commit bd13f6e

@github-actions
Copy link

Diff for pulumi-azuread with merge commit bd13f6e

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit bd13f6e

@github-actions
Copy link

Diff for pulumi-gcp with merge commit bd13f6e

@github-actions
Copy link

Diff for pulumi-azure with merge commit bd13f6e

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit 6dcd3ed

@github-actions
Copy link

Diff for pulumi-aws with merge commit bd13f6e

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit bd13f6e

These changes remove the `Ptr` variants of input/ouptut types for
resources. A `TPtr` input or output is normally generated for `T` if `T`
is present in an `optional(input(T))` or `optional(output(T))` and if
the Go representation for `T` is not nilable. The generation of `Ptr`
variants for resource types breaks the latter rule: the canonical
representation of a resource type named `Foo` is a pointer to a struct
type named `Foo` (i.e. `*Foo`). `Foo` itself is not a resource, as it
does not implement the Go `Resource` interface. Because this
representation already accommodates `nil` to indicate the lack of a
value, we need not generate `FooPtr{Input,Output}` types.

Besides being unnecessary, the implementation of `Ptr` types for
resources was incorrect. Rather than using `**Foo` as their element
type, these types use `*Foo`--identical to the element type used for
the normal input/output types. Furthermore, the generated code for
at least `FooOutput.ToFooPtrOutputWithContext` and `FooPtrOutput.Elem`
was incorrect, making these types virtually unusable in practice.

Finally, these `Ptr` types should never appear on input/output
properties in practice, as the logic we use to generate input and output
type references never generates them for `optional({input,output}(T)).
Instead, it generates references to the standard input/output types.

Though this is _technically_ a breaking change--it changes the set of
exported types for any package that defines resources--I believe that in
practice it will be invisible to users for the reasons stated above.
These types are not usable, and were never referenced.

This is preparatory work for #7943.
@github-actions
Copy link

Diff for pulumi-azuread with merge commit 43a8d0a

@github-actions
Copy link

Diff for pulumi-random with merge commit 43a8d0a

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 43a8d0a

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 43a8d0a

@github-actions
Copy link

Diff for pulumi-azure with merge commit 43a8d0a

@github-actions
Copy link

Diff for pulumi-aws with merge commit 43a8d0a

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit 43a8d0a

@pgavlin pgavlin merged commit 52d01bb into master Nov 23, 2021
@pulumi-bot pulumi-bot deleted the pgavlin/resourcePtr branch November 23, 2021 18:25
@emiliza emiliza added this to the 0.65 milestone Nov 24, 2021
@emiliza emiliza added capacity/quality size/M Estimated effort to complete (up to 5 days). labels Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Estimated effort to complete (up to 5 days).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants