From 80269b98ba1163485edf0972d1b9e93353f67e5b Mon Sep 17 00:00:00 2001 From: Ayke van Laethem Date: Fri, 12 Jul 2024 18:28:53 +0200 Subject: [PATCH] diagnostics: move diagnostic printing to a new package This is a refactor, which should (in theory) not change the behavior of the compiler. But since this is a pretty large change, there is a chance there will be some regressions. For that reason, the previous commits added a bunch of tests to make sure most error messages will not be changed due to this refactor. --- diagnostics/diagnostics.go | 196 +++++++++++++++++++++++++++++++++++++ errors_test.go | 6 +- main.go | 96 +----------------- main_test.go | 7 +- 4 files changed, 207 insertions(+), 98 deletions(-) create mode 100644 diagnostics/diagnostics.go diff --git a/diagnostics/diagnostics.go b/diagnostics/diagnostics.go new file mode 100644 index 00000000..c793ba4a --- /dev/null +++ b/diagnostics/diagnostics.go @@ -0,0 +1,196 @@ +// Package diagnostics formats compiler errors and prints them in a consistent +// way. +package diagnostics + +import ( + "bytes" + "fmt" + "go/scanner" + "go/token" + "go/types" + "io" + "path/filepath" + "strings" + + "github.com/tinygo-org/tinygo/builder" + "github.com/tinygo-org/tinygo/goenv" + "github.com/tinygo-org/tinygo/interp" + "github.com/tinygo-org/tinygo/loader" +) + +// A single diagnostic. +type Diagnostic struct { + Pos token.Position + Msg string +} + +// One or multiple errors of a particular package. +// It can also represent whole-program errors (like linker errors) that can't +// easily be connected to a single package. +type PackageDiagnostic struct { + ImportPath string // the same ImportPath as in `go list -json` + Diagnostics []Diagnostic +} + +// Diagnostics of a whole program. This can include errors belonging to multiple +// packages, or just a single package. +type ProgramDiagnostic []PackageDiagnostic + +// CreateDiagnostics reads the underlying errors in the error object and creates +// a set of diagnostics that's sorted and can be readily printed. +func CreateDiagnostics(err error) ProgramDiagnostic { + if err == nil { + return nil + } + switch err := err.(type) { + case *builder.MultiError: + var diags ProgramDiagnostic + for _, err := range err.Errs { + diags = append(diags, createPackageDiagnostic(err)) + } + return diags + default: + return ProgramDiagnostic{ + createPackageDiagnostic(err), + } + } +} + +// Create diagnostics for a single package (though, in practice, it may also be +// used for whole-program diagnostics in some cases). +func createPackageDiagnostic(err error) PackageDiagnostic { + var pkgDiag PackageDiagnostic + switch err := err.(type) { + case loader.Errors: + if err.Pkg != nil { + pkgDiag.ImportPath = err.Pkg.ImportPath + } + for _, err := range err.Errs { + diags := createDiagnostics(err) + pkgDiag.Diagnostics = append(pkgDiag.Diagnostics, diags...) + } + case *interp.Error: + pkgDiag.ImportPath = err.ImportPath + w := &bytes.Buffer{} + fmt.Fprintln(w, err.Error()) + if len(err.Inst) != 0 { + fmt.Fprintln(w, err.Inst) + } + if len(err.Traceback) > 0 { + fmt.Fprintln(w, "\ntraceback:") + for _, line := range err.Traceback { + fmt.Fprintln(w, line.Pos.String()+":") + fmt.Fprintln(w, line.Inst) + } + } + pkgDiag.Diagnostics = append(pkgDiag.Diagnostics, Diagnostic{ + Msg: w.String(), + }) + default: + pkgDiag.Diagnostics = createDiagnostics(err) + } + // TODO: sort + return pkgDiag +} + +// Extract diagnostics from the given error message and return them as a slice +// of errors (which in many cases will just be a single diagnostic). +func createDiagnostics(err error) []Diagnostic { + switch err := err.(type) { + case types.Error: + return []Diagnostic{ + { + Pos: err.Fset.Position(err.Pos), + Msg: err.Msg, + }, + } + case scanner.Error: + return []Diagnostic{ + { + Pos: err.Pos, + Msg: err.Msg, + }, + } + case scanner.ErrorList: + var diags []Diagnostic + for _, err := range err { + diags = append(diags, createDiagnostics(*err)...) + } + return diags + case loader.Error: + if err.Err.Pos.Filename != "" { + // Probably a syntax error in a dependency. + return createDiagnostics(err.Err) + } else { + // Probably an "import cycle not allowed" error. + buf := &bytes.Buffer{} + fmt.Fprintln(buf, "package", err.ImportStack[0]) + for i := 1; i < len(err.ImportStack); i++ { + pkgPath := err.ImportStack[i] + if i == len(err.ImportStack)-1 { + // last package + fmt.Fprintln(buf, "\timports", pkgPath+": "+err.Err.Error()) + } else { + // not the last pacakge + fmt.Fprintln(buf, "\timports", pkgPath) + } + } + return []Diagnostic{ + {Msg: buf.String()}, + } + } + default: + return []Diagnostic{ + {Msg: err.Error()}, + } + } +} + +// Write program diagnostics to the given writer with 'wd' as the relative +// working directory. +func (progDiag ProgramDiagnostic) WriteTo(w io.Writer, wd string) { + for _, pkgDiag := range progDiag { + pkgDiag.WriteTo(w, wd) + } +} + +// Write package diagnostics to the given writer with 'wd' as the relative +// working directory. +func (pkgDiag PackageDiagnostic) WriteTo(w io.Writer, wd string) { + if pkgDiag.ImportPath != "" { + fmt.Fprintln(w, "#", pkgDiag.ImportPath) + } + for _, diag := range pkgDiag.Diagnostics { + diag.WriteTo(w, wd) + } +} + +// Write this diagnostic to the given writer with 'wd' as the relative working +// directory. +func (diag Diagnostic) WriteTo(w io.Writer, wd string) { + if diag.Pos == (token.Position{}) { + fmt.Fprintln(w, diag.Msg) + return + } + pos := diag.Pos // make a copy + if !strings.HasPrefix(pos.Filename, filepath.Join(goenv.Get("GOROOT"), "src")) && !strings.HasPrefix(pos.Filename, filepath.Join(goenv.Get("TINYGOROOT"), "src")) { + // This file is not from the standard library (either the GOROOT or the + // TINYGOROOT). Make the path relative, for easier reading. Ignore any + // errors in the process (falling back to the absolute path). + pos.Filename = tryToMakePathRelative(pos.Filename, wd) + } + fmt.Fprintf(w, "%s: %s\n", pos, diag.Msg) +} + +// try to make the path relative to the current working directory. If any error +// occurs, this error is ignored and the absolute path is returned instead. +func tryToMakePathRelative(dir, wd string) string { + if wd == "" { + return dir // working directory not found + } + relpath, err := filepath.Rel(wd, dir) + if err != nil { + return dir + } + return relpath +} diff --git a/errors_test.go b/errors_test.go index 79174197..71eaff5e 100644 --- a/errors_test.go +++ b/errors_test.go @@ -2,7 +2,6 @@ package main import ( "bytes" - "fmt" "os" "path/filepath" "regexp" @@ -11,6 +10,7 @@ import ( "time" "github.com/tinygo-org/tinygo/compileopts" + "github.com/tinygo-org/tinygo/diagnostics" ) // Test the error messages of the TinyGo compiler. @@ -59,9 +59,7 @@ func testErrorMessages(t *testing.T, filename string) { // Write error message out as plain text. var buf bytes.Buffer - printCompilerError(err, func(v ...interface{}) { - fmt.Fprintln(&buf, v...) - }, wd) + diagnostics.CreateDiagnostics(err).WriteTo(&buf, wd) actual := strings.TrimRight(buf.String(), "\n") // Check whether the error is as expected. diff --git a/main.go b/main.go index 9af0b0fc..d0e1145c 100644 --- a/main.go +++ b/main.go @@ -8,8 +8,6 @@ import ( "errors" "flag" "fmt" - "go/scanner" - "go/types" "io" "os" "os/exec" @@ -30,8 +28,8 @@ import ( "github.com/mattn/go-colorable" "github.com/tinygo-org/tinygo/builder" "github.com/tinygo-org/tinygo/compileopts" + "github.com/tinygo-org/tinygo/diagnostics" "github.com/tinygo-org/tinygo/goenv" - "github.com/tinygo-org/tinygo/interp" "github.com/tinygo-org/tinygo/loader" "golang.org/x/tools/go/buildutil" "tinygo.org/x/go-llvm" @@ -1292,99 +1290,13 @@ func usage(command string) { } } -// try to make the path relative to the current working directory. If any error -// occurs, this error is ignored and the absolute path is returned instead. -func tryToMakePathRelative(dir, wd string) string { - if wd == "" { - return dir // working directory not found - } - relpath, err := filepath.Rel(wd, dir) - if err != nil { - return dir - } - return relpath -} - -// printCompilerError prints compiler errors using the provided logger function -// (similar to fmt.Println). -func printCompilerError(err error, logln func(...interface{}), wd string) { - switch err := err.(type) { - case types.Error: - printCompilerError(scanner.Error{ - Pos: err.Fset.Position(err.Pos), - Msg: err.Msg, - }, logln, wd) - case scanner.Error: - if !strings.HasPrefix(err.Pos.Filename, filepath.Join(goenv.Get("GOROOT"), "src")) && !strings.HasPrefix(err.Pos.Filename, filepath.Join(goenv.Get("TINYGOROOT"), "src")) { - // This file is not from the standard library (either the GOROOT or - // the TINYGOROOT). Make the path relative, for easier reading. - // Ignore any errors in the process (falling back to the absolute - // path). - err.Pos.Filename = tryToMakePathRelative(err.Pos.Filename, wd) - } - logln(err) - case scanner.ErrorList: - for _, scannerErr := range err { - printCompilerError(*scannerErr, logln, wd) - } - case *interp.Error: - logln("#", err.ImportPath) - logln(err.Error()) - if len(err.Inst) != 0 { - logln(err.Inst) - } - if len(err.Traceback) > 0 { - logln("\ntraceback:") - for _, line := range err.Traceback { - logln(line.Pos.String() + ":") - logln(line.Inst) - } - } - case loader.Errors: - // Parser errors, typechecking errors, or `go list` errors. - // err.Pkg is nil for `go list` errors. - if err.Pkg != nil { - logln("#", err.Pkg.ImportPath) - } - for _, err := range err.Errs { - printCompilerError(err, logln, wd) - } - case loader.Error: - if err.Err.Pos.Filename != "" { - // Probably a syntax error in a dependency. - printCompilerError(err.Err, logln, wd) - } else { - // Probably an "import cycle not allowed" error. - logln("package", err.ImportStack[0]) - for i := 1; i < len(err.ImportStack); i++ { - pkgPath := err.ImportStack[i] - if i == len(err.ImportStack)-1 { - // last package - logln("\timports", pkgPath+": "+err.Err.Error()) - } else { - // not the last pacakge - logln("\timports", pkgPath) - } - } - } - case *builder.MultiError: - for _, err := range err.Errs { - printCompilerError(err, logln, wd) - } - default: - logln("error:", err) - } -} - func handleCompilerError(err error) { if err != nil { wd, getwdErr := os.Getwd() if getwdErr != nil { wd = "" } - printCompilerError(err, func(args ...interface{}) { - fmt.Fprintln(os.Stderr, args...) - }, wd) + diagnostics.CreateDiagnostics(err).WriteTo(os.Stderr, wd) os.Exit(1) } } @@ -1790,9 +1702,7 @@ func main() { if getwdErr != nil { wd = "" } - printCompilerError(err, func(args ...interface{}) { - fmt.Fprintln(stderr, args...) - }, wd) + diagnostics.CreateDiagnostics(err).WriteTo(os.Stderr, wd) } if !passed { select { diff --git a/main_test.go b/main_test.go index d54af863..c294b3a8 100644 --- a/main_test.go +++ b/main_test.go @@ -24,6 +24,7 @@ import ( "github.com/aykevl/go-wasm" "github.com/tinygo-org/tinygo/builder" "github.com/tinygo-org/tinygo/compileopts" + "github.com/tinygo-org/tinygo/diagnostics" "github.com/tinygo-org/tinygo/goenv" ) @@ -380,7 +381,11 @@ func runTestWithConfig(name string, t *testing.T, options compileopts.Options, c return cmd.Run() }) if err != nil { - printCompilerError(err, t.Log, "") + w := &bytes.Buffer{} + diagnostics.CreateDiagnostics(err).WriteTo(w, "") + for _, line := range strings.Split(strings.TrimRight(w.String(), "\n"), "\n") { + t.Log(line) + } t.Fail() return }