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.

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: adds unexpected renamed imports #29520

Closed
rsc opened this issue Jan 3, 2019 · 2 comments
Closed

x/tools/cmd/goimports: adds unexpected renamed imports #29520

rsc opened this issue Jan 3, 2019 · 2 comments
Assignees
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Jan 3, 2019

Three mysterious problems here (running in GOPATH mode):

$ go get -u golang.org/x/tools/cmd/goimports
$ echo 'package p; var _ = f.hash' | goimports
package p

import f "golang.org/x/exp/winfsnotify"

var _ = f.hash
$ 
  1. goimports should not be trying to find an import for f.hash at all.
    No import could possibly provide a lower-case name.
    goimports should not be wasting time scanning packages at all.

  2. Even if goimports were to look for a package,
    why on earth did it decide to import golang.org/x/exp/winfsnotify to f?
    (That package is package winfsnotify.)

  3. Even if goimports were to look for a package and rename it to f,
    why did it pick golang.org/x/exp/winfsnotify? That package does not
    contain the string hash anywhere in its sources.

Like I said, mysterious (and really very annoying).

/cc @heschik @ianthehat @bradfitz

@rsc rsc added this to the Unreleased milestone Jan 3, 2019
@bradfitz bradfitz added the NeedsFix label Jan 3, 2019
@heschik
Copy link
Contributor

@heschik heschik commented Jan 3, 2019

Pretty funny bug, I couldn't quite believe it so I had to look. I think this is partly new in my rewritten code and partly old.

The example hits two corner cases in the goimports logic. First, the code that collects missing references knows that lower-case identifiers can't be exported by a package, so it doesn't add them as things to look for -- but it does that after it has registered that it's looking for a package named f. Now it's looking for f, and it doesn't have to export anything.

So then it goes looking for 'f'. It finds a bunch of directories with the letter f in their name, of course, and then tries to load their exports. As it does that it tries to check the package statement of each file, and if it finds one that doesn't match, rejects the package as mismatched. But winfsnotify is for Windows, and all of its files are build tagged as such. So it never finds a package statement to check. Normally that wouldn't matter since it also didn't find anything exported. Here, though, it's good enough.

Both should be easy fixes.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 3, 2019

Change https://golang.org/cl/156178 mentions this issue: imports: don't look for, or find, empty packages

@golang golang locked and limited conversation to collaborators Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.