-
Notifications
You must be signed in to change notification settings - Fork 21
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
[program-gen] Fix emitted import statements in generated programs such that imports with the same symbol are fully qualified #1573
Conversation
… same symbol are fully qualified
pkg/codegen/java/gen_program.go
Outdated
for _, importDefA := range importStatements { | ||
for _, importDefB := range importStatements { | ||
if importMember(importDefA) == importMember(importDefB) && importDefA != importDefB { | ||
importsWithDuplicateName.Add(importDefA) | ||
importsWithDuplicateName.Add(importDefB) | ||
} | ||
} | ||
} |
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.
I'm kind of not a fan of this n^2 approach when we can use a set of importMembers.
what if it is a super large program with many files each with tons of imports? It's premature optimization but seems reasonable to turn into a set check
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.
I've optimized the lookup ✅ can you have another look?
pkg/codegen/java/gen_program.go
Outdated
} | ||
|
||
g.importStatements = importStatements |
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.
this makes me think the name is misleading. They are not import statements they are dependencies because genImport is only called for non conflicting symbols
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.
Renamed to emittedImportTypeSymbols
✅
pkg/codegen/java/gen_program.go
Outdated
for _, importStatement := range g.importStatements { | ||
// if there is an import statement that ends with the same member name | ||
// return the fully qualified resource name | ||
if strings.HasSuffix(importStatement, "."+member) && pulumiImport(pkg, module, member) != importStatement { | ||
return pulumiImport(pkg, module, member) | ||
} | ||
} |
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.
instead of saving all import statements wouldn't it also be better to just save a set of all types that need to be fully qualified and just check this isn't in that set?
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.
Done ✅
Description
Fix emitted import statements in generated programs such that imports with the same symbol are fully qualified
if we have two imports such as the following:
com.pulumi.my_package.ResourceType
com.pulumi.your_package.ResourceType
then we shouldn't generate import statements for them instead, we use the fully qualified name at the usage site.
Unskips
l2-failed-create-continue-on-error
Fixes #1558