Repository: go
Issue: x/tools/refactor/rename: fails to move nested packages on Windows with error "invalid move destination ...  package or subpackage <origpkg> already exists" (suspect backslash vs. forward slash issue)
Labels: Tools,Refactoring

**1) What version of Go are you using (`go version`)?**

```
go version go1.5.2 windows/amd64
```

Note: One of my colleagues first hit this same problem on 1.6.2.  I personally happen to be still be using go 1.5.2 and would prefer not to upgrade right now on my laptop, but I at least endeavored to get the latest gomvpkg (which is where I think the bug is) and still saw this problem. In other words, I don't think this is specific to my having a core of 1.5.2, and I think this problem is present in the latest versions. 

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

```
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\temp\golang_bug_report\refactor\work
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GO15VENDOREXPERIMENT=
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
```

**3) What did you do?**

Try to move a nested package on Windows using gomvpkg, such as:

```
%GOPATH%\bin\gomvpkg -from github.com/user/originalhello -to github.com/user/newhello
```

Details below, including (hopefully) exact steps to reproduce at the bottom.

**4) What did you expect to see?**

The package moved.

**5) What did you see instead?**

Failed to move package, with error:

gomvpkg: invalid move destination: github.com/user/newhello; package or subpackage github.com/user/originalhello already exists.
## Details

When moving a nested package such as "github.com/user/originalhello", gomvpkg fails with error reporting that the _original_ package already exists, such as:

gomvpkg: invalid move destination: github.com/user/newhello; package or subpackage github.com/user/**originalhello** already exists.

On Linux, moving the same nested packages seems to work.

I suspect that the code is getting confused about backslash vs. slash on Windows, which causes a strings.Replace to fail, which then causes the code to think that the destination is the same as the source, which then causes sanity checking to fail, which then causes a failure to move the package as well as an error stating the (incorrect) destination already exists.

(The primary problem is that the package move fails -- the fact that the error message is complaining that the _original_ package exists is a little confusing, given that upon reading the error message one might think "Of course the original package exists, but why is that causing a failure??". But I think that's due to the later code I think appropriately being paranoid and I think appropriately detecting that the earlier code had come up with an incorrect destination).

Note: I am guessing that this particular failure is neither fixed by nor caused by #16384 ("refactor/rename: fix two bugs related to MS Windows' path separator") that was merged ~9 days ago. I could reproduce this particular failure both before the fix for #16384, as well as I seemed to still be able to reproduce this particular failure after updating to get the fix for #16384.

I'll confess I'm still relatively new to Go, so please take with a healthy dose of salt, but I'm guessing this is the problematic code:

https://github.com/golang/tools/blob/ed69e84b1518b5857a9f4e01d1f9cefdcc45246e/refactor/rename/mvpkg.go#L83

``` go
        // Ensure directories have a trailing separator.
        dest := strings.Replace(pkg,
            filepath.Join(from, ""),
            filepath.Join(to, ""),
            1)
        destinations[pkg] = filepath.ToSlash(dest)
```

If you set up a simple 'github.com/user/originalhello' package (detailed steps at end), and then do:

```
%GOPATH%\bin\gomvpkg -from github.com/user/originalhello -to github.com/user/newhello
```

Then, the corresponding values of the main variables in the snippet above will be:

```
              pkg:  "github.com/user/originalhello"
             from:  "github.com/user/originalhello"
               to:  "github.com/user/newhello"
```

And then strings.Replace will fail because `from` and `to` are first transformed to backslash on Windows by filepath.Join, with sample string values that are then passed into strings.Replace (substituting sample string values in here for illustration) would be:

``` go
  "github.com/user/originalhello" := strings.Replace("github.com/user/originalhello",
                                                     "github.com\\user\\originalhello",
                                                     "github.com\\user\\newhello",
                                                     1)
```

Note that the strings.Replace in this case is a no-op due to the mismatch between backslash vs. forward slash (that is, strings.Replace returns a string containing **originalhello**, rather than the expected **newhello**).

The net result is the destination ends up incorrectly being the **same** as the source:

```
       the destination for pkg:  "github.com/user/originalhello"
          is destinations[pkg]:  "github.com/user/originalhello"
```

Which I think then triggers this error in the later validation code that sees the incorrect _destination_ of 'originalhello' already exist:

gomvpkg: invalid move destination: github.com/user/newhello; package or subpackage github.com/user/**originalhello** already exists.

Detailed steps to reproduce at the end, including a few fmt.Print's that I used to output (more or less) the more detailed explanatory text above.
## Possible fix

I'm definitely not 100% sure of this, but I suspect one fix might be to change from 'filepath.Join' to be 'path.Join' given I **think** that `pkg`, `from`, and `to` are import paths (with forward slashes) at this point and not locations on disk (with back slashes). 

``` diff
diff --git a/refactor/rename/mvpkg.go b/refactor/rename/mvpkg.go
index cd416c5..e44b975 100644
--- a/refactor/rename/mvpkg.go
+++ b/refactor/rename/mvpkg.go
@@ -81,8 +81,8 @@ func Move(ctxt *build.Context, from, to, moveTmpl string) error {
                }
                // Ensure directories have a trailing separator.
                dest := strings.Replace(pkg,
-                       filepath.Join(from, ""),
-                       filepath.Join(to, ""),
+                       path.Join(from, ""),
+                       path.Join(to, ""),
                        1)
                destinations[pkg] = filepath.ToSlash(dest)
        }
```

That at least seemed to solve it for me, and "go test golang.org/x/tools/refactor/rename" still passes with that change (on Windows, anyway).

If that is the proper fix, then it might be the case that the subsequent "destinations[pkg] = filepath.ToSlash(dest)" might no longer be needed, but I did not look at that aspect carefully at all (nor did I try to test removing that filepath.ToSlash)

Or, if the appropriate fix is not changing filepath.Join to path.Join here, then I would guess the fix is otherwise to make the slash & backslash transformations consistent so that the strings.Replace succeeds. 
## Detailed steps to reproduce

Either follow these steps, or in theory pasting this into a Windows command prompt should work:

``` batch
REM  Follow steps from "How to Write Go Code" to set up a workspace, except 
REM  name your package 'originalhello' rather than 'hello':

mkdir C:\temp\golang_bug_report\refactor\work
set GOPATH=C:\temp\golang_bug_report\refactor\work
echo %GOPATH%
mkdir %GOPATH%\src\github.com\user

REM  Create a hello world in github.com/user/originalhello:
mkdir %GOPATH%\src\github.com\user\originalhello

REM  Create a file named hello.go in %GOPATH%\src\github.com\user\originalhello\hello.go, e.g.,:
notepad %GOPATH%\src\github.com\user\originalhello\hello.go
```

// ---------------------------
// sample file contents for hello.go:
// ---------------------------

``` go
package originalhello

import "fmt"

func main() {
    fmt.Printf("Hello, world.\n")
}
```

And then continue on:

``` batch
REM  Confirm we can build this
go install github.com/user/originalhello

REM  Get gomvpkg
go get golang.org/x/tools/cmd/gomvpkg

REM  Run it! Try to rename .../originalhello to .../newhello, which incorrectly fails:

%GOPATH%\bin\gomvpkg -from github.com/user/originalhello -to github.com/user/newhello
```

For me these steps then cause following error:

gomvpkg: invalid move destination: github.com/user/newhello; package or subpackage github.com/user/originalhello already exists.
## Sample fmt.Println to dump incorrect Windows behavior of gomvpkg

As mentioned above, here are the sample fmt.Print's that I used to try to illustrate the incorrect behavior when called on Windows with a nested package.  (Apologies in advance if this is confusing, but these fmt.Print's include some explanatory text that assume the presence of the bug and that the bug is being triggered via the detailed steps to reproduce above. In other words, these are the fmt.Print's I happen to use to generate the more detailed text for my explanation above in this bug write up, so sending these along as well in case helpful):

``` diff
diff --git a/refactor/rename/mvpkg.go b/refactor/rename/mvpkg.go
index cd416c5..46c651a 100644
--- a/refactor/rename/mvpkg.go
+++ b/refactor/rename/mvpkg.go
@@ -83,10 +83,30 @@ func Move(ctxt *build.Context, from, to, moveTmpl string) error {
                dest := strings.Replace(pkg,
                        filepath.Join(from, ""),
                        filepath.Join(to, ""),
                        1)
                destinations[pkg] = filepath.ToSlash(dest)
+
+               fmt.Printf("\nWith these inputs:\n\n")
+               fmt.Printf("              pkg:  %q\n", pkg)
+               fmt.Printf("             from:  %q\n", from)
+               fmt.Printf("               to:  %q\n", to)
+
+               fmt.Printf("\nReplace will fail when 'to' and 'from' are transformed to backslash on Windows:\n\n")
+               fmt.Printf(`  %q := strings.Replace(%q,
+                                                    %q,
+                                                    %q,
+                                                    1)`,
+                       destinations[pkg],
+                       pkg,
+                       filepath.Join(from, ""),
+                       filepath.Join(to, ""))
+
+               fmt.Printf("\n\nNet result is destination ends up incorrectly being same as source:\n\n")
+               fmt.Printf("   destination for:  %q\n", pkg)
+               fmt.Printf("                is:  %q\n\n", destinations[pkg])
+
        }
```