cmd/doc: args are treated case-insensitive in some cases

kanata2 commented 2 years ago

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

$ go version
go version go1.17 darwin/amd64

$ gotip version
go version devel go1.18-d34051b Thu Dec 2 07:04:05 2021 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/kanataninaoki/Library/Caches/go-build"
GOENV="/Users/kanataninaoki/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/kanataninaoki/dev/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/kanataninaoki/dev"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.17/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.17/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/kanataninaoki/dev/src/github.com/kanata2/go-doc-case-insensitive-issue/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 -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/02/8930c3s52tlfpw2_8ml_xm5w0000gn/T/go-build3338848241=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The following is one of the examples to reproduce.

$ go doc Net/HttP/httpTEST.DefaultRemoteAddr

What did you expect to see?

According to https://go.dev/blog/package-names, lowercase is recommended in Go, so I think that the output should also follow it. Thus I was expecting that the go doc will return an error(like not_found_error) or the output which converted to lowercase.

What did you see instead?

The following is the actual output. The import path import "Net/HttP/httpTEST" is not a recommended style.

package httptest // import "Net/HttP/httpTEST"

const DefaultRemoteAddr = "1.2.3.4"
    DefaultRemoteAddr is the default remote address to return in RemoteAddr if
    an explicit DefaultRemoteAddr isn't set on ResponseRecorder.

Investigation

This problem seems to occur in cases where the passed arguments are passed directly to build.Import function. I don't know if this is the intended behavior, but it is because the build.Import function treats path as case-insensitive.

package main

import (
    "go/build"
    "log"
    "os"
)

func main() {
    wd, _ := os.Getwd()
    pkg, err := build.Import("Net/HttP/httpTEST", wd, build.ImportComment)
    log.Printf("pkg: %v\nerr: %v", pkg, err) // err is nil
}

I'm thinking of submitting a CL based on the discussion here.

mknyszek commented 2 years ago

CC @mvdan via https://dev.golang.org/owners

Please CC anyone else you think this is relevant to.

mvdan commented 2 years ago

I agree that this seems like an unintended feature; we should add a test and fix the code.

To begin with, we should probably be using https://pkg.go.dev/golang.org/x/mod@v0.5.1/module#CheckImportPath on import paths, to reject clearly invalid ones outright.

As for uppercase, I do believe module/package paths can contain uppercase letters, but they are also case-sensitive. See https://pkg.go.dev/golang.org/x/mod@v0.5.1/module#hdr-Escaped_Paths. https://pkg.go.dev/golang.org/x/mod@v0.5.1/module#EscapePath implements the escaping, for instance, though I don't think build.Import will know what to do with escaped import paths.

The proper fix here might be to fix build.Import so it doesn't assume that NET/HTTP is equivalent to net/http.

cc @bcmills @matloob in case I got anything wrong in regards to x/mod and go/build.

kanata2 commented 2 years ago

@mvdan

Thanks for the confirmation!

As for uppercase, I do believe module/package paths can contain uppercase letters, but they are also case-sensitive. See https://pkg.go.dev/golang.org/x/mod@v0.5.1/module#hdr-Escaped_Paths.

I looked at the GoDoc for golang.org/x/mod which you posted and found a few things I hadn't considered.

Also, as you may already know, let me summarize the possible cases of this issue and expected behavior again as follows.

This issue is only possible with case-insensitive file systems such as macOS. For example, the following returns the correct result.

$ docker run --rm -it golang:1.17 go doc NET/HTTP
doc: package NET/HTTP is not in GOROOT (/usr/local/go/src/NET/HTTP)
exit status 1

On top of that, this issue should only occur for the following two cases:

search for standard packages (i.e., args are like NET/HTTP or STRINGS.Replace)
search for packages in relative path (i.e., args are like ./AUTH or ../GOLANG/MOCK/GOMOCK)

Based on the above, we hope that the behavior of go doc will be able to handle correct import paths by validating arguments even if the filesystem is handled case-insensitively.

For case 1, the standard library is always in lowercase as far as I know, so I think forcing the conversion to lowercase is not bad idea, but for case 2, I can't think of an appropriate way to validate... And in this case, it should not be possible to use module#EscapePath to make a decision since it is not in GOMODCACHE and is not escaped.

golang / go