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.

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

x/tools/cmd/goimports: doesn't add package if already imported for side effects (underscore import) #16411

Closed
dmitshur opened this issue Jul 18, 2016 · 6 comments
Closed

x/tools/cmd/goimports: doesn't add package if already imported for side effects (underscore import) #16411

dmitshur opened this issue Jul 18, 2016 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

I'm not sure if this is a bug or a feature request or known accepted behavior in goimports - I'll let @bradfitz decide, but consider the following unfinished Go program:

package main

import (
    "fmt"
    "image"
    _ "image/png"
    "os"
)

func main() {
    var img image.Image // some image
    err := png.Encode(os.Stdout, img)
    fmt.Println(err)
}

One would normally expect the png.Encode usage to cause goimports to add the image/png import.

However, it does nothing. It's because that package is already imported for side effects.

This was slightly unexpected. But I'm not sure what ideal behavior would be.

  • Should it replace the underscore import with normal import? Probably not.
  • Should it add a second entry for image/png, keeping the underscore import alone? Maybe?
  • What about renamed packages, if there's a pathpkg "path" import and someone writes path.Join, should goimports add a second import for path package? Or should underscore imports be treated differently than other renamed packages?

I'm only reporting this because I ran into it and it didn't seem to be reported already.
@bradfitz bradfitz self-assigned this Jul 18, 2016
@bradfitz bradfitz added this to the Unreleased milestone Jul 18, 2016
@bradfitz
Copy link
Contributor

Interesting case.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/37090 mentions this issue.

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 30, 2018 •
edited
Loading

/cc @heschik

From our conversation, it sounded like behavior 2 seemed like the most reasonable option.

I don't think CL 37090 is the right solution: we should not be modifying the low-level astutil helpers to ignore _ imports, because then it's not possible to manipulate such imports (not to mention it'd be a breaking API change).

@dmitshur dmitshur added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 30, 2018
@heschi
Copy link
Contributor

heschi commented Oct 30, 2018

Either CL 37090 is the right solution or goimports should not use the helpers, because I think @davidrjenni correctly identified the problem. goimports gets all the way to adding the import, calls astutil.AddImport to do it, and then nothing happens because AddImport thinks it's already there.

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 31, 2018 •
edited
Loading

Ok. If we're going to consider changing the astutil.{Add,Delete}{,Named}Import behavior (or adding new helpers to it), what would make sense to me is the behavior where those functions only operate on the exact {optional name, import path} pairs.

Edit: Delete{,Named}Import is already doing that, only Add{,Named}Import isn't. Filed #28605 for that.

AddImport and DeleteImport (and AddNamedImport/DeleteNamedImport with empty name parameter) should only operate on unnamed imports, and never touch named ones. AddNamedImport and DeleteNamedImport should only add/delete the exact named import, if it exists. The blank identifier import is not a special case. For example:

Example Usage
import ()

AddNamedImport(..., "", "path") -> added: true

import (
    "path"
)

AddNamedImport(..., "pathpkg", "path") -> added: true

import (
    "path"
    pathpkg "path"
)

AddNamedImport(..., "_", "path") -> added: true

import (
    "path"
    pathpkg "path"
    _ "path"
)

DeleteNamedImport(..., "", "path") -> deleted: true

import (
    pathpkg "path"
    _ "path"
)

// calling it again has no effect, the unnamed "path" import is already gone
DeleteNamedImport(..., "", "path") -> deleted: false

import (
    pathpkg "path"
    _ "path"
)

DeleteNamedImport(..., "pathpkg", "path") -> deleted: true

import (
    _ "path"
)

DeleteNamedImport(..., "_", "path") -> deleted: true

import ()

That way, it's possible to use these helpers to manipulate the AST with full precision (which is what I would've expected from them in the first place).

Does that sound reasonable?

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 31, 2018
@dmitshur dmitshur assigned dmitshur and unassigned bradfitz Oct 31, 2018
@dmitshur dmitshur changed the title x/tools/cmd/goimports: Doesn't add package if already imported for side effects (underscore import). x/tools/cmd/goimports: doesn't add package if already imported for side effects (underscore import) Oct 31, 2018
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 2, 2018
@dmitshur dmitshur mentioned this issue Nov 5, 2018
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/147448 mentions this issue: go/ast/astutil: allow AddNamedImport to add imports with different names

@golang golang locked and limited conversation to collaborators Nov 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @dmitshur @gopherbot @heschi