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: 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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rsc
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. label Jan 3, 2019
@heschi
Copy link
Contributor

heschi 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

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
@rsc rsc unassigned heschi Jun 23, 2022
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 @rsc @gopherbot @heschi