programgen(go): Handle conflicting names in imported packages #13289

abhinav · 2023-06-26T21:04:35Z

This fixes how programgen generates import statements
to handle conflicting imports when two imported packages
have the same name, e.g.

github.com/pulumi/pulumi-aws/sdk/v5/go/aws/ecs
github.com/pulumi/pulumi-awsx/sdk/go/awsx/ecs

To do this, we add a fileImporter type that tracks these imports.
It prefers unnamed imports for packages unless one of the following is
true:

the name of the package has already been used by another import
the name of the package does not match the last component
of the import path (e.g., example.com/foo-go with package foo).

If the name has already been used by another import,
it attempts the following in-order:

Combine the last two path components of the import path
into an identifier and use that if available.
e.g., awsxs3 from sdk/go/awsx/s3.
Append a number to the package name and increment it
until an unused name is found.
e.g. ecs2, ecs3, and so on.

There's a change in how this information is tracked as well.
Previously, this was a pull approach: various calls returned
programImports objects which all got merged together.

This change switches to a push approach:
as code is generated and imports are requested,
they're submitted to the fileImporter which keeps track of them
until the next Reset() call.
The above also has a nice side effect of dropping a parameter.

Another change worth explicitly calling out:
Previously, getModOrAlias partially duplicated some of the logic
implemented in getPulumiImport, and used mod, originalMod
in a non-obvious way.
This generated incorrect imports like the following
(note the two /aws at the end):

github.com/pulumi/pulumi-aws/sdk/v5/go/aws/aws

This change replicates more of the logic of getPulumiImport
(now called addPulumiImport) into this function,
and addresses the discrepancy in codegen caused by mod/originalMod.
The result leaves most existing code unchanged,
except in a couple existing cases where the resulting changes make sense
given the logic for named imports outlined above.

Resolves #11176

abhinav · 2023-06-26T21:04:48Z

Current dependencies on/for this PR:

master
- PR programgen(go): Handle conflicting names in imported packages #13289 👈

This comment was auto-generated by Graphite.

pulumi-bot · 2023-06-26T21:05:41Z

Changelog

[uncommitted] (2023-07-25)

Bug Fixes

[programgen/go] Fix conflicting imports generated when two imported packages have the same name.
#13289

abhinav · 2023-06-26T21:06:18Z

pkg/codegen/testing/test/testdata/modpath-pp/go/modpath.go

@@ -1,13 +1,13 @@
 package main

 import (
-	"git.example.org/thirdparty/sdk/go/pkg"
+	other "git.example.org/thirdparty/sdk/go/pkg"


note: basename of import path doesn't match package name, so this makes sense

abhinav · 2023-06-26T21:06:40Z

...n/testing/test/testdata/synthetic-resource-properties-pp/go/synthetic-resource-properties.go

@@ -1,7 +1,7 @@
 package main

 import (
-	resourceProperties "git.example.org/pulumi-synthetic/resourceProperties"
+	"git.example.org/pulumi-synthetic/resourceProperties"


base name matches package name so named import not necessary

abhinav · 2023-06-26T21:32:39Z

bors try

bors · 2023-06-26T22:14:40Z

try

Build failed:

bors-ok

abhinav · 2023-06-26T22:16:02Z

try

Build failed:

bors-ok

Build is green. Only the merge failed (for obvious reasons).

pkg/codegen/go/gen_program.go

Frassle · 2023-07-12T07:55:04Z

pkg/codegen/go/gen_program.go

+		candidate := strings.Join(parts[len(parts)-2:], "_")
+
+		// Just in case, drop common non-identifier characters.
+		// e.g., "foo-go" -> "foogo"


Do we not need to worry about a path like "go.dev/pkg" here? parts would be 2 ["go.dev", "pkg"] but "." isn't valid in the import name.

Yeah, I think this logic is more fragile than I intended. I'll have it drop any non-identifier symbols and add some more unit tests.

Frassle · 2023-07-12T07:56:24Z

pkg/codegen/go/gen_program.go

+	}
+
+	// The name is taken. We need a unique unused alias.
+	// We prefer a simple "$mod_$name" (e.g. "aws_s3", "awsx_s3") for


I think the underscore is probably better here, but I'm cognisant that Go recommends no underscores in package names and I wonder if we should be trying to maintain that here? "awss3"

Yeah, that sounds better, especially because this generated code is intended to be read and modified by users.

abhinav · 2023-07-12T21:16:48Z

Updated:

Prefer "$mod$name" over "$name$num"
Guarantee that the preferred name is a valid Go package name

justinvp

LGTM

Adds a minimal reproduction of the duplicate import issues reported in #11176.

This fixes how programgen generates import statements to handle conflicting imports when two imported packages have the same name, e.g. github.com/pulumi/pulumi-aws/sdk/v5/go/aws/ecs github.com/pulumi/pulumi-awsx/sdk/go/awsx/ecs To do this, we add a fileImporter type that tracks these imports. It prefers unnamed imports for packages unless one of the following is true: - the name of the package has already been used by another import - the name of the package does not match the last component of the import path (e.g., `example.com/foo-go` with `package foo`). If the name has already been used by another import, it attempts the following in-order: - Combine the last two path components of the import path into an identifier and use that if available. e.g., `awsxs3` from `sdk/go/awsx/s3`. - Append a number to the package name and increment it until an unused name is found. e.g. `ecs2`, `ecs3`, and so on. There's a change in how this information is tracked as well. Previously, this was a pull approach: various calls returned programImports objects which all got merged together. This change switches to a push approach: as code is generated and imports are requested, they're submitted to the fileImporter which keeps track of them until the next `Reset()` call. The above also has a nice side effect of dropping a parameter. Another change worth explicitly calling out: Previously, getModOrAlias partially duplicated some of the logic implemented in getPulumiImport, and used `mod`, `originalMod` in a non-obvious way. This generated incorrect imports like the following (note the two `/aws` at the end): github.com/pulumi/pulumi-aws/sdk/v5/go/aws/aws This change replicates more of the logic of getPulumiImport (now called addPulumiImport) into this function, and addresses the discrepancy in codegen caused by `mod`/`originalMod`. The result leaves most existing code unchanged, except in a couple existing cases where the resulting changes make sense given the logic for named imports outlined above. Resolves #11176

abhinav · 2023-07-25T20:51:04Z

bors merge

bors · 2023-07-25T21:31:41Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

bors-ok

abhinav commented Jun 26, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Jun 26, 2023

Try #13289:

9f8500e

abhinav requested a review from a team June 26, 2023 21:44

abhinav marked this pull request as ready for review June 26, 2023 22:15

abhinav force-pushed the abhinav/codegen-go-conflicting-imports branch from 69b7263 to 200bb9f Compare June 30, 2023 04:30

Frassle reviewed Jul 11, 2023

View reviewed changes

pkg/codegen/go/gen_program.go Outdated Show resolved Hide resolved

abhinav force-pushed the abhinav/codegen-go-conflicting-imports branch from 200bb9f to 0492ff9 Compare July 11, 2023 23:56

Frassle reviewed Jul 12, 2023

View reviewed changes

abhinav force-pushed the abhinav/codegen-go-conflicting-imports branch from 0492ff9 to cb91c5b Compare July 12, 2023 21:15

justinvp approved these changes Jul 25, 2023

View reviewed changes

abhinav added 2 commits July 25, 2023 12:49

test(codgen): Reproduce #11176

bb18a19

Adds a minimal reproduction of the duplicate import issues reported in #11176.

abhinav force-pushed the abhinav/codegen-go-conflicting-imports branch from cb91c5b to a24ad18 Compare July 25, 2023 19:49

bors bot merged commit e48af89 into master Jul 25, 2023
57 checks passed

bors bot deleted the abhinav/codegen-go-conflicting-imports branch July 25, 2023 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

programgen(go): Handle conflicting names in imported packages #13289

programgen(go): Handle conflicting names in imported packages #13289

abhinav commented Jun 26, 2023 •

edited

abhinav commented Jun 26, 2023

pulumi-bot commented Jun 26, 2023 •

edited

abhinav Jun 26, 2023

abhinav Jun 26, 2023

abhinav commented Jun 26, 2023

bors bot commented Jun 26, 2023

abhinav commented Jun 26, 2023

try

Frassle Jul 12, 2023

abhinav Jul 12, 2023

Frassle Jul 12, 2023

abhinav Jul 12, 2023

abhinav commented Jul 12, 2023

justinvp left a comment

abhinav commented Jul 25, 2023

bors bot commented Jul 25, 2023

programgen(go): Handle conflicting names in imported packages #13289

programgen(go): Handle conflicting names in imported packages #13289

Conversation

abhinav commented Jun 26, 2023 • edited

abhinav commented Jun 26, 2023

pulumi-bot commented Jun 26, 2023 • edited

Changelog

[uncommitted] (2023-07-25)

Bug Fixes

abhinav Jun 26, 2023

Choose a reason for hiding this comment

abhinav Jun 26, 2023

Choose a reason for hiding this comment

abhinav commented Jun 26, 2023

bors bot commented Jun 26, 2023

try

abhinav commented Jun 26, 2023

try

Frassle Jul 12, 2023

Choose a reason for hiding this comment

abhinav Jul 12, 2023

Choose a reason for hiding this comment

Frassle Jul 12, 2023

Choose a reason for hiding this comment

abhinav Jul 12, 2023

Choose a reason for hiding this comment

abhinav commented Jul 12, 2023

justinvp left a comment

Choose a reason for hiding this comment

abhinav commented Jul 25, 2023

bors bot commented Jul 25, 2023

abhinav commented Jun 26, 2023 •

edited

pulumi-bot commented Jun 26, 2023 •

edited