From 9f94c9dbd7ade0499c3324408f3844b34d680e06 Mon Sep 17 00:00:00 2001 From: Tom Date: Tue, 21 Feb 2023 22:10:55 +0400 Subject: [PATCH] feat: better report message (#27) --- musttag.go | 32 ++++++++++++++++++++------------ musttag_test.go | 29 ++++++++--------------------- testdata/builtins.go | 44 +++++++++++++++++++------------------------- utils.go | 20 +++++++++++++------- 4 files changed, 60 insertions(+), 65 deletions(-) diff --git a/musttag.go b/musttag.go index c5b4d68..5fb6bb5 100644 --- a/musttag.go +++ b/musttag.go @@ -6,6 +6,8 @@ import ( "go/ast" "go/token" "go/types" + "path" + "path/filepath" "reflect" "strconv" "strings" @@ -23,6 +25,11 @@ type Func struct { ArgPos int // ArgPos is the position of the argument to check. } +func (fn Func) shortName() string { + name := strings.NewReplacer("*", "", "(", "", ")", "").Replace(fn.Name) + return path.Base(name) +} + // New creates a new musttag analyzer. // To report a custom function provide its description via Func, // it will be added to the builtin ones. @@ -77,26 +84,25 @@ var ( // should the same struct be reported only once for the same tag? reportOnce = true - // reportf is a wrapper for pass.Reportf (as a variable, so it could be mocked in tests). - reportf = func(pass *analysis.Pass, pos token.Pos, fn Func) { - // TODO(junk1tm): print the name of the struct type as well? - pass.Reportf(pos, "exported fields should be annotated with the %q tag", fn.Tag) + reportf = func(pass *analysis.Pass, st *structType, fn Func, fnPos token.Position) { + const format = "`%s` should be annotated with the `%s` tag as it is passed to `%s` at %s" + pass.Reportf(st.Pos, format, st.Name, fn.Tag, fn.shortName(), fnPos) } // HACK(junk1tm): mainModulePackages() does not return packages from `testdata`, // because it is ignored by the go tool, and thus, by the `go list` command. - // For tests to pass we need to add the package with tests to the main module manually. - testPackage string + // For tests to pass we need to add the packages with tests to the main module manually. + testPackages []string ) // run starts the analysis. func run(pass *analysis.Pass, funcs map[string]Func) (any, error) { - mainModule, err := mainModulePackages() + moduleDir, modulePackages, err := mainModule() if err != nil { return nil, err } - if testPackage != "" { - mainModule[testPackage] = struct{}{} + for _, pkg := range testPackages { + modulePackages[pkg] = struct{}{} } // store previous reports to prevent reporting @@ -147,7 +153,7 @@ func run(pass *analysis.Pass, funcs map[string]Func) (any, error) { } checker := checker{ - mainModule: mainModule, + mainModule: modulePackages, seenTypes: make(map[string]struct{}), } @@ -167,7 +173,9 @@ func run(pass *analysis.Pass, funcs map[string]Func) (any, error) { return // already reported. } - reportf(pass, result.Pos, fn) + p := pass.Fset.Position(call.Pos()) + p.Filename, _ = filepath.Rel(moduleDir, p.Filename) + reportf(pass, result, fn, p) reports[r] = struct{}{} }) @@ -219,7 +227,7 @@ func (c *checker) parseStructType(t types.Type, pos token.Pos) (*structType, boo return &structType{ Struct: t, Pos: pos, - Name: "{anonymous struct}", + Name: "anonymous struct", }, true } diff --git a/musttag_test.go b/musttag_test.go index 96b10e9..a671f09 100644 --- a/musttag_test.go +++ b/musttag_test.go @@ -5,7 +5,6 @@ import ( "io" "os" "path/filepath" - "strings" "testing" "golang.org/x/tools/go/analysis" @@ -18,7 +17,7 @@ func TestAnalyzer(t *testing.T) { // So, to be able to run tests with external dependencies, // we first need to write a GOPATH-like tree of stubs. prepareTestFiles(t) - testPackage = "tests" + testPackages = []string{"tests", "builtins"} analyzer := New( Func{Name: "example.com/custom.Marshal", Tag: "custom", ArgPos: 0}, @@ -26,26 +25,21 @@ func TestAnalyzer(t *testing.T) { ) t.Run("builtins", func(t *testing.T) { + original := reportf + reportf = func(pass *analysis.Pass, st *structType, fn Func, fnPos token.Position) { + fnPos.Line = 0 // it's annoying to fix line numbers expectations when a new line is added. + original(pass, st, fn, fnPos) + } testdata := analysistest.TestData() analysistest.Run(t, testdata, analyzer, "builtins") }) t.Run("tests", func(t *testing.T) { - original := struct { - reportOnce bool - reportf func(pass *analysis.Pass, pos token.Pos, fn Func) - }{ - reportOnce: reportOnce, - reportf: reportf, - } - defer func() { reportOnce, reportf = original.reportOnce, original.reportf }() - // for the tests we want to record reports from all functions. reportOnce = false - reportf = func(pass *analysis.Pass, pos token.Pos, fn Func) { - pass.Reportf(pos, shortName(fn.Name)) + reportf = func(pass *analysis.Pass, st *structType, fn Func, fnPos token.Position) { + pass.Reportf(st.Pos, fn.shortName()) } - testdata := analysistest.TestData() analysistest.Run(t, testdata, analyzer, "tests") }) @@ -79,13 +73,6 @@ func TestFlags(t *testing.T) { }) } -func shortName(name string) string { - name = strings.ReplaceAll(name, "*", "") - name = strings.ReplaceAll(name, "(", "") - name = strings.ReplaceAll(name, ")", "") - return filepath.Base(name) -} - func prepareTestFiles(t *testing.T) { testdata := analysistest.TestData() diff --git a/testdata/builtins.go b/testdata/builtins.go index 3bddf03..ce11bc2 100644 --- a/testdata/builtins.go +++ b/testdata/builtins.go @@ -1,4 +1,4 @@ -package tests +package builtins import ( "encoding/json" @@ -13,11 +13,20 @@ import ( // TODO(junk1tm): drop `reportOnce` and test each builtin function. +type User struct { /* want + "`User` should be annotated with the `json` tag as it is passed to `json.Marshal` at testdata/src/builtins/builtins.go" + "`User` should be annotated with the `xml` tag as it is passed to `xml.Marshal` at testdata/src/builtins/builtins.go" + "`User` should be annotated with the `yaml` tag as it is passed to `yaml.v3.Marshal` at testdata/src/builtins/builtins.go" + "`User` should be annotated with the `toml` tag as it is passed to `toml.Unmarshal` at testdata/src/builtins/builtins.go" + "`User` should be annotated with the `mapstructure` tag as it is passed to `mapstructure.Decode` at testdata/src/builtins/builtins.go" + "`User` should be annotated with the `custom` tag as it is passed to `custom.Marshal` at testdata/src/builtins/builtins.go" + */ + Name string + Email string `json:"email" xml:"email" yaml:"email" toml:"email" mapstructure:"email" custom:"email"` +} + func testJSON() { - var user struct { // want `exported fields should be annotated with the "json" tag` - Name string - Email string `json:"email"` - } + var user User json.Marshal(user) json.MarshalIndent(user, "", "") json.Unmarshal(nil, &user) @@ -26,10 +35,7 @@ func testJSON() { } func testXML() { - var user struct { // want `exported fields should be annotated with the "xml" tag` - Name string - Email string `xml:"email"` - } + var user User xml.Marshal(user) xml.MarshalIndent(user, "", "") xml.Unmarshal(nil, &user) @@ -40,10 +46,7 @@ func testXML() { } func testYAML() { - var user struct { // want `exported fields should be annotated with the "yaml" tag` - Name string - Email string `yaml:"email"` - } + var user User yaml.Marshal(user) yaml.Unmarshal(nil, &user) yaml.NewEncoder(nil).Encode(user) @@ -51,10 +54,7 @@ func testYAML() { } func testTOML() { - var user struct { // want `exported fields should be annotated with the "toml" tag` - Name string - Email string `toml:"email"` - } + var user User toml.Unmarshal(nil, &user) toml.Decode("", &user) toml.DecodeFS(nil, "", &user) @@ -64,10 +64,7 @@ func testTOML() { } func testMapstructure() { - var user struct { // want `exported fields should be annotated with the "mapstructure" tag` - Name string - Email string `mapstructure:"email"` - } + var user User mapstructure.Decode(nil, &user) mapstructure.DecodeMetadata(nil, &user, nil) mapstructure.WeakDecode(nil, &user) @@ -75,10 +72,7 @@ func testMapstructure() { } func testCustom() { - var user struct { // want `exported fields should be annotated with the "custom" tag` - Name string - Email string `custom:"email"` - } + var user User custom.Marshal(user) custom.Unmarshal(nil, &user) } diff --git a/utils.go b/utils.go index 0c277ab..fecfc43 100644 --- a/utils.go +++ b/utils.go @@ -6,8 +6,8 @@ import ( "strings" ) -// mainModulePackages returns a set of packages that belong to the main module. -func mainModulePackages() (map[string]struct{}, error) { +// mainModule returns the directory and the set of packages of the main module. +func mainModule() (dir string, packages map[string]struct{}, _ error) { // https://pkg.go.dev/cmd/go#hdr-Package_lists_and_patterns // > When using modules, "all" expands to all packages in the main module // > and their dependencies, including dependencies needed by tests of any of those. @@ -20,15 +20,21 @@ func mainModulePackages() (map[string]struct{}, error) { out, err := exec.Command(cmd[0], cmd[1:]...).Output() if err != nil { - return nil, fmt.Errorf("running go list: %w", err) + return "", nil, fmt.Errorf("running `go list all`: %w", err) } list := strings.TrimSpace(string(out)) - m := make(map[string]struct{}, len(list)) + packages = make(map[string]struct{}, len(list)) for _, pkg := range strings.Split(list, "\n") { - m[pkg] = struct{}{} - m[pkg+"_test"] = struct{}{} // `*_test` packages belong to the main module, see issue #24. + packages[pkg] = struct{}{} + packages[pkg+"_test"] = struct{}{} // `*_test` packages belong to the main module, see issue #24. } - return m, nil + out, err = exec.Command("go", "list", "-m", "-f={{.Dir}}").Output() + if err != nil { + return "", nil, fmt.Errorf("running `go list -m`: %w", err) + } + + dir = strings.TrimSpace(string(out)) + return dir, packages, nil }