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

cmd/go: require directive too strict on requiring space before parentheses? #38144

Open
myitcv opened this issue Mar 29, 2020 · 2 comments
Open

cmd/go: require directive too strict on requiring space before parentheses? #38144

myitcv opened this issue Mar 29, 2020 · 2 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Mar 29, 2020

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

$ go version
go version devel +5aef51a729 Sun Mar 29 02:01:34 2020 +0000 linux/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="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build136829961=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Unclear whether this is by design or not, so raising for completeness. The following test fails when I would have expected it to pass:

# Test demonstrating the sensitivity of bracket placement
# for require directives

exec go mod tidy
cmp go.mod go.mod.golden

-- go.mod --
module mod.com

go 1.12

require()
-- go.mod.golden --
module mod.com

go 1.12

By comparison the Go tool chain is agnostic on whether there is a space after the keyword of a parenthesised general declaration.

What did you expect to see?

Passing test

What did you see instead?

# Test demonstrating the sensitivity of bracket placement
# for require directives (0.004s)
> exec go mod tidy
[stderr]
go: errors parsing go.mod:
$WORK/go.mod:5: unknown directive: require()

cc @bcmills @jayconrod @matloob

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Mar 30, 2020

I've been looking at this for #24031. For the most part, the go.mod parser just splits tokens on whitespace. So replace() is one token. But note that if ( or ) is the first character after a space, it's always interpreted as a separate token from non-whitespace characters immediately after (so require ()more is four tokens, not two).

I don't think it's safe to change this tokenization in general: none of the ASCII bracket characters (( ) [ ] { }) are allowed in module paths, but they are all allowed in file paths on the right side of replace directives. They're also allowed in unresolved versions. So if ( in the middle of a sequence of non-whitespace characters were lexed as a separate token, it might break some go.mod files for newer versions of the go command. And in any case, after such a change, old versions of the go command would not accept go.mod files written with this syntax.

replace example.com/m => ./foo() // this file path is one token
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 31, 2020

Change https://golang.org/cl/226639 mentions this issue: modfile: scan ASCII brackets and commas as separate tokens

gopherbot pushed a commit to golang/mod that referenced this issue Apr 29, 2020
The characters '( ) [ ] { } ,' are now scanned as separate tokens,
even when they are preceded by non-whitespace "identifier"
characters. Previously, '( )' were scanned like this when preceded by
whitespace, but they could be in the middle of an identifier
token. None of these characters are allowed in module paths or
versions. They are allowed within file paths, so file paths containing
them will need to be quoted in the future. Using these characters
should not break ParseLax, since replace directives (the only directive
that allows files paths) are ignored by ParseLax.

Additionally, '(' is only treated as the beginning of a block if it
appears at the end of the line or is immediately followed by ')' at the
end of the line. ')' is treated as the end of a block if it
appears within a block at the beginning of a line.

Fixes golang/go#38167
Updates golang/go#38144
Updates golang/go#24031

Change-Id: I5a7fb522163802c3723d289cf0fbc5856ca075ec
Reviewed-on: https://go-review.googlesource.com/c/mod/+/226639
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.