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

go program gen: lift expressions where prompt optionals are required #4875

Merged

Conversation

EvanBoyle
Copy link
Contributor

Fixes #4768

Prompt optional values for primitives must be passed as pointers. This change adds a lowering step that hoists the concrete value into the outer scope where appropriate (currently only invoke args) and replaces the original reference with a scope traversal (&optN).

This changeset is based on another open PR in an effort to keep the changes small and easy to review: #4858

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 689b1e5

@github-actions
Copy link

Diff for pulumi-random with merge commit 689b1e5

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 689b1e5

@github-actions
Copy link

Diff for pulumi-random with merge commit 10c8074

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 10c8074

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 10c8074

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 689b1e5

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 10c8074

@github-actions
Copy link

Diff for pulumi-azure with merge commit 689b1e5

@github-actions
Copy link

Diff for pulumi-aws with merge commit 689b1e5

@github-actions
Copy link

Diff for pulumi-aws with merge commit 10c8074

@github-actions
Copy link

Diff for pulumi-azure with merge commit 10c8074

@@ -25,6 +25,7 @@ func main() {
if err != nil {
return err
}
opt0 := true
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this PR, but it might be nice to have helper methods to generate references for optional types without requiring a temporary variable. I believe Kubernetes client-go has something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm in this camp as well. My understanding was that it is more idiomatic in go to spell stuff like this out even if repetitive. This was the general feedback I got when suggesting we add helpers to the go SDK to translate prompt arrays to inputty arrays. @pgavlin can comment more here.

Copy link
Member

Choose a reason for hiding this comment

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

It is less idiomatic to export simple, type-specific helper methods. I'm ambivalent about adding unexported helper methods to the generated code.

pkg/codegen/go/gen_program_optionals.go Show resolved Hide resolved
@EvanBoyle EvanBoyle merged commit b302dbc into evan/goProgramAllApplyRewrite Jun 23, 2020
@pulumi-bot pulumi-bot deleted the evan/goProgramGenPromptOptionals branch June 23, 2020 17:59
@github-actions
Copy link

Diff for pulumi-random with merge commit 2cb2c9e

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 2cb2c9e

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 2cb2c9e

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 2cb2c9e

@github-actions
Copy link

Diff for pulumi-azure with merge commit 2cb2c9e

@github-actions
Copy link

Diff for pulumi-aws with merge commit 2cb2c9e

Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

We really need to figure out how to assign better names to these temporaries (also something we need to do for the Python quote temps). This isn't something we need to do immediately, but it's getting more pressing as we generate more of these spills.

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