path/filepath: unexpected behavior of filepath.Join on Windows

[JerryKwan]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.2 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Jerry\AppData\Local\go-build
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Work\MyProjects\workspace\go
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\Jerry\AppData\Local\Temp\go-build770456046=/tmp/go-build -gno-record-gcc-switches

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

filepath.Join seems does not work well in Windows, please see the following snippet

package main

import (
"os"
"fmt"
"path/filepath"
)

func main() {
parts := []string {"C:", "Windows", "WinSXS" }
fmt.Println(filepath.Join(parts...))
parts = []string {fmt.Sprintf("C:%c", os.PathSeparator), "Windows", "WinSXS" }
fmt.Println(filepath.Join(parts...))
parts = []string {"C:", "","Program Files", "Microsoft Office 15", "ClientX64" }
fmt.Println(filepath.Join(parts...))
}

filepath.Join is expected to process the path separator properly, it works well for the other parts except the drive letter without the terminating slash. but if there was an empty string in the other parts, it works.

What did you expect to see?

It should output something like the following,
C:\Windows\WinSXS
C:\Windows\WinSXS
C:\Program Files\Microsoft Office 15\ClientX64

What did you see instead?

C:Windows\WinSXS
C:\Windows\WinSXS
C:\Program Files\Microsoft Office 15\ClientX64

[FMNSSun]

I think this is the intended behaviour. The reason is that C:foo.txt is a valid path on windows. Specifically C:foo.txt is a RELATIVE path. D:foo/bar refers to foo/bar relative to the current directory on drive D. C: and C:\ are actually different things. C:\ designates an absolute path, C: designates a relative path.

Edit: Yep. The source code of filepath.Join confirms this:

if len(elem[0]) == 2 && elem[0][1] == ':' {
	// First element is drive letter without terminating slash.
	// Keep path relative to current directory on that drive.
	return Clean(elem[0] + strings.Join(elem[1:], string(Separator)))
}

[JerryKwan]

@FMNSSun , thanks for looking into it.
YES, C:foo.txt can be viewed as a valid path.
Would you please run the following snippet on windows and what's the output? I am curious why it append the path separator after the drive letter

parts := []string {"C:", "","Program Files", "Microsoft Office 15", "ClientX64" }
fmt.Println(filepath.Join(parts...))

[FMNSSun]

TL/DR: filepath.Join([]string{"C:","","Program Files"}) incorrectly returns C:\Program Files due to the way it uses strings.Join and Clean.

I don't have to run the snippet because the source code is clear enough to see what's going on:

The problem is that strings.Join([]string{"","foo"}, "/") results in /foo. strings.Join does not ignore empty strings and I'd say this is at least a bug indeed (not of strings.Join but of filepath.Join) imo because it violates a promise the documentation makes:

in particular, all empty strings are ignored

which is actually not the case with []string{"C:","","Program Files"} because the "" there has an effect. The reason for this is that the join function is a wrapper around strings.Join and strings.Join does not ignore empty strings. After filepath.Join is done it invokes the Clean function which turns things like a\\b\c into a\b\c which makes it look like it ignores empty strings. However, since C:\Foo doesn't have \\ in it, Clean doesn't do anything to it. filepath.Join is a strings.Join followed by a filepath.Clean which means that []string{"a","","","b"} temporarily results in a\\\b which filepath.Clean turns back into a\b.

In the case of windows the code simply adds the drive letter + colon + strings.Join which means an empty string after drive letter + colon will result in a separator added by strings.Join. "c:" + strings.Join([]string{"","foo"}, "\\") is c:\foo and not c:foo.

return Clean(elem[0] + strings.Join(elem[1:], string(Separator)))

should perhaps invoke filepath.join instead which skips over leading empty strings (or at least scan forward to the first non-empty string).

Edit: Another solution might be using return elem[0] + Clean(...) instead there.

[andybons]

@alexbrainman

[alexbrainman]

TL/DR: filepath.Join([]string{"C:","","Program Files"}) incorrectly returns C:\Program Files

I agree to that. I modified existing test:

diff --git a/src/path/filepath/path_test.go b/src/path/filepath/path_test.go
index dde0872..e7e4167 100644
--- a/src/path/filepath/path_test.go
+++ b/src/path/filepath/path_test.go
@@ -271,6 +271,7 @@ var winjointests = []JoinTest{
 	{[]string{`C:`, `a`}, `C:a`},
 	{[]string{`C:`, `a\b`}, `C:a\b`},
 	{[]string{`C:`, `a`, `b`}, `C:a\b`},
+	{[]string{`C:`, ``, `b`}, `C:b`},
 	{[]string{`C:.`, `a`}, `C:a`},
 	{[]string{`C:a`, `b`}, `C:a\b`},
 	{[]string{`C:a`, `b`, `d`}, `C:a\b\d`},

and I see:

--- FAIL: TestJoin (0.00s)
    path_test.go:299: join(["C:" "" "b"]) = "C:\\b", want "C:b"
FAIL
FAIL    path/filepath   0.483s

So it is a bug.

/cc @mattn who wrote a lot of that code.

Alex

[gopherbot]

Change https://golang.org/cl/129758 mentions this issue: path/filepath: fix Join with Windows drive letter

[B-Art]

I am missing this one:
C:b is equal to C:.\b.
and Windows interprets consecutive \ like \\\\ as one \.

[FMNSSun]

@B-Art Clean removes consecutive separators. What do you mean by C:b is equal to C:.\b? Do you want Clean to turn C:.\b into C:b?

[alexbrainman]

If anyone wants to verify change

https://go-review.googlesource.com/c/go/+/129758

before I submit it.

Thank you.

Alex