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)

[thepudds]

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

        // 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:

  "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 --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:

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:
// ---------------------------

package originalhello

import "fmt"

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

And then continue on:

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 --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])
+
        }

[thepudds]

A side note regarding the related existing comment:

//Ensure directories have a trailing separator

from here:

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

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

Either that comment is misleading, or I don't understand that comment (either option being relatively likely ;-)

The Ensure directories have a trailing separator comment seems to imply that filepath.Join(foo, "") would add a trailing separator but I'm not sure that's true. I think that would be true for strings.Join with final empty string being joined, but I'm not sure that filepath.Join(foo, "") would add a trailing separator.

I'm not 100% sure of those statements, but here's a quick example:

https://play.golang.org/p/QkQ0Y1Pvbn

    // Sample data. Note 2 empty strings (especially note that last element is empty string)
    s := []string{"foo", "", "bar", "baz", ""}

    // Compare strings.Join vs. filepath.Join vs. path.Jon
    fmt.Println("strings.Join:  ", strings.Join(s, ","))
    fmt.Println("filepath.Join: ", filepath.Join(s...))
    fmt.Println("path.Join:     ", path.Join(s...))

    // Output:
    // strings.Join:   foo,,bar,baz,
    // filepath.Join:  foo/bar/baz
    // path.Join:      foo/bar/baz

Note that that filepath.Join behavior is a little different from what seems to be the Python and Ruby behavior for a similar filepath join with an empty string, where Python and Ruby would seem to include a final trailing separator if passed a final empty string (live Python/Ruby examples at bottom of this comment).

So, perhaps the author of this Go code incorrectly expected filepath.Join to behave either similar to how either Go behaves for strings.Join, and/or how other programming languages handle filepath join?

So perhaps the intent of that comment was something closer to the following (untested, sorry):

        // Normalize any trailing separators (via Clean) to ensure we 
        // replace the substrings in pkg consistently.
        dest := strings.Replace(pkg,
            path.Clean(from),
            path.Clean(to),
            1)

Python filepath join example:

https://repl.it/CfZD

print 'python string join (input includes 2 empty strings):'
print ','.join(['foo', '', 'bar', 'baz', ''])

print '\npython os.path join (input includes 2 empty strings):'
print os.path.join('foo', '', 'bar', 'baz', '')

# Outputs
# python string join (input includes 2 empty strings):
# foo,,bar,baz,
# 
# python os.path join (input includes 2 empty strings):
# foo/bar/baz/

Ruby filepath join example

(and sorry, I don't really know Ruby):

https://repl.it/CfZA

puts "ruby string join (input includes 2 empty strings):"
puts ["foo", "", "bar", "baz", ''].join(",")

puts "\nruby file join (input includes 2 empty strings):"
puts File.join("foo", "", "bar", "baz", "")

# Output:
# ruby string join (input includes 2 empty strings):
# foo,,bar,baz,
#
# ruby file join (input includes 2 empty strings):
# foo/bar/baz/

[thepudds]

I also now see that #16384 recently added that filepath.ToSlash(dest) after the strings.Replace call.

I haven't gone through #16384 in detail to track down exactly why filepath.ToSlash(dest) was added there, but as I mentioned in my first comment above, that filepath.ToSlash(dest) might very well be unnecessary after the suggested possible fix from my first comment above. That said, it would likely require a review of the problem from #16384 to determine that, though.

Finally, sorry for the long report, but to more explicitly tie together my first comment and second comment above: if those guesses about original intent there are correct, the net result for all my comments would be:

diff --git a/refactor/rename/mvpkg.go b/refactor/rename/mvpkg.go
index cd416c5..26905ab 100644
--- a/refactor/rename/mvpkg.go
+++ b/refactor/rename/mvpkg.go
@@ -79,12 +79,13 @@ func Move(ctxt *build.Context, from, to, moveTmpl string) error {
                for r := range rev[pkg] {
                        affectedPackages[r] = true
                }
-               // Ensure directories have a trailing separator.
+               // Normalize any trailing separators (via Clean) to ensure we
+               // replace the substrings in pkg consistently.
                dest := strings.Replace(pkg,
-                       filepath.Join(from, ""),
-                       filepath.Join(to, ""),
+                       path.Clean(from),
+                       path.Clean(to),
                        1)
-               destinations[pkg] = filepath.ToSlash(dest)
+               destinations[pkg] = dest
        }

[alexbrainman]

Thank you for problem report. I agree there is a problem in that code.

We even have test for it - TestMoves. The test is broken, but no one noticed because TestMoves is skipped on windows (by mistake). @alandonovan sent https://go-review.googlesource.com/#/c/24969 to enable the test, but then he discovered the test is broken.

I came to similar conclusion to yours. You can see my comment at the end of https://go-review.googlesource.com/#/c/24969

You can wait for Alan to get around to complete CL 24969. Or you can send alternative solution, if you like - it sounds like you have researched this area quite a bit. This https://golang.org/doc/contribute.html is how to contribute.

Alex

[thepudds]

Ahh, I did not see that comment, but that would have saved me some time. ;-)

(Also, I am not very familiar with Gerrit, but I had thought that particular CL was merged already in what I was running, but as you say, looks like it hasn't been merged yet).

I suspect the original intent of that (confusing) filepath.Join(foo, "") was to avoid something like from in that stanza being something like a/b/ (trailing slash) but to being something like z/b (no trailing slash) and then transforming pkg from something like a/b/c to something like z/bc (lost the slash between b and c). Such as:

https://play.golang.org/p/gKPkO0JuUI

or in the opposite direction, which might end up with a double slash, or some other variation of that.

I don't know if that would happen in practice (e.g., maybe when switching levels?), but I was able to trigger a problem with a simple replace with something like a trailing slash on the command line:

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

which triggered similar error of:

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

But that same example on the command line with the path.Clean() seemed to work...

In any event, thanks for the comment, and I guess I'll see if there are further comments about original intent or otherwise desired approach here.

[thepudds]

It looks like #16384 is closed in github, which is why I thought that associated CL was merged, but looking more carefully now I see that associated CL is still open.

[thepudds]

I was curious if that seemingly incorrectly used filepath.Join(foo, "") was used elsewhere, so took a quick look last night and noticed a similar issue here in master in the method that computes subpackages:

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

        if !strings.HasPrefix(pkg, path.Join(dir, "")) {
            return
        }

I think the expectation of the author here might have been that appending the empty string would cause a slash to be appended (similar to what might have been the incorrect expectation about the filepath.Join(from, "") earlier that had the // Ensure directories have a trailing separator comment).

In any event, it seems the logic here in is wrong, because packages with shared prefixes can incorrectly match here in this subpackages method.

For example, if you set up the following (and note the shared prefix between the first two):

github.com/user/old_something
github.com/user/old_something_else
github.com/user/new_something_else

And then try to move old_something to new_something, in theory that should work (and old_something_else should be left alone).

However, because old_something_else shares a prefix with old_something (where old_something is the user's requested 'from'), the code here in subpackages incorrectly thinks old_something_else is a sub-package of old_something, and incorrectly sets up the destinations map (that is, intending to move old_something_else when it shouldn't), and hence the code gets ready to (incorrectly) move old_something_else to new_something_else, which then collides with the pre-existing new_something_else, which reports an error

gomvpkg: invalid move destination: github.com/user/new_something; package or subpackage github.com/user/new_something_else already exists.

I think (but did not confirm) that would happen on Linux as well (that is, not specific to backslash vs. forward slash confusion on Windows).

That's one example I could see would fail and then tested it (and it did give incorrect error above), but I think in theory a different example could also incorrectly update an "innocent bystander" package's contents based on a second "innocent bystander" package sharing a prefix with a package that is being moved (though some chance other defensive sanity checks would prevent it and/or the subsequent code might otherwise "get lucky"; I didn't try a different test).

Finally, I believe this is likely already 100% clear to anyone who worked on #16384, but to summarize the current state of #16384 (as of right now and when I looked at it a couple days ago):

• Issue x/tools/refactor/rename: TestMoves is failing on Windows #16384 is marked closed in github
• Associated CL that is merged: https://go-review.googlesource.com/#/c/24943/
• Associated CL that is not yet merged: https://go-review.googlesource.com/#/c/24969/

When I had looked at it before, I had incorrectly thought #16384 was 'done' and in master. Alex above pointed out above that CL 24969 is still pending, which is the CL that re-enables the tests, so only part of the changes for #16384 are currently in master.

[alexbrainman]

Like I said before, feel free to send a change. :-) If you can add new broken test (that gets fixed by your changes), no-one will be able to object.

Alex