Skip to content

x/tools/cmd/bundle: make import resolution easier #57088

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.

Sign up for GitHub

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

Jump to bottom
Open
switchupcb opened this issue Dec 5, 2022 · 5 comments
Open

x/tools/cmd/bundle: make import resolution easier #57088

switchupcb opened this issue Dec 5, 2022 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@switchupcb
Copy link

Step 1: https://github.com/golang/proposal#the-proposal-process
The proposal author creates a brief issue describing the proposal.
Note: There is no need for a design document at this point.
Note: A non-proposal issue can be turned into a proposal by simply adding the proposal label.
Note: Language changes should follow a separate template

Its hard to resolve import conflicts with https://pkg.go.dev/golang.org/x/tools/cmd/bundle.

https://cs.opensource.google/go/x/tools/+/refs/tags/v0.3.0:cmd/bundle/main.go;l=256
@adonovan @shurcooL

If the "shadow" thing can't be solved, maybe you can do any of the following?

  • default to alias
  • allow user to replace an import with "" (removing it) using -import old=""
  • allow user to replace an import with an alias -import old="alias import"
    • but don't reuse the same import twice in output.
  • allow user to define file that imports come from
@gopherbot gopherbot added this to the Proposal milestone Dec 5, 2022
@adonovan
Copy link
Member

adonovan commented Dec 5, 2022

At a quick glance, it seems like it shouldn't be hard to scan the set of imports and package-level names, compute their intersection, and rename the necessary subset of imports to avoid the conflicts.

Are you comfortable sending a CL to fix it? I'd be happy to review and approve it.

@switchupcb
Copy link
Author

switchupcb commented Dec 5, 2022 •
edited
Loading

@adonovan I can do it in late December or January. By the way, I don't work at Google if that changes anything.

I parsed imports in copygen too. Will I have to use one-letter naming scheme?

@adonovan
Copy link
Member

adonovan commented Dec 5, 2022

@adonovan I can do it in late December or January.

Great!

By the way, I don't work at Google if that changes anything.

Non-googlers must go through the CLA (copyright) process, but it's very quick. Other than that, it doesn't change anything.

I parsed imports in copygen too. Will I have to use one-letter naming scheme?

No, you just need to add a numeric suffix to each conflicting import.
This loop computes the set of imports: https://cs.opensource.google/go/x/tools/+/refs/tags/v0.3.0:cmd/bundle/main.go;l=271-296
And this one renames all the package-level decls: https://cs.opensource.google/go/x/tools/+/refs/tags/v0.3.0:cmd/bundle/main.go;l=223-227

I think you need to detect when an import name is a member of scope.Names() by calling scope.Lookup, and then adjust the import name by adding a numeric suffix and record a map entry from imported *types.PkgName to new name. Then fix up all references to the PkgName by adding an extra case here
https://cs.opensource.google/go/x/tools/+/refs/tags/v0.3.0:cmd/bundle/main.go;l=286 to consult the mapping you just built.

@seankhliao seankhliao changed the title proposal: x/tools/bundle: make import resolution easier proposal: x/tools/cmd/bundle: make import resolution easier Dec 5, 2022
@ianlancetaylor
Copy link
Member

I think this is a bug fix. I don't see any reason why it has to go through the proposal process, so taking it out.

@ianlancetaylor ianlancetaylor changed the title proposal: x/tools/cmd/bundle: make import resolution easier x/tools/cmd/bundle: make import resolution easier Dec 7, 2022
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Dec 7, 2022
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 7, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unreleased Dec 7, 2022
@meling
Copy link

meling commented Dec 25, 2022

Looks like this could be (partially) related to #37689.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@meling @ianlancetaylor @adonovan @gopherbot @switchupcb