refactor/rename: fail cleanly when attempting to rename cgo files

Previous implementation will overwrite files the import "C" with the
cgo preprocessing and renaming. Rename will now emit an error when
rename must edit files that import "C". Will also emit more useful
error when using -offset in a "C" importing file.

Fixes golang/go#17839

Change-Id: I072100b22197ec145b56d727feca58be7529e359
Reviewed-on: https://go-review.googlesource.com/45930
Reviewed-by: Alan Donovan <adonovan@google.com>

cmd/gorename/gorename_test.go

@@ -0,0 +1,364 @@
+package main_test
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+type test struct {
+	offset, from, to string // specify the arguments
+	fileSpecified    bool   // true if the offset or from args specify a specific file
+	pkgs             map[string][]string
+	wantErr          bool
+	wantOut          string              // a substring expected to be in the output
+	packages         map[string][]string // a map of the package name to the files contained within, which will be numbered by i.go where i is the index
+}
+
+// Test that renaming that would modify cgo files will produce an error and not modify the file.
+func TestGeneratedFiles(t *testing.T) {
+	tmp, bin, cleanup := buildGorename(t)
+	defer cleanup()
+
+	srcDir := filepath.Join(tmp, "src")
+	err := os.Mkdir(srcDir, os.ModePerm)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	env := append(os.Environ(), fmt.Sprintf("GOPATH=%s", tmp))
+	// Testing renaming in packages that include cgo files:
+	for iter, renameTest := range []test{
+		{
+			// Test: variable not used in any cgo file -> no error
+			from: `"mytest"::f`, to: "g",
+			packages: map[string][]string{
+				"mytest": []string{`package mytest; func f() {}`,
+					`package mytest
+// #include <stdio.h>
+import "C"
+
+func z() {C.puts(nil)}`},
+			},
+			wantErr: false,
+			wantOut: "Renamed 1 occurrence in 1 file in 1 package.",
+		}, {
+			// Test: to name used in cgo file -> rename error
+			from: `"mytest"::f`, to: "g",
+			packages: map[string][]string{
+				"mytest": []string{`package mytest; func f() {}`,
+					`package mytest
+// #include <stdio.h>
+import "C"
+
+func g() {C.puts(nil)}`},
+			},
+			wantErr: true,
+			wantOut: "conflicts with func in same block",
+		},
+		{
+			// Test: from name in package in cgo file -> error
+			from: `"mytest"::f`, to: "g",
+			packages: map[string][]string{
+				"mytest": []string{`package mytest
+
+// #include <stdio.h>
+import "C"
+
+func f() { C.puts(nil); }
+`},
+			},
+			wantErr: true,
+			wantOut: "gorename: refusing to modify generated file containing DO NOT EDIT marker:",
+		}, {
+			// Test: from name in cgo file -> error
+			from: filepath.Join("mytest", "0.go") + `::f`, to: "g",
+			fileSpecified: true,
+			packages: map[string][]string{
+				"mytest": []string{`package mytest
+
+// #include <stdio.h>
+import "C"
+
+func f() { C.puts(nil); }
+`},
+			},
+			wantErr: true,
+			wantOut: "gorename: refusing to modify generated file containing DO NOT EDIT marker:",
+		}, {
+			// Test: offset in cgo file -> identifier in cgo error
+			offset: filepath.Join("main", "0.go") + `:#78`, to: "bar",
+			fileSpecified: true,
+			wantErr:       true,
+			packages: map[string][]string{
+				"main": {`package main
+
+// #include <unistd.h>
+import "C"
+import "fmt"
+
+func main() {
+	foo := 1
+	C.close(2)
+	fmt.Println(foo)
+}
+`},
+			},
+			wantOut: "cannot rename identifiers in generated file containing DO NOT EDIT marker:",
+		}, {
+			// Test: from identifier appears in cgo file in another package -> error
+			from: `"test"::Foo`, to: "Bar",
+			packages: map[string][]string{
+				"test": []string{
+					`package test
+
+func Foo(x int) (int){
+	return x * 2
+}
+`,
+				},
+				"main": []string{
+					`package main
+
+import "test"
+import "fmt"
+// #include <unistd.h>
+import "C"
+
+func fun() {
+	x := test.Foo(3)
+	C.close(3)
+	fmt.Println(x)
+}
+`,
+				},
+			},
+			wantErr: true,
+			wantOut: "gorename: refusing to modify generated file containing DO NOT EDIT marker:",
+		}, {
+			// Test: from identifier doesn't appear in cgo file that includes modified package -> rename successful
+			from: `"test".Foo::x`, to: "y",
+			packages: map[string][]string{
+				"test": []string{
+					`package test
+
+func Foo(x int) (int){
+	return x * 2
+}
+`,
+				},
+				"main": []string{
+					`package main
+import "test"
+import "fmt"
+// #include <unistd.h>
+import "C"
+
+func fun() {
+	x := test.Foo(3)
+	C.close(3)
+	fmt.Println(x)
+}
+`,
+				},
+			},
+			wantErr: false,
+			wantOut: "Renamed 2 occurrences in 1 file in 1 package.",
+		}, {
+			// Test: from name appears in cgo file in same package -> error
+			from: `"mytest"::f`, to: "g",
+			packages: map[string][]string{
+				"mytest": []string{`package mytest; func f() {}`,
+					`package mytest
+// #include <stdio.h>
+import "C"
+
+func z() {C.puts(nil); f()}`,
+					`package mytest
+// #include <unistd.h>
+import "C"
+
+func foo() {C.close(3); f()}`,
+				},
+			},
+			wantErr: true,
+			wantOut: "gorename: refusing to modify generated files containing DO NOT EDIT marker:",
+		}, {
+			// Test: from name in file, identifier not used in cgo file -> rename successful
+			from: filepath.Join("mytest", "0.go") + `::f`, to: "g",
+			fileSpecified: true,
+			packages: map[string][]string{
+				"mytest": []string{`package mytest; func f() {}`,
+					`package mytest
+// #include <stdio.h>
+import "C"
+
+func z() {C.puts(nil)}`},
+			},
+			wantErr: false,
+			wantOut: "Renamed 1 occurrence in 1 file in 1 package.",
+		}, {
+			// Test: from identifier imported to another package but does not modify cgo file -> rename successful
+			from: `"test".Foo`, to: "Bar",
+			packages: map[string][]string{
+				"test": []string{
+					`package test
+
+func Foo(x int) (int){
+	return x * 2
+}
+`,
+				},
+				"main": []string{
+					`package main
+// #include <unistd.h>
+import "C"
+
+func fun() {
+	C.close(3)
+}
+`,
+					`package main
+import "test"
+import "fmt"
+func g() { fmt.Println(test.Foo(3)) }
+`,
+				},
+			},
+			wantErr: false,
+			wantOut: "Renamed 2 occurrences in 2 files in 2 packages.",
+		},
+	} {
+		// Write the test files
+		testCleanup := setUpPackages(t, srcDir, renameTest.packages)
+
+		// Set up arguments
+		var args []string
+
+		var arg, val string
+		if renameTest.offset != "" {
+			arg, val = "-offset", renameTest.offset
+		} else {
+			arg, val = "-from", renameTest.from
+		}
+
+		prefix := fmt.Sprintf("%d: %s %q -to %q", iter, arg, val, renameTest.to)
+
+		if renameTest.fileSpecified {
+			// add the src dir to the value of the argument
+			val = filepath.Join(srcDir, val)
+		}
+
+		args = append(args, arg, val, "-to", renameTest.to)
+
+		// Run command
+		cmd := exec.Command(bin, args...)
+		cmd.Args[0] = "gorename"
+		cmd.Env = env
+
+		// Check the output
+		out, err := cmd.CombinedOutput()
+		// errors should result in no changes to files
+		if err != nil {
+			if !renameTest.wantErr {
+				t.Errorf("%s: received unexpected error %s", prefix, err)
+			}
+			// Compare output
+			if ok := strings.Contains(string(out), renameTest.wantOut); !ok {
+				t.Errorf("%s: unexpected command output: %s (want: %s)", prefix, out, renameTest.wantOut)
+			}
+			// Check that no files were modified
+			if modified := modifiedFiles(t, srcDir, renameTest.packages); len(modified) != 0 {
+				t.Errorf("%s: files unexpectedly modified: %s", prefix, modified)
+			}
+
+		} else {
+			if !renameTest.wantErr {
+				if ok := strings.Contains(string(out), renameTest.wantOut); !ok {
+					t.Errorf("%s: unexpected command output: %s (want: %s)", prefix, out, renameTest.wantOut)
+				}
+			} else {
+				t.Errorf("%s: command succeeded unexpectedly, output: %s", prefix, out)
+			}
+		}
+		testCleanup()
+	}
+}
+
+// buildGorename builds the gorename executable.
+// It returns its path, and a cleanup function.
+func buildGorename(t *testing.T) (tmp, bin string, cleanup func()) {
+
+	tmp, err := ioutil.TempDir("", "gorename-regtest-")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	defer func() {
+		if cleanup == nil { // probably, go build failed.
+			os.RemoveAll(tmp)
+		}
+	}()
+
+	bin = filepath.Join(tmp, "gorename")
+	if runtime.GOOS == "windows" {
+		bin += ".exe"
+	}
+	cmd := exec.Command("go", "build", "-o", bin)
+	if err := cmd.Run(); err != nil {
+		t.Fatalf("Building gorename: %v", err)
+	}
+	return tmp, bin, func() { os.RemoveAll(tmp) }
+}
+
+// setUpPackages sets up the files in a temporary directory provided by arguments.
+func setUpPackages(t *testing.T, dir string, packages map[string][]string) (cleanup func()) {
+	var pkgDirs []string
+
+	for pkgName, files := range packages {
+		// Create a directory for the package.
+		pkgDir := filepath.Join(dir, pkgName)
+		pkgDirs = append(pkgDirs, pkgDir)
+
+		if err := os.Mkdir(pkgDir, os.ModePerm); err != nil {
+			t.Fatal(err)
+		}
+		// Write the packages files
+		for i, val := range files {
+			file := filepath.Join(pkgDir, strconv.Itoa(i)+".go")
+			if err := ioutil.WriteFile(file, []byte(val), os.ModePerm); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+	return func() {
+		for _, dir := range pkgDirs {
+			os.RemoveAll(dir)
+		}
+	}
+}
+
+// modifiedFiles returns a list of files that were renamed (without the prefix dir).
+func modifiedFiles(t *testing.T, dir string, packages map[string][]string) (results []string) {
+
+	for pkgName, files := range packages {
+		pkgDir := filepath.Join(dir, pkgName)
+
+		for i, val := range files {
+			file := filepath.Join(pkgDir, strconv.Itoa(i)+".go")
+			// read file contents and compare to val
+			if contents, err := ioutil.ReadFile(file); err != nil {
+				t.Fatal("File missing: %s", err)
+			} else if string(contents) != val {
+				results = append(results, strings.TrimPrefix(dir, file))
+			}
+		}
+	}
+	return results
+}

refactor/rename/rename.go

@@ -363,7 +363,6 @@ func loadProgram(ctxt *build.Context, pkgs map[string]bool) (*loader.Program, er
 		// TODO(adonovan): enable this.  Requires making a lot of code more robust!
 		AllowErrors: false,
 	}
-
 	// Optimization: don't type-check the bodies of functions in our
 	// dependencies, since we only need exported package members.
 	conf.TypeCheckFuncBodies = func(p string) bool {
@@ -395,6 +394,7 @@ func loadProgram(ctxt *build.Context, pkgs map[string]bool) (*loader.Program, er
 	if err != nil {
 		return nil, err
 	}
+
 	var errpkgs []string
 	// Report hard errors in indirectly imported packages.
 	for _, info := range prog.AllPackages {
@@ -455,9 +455,9 @@ func (r *renamer) update() error {
 	// We use token.File, not filename, since a file may appear to
 	// belong to multiple packages and be parsed more than once.
 	// token.File captures this distinction; filename does not.
+
 	var nidents int
 	var filesToUpdate = make(map[*token.File]bool)
-
 	docRegexp := regexp.MustCompile(`\b` + r.from + `\b`)
 	for _, info := range r.packages {
 		// Mutate the ASTs and note the filenames.
@@ -484,7 +484,21 @@ func (r *renamer) update() error {
 		}
 	}
 
-	// TODO(adonovan): don't rewrite cgo + generated files.
+	// Renaming not supported if cgo files are affected.
+	var generatedFileNames []string
+	for _, info := range r.packages {
+		for _, f := range info.Files {
+			tokenFile := r.iprog.Fset.File(f.Pos())
+			if filesToUpdate[tokenFile] && generated(f, tokenFile) {
+				generatedFileNames = append(generatedFileNames, tokenFile.Name())
+			}
+		}
+	}
+	if len(generatedFileNames) > 0 {
+		return fmt.Errorf("refusing to modify generated file%s containing DO NOT EDIT marker: %v", plural(len(generatedFileNames)), generatedFileNames)
+	}
+
+	// Write affected files.
 	var nerrs, npkgs int
 	for _, info := range r.packages {
 		first := true

refactor/rename/spec.go

@@ -19,6 +19,7 @@ import (
 	"log"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -319,6 +320,11 @@ func findFromObjectsInFile(iprog *loader.Program, spec *spec) ([]types.Object, e
 			// This package contains the query file.
 
 			if spec.offset != 0 {
+				// We cannot refactor generated files since position information is invalidated.
+				if generated(f, thisFile) {
+					return nil, fmt.Errorf("cannot rename identifiers in generated file containing DO NOT EDIT marker: %s", thisFile.Name())
+				}
+
 				// Search for a specific ident by file/offset.
 				id := identAtOffset(iprog.Fset, f, spec.offset)
 				if id == nil {
@@ -564,3 +570,24 @@ func ambiguityError(fset *token.FileSet, objects []types.Object) error {
 	return fmt.Errorf("ambiguous specifier %s matches %s",
 		objects[0].Name(), buf.String())
 }
+
+// Matches cgo generated comment as well as the proposed standard:
+//	https://golang.org/s/generatedcode
+var generatedRx = regexp.MustCompile(`// .*DO NOT EDIT\.?`)
+
+// generated reports whether ast.File is a generated file.
+func generated(f *ast.File, tokenFile *token.File) bool {
+
+	// Iterate over the comments in the file
+	for _, commentGroup := range f.Comments {
+		for _, comment := range commentGroup.List {
+			if matched := generatedRx.MatchString(comment.Text); matched {
+				// Check if comment is at the beginning of the line in source
+				if pos := tokenFile.Position(comment.Slash); pos.Column == 1 {
+					return true
+				}
+			}
+		}
+	}
+	return false
+}