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: goimports still fails to distinguish package from global variable #23206

Closed
wjessop opened this issue Dec 21, 2017 · 8 comments
Milestone

Comments

@wjessop
Copy link

@wjessop wjessop commented Dec 21, 2017

This is basically this issue. It looks like a fix was started, but wether it never got merged, or this is a regression I don't know, but It seems to have cropped up on a project I'm working on where it wasn't an issue before.

What version of Go are you using (go version)?

go version go1.10beta1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/will/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/will/www/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.10beta1/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.10beta1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/r7/kjzbwmx533b20hcf1_s9kc9c0000gn/T/go-build668942595=/tmp/go-build -gno-record-gcc-switches -fno-common"
@thepudds
Copy link

@thepudds thepudds commented Dec 21, 2017

@wjessop Sorry for the basic question, but just to help with triage, are you sure you're running the latest version of goimports, but maybe more importantly, do you have a simple reproducer?

If it's not easy to extract a simple standalone example that demonstrates this problem from your actual program, you could consider adapting one examples from the comments in #7463, or you could extract an example from one of the tests such as:
https://github.com/golang/tools/blob/master/imports/fix_test.go#L1569

@wjessop
Copy link
Author

@wjessop wjessop commented Dec 21, 2017

Sure thing! I just ran go get -u golang.org/x/tools/cmd/goimports so should be on the latest version (ae8cc59).

To replicate

Create a dir:

$ mkdir imports
$ cd imports/

Add main.go:

$ cat > main.go
package main

func main() {
	createLogger(true)
	log.Debug("foo")
}

Add logger.go:

$ cat > logger.go
package main

import (
	logging "github.com/op/go-logging"

	"os"
)

var (
	log       = logging.MustGetLogger("myprog")
	logFormat = logging.MustStringFormatter(`%{color}%{time:15:04:05.000} %{shortfunc} ▶ %{level:.5s} %{id:03x}%{color:reset} %{message}`)
)

func createLogger(isDebug bool) {
	logBackend := logging.NewLogBackend(os.Stderr, "", 0)
	logFormatter := logging.NewBackendFormatter(logBackend, logFormat)
	logLeveled := logging.AddModuleLevel(logFormatter)
	logging.SetBackend(logLeveled)

	if isDebug {
		logLeveled.SetLevel(logging.DEBUG, "")
		log.Debugf("Setting log level to DEBUG")
	} else {
		logLeveled.SetLevel(logging.INFO, "")
	}
}

See the program runs:

$ go run main.go logger.go 
04:20:59.215 createLogger ▶ DEBUG 001 Setting log level to DEBUG
04:20:59.215 main ▶ DEBUG 002 foo

Run goimports:

$ goimports -srcdir -d -w .

Re-run the program:

$ go run main.go logger.go 
# command-line-arguments
./main.go:3:8: log redeclared in this block
	previous declaration at ./logger.go:10:2

Inspect the new contents of main.go:

$ cat main.go 
package main

import log "github.com/mgutz/logxi/v1"

func main() {
	createLogger(true)
	log.Debug("foo")
}
@ALTree ALTree changed the title goimports still fails to distinguish package from global variable x/tools/cmd/goimports: goimports still fails to distinguish package from global variable Dec 21, 2017
@gopherbot gopherbot added this to the Unreleased milestone Dec 21, 2017
@aultimus
Copy link

@aultimus aultimus commented Dec 22, 2017

another example

aulty@aulty-XPS-13-karhoo ~/Desktop/go_sandbox/goimports_bug $ cd /home/aulty/go/src/golang.org/x/tools/cmd/goimports/ && git rev-parse --short HEAD && cd -
ae8cc59
/home/aulty/Desktop/go_sandbox/goimports_bug
aulty@aulty-XPS-13-karhoo ~/Desktop/go_sandbox/goimports_bug $ cat $GOPATH/src/github.com/aultimus/service/service.go
package service

func Client() {
}
aulty@aulty-XPS-13-karhoo ~/Desktop/go_sandbox/goimports_bug $ cat a.go 
package main

func main() {
	service.Client()
}
aulty@aulty-XPS-13-karhoo ~/Desktop/go_sandbox/goimports_bug $ cat b.go 
package main

type Service struct {
}

func (s *Service) Client() {
}

var service Service
aulty@aulty-XPS-13-karhoo ~/Desktop/go_sandbox/goimports_bug $ goimports a.go 
package main

import "github.com/aultimus/service"

func main() {
	service.Client()
}
@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Dec 22, 2017

The problem is the sibling code optimization which doesn't take much into account. There is already a separate issue on incorrect imports, this highlights the lack of Global tracking. I will take a crack at this with my existing PR on the import issue, #23001.

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Dec 22, 2017

Upon further evaluation, this is actually not an issue.
Your invocation of goimports is incorrect, which is causing the problem.

-srcdir requires a value, preferably . or something valid. Once I do that, it executes properly.

@thepudds
Copy link

@thepudds thepudds commented Dec 22, 2017

@wjessop I’m slightly curious how you first encountered this? For example, were you using goimports via your editor and saw something unexpected, and then you switched to the command line to troubleshoot, or _____?

@thepudds
Copy link

@thepudds thepudds commented Dec 22, 2017

-srcdir defaults to the empty string. It seems -srcdir is (or at least was) related to the logic to detect globals within your package in a separate file (for example, from fatih/vim-go#956:

goimports -srcdir=.... will accept a full filename of the file being edited, instead of just its directory. (But directory will still work as it does today)
You'll probably want to update vim-go to pass the full name, which will enable some new features in goimports. (analyzing other files in your package's directory for better recommendations, and being aware of global variables)

There’s also related discussion here:
dominikh/go-mode.el#146

I’ll confess I do not 100% follow the exact interrelationship between the -srcdir option and the trailing [path ...] arguments (which was “.” in this case), but I wonder if -srcdir was not explicitly specified, should -srcdir effectively be picked up from the path of the trailing argument(s), which was “.“ in this case, or something along those lines?

All that said, given the use case for goimports is really tuned to being invoked by editors, it sounds like there might have been a conscious choice to do things this way based on other trade-offs or constraints (in other words, this might all be operating as currently designed).

@wjessop
Copy link
Author

@wjessop wjessop commented Dec 23, 2017

@thepudds I first hit an issue with VSCode-go, and decided to try to goimports directly and it looks like I fudged the command line! Sorry for that!

@wjessop wjessop closed this Dec 23, 2017
@golang golang locked and limited conversation to collaborators Dec 23, 2018
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
5 participants
You can’t perform that action at this time.