cmd/stringer: compile error when constants change

When constant values change but stringer has not
been run again, we can get misleading string values.
Protect against this by generating code that will fail
with a compiler error when this happens.
Most compilers should be smart enough to omit the
code containing the checks.

Change-Id: I7a36d20f014cba0e7d88851d1b649a098ee30d76
Reviewed-on: https://go-review.googlesource.com/c/tools/+/163637
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>

cmd/stringer/endtoend_test.go

@@ -45,8 +45,8 @@ func TestEndToEnd(t *testing.T) {
 			t.Errorf("%s is not a Go file", name)
 			continue
 		}
-		if strings.HasPrefix(name, "tag_") {
-			// This file is used for tag processing in TestTags, below.
+		if strings.HasPrefix(name, "tag_") || strings.HasPrefix(name, "vary_") {
+			// This file is used for tag processing in TestTags or TestConstValueChange, below.
 			continue
 		}
 		if name == "cgo.go" && !build.Default.CgoEnabled {
@@ -68,12 +68,10 @@ func TestTags(t *testing.T) {
 		output         = filepath.Join(dir, "const_string.go")
 	)
 	for _, file := range []string{"tag_main.go", "tag_tag.go"} {
-
 		err := copy(filepath.Join(dir, file), filepath.Join("testdata", file))
 		if err != nil {
 			t.Fatal(err)
 		}
-
 	}
 	// Run stringer in the directory that contains the package files.
 	// We cannot run stringer in the current directory for the following reasons:
@@ -108,6 +106,48 @@ func TestTags(t *testing.T) {
 	}
 }
 
+// TestConstValueChange verifies that if a constant value changes and
+// the stringer code is not regenerated, we'll get a compiler error.
+func TestConstValueChange(t *testing.T) {
+	dir, stringer := buildStringer(t)
+	defer os.RemoveAll(dir)
+	source := filepath.Join(dir, "day.go")
+	err := copy(source, filepath.Join("testdata", "day.go"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	stringSource := filepath.Join(dir, "day_string.go")
+	// Run stringer in the directory that contains the package files.
+	err = runInDir(dir, stringer, "-type", "Day", "-output", stringSource)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Run the binary in the temporary directory as a sanity check.
+	err = run("go", "run", stringSource, source)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Overwrite the source file with a version that has changed constants.
+	err = copy(source, filepath.Join("testdata", "vary_day.go"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Unfortunately different compilers may give different error messages,
+	// so there's no easy way to verify that the build failed specifically
+	// because the constants changed rather than because the vary_day.go
+	// file is invalid.
+	//
+	// Instead we'll just rely on manual inspection of the polluted test
+	// output. An alternative might be to check that the error output
+	// matches a set of possible error strings emitted by known
+	// Go compilers.
+	fmt.Fprintf(os.Stderr, "Note: the following messages should indicate an out-of-bounds compiler error\n")
+	err = run("go", "build", stringSource, source)
+	if err == nil {
+		t.Fatal("unexpected compiler success")
+	}
+}
+
 // buildStringer creates a temporary directory and installs stringer there.
 func buildStringer(t *testing.T) (dir string, stringer string) {
 	t.Helper()

cmd/stringer/golden_test.go

@@ -52,7 +52,19 @@ const (
 )
 `
 
-const day_out = `
+const day_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[Monday-0]
+	_ = x[Tuesday-1]
+	_ = x[Wednesday-2]
+	_ = x[Thursday-3]
+	_ = x[Friday-4]
+	_ = x[Saturday-5]
+	_ = x[Sunday-6]
+}
+
 const _Day_name = "MondayTuesdayWednesdayThursdayFridaySaturdaySunday"
 
 var _Day_index = [...]uint8{0, 6, 13, 22, 30, 36, 44, 50}
@@ -77,7 +89,15 @@ const (
 )
 `
 
-const offset_out = `
+const offset_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[One-1]
+	_ = x[Two-2]
+	_ = x[Three-3]
+}
+
 const _Number_name = "OneTwoThree"
 
 var _Number_index = [...]uint8{0, 3, 6, 11}
@@ -105,7 +125,20 @@ const (
 )
 `
 
-const gap_out = `
+const gap_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[Two-2]
+	_ = x[Three-3]
+	_ = x[Five-5]
+	_ = x[Six-6]
+	_ = x[Seven-7]
+	_ = x[Eight-8]
+	_ = x[Nine-9]
+	_ = x[Eleven-11]
+}
+
 const (
 	_Gap_name_0 = "TwoThree"
 	_Gap_name_1 = "FiveSixSevenEightNine"
@@ -144,7 +177,17 @@ const (
 )
 `
 
-const num_out = `
+const num_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[m_2 - -2]
+	_ = x[m_1 - -1]
+	_ = x[m0-0]
+	_ = x[m1-1]
+	_ = x[m2-2]
+}
+
 const _Num_name = "m_2m_1m0m1m2"
 
 var _Num_index = [...]uint8{0, 3, 6, 8, 10, 12}
@@ -172,7 +215,17 @@ const (
 )
 `
 
-const unum_out = `
+const unum_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[m_2-253]
+	_ = x[m_1-254]
+	_ = x[m0-0]
+	_ = x[m1-1]
+	_ = x[m2-2]
+}
+
 const (
 	_Unum_name_0 = "m0m1m2"
 	_Unum_name_1 = "m_2m_1"
@@ -217,7 +270,26 @@ const (
 )
 `
 
-const prime_out = `
+const prime_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[p2-2]
+	_ = x[p3-3]
+	_ = x[p5-5]
+	_ = x[p7-7]
+	_ = x[p77-7]
+	_ = x[p11-11]
+	_ = x[p13-13]
+	_ = x[p17-17]
+	_ = x[p19-19]
+	_ = x[p23-23]
+	_ = x[p29-29]
+	_ = x[p37-31]
+	_ = x[p41-41]
+	_ = x[p43-43]
+}
+
 const _Prime_name = "p2p3p5p7p11p13p17p19p23p29p37p41p43"
 
 var _Prime_map = map[Prime]string{
@@ -256,7 +328,19 @@ const (
 )
 `
 
-const prefix_out = `
+const prefix_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[TypeInt-0]
+	_ = x[TypeString-1]
+	_ = x[TypeFloat-2]
+	_ = x[TypeRune-3]
+	_ = x[TypeByte-4]
+	_ = x[TypeStruct-5]
+	_ = x[TypeSlice-6]
+}
+
 const _Type_name = "IntStringFloatRuneByteStructSlice"
 
 var _Type_index = [...]uint8{0, 3, 9, 14, 18, 22, 28, 33}
@@ -286,7 +370,21 @@ const (
 )
 `
 
-const tokens_out = `
+const tokens_out = `func _() {
+	// An "invalid array index" compiler error signifies that the constant values have changed.
+	// Re-run the stringer command to generate them again.
+	var x [1]struct{}
+	_ = x[And-0]
+	_ = x[Or-1]
+	_ = x[Add-2]
+	_ = x[Sub-3]
+	_ = x[Ident-4]
+	_ = x[Period-5]
+	_ = x[SingleBefore-6]
+	_ = x[BeforeAndInline-7]
+	_ = x[InlineGeneral-8]
+}
+
 const _Token_name = "&|+-Ident.SingleBeforeinlineinline general"
 
 var _Token_index = [...]uint8{0, 1, 2, 3, 4, 9, 10, 22, 28, 42}
@@ -328,7 +426,7 @@ func TestGolden(t *testing.T) {
 		g.generate(tokens[1])
 		got := string(g.format())
 		if got != test.output {
-			t.Errorf("%s: got\n====\n%s====\nexpected\n====%s", test.name, got, test.output)
+			t.Errorf("%s: got(%d)\n====\n%q====\nexpected(%d)\n====%q", test.name, len(got), got, len(test.output), test.output)
 		}
 	}
 }

cmd/stringer/stringer.go

@@ -258,6 +258,15 @@ func (g *Generator) generate(typeName string) {
 	if len(values) == 0 {
 		log.Fatalf("no values defined for type %s", typeName)
 	}
+	// Generate code that will fail if the constants change value.
+	g.Printf("func _() {\n")
+	g.Printf("\t// An \"invalid array index\" compiler error signifies that the constant values have changed.\n")
+	g.Printf("\t// Re-run the stringer command to generate them again.\n")
+	g.Printf("\tvar x [1]struct{}\n")
+	for _, v := range values {
+		g.Printf("\t_ = x[%s - %s]\n", v.originalName, v.str)
+	}
+	g.Printf("}\n")
 	runs := splitIntoRuns(values)
 	// The decision of which pattern to use depends on the number of
 	// runs in the numbers. If there's only one, it's easy. For more than
@@ -328,7 +337,8 @@ func (g *Generator) format() []byte {
 
 // Value represents a declared constant.
 type Value struct {
-	name string // The name of the constant.
+	originalName string // The name of the constant.
+	name         string // The name with trimmed prefix.
 	// The value is stored as a bit pattern alone. The boolean tells us
 	// whether to interpret it as an int64 or a uint64; the only place
 	// this matters is when sorting.
@@ -436,15 +446,16 @@ func (f *File) genDecl(node ast.Node) bool {
 				u64 = uint64(i64)
 			}
 			v := Value{
-				name:   name.Name,
-				value:  u64,
-				signed: info&types.IsUnsigned == 0,
-				str:    value.String(),
+				originalName: name.Name,
+				value:        u64,
+				signed:       info&types.IsUnsigned == 0,
+				str:          value.String(),
 			}
 			if c := vspec.Comment; f.lineComment && c != nil && len(c.List) == 1 {
 				v.name = strings.TrimSpace(c.Text())
+			} else {
+				v.name = strings.TrimPrefix(v.originalName, f.trimPrefix)
 			}
-			v.name = strings.TrimPrefix(v.name, f.trimPrefix)
 			f.values = append(f.values, v)
 		}
 	}

cmd/stringer/testdata/vary_day.go

@@ -0,0 +1,39 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This file is the same as day.go except the constants have different values.
+
+package main
+
+import "fmt"
+
+type Day int
+
+const (
+	Sunday Day = iota
+	Monday
+	Tuesday
+	Wednesday
+	Thursday
+	Friday
+	Saturday
+)
+
+func main() {
+	ck(Monday, "Monday")
+	ck(Tuesday, "Tuesday")
+	ck(Wednesday, "Wednesday")
+	ck(Thursday, "Thursday")
+	ck(Friday, "Friday")
+	ck(Saturday, "Saturday")
+	ck(Sunday, "Sunday")
+	ck(-127, "Day(-127)")
+	ck(127, "Day(127)")
+}
+
+func ck(day Day, str string) {
+	if fmt.Sprint(day) != str {
+		panic("day.go: " + str)
+	}
+}

cmd/stringer/util_test.go

@@ -54,7 +54,7 @@ Outer:
 	for n, test := range splitTests {
 		values := make([]Value, len(test.input))
 		for i, v := range test.input {
-			values[i] = Value{"", v, test.signed, fmt.Sprint(v)}
+			values[i] = Value{"", "", v, test.signed, fmt.Sprint(v)}
 		}
 		runs := splitIntoRuns(values)
 		if len(runs) != len(test.output) {