rules_go improvement to externalize the nogo fix

Author: peng3141

go/.DS_Store

@@ -90,6 +90,7 @@ def emit_compilepkg(
         fail("nogo must be specified if and only if out_nogo_validation is specified")
     if bool(nogo) != bool(out_nogo_fix):
         fail("nogo must be specified if and only if out_nogo_fix is specified")
+
     if cover and go.coverdata:
         archives = archives + [go.coverdata]
 

go/private/actions/compilepkg.bzl

@@ -148,12 +148,14 @@ def _go_binary_impl(ctx):
         executable = executable,
     )
     validation_output = archive.data._validation_output
+    nogo_fix_output = archive.data._out_nogo_fix
 
     providers = [
         archive,
         OutputGroupInfo(
             cgo_exports = archive.cgo_exports,
             compilation_outputs = [archive.data.file],
+            nogo_fix = [nogo_fix_output] if nogo_fix_output else [],
             _validation = [validation_output] if validation_output else [],
         ),
     ]

go/private/rules/binary.bzl

@@ -66,7 +66,7 @@ def _go_library_impl(ctx):
         OutputGroupInfo(
             cgo_exports = archive.cgo_exports,
             compilation_outputs = [archive.data.file],
-            out_nogo_fix = [nogo_fix_output] if nogo_fix_output else [],
+            nogo_fix = [nogo_fix_output] if nogo_fix_output else [],
             _validation = [validation_output] if validation_output else [],
         ),
     ]

go/private/rules/library.bzl

@@ -68,13 +68,19 @@ def _go_test_impl(ctx):
     )
 
     validation_outputs = []
+    nogo_fix_outputs = []
 
     # Compile the library to test with internal white box tests
     internal_library = go.new_library(go, testfilter = "exclude")
     internal_source = go.library_to_source(go, ctx.attr, internal_library, ctx.coverage_instrumented())
     internal_archive = go.archive(go, internal_source)
     if internal_archive.data._validation_output:
         validation_outputs.append(internal_archive.data._validation_output)
+    if internal_archive.data._out_nogo_fix:
+        # Note we will not include external_archive.data._out_nogo_fix since
+        # they correspond to the packages external to the current workspace.
+        nogo_fix_outputs.append(internal_archive.data._out_nogo_fix)
+
     go_srcs = [src for src in internal_source.srcs if src.extension == "go"]
 
     # Compile the library with the external black box tests
@@ -199,6 +205,7 @@ def _go_test_impl(ctx):
         ),
         OutputGroupInfo(
             compilation_outputs = [internal_archive.data.file],
+            nogo_fix = nogo_fix_outputs,
             _validation = validation_outputs,
         ),
         coverage_common.instrumented_files_info(

go/private/rules/test.bzl

@@ -91,6 +91,7 @@ def _go_download_sdk_impl(ctx):
         )
 
         data = ctx.read("versions.json")
+        ctx.delete("versions.json")
         sdks_by_version = _parse_versions_json(data)
 
         if not version:

go/private/sdk.bzl

@@ -31,6 +31,20 @@ go_test(
     ],
 )
 
+go_test(
+    name = "nogo_change_test",
+    size = "small",
+    srcs = [
+        "nogo_change.go",
+        "nogo_change_serialization.go",
+        "nogo_change_serialization_test.go",
+        "nogo_change_test.go",
+    ],
+    deps = [
+        "@org_golang_x_tools//go/analysis",
+    ],
+)
+
 go_test(
     name = "stdliblist_test",
     size = "small",
@@ -99,7 +113,6 @@ go_source(
         "flags.go",
         "nogo_change.go",
         "nogo_change_serialization.go",
-        "nogo_edit.go",
         "nogo_main.go",
         "nogo_typeparams_go117.go",
         "nogo_typeparams_go118.go",

go/tools/builders/BUILD.bazel

@@ -62,6 +62,6 @@ func main() {
 	log.SetPrefix(verb + ": ")
 
 	if err := action(rest); err != nil {
-		log.Fatalf("\n$$$$$$$$$$$$$$$$$$$$$$$$ fatal: %+v", err)
+		log.Fatal(err)
 	}
 }

go/tools/builders/builder.go

@@ -40,8 +40,7 @@ func nogo(args []string) error {
 	fs.StringVar(&nogoPath, "nogo", "", "The nogo binary")
 	fs.StringVar(&outFactsPath, "out_facts", "", "The file to emit serialized nogo facts to")
 	fs.StringVar(&outLogPath, "out_log", "", "The file to emit nogo logs into")
-
-	fs.StringVar(&nogoFixPath, "fixpath", "", "The fix path")
+	fs.StringVar(&nogoFixPath, "fixpath", "", "The path of the file that stores the nogo fixes")
 
 	if err := fs.Parse(args); err != nil {
 		return err
@@ -90,6 +89,7 @@ func nogo(args []string) error {
 }
 
 func runNogo(workDir string, nogoPath string, srcs, ignores []string, facts []archive, packagePath, importcfgPath, outFactsPath string, outLogPath string, nogoFixPath string) error {
+
 	if len(srcs) == 0 {
 		// emit_compilepkg expects a nogo facts file, even if it's empty.
 		// We also need to write the validation output log.
@@ -101,14 +101,15 @@ func runNogo(workDir string, nogoPath string, srcs, ignores []string, facts []ar
 		if err != nil {
 			return fmt.Errorf("error writing empty nogo log file: %v", err)
 		}
+		err = os.WriteFile(nogoFixPath, nil, 0o666)
+		if err != nil {
+			return fmt.Errorf("error writing empty nogo fix file: %v", err)
+		}
 		return nil
 	}
 	args := []string{nogoPath}
 	args = append(args, "-p", packagePath)
 	args = append(args, "-fixpath", nogoFixPath)
-
-
-	// args = append(args, "-json")
 	args = append(args, "-importcfg", importcfgPath)
 	for _, fact := range facts {
 		args = append(args, "-fact", fmt.Sprintf("%s=%s", fact.importPath, fact.file))

go/tools/builders/nogo.go

@@ -1,85 +1,88 @@
 package main
 
+
 import (
 	"fmt"
 	"go/token"
-	"strings"
+	"os"
+	"path/filepath"
 
 	"golang.org/x/tools/go/analysis"
 )
 
+// DiagnosticEntry represents a diagnostic entry with the corresponding analyzer.
+type DiagnosticEntry struct {
+	analysis.Diagnostic
+	*analysis.Analyzer
+}
+
+// An Edit describes the replacement of a portion of a text file.
+type Edit struct {
+	New   string `json:"new"`   // the replacement
+	Start int    `json:"start"` // starting byte offset of the region to replace
+	End   int    `json:"end"`   // ending byte offset of the region to replace
+}
+
+
 // Change represents a set of edits to be applied to a set of files.
 type Change struct {
-	AnalysisName string            `json:"analysis_name"`
-	FileToEdits  map[string][]Edit `json:"file_to_edits"`
+	AnalyzerToFileToEdits map[string]map[string][]Edit `json:"analyzer_file_to_edits"`
 }
 
+
+
 // NewChange creates a new Change object.
 func NewChange() *Change {
 	return &Change{
-		FileToEdits: make(map[string][]Edit),
+		AnalyzerToFileToEdits: make(map[string]map[string][]Edit),
 	}
 }
 
-// SetAnalysisName sets the name of the analysis that produced the change.
-func (c *Change) SetAnalysisName(name string) {
-	c.AnalysisName = name
-}
-
-// AddEdit adds an edit to the change.
-func (c *Change) AddEdit(file string, edit Edit) {
-	c.FileToEdits[file] = append(c.FileToEdits[file], edit)
-}
-
-// BuildFromDiagnostics builds a Change from a set of diagnostics.
+// NewChangeFromDiagnostics builds a Change from a set of diagnostics.
 // Unlike Diagnostic, Change is independent of the FileSet given it uses perf-file offsets instead of token.Pos.
 // This allows Change to be used in contexts where the FileSet is not available, e.g., it remains applicable after it is saved to disk and loaded back.
 // See https://github.com/golang/tools/blob/master/go/analysis/diagnostic.go for details.
-func (c *Change) BuildFromDiagnostics(diagnostics []analysis.Diagnostic, fileSet *token.FileSet) error {
-	for _, diag := range diagnostics {
-		for _, sf := range diag.SuggestedFixes {
+func NewChangeFromDiagnostics(entries []DiagnosticEntry, fileSet *token.FileSet) (*Change, error) {
+	c := NewChange()
+	cwd, err := os.Getwd() // workspace root
+	if err != nil {
+		return c, fmt.Errorf("Error getting current working directory: (%v)", err)
+	}
+	for _, entry := range entries {
+		analyzer := entry.Analyzer.Name
+		for _, sf := range entry.Diagnostic.SuggestedFixes {
 			for _, edit := range sf.TextEdits {
 				file := fileSet.File(edit.Pos)
 
 				if file == nil {
-					return fmt.Errorf("invalid fix: missing file info for pos (%v)", edit.Pos)
+					return c, fmt.Errorf("invalid fix: missing file info for pos (%v)", edit.Pos)
 				}
 				if edit.Pos > edit.End {
-					return fmt.Errorf("invalid fix: pos (%v) > end (%v)", edit.Pos, edit.End)
+					return c, fmt.Errorf("invalid fix: pos (%v) > end (%v)", edit.Pos, edit.End)
 				}
 				if eof := token.Pos(file.Base() + file.Size()); edit.End > eof {
-					return fmt.Errorf("invalid fix: end (%v) past end of file (%v)", edit.End, eof)
+					return c, fmt.Errorf("invalid fix: end (%v) past end of file (%v)", edit.End, eof)
 				}
 				edit := Edit{Start: file.Offset(edit.Pos), End: file.Offset(edit.End), New: string(edit.NewText)}
-				fileRelativePath := file.Name()
-				c.AddEdit(fileRelativePath, edit)
+				fileRelativePath, err := filepath.Rel(cwd, file.Name())
+				if err != nil {
+					fileRelativePath = file.Name() // fallback logic
+				}
+				c.AddEdit(analyzer, fileRelativePath, edit)
 			}
 		}
 	}
-	return nil
+	return c, nil
 }
 
-// MergeChanges merges multiple changes into a single change.
-func MergeChanges(changes []Change) Change {
-	mergedChange := NewChange() // Create a new Change object for the result
-	analysisNames := []string{} // no deduplication needed
-
-	for _, change := range changes {
-		if change.AnalysisName != "" {
-			analysisNames = append(analysisNames, change.AnalysisName)
-		}
-		for file, edits := range change.FileToEdits {
-			// If the file already exists in the merged change, append the edits
-			if existingEdits, found := mergedChange.FileToEdits[file]; found {
-				// checking the overlapping of edits happens in edit.go during the ApplyEdits function.
-				// so we don't need to check it here.
-				mergedChange.FileToEdits[file] = append(existingEdits, edits...)
-			} else {
-				// Otherwise, just set the file and edits
-				mergedChange.FileToEdits[file] = edits
-			}
-		}
+// AddEdit adds an edit to the change.
+func (c *Change) AddEdit(analyzer string, file string, edit Edit) {
+	// Check if the analyzer exists in the map
+	if _, ok := c.AnalyzerToFileToEdits[analyzer]; !ok {
+		// Initialize the map for the analyzer if it doesn't exist
+		c.AnalyzerToFileToEdits[analyzer] = make(map[string][]Edit)
 	}
-	mergedChange.AnalysisName = strings.Join(analysisNames, ",")
-	return *mergedChange
+
+	// Append the edit to the list of edits for the specific file under the analyzer
+	c.AnalyzerToFileToEdits[analyzer][file] = append(c.AnalyzerToFileToEdits[analyzer][file], edit)
 }

go/tools/builders/nogo_change.go

@@ -4,19 +4,25 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
-	// "log"
+	"os"
 )
 
 // SaveToFile saves the Change struct to a JSON file.
 func SaveToFile(filename string, change Change) error {
 	// Serialize Change to JSON
 	jsonData, err := json.MarshalIndent(change, "", "  ")
 	if err != nil {
-		return fmt.Errorf("error serializing to JSON: %v", err)
+		// needs to write to the bazel-declared output anyway.
+		errWrite := os.WriteFile(filename, []byte(""), 0644)
+		if errWrite != nil {
+			return fmt.Errorf("error serializing to JSON: %v and error writing to the file: %v", err, errWrite)
+		} else {
+			return fmt.Errorf("error serializing to JSON: %v", err)
+		}
 	}
-	// log.Fatalf("!!!!: %v", change)
+
 	// Write the JSON data to the file
-	err = ioutil.WriteFile(filename, jsonData, 0644)
+	err = os.WriteFile(filename, jsonData, 0644)
 	if err != nil {
 		return fmt.Errorf("error writing to file: %v", err)
 	}

go/tools/builders/nogo_change_serialization.go

@@ -0,0 +1,39 @@
+package main
+
+import (
+	"os"
+	"reflect"
+	"testing"
+)
+
+func TestSaveLoad(t *testing.T) {
+	// Create a temporary file
+	file, err := os.CreateTemp("", "tmp_file")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(file.Name())
+
+	// Initialize a Change struct with some edits and analyzers
+	change := *NewChange()
+	change.AddEdit("AnalyzerA", "file1.txt", Edit{New: "replacement1", Start: 0, End: 5})
+	change.AddEdit("AnalyzerA", "file1.txt", Edit{New: "replacement2", Start: 10, End: 15})
+	change.AddEdit("AnalyzerB", "file2.txt", Edit{New: "new text", Start: 20, End: 25})
+
+	// Test saving to file
+	err = SaveToFile(file.Name(), change)
+	if err != nil {
+		t.Fatalf("Failed to save Change struct to file: %v", err)
+	}
+
+	// Test loading from file
+	loadedChange, err := LoadFromFile(file.Name())
+	if err != nil {
+		t.Fatalf("Failed to load Change struct from file: %v", err)
+	}
+
+	// Compare original and loaded Change structs
+	if !reflect.DeepEqual(change, loadedChange) {
+		t.Fatalf("Loaded Change struct does not match original.\nOriginal: %+v\nLoaded: %+v", change, loadedChange)
+	}
+}