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 <bcmills@google.com>

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"

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",
 	},
 }
 

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
 			)`,
 		},
 	}

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.

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") {

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:

vendor/cmd/go/internal/modfile/testdata/module.golden

@@ -1 +1 @@
-module "abc"
+module abc

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

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
 )

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
 )