From 8a75bf493f03fd163ae3604dcc8aedd9b6141315 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Fri, 6 Apr 2018 16:00:07 -0400 Subject: [PATCH] cmd/go/internal/modfile: allow, prefer unquoted strings in mod files Before, module paths and directory names were always quoted and versions were never quoted. Now, either may be quoted, but the canonical form is to omit quotation marks wherever possible. Fixes golang/go#24641. Change-Id: I3bde7356ae34a78f4eb7c0e95807bc11d12b9ffd Reviewed-on: https://go-review.googlesource.com/105215 Reviewed-by: Bryan C. Mills --- vendor/cmd/go/internal/modconv/modconv.go | 2 +- .../cmd/go/internal/modfetch/coderepo_test.go | 6 +- .../cmd/go/internal/modfetch/convert_test.go | 64 +++++++++---------- vendor/cmd/go/internal/modfile/read.go | 34 +++++++--- vendor/cmd/go/internal/modfile/read_test.go | 22 ++++++- vendor/cmd/go/internal/modfile/rule.go | 60 ++++++++++++----- .../internal/modfile/testdata/module.golden | 2 +- .../internal/modfile/testdata/replace.golden | 6 +- .../internal/modfile/testdata/replace2.golden | 8 ++- .../go/internal/modfile/testdata/replace2.in | 4 +- 10 files changed, 136 insertions(+), 72 deletions(-) diff --git a/vendor/cmd/go/internal/modconv/modconv.go b/vendor/cmd/go/internal/modconv/modconv.go index 478e76f..b689b52 100644 --- a/vendor/cmd/go/internal/modconv/modconv.go +++ b/vendor/cmd/go/internal/modconv/modconv.go @@ -22,4 +22,4 @@ var Converters = map[string]func(string, []byte) ([]module.Version, error){ // for dependencies before caching them. // In case of bugs in the converter, if we bump this version number, // then all the cached copies will be ignored. -const Prefix = "//vgo 0.0.3\n" +const Prefix = "//vgo 0.0.4\n" diff --git a/vendor/cmd/go/internal/modfetch/coderepo_test.go b/vendor/cmd/go/internal/modfetch/coderepo_test.go index e23a575..ff25516 100644 --- a/vendor/cmd/go/internal/modfetch/coderepo_test.go +++ b/vendor/cmd/go/internal/modfetch/coderepo_test.go @@ -198,7 +198,7 @@ var codeRepoTests = []struct { name: "0f30252985809011f026b5a2d5cf456e021623da", short: "0f3025298580", time: time.Date(2018, 2, 20, 2, 47, 20, 0, time.UTC), - gomod: "//vgo 0.0.3\n\nmodule \"go.googlesource.com/scratch\"\n", + gomod: "//vgo 0.0.4\n\nmodule go.googlesource.com/scratch\n", }, { path: "go.googlesource.com/scratch/rsc", @@ -287,7 +287,7 @@ var codeRepoTests = []struct { name: "d670f9405373e636a5a2765eea47fac0c9bc91a4", short: "d670f9405373", time: time.Date(2018, 1, 9, 11, 43, 31, 0, time.UTC), - gomod: "//vgo 0.0.3\n\nmodule \"gopkg.in/yaml.v2\"\n", + gomod: "//vgo 0.0.4\n\nmodule gopkg.in/yaml.v2\n", }, { path: "gopkg.in/check.v1", @@ -296,7 +296,7 @@ var codeRepoTests = []struct { name: "20d25e2804050c1cd24a7eea1e7a6447dd0e74ec", short: "20d25e280405", time: time.Date(2016, 12, 8, 18, 13, 25, 0, time.UTC), - gomod: "//vgo 0.0.3\n\nmodule \"gopkg.in/check.v1\"\n", + gomod: "//vgo 0.0.4\n\nmodule gopkg.in/check.v1\n", }, } diff --git a/vendor/cmd/go/internal/modfetch/convert_test.go b/vendor/cmd/go/internal/modfetch/convert_test.go index fd69b94..ad9f690 100644 --- a/vendor/cmd/go/internal/modfetch/convert_test.go +++ b/vendor/cmd/go/internal/modfetch/convert_test.go @@ -27,23 +27,23 @@ func TestConvertLegacyConfig(t *testing.T) { { // Gopkg.lock parsing. "github.com/golang/dep", "v0.4.0", - `module "github.com/golang/dep" + `module github.com/golang/dep require ( - "github.com/Masterminds/semver" v0.0.0-20170726230514-a93e51b5a57e - "github.com/Masterminds/vcs" v1.11.1 - "github.com/armon/go-radix" v0.0.0-20160115234725-4239b77079c7 - "github.com/boltdb/bolt" v1.3.1 - "github.com/go-yaml/yaml" v0.0.0-20170407172122-cd8b52f8269e - "github.com/golang/protobuf" v0.0.0-20170901042739-5afd06f9d81a - "github.com/jmank88/nuts" v0.3.0 - "github.com/nightlyone/lockfile" v0.0.0-20170707060451-e83dc5e7bba0 - "github.com/pelletier/go-toml" v0.0.0-20171218135716-b8b5e7696574 - "github.com/pkg/errors" v0.8.0 - "github.com/sdboyer/constext" v0.0.0-20170321163424-836a14457353 - "golang.org/x/net" v0.0.0-20170828231752-66aacef3dd8a - "golang.org/x/sync" v0.0.0-20170517211232-f52d1811a629 - "golang.org/x/sys" v0.0.0-20170830134202-bb24a47a89ea + github.com/Masterminds/semver v0.0.0-20170726230514-a93e51b5a57e + github.com/Masterminds/vcs v1.11.1 + github.com/armon/go-radix v0.0.0-20160115234725-4239b77079c7 + github.com/boltdb/bolt v1.3.1 + github.com/go-yaml/yaml v0.0.0-20170407172122-cd8b52f8269e + github.com/golang/protobuf v0.0.0-20170901042739-5afd06f9d81a + github.com/jmank88/nuts v0.3.0 + github.com/nightlyone/lockfile v0.0.0-20170707060451-e83dc5e7bba0 + github.com/pelletier/go-toml v0.0.0-20171218135716-b8b5e7696574 + github.com/pkg/errors v0.8.0 + github.com/sdboyer/constext v0.0.0-20170321163424-836a14457353 + golang.org/x/net v0.0.0-20170828231752-66aacef3dd8a + golang.org/x/sync v0.0.0-20170517211232-f52d1811a629 + golang.org/x/sys v0.0.0-20170830134202-bb24a47a89ea )`, }, @@ -53,25 +53,25 @@ func TestConvertLegacyConfig(t *testing.T) { // Godeps.json parsing. // TODO: Should v2.0.0 work here too? "github.com/docker/distribution", "v0.0.0-20150410205453-85de3967aa93", - `module "github.com/docker/distribution" + `module github.com/docker/distribution require ( - "github.com/AdRoll/goamz" v0.0.0-20150130162828-d3664b76d905 - "github.com/bugsnag/bugsnag-go" v0.0.0-20141110184014-b1d153021fcd - "github.com/bugsnag/osext" v0.0.0-20130617224835-0dd3f918b21b - "github.com/bugsnag/panicwrap" v0.0.0-20141110184334-e5f9854865b9 - "github.com/docker/libtrust" v0.0.0-20150114040149-fa567046d9b1 - "github.com/garyburd/redigo" v0.0.0-20150301180006-535138d7bcd7 - "github.com/gorilla/context" v0.0.0-20140604161150-14f550f51af5 - "github.com/gorilla/handlers" v0.0.0-20140825150757-0e84b7d810c1 - "github.com/gorilla/mux" v0.0.0-20140926153814-e444e69cbd2e - "github.com/jlhawn/go-crypto" v0.0.0-20150401213827-cd738dde20f0 - "github.com/yvasiyarov/go-metrics" v0.0.0-20140926110328-57bccd1ccd43 - "github.com/yvasiyarov/gorelic" v0.0.0-20141212073537-a9bba5b9ab50 - "github.com/yvasiyarov/newrelic_platform_go" v0.0.0-20140908184405-b21fdbd4370f - "golang.org/x/net" v0.0.0-20150202051010-1dfe7915deaf - "gopkg.in/check.v1" v0.0.0-20141024133853-64131543e789 - "gopkg.in/yaml.v2" v0.0.0-20150116202057-bef53efd0c76 + github.com/AdRoll/goamz v0.0.0-20150130162828-d3664b76d905 + github.com/bugsnag/bugsnag-go v0.0.0-20141110184014-b1d153021fcd + github.com/bugsnag/osext v0.0.0-20130617224835-0dd3f918b21b + github.com/bugsnag/panicwrap v0.0.0-20141110184334-e5f9854865b9 + github.com/docker/libtrust v0.0.0-20150114040149-fa567046d9b1 + github.com/garyburd/redigo v0.0.0-20150301180006-535138d7bcd7 + github.com/gorilla/context v0.0.0-20140604161150-14f550f51af5 + github.com/gorilla/handlers v0.0.0-20140825150757-0e84b7d810c1 + github.com/gorilla/mux v0.0.0-20140926153814-e444e69cbd2e + github.com/jlhawn/go-crypto v0.0.0-20150401213827-cd738dde20f0 + github.com/yvasiyarov/go-metrics v0.0.0-20140926110328-57bccd1ccd43 + github.com/yvasiyarov/gorelic v0.0.0-20141212073537-a9bba5b9ab50 + github.com/yvasiyarov/newrelic_platform_go v0.0.0-20140908184405-b21fdbd4370f + golang.org/x/net v0.0.0-20150202051010-1dfe7915deaf + gopkg.in/check.v1 v0.0.0-20141024133853-64131543e789 + gopkg.in/yaml.v2 v0.0.0-20150116202057-bef53efd0c76 )`, }, } diff --git a/vendor/cmd/go/internal/modfile/read.go b/vendor/cmd/go/internal/modfile/read.go index 7fdf827..a0c88d6 100644 --- a/vendor/cmd/go/internal/modfile/read.go +++ b/vendor/cmd/go/internal/modfile/read.go @@ -238,6 +238,18 @@ func (in *input) peekRune() int { return int(r) } +// peekPrefix reports whether the remaining input begins with the given prefix. +func (in *input) peekPrefix(prefix string) bool { + // This is like bytes.HasPrefix(in.remaining, []byte(prefix)) + // but without the allocation of the []byte copy of prefix. + for i := 0; i < len(prefix); i++ { + if i >= len(in.remaining) || in.remaining[i] != prefix[i] { + return false + } + } + return true +} + // readRune consumes and returns the next rune in the input. func (in *input) readRune() int { if len(in.remaining) == 0 { @@ -302,7 +314,7 @@ func (in *input) lex(sym *symType) int { } // Comment runs to end of line. - if c == '/' { + if in.peekPrefix("//") { in.startToken(sym) // Is this comment the only thing on its line? @@ -311,13 +323,7 @@ func (in *input) lex(sym *symType) int { i := bytes.LastIndex(in.complete[:in.pos.Byte], []byte("\n")) suffix := len(bytes.TrimSpace(in.complete[i+1:in.pos.Byte])) > 0 in.readRune() - c = in.peekRune() - if c == '*' { - in.Error(fmt.Sprintf("mod files must use // comments (not /* */ comments)")) - } - if c != '/' { - in.Error(fmt.Sprintf("unexpected input character %#q", c)) - } + in.readRune() // Consume comment. for len(in.remaining) > 0 && in.readRune() != '\n' { @@ -344,6 +350,10 @@ func (in *input) lex(sym *symType) int { return _EOL } + if in.peekPrefix("/*") { + in.Error(fmt.Sprintf("mod files must use // comments (not /* */ comments)")) + } + // Found non-space non-comment. break } @@ -406,6 +416,12 @@ func (in *input) lex(sym *symType) int { // Scan over identifier. for isIdent(in.peekRune()) { + if in.peekPrefix("//") { + break + } + if in.peekPrefix("/*") { + in.Error(fmt.Sprintf("mod files must use // comments (not /* */ comments)")) + } in.readRune() } return _IDENT @@ -414,7 +430,7 @@ func (in *input) lex(sym *symType) int { // isIdent reports whether c is an identifier rune. // We treat nearly all runes as identifier runes. func isIdent(c int) bool { - return c != 0 && !unicode.IsSpace(rune(c)) && c != '/' && c != '(' && c != ')' && c != '"' && c != '`' + return c != 0 && !unicode.IsSpace(rune(c)) } // Comment assignment. diff --git a/vendor/cmd/go/internal/modfile/read_test.go b/vendor/cmd/go/internal/modfile/read_test.go index 9879603..3c73528 100644 --- a/vendor/cmd/go/internal/modfile/read_test.go +++ b/vendor/cmd/go/internal/modfile/read_test.go @@ -97,10 +97,13 @@ func TestPrintParse(t *testing.T) { eq := eqchecker{file: base} if err := eq.check(f, f2); err != nil { - t.Errorf("not equal: %v", err) + t.Errorf("not equal (parse/Format/parse): %v", err) } pf1, err := Parse(base, data, nil) + if err != nil && base == "testdata/replace2.in" { + t.Errorf("should parse %v: %v", base, err) + } if err == nil { pf2, err := Parse(base, ndata, nil) if err != nil { @@ -109,8 +112,23 @@ func TestPrintParse(t *testing.T) { } eq := eqchecker{file: base} if err := eq.check(pf1, pf2); err != nil { - t.Errorf("not equal: %v", err) + t.Errorf("not equal (parse/Format/Parse): %v", err) + } + + ndata2, err := pf1.Format() + if err != nil { + t.Errorf("reformat: %v", err) + } + pf3, err := Parse(base, ndata2, nil) + if err != nil { + t.Errorf("Parsing reformatted2: %v", err) + continue + } + eq = eqchecker{file: base} + if err := eq.check(pf1, pf3); err != nil { + t.Errorf("not equal (Parse/Format/Parse): %v", err) } + ndata = ndata2 } if strings.HasSuffix(out, ".in") { diff --git a/vendor/cmd/go/internal/modfile/rule.go b/vendor/cmd/go/internal/modfile/rule.go index 8a69f10..11fa36c 100644 --- a/vendor/cmd/go/internal/modfile/rule.go +++ b/vendor/cmd/go/internal/modfile/rule.go @@ -12,6 +12,7 @@ import ( "sort" "strconv" "strings" + "unicode" "cmd/go/internal/module" "cmd/go/internal/semver" @@ -56,7 +57,7 @@ func (f *File) AddModuleStmt(path string) { f.Syntax = new(FileSyntax) } f.Syntax.Stmt = append(f.Syntax.Stmt, &Line{ - Token: []string{"module", strconv.Quote(path)}, + Token: []string{"module", AutoQuote(path)}, }) } @@ -120,9 +121,9 @@ func (f *File) add(errs *bytes.Buffer, line *Line, verb string, args []string, f return } f.Module = new(Module) - if len(args) != 1 || !isString(args[0]) { + if len(args) != 1 { - fmt.Fprintf(errs, "%s:%d: usage: module \"module/path\" [version]\n", f.Syntax.Name, line.Start.Line) + fmt.Fprintf(errs, "%s:%d: usage: module module/path [version]\n", f.Syntax.Name, line.Start.Line) return } s, err := parseString(&args[0]) @@ -132,8 +133,8 @@ func (f *File) add(errs *bytes.Buffer, line *Line, verb string, args []string, f } f.Module.Mod = module.Version{Path: s} case "require", "exclude": - if len(args) != 2 || !isString(args[0]) || isString(args[1]) { - fmt.Fprintf(errs, "%s:%d: usage: %s \"module/path\" v1.2.3\n", f.Syntax.Name, line.Start.Line, verb) + if len(args) != 2 { + fmt.Fprintf(errs, "%s:%d: usage: %s module/path v1.2.3\n", f.Syntax.Name, line.Start.Line, verb) return } s, err := parseString(&args[0]) @@ -168,8 +169,8 @@ func (f *File) add(errs *bytes.Buffer, line *Line, verb string, args []string, f }) } case "replace": - if len(args) < 4 || len(args) > 5 || !isString(args[0]) || isString(args[1]) || args[2] != "=>" || !isString(args[3]) || len(args) == 5 && isString(args[4]) { - fmt.Fprintf(errs, "%s:%d: usage: %s \"module/path\" v1.2.3 -> \"other/module\" v1.4\n\t or %s \"module/path\" v1.2.3 -> \"../local/directory\"", f.Syntax.Name, line.Start.Line, verb, verb) + if len(args) < 4 || len(args) > 5 || args[2] != "=>" { + fmt.Fprintf(errs, "%s:%d: usage: %s module/path v1.2.3 => other/module v1.4\n\t or %s module/path v1.2.3 => ../local/directory", f.Syntax.Name, line.Start.Line, verb, verb) return } s, err := parseString(&args[0]) @@ -237,21 +238,46 @@ func isDirectoryPath(ns string) bool { len(ns) >= 2 && ('A' <= ns[0] && ns[0] <= 'Z' || 'a' <= ns[0] && ns[0] <= 'z') && ns[1] == ':' } -func isString(s string) bool { - return s != "" && s[0] == '"' +func mustQuote(t string) bool { + for _, r := range t { + if !unicode.IsPrint(r) || r == ' ' || r == '"' || r == '\'' || r == '`' { + return true + } + } + return t == "" || strings.Contains(t, "//") || strings.Contains(t, "/*") +} + +// AutoQuote returns s or, if quoting is required for s to appear in a go.mod, +// the quotation of s. +func AutoQuote(s string) string { + if mustQuote(s) { + return strconv.Quote(s) + } + return s } func parseString(s *string) (string, error) { - t, err := strconv.Unquote(*s) - if err != nil { - return "", err + t := *s + if strings.HasPrefix(t, `"`) { + var err error + if t, err = strconv.Unquote(t); err != nil { + return "", err + } + } else if strings.ContainsAny(t, "\"'`") { + // Other quotes are reserved both for possible future expansion + // and to avoid confusion. For example if someone types 'x' + // we want that to be a syntax error and not a literal x in literal quotation marks. + return "", fmt.Errorf("unquoted string cannot contain quote") } - *s = strconv.Quote(t) + *s = AutoQuote(t) return t, nil } func parseVersion(path string, s *string, fix VersionFixer) (string, error) { - t := *s + t, err := parseString(s) + if err != nil { + return "", err + } if fix != nil { var err error t, err = fix(path, t) @@ -309,14 +335,14 @@ func (x *File) AddRequire(path, vers string) { switch stmt := stmt.(type) { case *LineBlock: if len(stmt.Token) > 0 && stmt.Token[0] == "require" { - syntax = &Line{Token: []string{strconv.Quote(path), vers}} + syntax = &Line{Token: []string{AutoQuote(path), vers}} stmt.Line = append(stmt.Line, syntax) goto End } case *Line: if len(stmt.Token) > 0 && stmt.Token[0] == "require" { stmt.Token = stmt.Token[1:] - syntax = &Line{Token: []string{strconv.Quote(path), vers}} + syntax = &Line{Token: []string{AutoQuote(path), vers}} x.Syntax.Stmt[i] = &LineBlock{ Comments: stmt.Comments, Token: []string{"require"}, @@ -330,7 +356,7 @@ func (x *File) AddRequire(path, vers string) { } } - syntax = &Line{Token: []string{"require", strconv.Quote(path), vers}} + syntax = &Line{Token: []string{"require", AutoQuote(path), vers}} x.Syntax.Stmt = append(x.Syntax.Stmt, syntax) End: diff --git a/vendor/cmd/go/internal/modfile/testdata/module.golden b/vendor/cmd/go/internal/modfile/testdata/module.golden index 08f3836..78ba943 100644 --- a/vendor/cmd/go/internal/modfile/testdata/module.golden +++ b/vendor/cmd/go/internal/modfile/testdata/module.golden @@ -1 +1 @@ -module "abc" +module abc diff --git a/vendor/cmd/go/internal/modfile/testdata/replace.golden b/vendor/cmd/go/internal/modfile/testdata/replace.golden index 6852499..5d6abcf 100644 --- a/vendor/cmd/go/internal/modfile/testdata/replace.golden +++ b/vendor/cmd/go/internal/modfile/testdata/replace.golden @@ -1,5 +1,5 @@ -module "abc" +module abc -replace "xyz" v1.2.3 => "/tmp/z" +replace xyz v1.2.3 => /tmp/z -replace "xyz" v1.3.4 => "my/xyz" v1.3.4-me +replace xyz v1.3.4 => my/xyz v1.3.4-me diff --git a/vendor/cmd/go/internal/modfile/testdata/replace2.golden b/vendor/cmd/go/internal/modfile/testdata/replace2.golden index e80962e..1d18a3b 100644 --- a/vendor/cmd/go/internal/modfile/testdata/replace2.golden +++ b/vendor/cmd/go/internal/modfile/testdata/replace2.golden @@ -1,6 +1,8 @@ -module "abc" +module abc replace ( - "xyz" v1.2.3 => "/tmp/z" - "xyz" v1.3.4 => "my/xyz" v1.3.4-me + xyz v1.2.3 => /tmp/z + xyz v1.3.4 => my/xyz v1.3.4-me + xyz v1.4.5 => "/tmp/my dir" + xyz v1.5.6 => my/xyz v1.5.6 ) diff --git a/vendor/cmd/go/internal/modfile/testdata/replace2.in b/vendor/cmd/go/internal/modfile/testdata/replace2.in index e80962e..78c4669 100644 --- a/vendor/cmd/go/internal/modfile/testdata/replace2.in +++ b/vendor/cmd/go/internal/modfile/testdata/replace2.in @@ -2,5 +2,7 @@ module "abc" replace ( "xyz" v1.2.3 => "/tmp/z" - "xyz" v1.3.4 => "my/xyz" v1.3.4-me + "xyz" v1.3.4 => "my/xyz" "v1.3.4-me" + xyz "v1.4.5" => "/tmp/my dir" + xyz v1.5.6 => my/xyz v1.5.6 )