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: incorrectly assumes module cache directory paths match module paths #29550

Closed
subash-a opened this issue Jan 4, 2019 · 12 comments

Comments

@subash-a
Copy link

@subash-a subash-a commented Jan 4, 2019

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

$ go version
go version go1.12beta1 darwin/amd64

Does this issue reproduce with the latest release?

Yes the issue is reproducible with the latest version of go (go1.12beta1)

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/subhash_sharma/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/subhash_sharma/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/subhash_sharma/.goversions/go1.12beta1"
GOTMPDIR=""
GOTOOLDIR="/Users/subhash_sharma/.goversions/go1.12beta1/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/subhash_sharma/subash/go.mod"
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/w0/d6l19r7j3dbg7fq56_nn4l340000gp/T/go-build083506298=/tmp/go-build -gno-record-
gcc-switches -fno-common"

What did you do?

The following testscript script reproduces the issue:

# install goimports
env GOMODPROXY=$GOPROXY
env GOPROXY=
go install golang.org/x/tools/cmd/goimports

# test goimports
env GOPROXY=$GOMODPROXY
cd mod
go mod download fruit.com@v1.0.0
go mod download
exec goimports -l main.go
stdout main.go

-- go.mod --
module goimports

require golang.org/x/tools v0.0.0-20181221235234-d00ac6d27372

-- mod/go.mod --
module mod

require fruit.com v1.0.0

replace fruit.com => github.com/user/banana v1.0.0

-- mod/main.go --
package main

import (
    "fmt"

    // we are missing an import to satisfy core
)

func main() {
    fmt.Println(core.Orange)
}
-- mod/core/core.go --
package core

type T struct{}
var Orange T

-- .gomodproxy/fruit.com_v1.0.0/.mod --
module fruit.com

-- .gomodproxy/fruit.com_v1.0.0/.info --
{"Version":"v1.0.0","Time":"2018-10-22T18:45:39Z"}

-- .gomodproxy/fruit.com_v1.0.0/go.mod --
module fruit.com

-- .gomodproxy/fruit.com_v1.0.0/fruit/fruit.go --
package fruit

const Apple = "apple"
-- .gomodproxy/fruit.com_v1.0.0/coretest/coretest.go --
// package coretest becomes a candidate for the missing
// core import in main above
package coretest

const Mandarin = "mandarin"

-- .gomodproxy/github.com_user_banana_v1.0.0/.mod --
module fruit.com

-- .gomodproxy/github.com_user_banana_v1.0.0/.info --
{"Version":"v1.0.0","Time":"2018-10-22T18:45:39Z"}

-- .gomodproxy/github.com_user_banana_v1.0.0/go.mod --
module fruit.com

-- .gomodproxy/github.com_user_banana_v1.0.0/fruit/fruit.go --
package fruit

const Apple = "apple"
-- .gomodproxy/github.com_user_banana_v1.0.0/coretest/coretest.go --
// package coretest becomes a candidate for the missing
// core import in main above
package coretest

const Mandarin = "mandarin"

What did you expect to see?

A successful run (the stdout main.go check should succeed because main.go is missing an import to satisfy the qualified identifier core.).

What did you see instead?

$ testscript goimports_directory_walking.txt
# install goimports (5.447s)
# test goimports (0.314s)
> env GOPROXY=$GOMODPROXY
> cd mod
$WORK/mod
> go mod download fruit.com@v1.0.0
[stderr]
go: finding github.com/user/banana v1.0.0
go: finding fruit.com v1.0.0

> go mod download
> exec goimports -l main.go
[stderr]
querying module cache matches: go [list -e -json -compiled -test=false -export=false -deps=false -find=true -- fruit.com/coretest golang.org/x/tools/cmd/heapview/internal/core github.com/user/banana/coretest]: exit status 1: go: github.com/user/banana@v1.0.0: parsing go.mod: unexpected module path "fruit.com"
go: error loading module requirements

[exit status 2]
FAIL: /tmp/testscript845630670/0/script.txt:11: unexpected command failure
error running goimports_directory_walking.txt in /tmp/testscript845630670/0

Our main module includes a replace directive; therefore our module cache contains a directory structure where the path does not match the contained module's path (the directory $GOPATH/pkg/mod/github.com/user/banana@v1.0.0 contains the module fruit.com@v1.0.0).

goimports appears to assume that directories within the module cache contain modules with the same path (which results in the above error message). But with replacements that will not be the case.

cc @heschik @bradfitz @ianthehat @myitcv

@gopherbot gopherbot added this to the Unreleased milestone Jan 4, 2019
@myitcv myitcv changed the title x/tools/cmd/goimports: fails when a replace directive is present in the go.mod x/tools/cmd/goimports: incorrectly assumes module cache directory paths match module paths Jan 4, 2019
@heschik
Copy link
Contributor

@heschik heschik commented Jan 4, 2019

(How do I run the repro? I'm slightly familiar with the test script language, but this thing where it's split into three files is new to me.)

Even without running the repro I think I understand what's happening. When doing a name= query, go/packages makes a temporary module to run go list in, and adds the things it finds in the module cache as dependencies of that temporary module. If the thing it found is the target of a replace directive, then it can't be used directly as a dependency.

Nasty bug. I'm not sure how to fix it. In this particular case, goimports could look at the module it's running in, notice that banana is mentioned in a replace directive, and skip it. But we won't always be running in the right module to see that. The only thing I can think of is to manually parse the module file and skip mismatches. Yuck.

@bcmills, @jayconrod, is there any chance that with -e this could be in the JSON rather than a fatal error?

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 4, 2019

How do I run the repro?

I'm just writing a simple rig for running arbitrary testscript tests (with goproxytest mod files).

Even without running the repro I think I understand what's happening

Isn't the logic you want here to only add a directory (for which the directory path does not equal the module path) if that directory is "in use" as the target of a replace, else don't add such mismatched directories?

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 4, 2019

PS this bug is now killing me because my module cache contains a number of such replace targets where the directory path does not match the module path.

@heschik
Copy link
Contributor

@heschik heschik commented Jan 4, 2019

for which the directory path does not equal the module path

Yeah. That's what I meant by manually parsing the module file and skipping mismatches. Sigh. I guess I can copy cmd/go/internal/modfile and its deps into x/tools.

I'll try to get to this early next week after I've talked with @ianthehat. If it's convenient, pushing the repro to GitHub would save me a little time -- I need something go mod download-able to build the test case.

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 7, 2019

@heschik - apologies, haven't had a chance to look at this yet.

Just to also flag, goimports also appears to do the wrong thing with directory paths that have been encoded. I'll try and add a repro for that too.

I need something go mod download-able to build the test case.

It should be possible to get what we need from these testscript repros, i.e. set GOPATH to something known so that we can then use the pkg/mod/cache/download directory afterwards; it's just much easier not to have to create separate GitHub repos for all this stuff.

@heschik
Copy link
Contributor

@heschik heschik commented Jan 7, 2019

Good point on encoding.

I'm afraid plans have changed and I've got some other goimports work to do so this fix will be delayed. But I'll get to it as part of that work or ASAP afterward.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 7, 2019

The only thing I can think of is to manually parse the module file and skip mismatches.

Note that there is an existing function in cmd/go/internal/modfile that parses only the module line.

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 7, 2019

@bcmills, @jayconrod, is there any chance that with -e this could be in the JSON rather than a fatal error?

Yes, that's arguably what it ought to do. Might not be feasible until 1.13, though.

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 9, 2019

I updated @subash-a's description of this issue to include a fully reproducible script.

@myitcv
Copy link
Member

@myitcv myitcv commented Jan 11, 2019

@heschik - as promised, here's a repro of the related path encoding bug:

# install goimports
env GOMODPROXY=$GOPROXY
env GOPROXY=
go install golang.org/x/tools/cmd/goimports

# test goimports
env GOPROXY=$GOMODPROXY
cd mod
go mod download github.com/Fruit/apple@v1.0.0
exec goimports -l main.go
stdout main.go

-- go.mod --
module goimports

require golang.org/x/tools v0.0.0-20181221235234-d00ac6d27372

-- mod/go.mod --
module mod

-- mod/main.go --
package main

import (
    "fmt"

    // we are missing an import to satisfy core
)

func main() {
    fmt.Println(core.Orange)
}
-- mod/core/core.go --
package core

type T struct{}
var Orange T

-- .gomodproxy/github.com_!fruit_apple_v1.0.0/.mod --
module fruit.com

-- .gomodproxy/github.com_!fruit_apple_v1.0.0/.info --
{"Version":"v1.0.0","Time":"2018-10-22T18:45:39Z"}

-- .gomodproxy/github.com_!fruit_apple_v1.0.0/go.mod --
module fruit.com

-- .gomodproxy/github.com_!fruit_apple_v1.0.0/fruit/fruit.go --
package fruit

const Apple = "apple"
-- .gomodproxy/github.com_!fruit_apple_v1.0.0/coretest/coretest.go --
// package coretest becomes a candidate for the missing
// core import in main above
package coretest

const Mandarin = "mandarin"

which should pass but gives:

$ testscript goimports_encoded_paths_walking.txt

# install goimports (4.603s)
# test goimports (0.222s)
> env GOPROXY=$GOMODPROXY
> cd mod
$WORK/mod
> go mod download github.com/Fruit/apple@v1.0.0
[stderr]
go: finding github.com/Fruit/apple v1.0.0

> exec goimports -l main.go
[stderr]
querying module cache matches: go [list -e -json -compiled -test=false -export=false -deps=false -find=true -- github.com/!fruit/apple/coretest golang.org/x/tools/cmd/heapview/internal/core]: exit status 1: go: github.com/!fruit/apple@v1.0.0: malformed module path "github.com/!fruit/apple": invalid char '!'
go: error loading module requirements

[exit status 2]
FAIL: /tmp/testscript247052820/0/script.txt:10: unexpected command failure
error running goimports_encoded_paths_walking.txt in /tmp/testscript247052820/0
@heschik
Copy link
Contributor

@heschik heschik commented Jan 16, 2019

Thanks for the repros. Both of them pass with https://golang.org/cl/158097, and I believe the test cases I copied from cmd/go cover these cases.

@heschik
Copy link
Contributor

@heschik heschik commented Jan 22, 2019

That CL is submitted, so this should be fixed. Let me know if not.

@heschik heschik closed this Jan 22, 2019
@golang golang locked and limited conversation to collaborators Jan 22, 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
6 participants
You can’t perform that action at this time.