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

Fix 8110 #8116

Merged
merged 14 commits into from
Oct 4, 2021
Merged

Fix 8110 #8116

merged 14 commits into from
Oct 4, 2021

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Oct 1, 2021

Description

Fix Node codegen to not create incorrectly parsed type references for unions and optionals.

Fixes #8110

Based on discussion, some effort was put into minimally inserting parens, rather than inserting them everywhere.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-kubernetes with merge commit 17a7d60

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-kubernetes with merge commit f419abd

@t0yv0 t0yv0 requested a review from a team October 1, 2021 18:55
@t0yv0 t0yv0 marked this pull request as ready for review October 1, 2021 18:55
@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-kubernetes with merge commit 7e61c54

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-kubernetes with merge commit 58dec95

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-random with merge commit 34245e7

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-azuread with merge commit 34245e7

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-kubernetes with merge commit 34245e7

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.

This feels like an annoying question, but how hard would it be to only generate the parens when they're necessary? It'd minimize the diffs and would keep the types a little less noisy.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 1, 2021

Not annoying at all I was asking the same thing.

It would also make diff checking easier.

It's some work mostly to find TypeScript grammar precedence rules. If someone remembers how those work that could be a great help.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 1, 2021

Not seeing BNF in https://www.typescriptlang.org/docs/handbook/2/everyday-types.html

But at first glance, [] has higher precedence than | and we don't have other "operators" at play so I can just try to handle those.

@github-actions
Copy link

github-actions bot commented Oct 1, 2021

Diff for pulumi-azure-native with merge commit 34245e7

@github-actions
Copy link

github-actions bot commented Oct 2, 2021

Diff for pulumi-azuread with merge commit dd81529

@github-actions
Copy link

github-actions bot commented Oct 2, 2021

Diff for pulumi-random with merge commit dd81529

@github-actions
Copy link

github-actions bot commented Oct 2, 2021

Diff for pulumi-kubernetes with merge commit dd81529

@github-actions
Copy link

github-actions bot commented Oct 2, 2021

Diff for pulumi-gcp with merge commit dd81529

"pgregory.net/rapid"
)

func TestParenInsert(t *testing.T) {
Copy link
Member Author

@t0yv0 t0yv0 Oct 2, 2021

Choose a reason for hiding this comment

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

Admittedly verbose but foolproof. Works on a simplified grammar but can be easily extended if we run into more grammar peculiarities. This checks that unparsing and auto-parenthesizing the AST into tokens gets parsed back into the same AST.

})
}

func astGenerator() *rapid.Generator {
Copy link
Member Author

Choose a reason for hiding this comment

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

Lib remark: shrinking trees seems not entirely satisfactory, I'd like to compare to what Haskell has and if necessary send a patch upstream. Once a bad tree T has been found, I'd like it to be minimized heavily by pruning branches that do not matter for the test before presenting to the user.

@github-actions
Copy link

github-actions bot commented Oct 2, 2021

Diff for pulumi-azure with merge commit dd81529

@@ -0,0 +1,64 @@
// *** WARNING: this file was generated by test. ***
// *** Do not edit by hand unless you're certain you know what you are doing! ***
Copy link
Member Author

Choose a reason for hiding this comment

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

Non-node code noisy, relatively low value, we should probably support skipping codegen by lang.

@t0yv0 t0yv0 requested review from pgavlin and komalali October 2, 2021 01:36
@github-actions
Copy link

github-actions bot commented Oct 2, 2021

Diff for pulumi-aws with merge commit dd81529

@github-actions
Copy link

github-actions bot commented Oct 2, 2021

Diff for pulumi-azure-native with merge commit dd81529

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-random with merge commit 3a908b3

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-azuread with merge commit 3a908b3

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-kubernetes with merge commit 3a908b3

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-gcp with merge commit 3a908b3

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-aws with merge commit 3a908b3

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-azure with merge commit 3a908b3


type typeScriptTypeUnparser struct{}

func (u *typeScriptTypeUnparser) unparse(ast TypeAst) []typeToken {
Copy link
Member

Choose a reason for hiding this comment

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

are these methods just for scoping? if so, these can take a value receiver:

Suggested change
func (u *typeScriptTypeUnparser) unparse(ast TypeAst) []typeToken {
func (u typeScriptTypeUnparser) unparse(ast TypeAst) []typeToken {

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-azure-native with merge commit 3a908b3

@t0yv0 t0yv0 merged commit 1ee85d8 into master Oct 4, 2021
@pulumi-bot pulumi-bot deleted the t0yv0/fix-8110 branch October 4, 2021 21:26
@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-azuread with merge commit ede002f

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-random with merge commit ede002f

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-kubernetes with merge commit ede002f

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-gcp with merge commit ede002f

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-aws with merge commit ede002f

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-azure with merge commit ede002f

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

Diff for pulumi-azure-native with merge commit ede002f

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.

Node codegen emits incorrectly nested enum-array type literals
2 participants