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

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

Comments

Projects
None yet
4 participants
@dmitshur
Member

dmitshur commented Jul 18, 2016

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

This comment has been minimized.

Member

bradfitz commented Jul 18, 2016

Interesting case.

@gopherbot

This comment has been minimized.

gopherbot commented Feb 15, 2017

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

@dmitshur

This comment has been minimized.

Member

dmitshur commented Oct 30, 2018

/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).

@heschik

This comment has been minimized.

Contributor

heschik 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

This comment has been minimized.

Member

dmitshur commented Oct 31, 2018

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 assigned dmitshur and unassigned bradfitz Oct 31, 2018

@dmitshur dmitshur changed the title from x/tools/cmd/goimports: Doesn't add package if already imported for side effects (underscore import). to x/tools/cmd/goimports: doesn't add package if already imported for side effects (underscore import) Oct 31, 2018

@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels Nov 2, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Nov 5, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment