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

Jump to bottom

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

Open
kanata2 opened this issue Dec 2, 2021 · 3 comments
Open

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

kanata2 opened this issue Dec 2, 2021 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@kanata2
Copy link
Contributor

kanata2 commented Dec 2, 2021 •
edited

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
Copy link
Contributor

mknyszek commented Dec 3, 2021

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

Please CC anyone else you think this is relevant to.

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 3, 2021
@mknyszek mknyszek added this to the Backlog milestone Dec 3, 2021
@mvdan
Copy link
Member

mvdan commented Dec 3, 2021 •
edited

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
Copy link
Contributor Author

kanata2 commented Dec 4, 2021 •
edited

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

  1. search for standard packages (i.e., args are like NET/HTTP or STRINGS.Replace)
  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@mknyszek @mvdan @kanata2