Skip to content

Commit

Permalink
rules_go improvement to externalize the nogo fix
Browse files Browse the repository at this point in the history
  • Loading branch information
peng3141 committed Sep 25, 2024
1 parent 36618c9 commit f506b28
Show file tree
Hide file tree
Showing 17 changed files with 368 additions and 236 deletions.
Binary file removed go/.DS_Store
Binary file not shown.
1 change: 1 addition & 0 deletions go/private/actions/compilepkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
2 changes: 2 additions & 0 deletions go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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 [],
),
]
Expand Down
2 changes: 1 addition & 1 deletion go/private/rules/library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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 [],
),
]
Expand Down
7 changes: 7 additions & 0 deletions go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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 represent 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
Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions go/private/sdk.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
15 changes: 14 additions & 1 deletion go/tools/builders/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion go/tools/builders/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,6 @@ func main() {
log.SetPrefix(verb + ": ")

if err := action(rest); err != nil {
log.Fatalf("\n$$$$$$$$$$$$$$$$$$$$$$$$ fatal: %+v", err)
log.Fatal(err)
}
}
11 changes: 6 additions & 5 deletions go/tools/builders/nogo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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))
Expand Down
95 changes: 49 additions & 46 deletions go/tools/builders/nogo_change.go
Original file line number Diff line number Diff line change
@@ -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)
}
14 changes: 10 additions & 4 deletions go/tools/builders/nogo_change_serialization.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
39 changes: 39 additions & 0 deletions go/tools/builders/nogo_change_serialization_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
Loading

0 comments on commit f506b28

Please sign in to comment.