From ad91285dd1692d33b226f95036dd6e8412cb8cbb Mon Sep 17 00:00:00 2001 From: erdii Date: Mon, 6 May 2024 16:27:06 +0200 Subject: [PATCH] explore writing linters for cardboard users by piecing together a linter that identifies calls to manager.SerialDeps and manager.ParallelDeps and validates that `self` is not inlined in the method call and another one that validates correct receiver/method pairs in calls to run.MethX Example run against package ./cmd/sample: ``` go run ./cmd/cardboard-lint ./cmd/sample /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:53:9: second arg to ParallelDeps call should be identifier "self" but is "this". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:57:9: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth1(c, c.MethTarget, args)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:61:2: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth1(c, c.MethTarget, args)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:66:5: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth1(c, c.MethTarget, args)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:73:12: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth1(c, c.MethTarget, args)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:80:12: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth(c, c.MethNoArgs)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:87:12: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth2(c, c.MethTwoArgs, foo, bar)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:94:12: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth2(empty{}, c.MethTwoArgs, foo, bar)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:101:12: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth2(suspect{}, c.MethTwoArgs, foo, bar)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:109:12: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth2(c, methTwoArgs, foo, bar)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:117:12: Second arg to ParallelDeps call should be identifier "self" but is expression "run.Meth(c, meth)". /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:94:53: method "c.MethTwoArgs" does not belong to type of "empty{}": "run.Meth2(empty{}, c.MethTwoArgs, foo, bar)" *pkg.package-operator.run/cardboard/cmd/sample.coll /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:101:55: method "c.MethTwoArgs" does not belong to type of "suspect{}": "run.Meth2(suspect{}, c.MethTwoArgs, foo, bar)" *pkg.package-operator.run/cardboard/cmd/sample.coll /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:109:47: identifier "methTwoArgs" should be removed and method selector expression inlined: "run.Meth2(c, methTwoArgs, foo, bar)" /home/erdii/projects/redhat/github.com/package-operator/cardboard/cmd/sample/main.go:117:46: identifier "meth" should be removed and method selector expression inlined: "run.Meth(c, meth)" ``` PoC-Done: - Linters: (`go run ./cmd/cardboard-lint` - behaves likes other linters) - `cardboardselfident`: enforce self arg to be named `self` - can propose a fix where an inline expression is factored into a prepended `self := expression` statement - `cardboardmeth`: enforce valid receiver/method combinations in run.MethX arguments Missing: - The Fix for the case where the `self` argument is correct, but named different. It would probably be better generated by something like gorename? essentially, this is "pls gorename the wrongly named identifier to `self`" - Additional linters: - `cardboardself`: self must actually be a dep wrapped version of the caller! - Testing Apparently if i modify the parentBlock ast node, I should expect other linters to break when running multiple linters from the same binary. So far it seems that the AST fumbling is not breaking the combined linters, but this should assumption should be validated via extensive tests. Apparently deep cloning a (sub) ast is not part of the go stdlib. I found these libs/implementations: https://github.com/go-toolsmith/astcopy/blob/v1.1.0/astcopy.go https://github.com/google/wire/blob/main/internal/wire/copyast.go Signed-off-by: erdii --- cmd/cardboard-lint/main.go | 15 ++ cmd/sample/main.go | 121 ++++++++++++++ go.mod | 2 + go.sum | 4 + internal/analyze/cardboardmeth/analyzer.go | 99 ++++++++++++ internal/analyze/cardboardself/analyzer.go | 29 ++++ .../analyze/cardboardselfident/analyzer.go | 149 ++++++++++++++++++ 7 files changed, 419 insertions(+) create mode 100644 cmd/cardboard-lint/main.go create mode 100644 cmd/sample/main.go create mode 100644 internal/analyze/cardboardmeth/analyzer.go create mode 100644 internal/analyze/cardboardself/analyzer.go create mode 100644 internal/analyze/cardboardselfident/analyzer.go diff --git a/cmd/cardboard-lint/main.go b/cmd/cardboard-lint/main.go new file mode 100644 index 00000000..25815cb9 --- /dev/null +++ b/cmd/cardboard-lint/main.go @@ -0,0 +1,15 @@ +package main + +import ( + "golang.org/x/tools/go/analysis/multichecker" + + "pkg.package-operator.run/cardboard/internal/analyze/cardboardmeth" + "pkg.package-operator.run/cardboard/internal/analyze/cardboardselfident" +) + +func main() { + multichecker.Main( + cardboardselfident.Analyzer, + cardboardmeth.Analyzer, + ) +} diff --git a/cmd/sample/main.go b/cmd/sample/main.go new file mode 100644 index 00000000..44ae7056 --- /dev/null +++ b/cmd/sample/main.go @@ -0,0 +1,121 @@ +package main + +import ( + "context" + "embed" + "fmt" + "os" + + "pkg.package-operator.run/cardboard/run" +) + +var ( + mgr *run.Manager + + //go:embed *.go + source embed.FS +) + +func main() { + ctx := context.Background() + + mgr = run.New(run.WithSources(source)) + mgr.Register() + if err := mgr.Run(ctx); err != nil { + fmt.Fprintf(os.Stderr, "\n%s\n", err) + os.Exit(1) + } +} + +func FuncTarget(ctx context.Context) error { + self := run.Fn(FuncTarget) + return mgr.ParallelDeps(ctx, self, run.Fn(func() { + fmt.Println("ok") + })) +} + +type ( + empty struct{} + suspect struct{} +) + +func (s *suspect) Meth(ctx context.Context) error { return nil } + +type coll struct{} + +func (c *coll) MethTarget(ctx context.Context, args []string) error { + self := run.Meth1(c, c.MethTarget, args) + return mgr.ParallelDeps(ctx, self, run.Fn(FuncTarget)) +} + +func (c *coll) MethTargetWrongName(ctx context.Context, args []string) error { + this := run.Meth1(c, c.MethTarget, args) + return mgr.ParallelDeps(ctx, this, run.Fn(FuncTarget)) +} + +func (c *coll) MethTargetWrongInline(ctx context.Context, args []string) error { + return mgr.ParallelDeps(ctx, run.Meth1(c, c.MethTarget, args), run.Fn(FuncTarget)) +} + +func (c *coll) MethTargetWrongInlineAndBare(ctx context.Context, args []string) error { + mgr.ParallelDeps(ctx, run.Meth1(c, c.MethTarget, args), run.Fn(FuncTarget)) + return nil +} + +func (c *coll) MethTargetWrongInlineAndIf(ctx context.Context, args []string) error { + if mgr.ParallelDeps(ctx, run.Meth1(c, c.MethTarget, args), run.Fn(FuncTarget)) != nil { + return nil + } + return nil +} + +func (c *coll) MethTargetWrongInlineAndIfBlock(ctx context.Context, args []string) error { + if err := mgr.ParallelDeps(ctx, run.Meth1(c, c.MethTarget, args), run.Fn(FuncTarget)); err != nil { + return err + } + return nil +} + +func (c *coll) MethNoArgs(ctx context.Context) error { + if err := mgr.ParallelDeps(ctx, run.Meth(c, c.MethNoArgs), run.Fn(FuncTarget)); err != nil { + return err + } + return nil +} + +func (c *coll) MethTwoArgs(ctx context.Context, foo, bar string) error { + if err := mgr.ParallelDeps(ctx, run.Meth2(c, c.MethTwoArgs, foo, bar), run.Fn(FuncTarget)); err != nil { + return err + } + return nil +} + +func (c *coll) MethInvalidTwoArgs(ctx context.Context, foo, bar string) error { + if err := mgr.ParallelDeps(ctx, run.Meth2(empty{}, c.MethTwoArgs, foo, bar), run.Fn(FuncTarget)); err != nil { + return err + } + return nil +} + +func (c *coll) MethInvalidWrongReceiverWithSameSignatureReceiverTwoArgs(ctx context.Context, foo, bar string) error { + if err := mgr.ParallelDeps(ctx, run.Meth2(suspect{}, c.MethTwoArgs, foo, bar), run.Fn(FuncTarget)); err != nil { + return err + } + return nil +} + +func (c *coll) MethExtractedTwoArgs(ctx context.Context, foo, bar string) error { + methTwoArgs := c.MethTwoArgs + if err := mgr.ParallelDeps(ctx, run.Meth2(c, methTwoArgs, foo, bar), run.Fn(FuncTarget)); err != nil { + return err + } + return nil +} + +func (c *coll) MethInvalidExtracted(ctx context.Context) error { + meth := (&suspect{}).Meth + if err := mgr.ParallelDeps(ctx, run.Meth(c, meth)); err != nil { + return err + } + return nil +} diff --git a/go.mod b/go.mod index f8d592a7..641d8af0 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/neilotoole/slogt v1.1.0 github.com/stretchr/testify v1.9.0 github.com/xlab/treeprint v1.2.0 + golang.org/x/tools v0.1.12 ) require ( @@ -23,6 +24,7 @@ require ( github.com/kr/pretty v0.3.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/rogpeppe/go-internal v1.12.0 // indirect + golang.org/x/mod v0.9.0 // indirect golang.org/x/sys v0.20.0 // indirect gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index 46d5ef01..cc923789 100644 --- a/go.sum +++ b/go.sum @@ -25,9 +25,13 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/xlab/treeprint v1.2.0 h1:HzHnuAF1plUN2zGlAFHbSQP2qJ0ZAD3XF5XD7OesXRQ= github.com/xlab/treeprint v1.2.0/go.mod h1:gj5Gd3gPdKtR1ikdDK6fnFLdmIS0X30kTTuNd/WEJu0= +golang.org/x/mod v0.9.0 h1:KENHtAZL2y3NLMYZeHY9DW8HW8V+kQyJsY/V9JlKvCs= +golang.org/x/mod v0.9.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/tools v0.1.12 h1:VveCTK38A2rkS8ZqFY25HIDFscX5X9OoEhJd3quQmXU= +golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= diff --git a/internal/analyze/cardboardmeth/analyzer.go b/internal/analyze/cardboardmeth/analyzer.go new file mode 100644 index 00000000..d8a8acf5 --- /dev/null +++ b/internal/analyze/cardboardmeth/analyzer.go @@ -0,0 +1,99 @@ +package cardboardmeth + +import ( + "bytes" + "go/ast" + "go/printer" + "go/token" + "go/types" + "reflect" + "regexp" + "strconv" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/types/typeutil" +) + +const ( + cardboardRunPackage = "pkg.package-operator.run/cardboard/run" +) + +var methFnRegex = regexp.MustCompile("^Meth(.*)") + +var Analyzer = &analysis.Analyzer{ + Name: "cardboardmeth", + Doc: "Checks that cardboard run.MethX calls pass correct struct and method pairs.", + Run: run, + + Requires: []*analysis.Analyzer{inspect.Analyzer}, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + inspect.Preorder(nodeFilter, func(n ast.Node) { + ce := n.(*ast.CallExpr) + + fn := typeutil.Callee(pass.TypesInfo, ce) + if fn == nil { + return + } + if pkg := fn.Pkg(); pkg == nil || pkg.Path() != cardboardRunPackage { + return // This analyzer is only interested in calls to types from the `cardboardRunPackage` package. + } + + recv := fn.Type().(*types.Signature).Recv() + if recv != nil { + return // This analyzer is only interested in function calls. + } + + captures := methFnRegex.FindStringSubmatch(fn.Name()) + if captures == nil { + return // This analyzer is only interested in calls to run.MethX. + } + + if raw := captures[1]; raw != "" { + parsed, err := strconv.Atoi(raw) + if err != nil { + pass.Reportf(ce.Pos(), "call ro run.MethX with unparseable arity: %s %q", err, render(pass.Fset, ce)) + return + } + if parsed < 1 { + pass.Reportf(ce.Pos(), "call ro run.MethX with arity <1: %q", render(pass.Fset, ce)) + return + } + } + + groupType := pass.TypesInfo.TypeOf(ce.Args[0]) + + var xType types.Type + switch meth := ce.Args[1].(type) { + case *ast.SelectorExpr: + xType = pass.TypesInfo.TypeOf(meth.X) + if !reflect.DeepEqual(xType, groupType) { + pass.Reportf(ce.Args[1].Pos(), "method %q does not belong to type of %q: %q %s", render(pass.Fset, ce.Args[1]), render(pass.Fset, ce.Args[0]), render(pass.Fset, ce), xType) + } + case *ast.Ident: + pass.Reportf(ce.Args[1].Pos(), "identifier %q should be removed and method selector expression inlined: %q", render(pass.Fset, ce.Args[1]), render(pass.Fset, ce)) + default: + pass.Reportf(ce.Args[1].Pos(), "not implemented: %q has method arg node type %T", render(pass.Fset, ce), meth) + } + }) + + return nil, nil +} + +// from https://arslan.io/2019/06/13/using-go-analysis-to-write-a-custom-linter/ +func render(fset *token.FileSet, x interface{}) string { + var buf bytes.Buffer + if err := printer.Fprint(&buf, fset, x); err != nil { + panic(err) + } + return buf.String() +} diff --git a/internal/analyze/cardboardself/analyzer.go b/internal/analyze/cardboardself/analyzer.go new file mode 100644 index 00000000..8daadee8 --- /dev/null +++ b/internal/analyze/cardboardself/analyzer.go @@ -0,0 +1,29 @@ +package cardboardself + +import ( + "go/ast" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "cardboardself", + Doc: "Checks that self is a wrapped version of the caller.", + Run: run, + + Requires: []*analysis.Analyzer{inspect.Analyzer}, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{} + + inspect.WithStack(nodeFilter, func(n ast.Node, _ bool, stack []ast.Node) bool { + return false + }) + + return nil, nil +} diff --git a/internal/analyze/cardboardselfident/analyzer.go b/internal/analyze/cardboardselfident/analyzer.go new file mode 100644 index 00000000..4616160a --- /dev/null +++ b/internal/analyze/cardboardselfident/analyzer.go @@ -0,0 +1,149 @@ +package cardboardselfident + +import ( + "bytes" + "fmt" + "go/ast" + "go/printer" + "go/token" + "go/types" + "reflect" + "regexp" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/types/typeutil" +) + +const ( + cardboardRunPackage = "pkg.package-operator.run/cardboard/run" + runManagerTypeName = "*pkg.package-operator.run/cardboard/run.Manager" +) + +var depsFnRegex = regexp.MustCompile("^(Serial|Parallel)Deps$") + +var Analyzer = &analysis.Analyzer{ + Name: "cardboardselfident", + Doc: "Checks that cardboard targets pass correct self arg to their dependencies.", + Run: run, + + Requires: []*analysis.Analyzer{inspect.Analyzer}, +} + +// playground to explore a linter that enforces a separate non-inline `self` argument in calls to mgr.Serial/ParallelDeps + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + inspect.WithStack(nodeFilter, func(n ast.Node, _ bool, stack []ast.Node) bool { + ce := n.(*ast.CallExpr) + + fn := typeutil.Callee(pass.TypesInfo, ce) + if fn == nil { + return false + } + if pkg := fn.Pkg(); pkg == nil || pkg.Path() != cardboardRunPackage { + return false // This analyzer is only interested in calls to types from the `cardboardRunPackage` package. + } + + recv := fn.Type().(*types.Signature).Recv() + if recv == nil { + return false // This analyzer is only interested in method calls. + } + recvName := recv.Type().Underlying().String() + if recvName != runManagerTypeName { + return false // This analyzer is only interested in calls to the `runManagerTypeName` type. + } + + if !depsFnRegex.MatchString(fn.Name()) { + return false // This analyzer is only interested in calls to Parallel-/SerialDeps. + } + + parentIndex, parentBlock := nextBlock(stack) + + // second arg should be `self` + if selfIdent, ok := ce.Args[1].(*ast.Ident); !ok { + // second arg is not an identifier + invalidSelfArgExpr := ce.Args[1] + offendingCallSrc := render(pass.Fset, ce.Args[1]) + + childStmt := stack[parentIndex+1] + childIndex := -1 + for i, stmt := range parentBlock.List { + if reflect.DeepEqual(stmt, childStmt) { + childIndex = i + break + } + } + + // prepare assignment statement `self := invalidSelfArgExpr` + // https://yuroyoro.github.io/goast-viewer/index.html + // turned out very helpful when I constructed the assignment statement below. + assignment := &ast.AssignStmt{ + Tok: token.DEFINE, + Lhs: []ast.Expr{ast.NewIdent("self")}, + Rhs: []ast.Expr{invalidSelfArgExpr}, + } + + // Inject the assignment just before the statement that contains the invalid call. + list := make([]ast.Stmt, len(parentBlock.List)+1) + copy(list, parentBlock.List[:childIndex]) + list[childIndex] = assignment + copy(list[childIndex+1:], parentBlock.List[childIndex:]) + + // Replace call arg with `self` identifier. + ce.Args[1] = ast.NewIdent("self") + // Replace parent block statement list. + parentBlock.List = list + + pass.Report(analysis.Diagnostic{ + Pos: ce.Pos(), + Message: fmt.Sprintf(`Second arg to %s call should be identifier "self" but is expression %q.`, fn.Name(), offendingCallSrc), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf(`Separate %q out into separate short variable declaration.`, offendingCallSrc), + TextEdits: []analysis.TextEdit{ + { + Pos: parentBlock.Pos(), + End: parentBlock.End(), + NewText: []byte(render(pass.Fset, parentBlock)), + }, + }, + }, + }, + }) + } else if selfIdent.Name != "self" { + // second arg is an identifier but not named "self" + pass.Reportf(ce.Pos(), `second arg to %s call should be identifier "self" but is "%s".`, fn.Name(), selfIdent.Name) + } + return false + }) + + return nil, nil +} + +// from https://arslan.io/2019/06/13/using-go-analysis-to-write-a-custom-linter/ +func render(fset *token.FileSet, x interface{}) string { + var buf bytes.Buffer + if err := printer.Fprint(&buf, fset, x); err != nil { + panic(err) + } + return buf.String() +} + +// Finds surrounding block statement by travelling up the stack. +func nextBlock(stack []ast.Node) (int, *ast.BlockStmt) { + for i := len(stack) - 1; i >= 0; i-- { + block, ok := stack[i].(*ast.BlockStmt) + if !ok { + continue + } + return i, block + } + return -1, nil +}