-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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] Rewrite cyclic types. #8049
Conversation
0337ded
to
59a9ec1
Compare
Diff for pulumi-random with merge commit e74dc64 |
Diff for pulumi-azuread with merge commit e74dc64 |
Diff for pulumi-random with merge commit a50a4c0 |
Diff for pulumi-azuread with merge commit a50a4c0 |
Diff for pulumi-kubernetes with merge commit e74dc64 |
Diff for pulumi-gcp with merge commit e74dc64 |
Diff for pulumi-kubernetes with merge commit a50a4c0 |
A note on this: it is a bit of a heavy hammer, and it's really solving the problem at the wrong layer. Unfortunately, we don't have the layer this should really be solved at in the Go code generator, at least not in any real sense. Given its needs, the Go code generator should probably look like this: As it stands, we're missing the "Go type graph" portion of that pipeline. If we had that such a representation, this rewrite would likely be part of the lowering step from In any case, the most unfortunate side effect of these changes is that Go SDKs may represent required but cyclic properties as optional. |
Diff for pulumi-gcp with merge commit a50a4c0 |
Diff for pulumi-aws with merge commit e74dc64 |
Diff for pulumi-aws with merge commit a50a4c0 |
Diff for pulumi-azure with merge commit e74dc64 |
Diff for pulumi-azure with merge commit a50a4c0 |
Diff for pulumi-azure-native with merge commit e74dc64 |
Diff for pulumi-azure-native with merge commit a50a4c0 |
} | ||
|
||
type IndirectCycleT struct { | ||
Foo3 *IndirectCycleS `pulumi:"foo3"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both IndirectCycleS and IndirectCycleT properties get '*' treatment, so this is symmetric/deterministic, good!
This looks well motivated. Coupe of notes.
|
This certainly resonates and this is why I feel we need to think about unexpected potential interactions with other features a little harder, to be on the safe side here. |
No worries--it's more important that this lands reliably and with robust review than that it lands now.
I think that looking at the changed files and trying to build those packages that saw changes should be sufficient.
In some aspects this is non-breaking:
Importantly, though, existing outputs may break if they are passed to |
|
||
// If we've found a cycle, mark each property involved in the cycle as optional. | ||
// | ||
// NOTE: this overestimates the set of properties that must be marked as optional. For example, in case (2) above, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// only one of T.Invalid or S.Invalid needs to be marked as optional in order to break the cycle. However, choosing | ||
// a minimal set of properties that is also deterministic and resilient to changes in visit order is difficult and | ||
// seems to add little value. | ||
for _, p := range cycle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to log any time we do this cycle rewrite? E.g. which type and field? I'm not sure how this surfaces to codegen users but that sounds valuable to see what's happening, as a user of codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging might be useful, yes, but we don't really have a logging framework for the code generators, and I'd prefer not to just log to stdout/stderr. File this one under "enhancements" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, I read through the recursion.
Test failure unrelated (merge master should fix).
Reassuring zero diff on pulumi-aws.
Expect to have very low blast radius since few cycles are identified in existing schmas. Logging rewrites might help "see" the blast radius.
I may try this out on pulumi-gcp later on today just to get in the habit of checking codegen PRs against affected providers.
When computing the type name for a field of an object type, we must ensure that we do not generate invalid recursive struct types. A struct type T contains invalid recursion if the closure of its fields and its struct-typed fields' fields includes a field of type T. A few examples: Directly invalid: type T struct { Invalid T } Indirectly invalid: type T struct { Invalid S } type S struct { Invalid T } In order to avoid generating invalid struct types, we replace all references to types involved in a cyclical definition with *T. The examples above therefore become: (1) type T struct { Valid *T } (2) type T struct { Valid *S } type S struct { Valid *T } We do this using a rewriter that turns all fields involved in reference cycles into optional fields. These changes also include two enhancements to the SDK codegen test driver in the interest of making debugging more convenient. 1. If the -sdk.run flag is passed, the driver will only run the test with the given directory name 2. If the -sdk.no-checks flag is passed, the driver will not run post-generation checks This allows the use of e.g. `dlv` to debug a particular case without editing the driver itself, for example: $ go test -c -gcflags="all=-N -l" . $ dlv exec ./go.test -- -test.run TestGeneratePackage -sdk.run=cyclic-types -sdk.no-checks
Diff for pulumi-azure-native with merge commit b532476 |
Diff for pulumi-azure-native with merge commit 37db7fc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but you'll have to regen the codegen tests (if you've already pulled in changes in master)
Diff for pulumi-random with merge commit 5147b78 |
Diff for pulumi-azuread with merge commit 5147b78 |
Diff for pulumi-kubernetes with merge commit 5147b78 |
Diff for pulumi-gcp with merge commit 5147b78 |
Diff for pulumi-aws with merge commit 5147b78 |
Diff for pulumi-azure with merge commit 5147b78 |
Diff for pulumi-azure-native with merge commit 5147b78 |
Diff for pulumi-azuread with merge commit 1a50f98 |
Diff for pulumi-random with merge commit 1a50f98 |
Diff for pulumi-kubernetes with merge commit 1a50f98 |
Diff for pulumi-gcp with merge commit 1a50f98 |
Diff for pulumi-aws with merge commit 1a50f98 |
Diff for pulumi-azure with merge commit 1a50f98 |
Diff for pulumi-azuread with merge commit 8d37d61 |
Diff for pulumi-random with merge commit 8d37d61 |
Diff for pulumi-kubernetes with merge commit 8d37d61 |
Diff for pulumi-gcp with merge commit 8d37d61 |
Diff for pulumi-azure-native with merge commit 1a50f98 |
Diff for pulumi-aws with merge commit 8d37d61 |
Diff for pulumi-azure with merge commit 8d37d61 |
Diff for pulumi-azure-native with merge commit 8d37d61 |
When computing the type name for a field of an object type, we must
ensure that we do not generate invalid recursive struct types. A struct
type T contains invalid recursion if the closure of its fields and its
struct-typed fields' fields includes a field of type T. A few examples:
Directly invalid:
Indirectly invalid:
In order to avoid generating invalid struct types, we replace all
references to types involved in a cyclical definition with *T. The
examples above therefore become:
We do this using a rewriter that turns all fields involved in reference
cycles into optional fields.
These changes also include two enhancements to the SDK codegen test
driver in the interest of making debugging more convenient.
with the given directory name
post-generation checks
This allows the use of e.g.
dlv
to debug a particular case withoutediting the driver itself, for example:
Fixes #7622.