explore writing linters for cardboard users #229
explore writing linters for cardboard users #229erdii wants to merge 1 commit intopackage-operator:mainfrom
Conversation
…ter 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 <me@erdii.engineering>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR introduces a static analysis framework comprising three new analyzers (cardboardselfident, cardboardself, cardboardmeth) that inspect Go AST for calls to the cardboard/run package. It adds a command-line entrypoint to run these analyzers via multichecker and a sample application demonstrating various run.Manager usage patterns and method call scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor main as cardboard-lint<br/>main
participant mc as multichecker
participant cardboardself as cardboardself<br/>Analyzer
participant cardboardmeth as cardboardmeth<br/>Analyzer
participant cardboardselfident as cardboardselfident<br/>Analyzer
main->>mc: multichecker.Main([analyzers...])
mc->>cardboardself: run(pass)
cardboardself->>cardboardself: inspector.WithStack([])
mc->>cardboardmeth: run(pass)
cardboardmeth->>cardboardmeth: walk CallExpr nodes
cardboardmeth->>cardboardmeth: validate MethX patterns<br/>and arity
mc->>cardboardselfident: run(pass)
cardboardselfident->>cardboardselfident: walk CallExpr nodes<br/>filter SerialDeps/ParallelDeps
cardboardselfident->>cardboardselfident: validate "self" identifier<br/>or inject variable
mc-->>main: report diagnostics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/analyze/cardboardself/analyzer.go (1)
1-29: Analyzer is currently a no‑op stub due to empty node filter
runretrieves the inspector but passes an emptynodeFilterslice intoinspect.WithStack, so the callback effectively never sees any nodes. That’s fine for a scaffold, but once you start implementingcardboardselfyou’ll want to decide which node kinds to inspect (e.g.*ast.CallExpr,*ast.FuncDecl, etc.) and plug them intonodeFilter.When you start fleshing this out, it’s worth re‑checking the
inspector.WithStackdocs for the exact semantics of an empty filter vs.nilto ensure you get the traversal you expect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/cardboard-lint/main.go(1 hunks)cmd/sample/main.go(1 hunks)go.mod(1 hunks)internal/analyze/cardboardmeth/analyzer.go(1 hunks)internal/analyze/cardboardself/analyzer.go(1 hunks)internal/analyze/cardboardselfident/analyzer.go(1 hunks)
🔇 Additional comments (5)
go.mod (1)
19-27: New x/tools/x/mod dependencies look consistent with the new analyzersAdding
golang.org/x/toolsandgolang.org/x/modis aligned with the go/analysis-based linters introduced in this PR. From a code-structure perspective this wiring looks fine; I don’t see any immediate issues here for the PoC.If you plan to evolve this beyond a PoC, it may be worth double‑checking that these specific versions work well with Go 1.22 and the rest of your tooling (e.g. via
go list -m -u golang.org/x/tools golang.org/x/modin your environment).cmd/sample/main.go (1)
12-121: Sample targets nicely exercise both “good” and “bad” patternsThe sample wiring looks well‑structured for exercising the linters:
FuncTargetandcoll.MethTargetshow the “canonical”self := run.Fn/MethX(...)+mgr.ParallelDeps(ctx, self, ...)pattern.- The various
MethTargetWrong*andMethInvalid*methods clearly cover inlineself, wrong identifier names, mismatched receivers, extracted methods, etc., which should give you good coverage for both analyzers.From a code‑correctness perspective this file is fine as a dedicated playground; the global
mgrand intentionally invalid calls are appropriate in this context.Once you refine the analyzers, it may be useful to add a small script/Make target that runs
go run ./cmd/cardboard-lint ./cmd/sampleand asserts the expected set of diagnostics as a regression test.cmd/cardboard-lint/main.go (1)
1-15: Multichecker wiring is straightforward and correctThe
multichecker.Mainsetup withcardboardselfident.Analyzerandcardboardmeth.Analyzeris idiomatic and matches the analyzer definitions in this PR. Nothing to change here.If/when you add
cardboardself.Analyzer, it should be straightforward to extend this list; just re‑rungo vet/go testto ensure there are no analyzer dependency cycles.internal/analyze/cardboardmeth/analyzer.go (2)
25-41: Analyzer wiring and inspector usage look solidThe analyzer registration, dependency on
inspect.Analyzer, and use ofinspect.Preorderover*ast.CallExprnodes are idiomatic and read cleanly for a go/analysis pass.
92-99:renderhelper is fine for diagnosticsThe
renderhelper usingprinter.Fprintinto abytes.Bufferis a standard pattern for analysis diagnostics; the panic is effectively unreachable and acceptable in this context.
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix typo in diagnostics for MethX arity errors
Both diagnostics use “call ro run.MethX”; fix spelling to “call to run.MethX” for cleaner user-facing messages.
You can apply this minimal change:
- pass.Reportf(ce.Pos(), "call ro run.MethX with unparseable arity: %s %q", err, render(pass.Fset, ce))
+ pass.Reportf(ce.Pos(), "call to run.MethX with unparseable arity: %s %q", err, render(pass.Fset, ce))
…
- pass.Reportf(ce.Pos(), "call ro run.MethX with arity <1: %q", render(pass.Fset, ce))
+ pass.Reportf(ce.Pos(), "call to run.MethX with arity <1: %q", render(pass.Fset, ce))If you rely on any specific wording conventions for other analyzers, please align these messages accordingly.
🏁 Script executed:
#!/bin/bash
# Verify the typo exists in the analyzer file
rg -n "call ro run\.MethX" internal/analyze/cardboardmeth/Length of output: 286
🏁 Script executed:
#!/bin/bash
# Also check for the corrected version to ensure it doesn't already exist
rg -n "call to run\.MethX" internal/analyze/cardboardmeth/Length of output: 0
Fix typo in diagnostics for MethX arity errors
Both diagnostics contain "call ro run.MethX"; fix spelling to "call to run.MethX" at lines 64 and 68:
- pass.Reportf(ce.Pos(), "call ro run.MethX with unparseable arity: %s %q", err, render(pass.Fset, ce))
+ pass.Reportf(ce.Pos(), "call to run.MethX with unparseable arity: %s %q", err, render(pass.Fset, ce))- pass.Reportf(ce.Pos(), "call ro run.MethX with arity <1: %q", render(pass.Fset, ce))
+ pass.Reportf(ce.Pos(), "call to run.MethX with arity <1: %q", render(pass.Fset, ce))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| } | |
| } | |
| if raw := captures[1]; raw != "" { | |
| parsed, err := strconv.Atoi(raw) | |
| if err != nil { | |
| pass.Reportf(ce.Pos(), "call to run.MethX with unparseable arity: %s %q", err, render(pass.Fset, ce)) | |
| return | |
| } | |
| if parsed < 1 { | |
| pass.Reportf(ce.Pos(), "call to run.MethX with arity <1: %q", render(pass.Fset, ce)) | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
internal/analyze/cardboardmeth/analyzer.go around lines 61 to 71: the diagnostic
strings currently read "call ro run.MethX" which is a typo; update both reported
messages (the one at line ~64 for unparseable arity and the one at line ~68 for
arity <1) to read "call to run.MethX" instead, preserving the rest of each
message and existing formatting/arguments so only the typo is corrected.
| 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) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Use types.Identical instead of reflect.DeepEqual and guard arg count
Comparing go/types.Type values via reflect.DeepEqual is not idiomatic and can be subtly incorrect; types.Identical (or friends) is the standard way to decide whether two types are the same. Also, adding a small guard on len(ce.Args) avoids panics if the analyzer ever runs on malformed or future MethX signatures.
A possible refactor (core part within the current block):
- groupType := pass.TypesInfo.TypeOf(ce.Args[0])
-
- var xType types.Type
- switch meth := ce.Args[1].(type) {
+ if len(ce.Args) < 2 {
+ // Signature of run.MethX requires at least two arguments; be defensive.
+ 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)
- }
+ xType = pass.TypesInfo.TypeOf(meth.X)
+ if !types.Identical(xType, groupType) {
+ pass.Reportf(
+ ce.Args[1].Pos(),
+ "method %q does not belong to type of %q: %q (receiver type: %v, group type: %v)",
+ render(pass.Fset, ce.Args[1]),
+ render(pass.Fset, ce.Args[0]),
+ render(pass.Fset, ce),
+ xType,
+ groupType,
+ )
+ }After this change, you can also drop the now-unused reflect import at the top. Please double-check whether you prefer types.Identical, types.IdenticalIgnoreTags, or something like types.AssignableTo depending on how strict you want the receiver/group type relationship to be in run.MethX.
I'll verify the review comment by examining the code file and checking the context of the suggested changes.
🏁 Script executed:
#!/bin/bash
# Examine the file at the specified lines
echo "=== File content at lines 60-100 ==="
sed -n '60,100p' internal/analyze/cardboardmeth/analyzer.go
echo ""
echo "=== Check imports in the file ==="
head -30 internal/analyze/cardboardmeth/analyzer.go | grep -E "^import|reflect|types"Length of output: 1484
🏁 Script executed:
#!/bin/bash
# Search for reflect.DeepEqual usage in the file
echo "=== reflect.DeepEqual usage in analyzer.go ==="
grep -n "reflect\.DeepEqual" internal/analyze/cardboardmeth/analyzer.go
echo ""
echo "=== All reflect usage in analyzer.go ==="
grep -n "reflect\." internal/analyze/cardboardmeth/analyzer.goLength of output: 184
🏁 Script executed:
#!/bin/bash
# Search for MethX to understand the context
echo "=== MethX references in the codebase ==="
rg -n "MethX" --type go -C 3Length of output: 1853
🏁 Script executed:
#!/bin/bash
# Check if types.Identical is standard in go/types package
echo "=== Checking if go/types package exists and types.Identical is documented ==="
# We'll search the codebase for existing uses of types package functions
echo "Existing types package usage in codebase:"
rg -n "types\." internal/analyze/cardboardmeth/analyzer.go | head -20Length of output: 194
🏁 Script executed:
#!/bin/bash
# Check the full function context where this code appears
echo "=== Full context around the analyzed code (wider range) ==="
sed -n '50,120p' internal/analyze/cardboardmeth/analyzer.goLength of output: 1688
🌐 Web query:
go/types.Identical function documentation and usage
💡 Result:
Summary — go/types.Identical
-
Signature: func Identical(x, y Type) bool. It reports whether x and y are identical types. Receivers of Signature types are ignored. [1]
-
Semantics: Identical implements the Go language notion of type identity (named types are identical only if they are the same named symbol; unnamed types are identical if their underlying type literals are structurally equivalent — arrays, slices, structs, funcs, interfaces, maps, channels, and instantiated generic rules follow the language spec). See the Go spec for the structural rules (arrays lengths, struct field names/tags/embeddedness, method sets for interfaces, etc.). [2]
-
Important caveats:
- Predicates like Identical assume both operands belong to a consistent collection of symbols (Object values). Two distinct TypeName objects that represent the same source-qualified name but were created in different contexts (for example, by separate package-loading runs) are not considered the same symbol, so Identical will return false even if the printed type looks identical. To avoid this, construct/load packages with a single Importer/loader so symbols are shared. [1][3]
- There is also IdenticalIgnoreTags(x,y Type) which behaves like Identical but ignores struct field tags. Receivers of Signature types are still ignored. [1]
-
Practical usage:
- Use Identical when you need to test lexical/semantic type identity according to the Go type system (e.g., for type-checking, tool chains).
- Ensure types compared come from a consistent symbol space (use a single go/types Config/Importer or packages.Load invocation) when comparing across packages to avoid false negatives. If you must compare types from separate contexts, canonicalize symbols or use string representations only as a heuristic (but beware of false positives/negatives). [1][3]
References
- go/types package docs (Identical, IdenticalIgnoreTags). [1]
- The Go language specification — rules for identical types. [2]
- golang/go issue discussing Identical returning false for types loaded in different packages.Load calls. [3]
Replace reflect.DeepEqual with types.Identical and add bounds check for ce.Args
The review comment is correct and identifies a legitimate major issue. Verification confirms:
- The code currently uses
reflect.DeepEqualto comparego/types.Typevalues (line 79), which is not idiomatic - The standard approach is
types.Identical, which is defined in the go/types package - The code accesses
ce.Args[0]andce.Args[1]directly (lines 74–75) without verifyinglen(ce.Args) >= 2, creating a panic risk on malformed input - The
reflectpackage is imported but used only for this singleDeepEqualcall; it will become unused after the refactor
Apply the suggested changes:
- Add the
len(ce.Args) < 2guard before accessing arguments - Replace
reflect.DeepEqual(xType, groupType)withtypes.Identical(xType, groupType) - Remove the
"reflect"import from the file
🤖 Prompt for AI Agents
internal/analyze/cardboardmeth/analyzer.go lines 73-86: currently the code
indexes ce.Args[0] and ce.Args[1] without checking length and uses
reflect.DeepEqual to compare go/types.Type values; add a guard at the top that
returns or reports if len(ce.Args) < 2 to avoid panics, replace
reflect.DeepEqual(xType, groupType) with types.Identical(xType, groupType), and
remove the now-unused "reflect" import from the file.
| 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 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Guard against panics when locating the parent block and second argument
There are a few places here where malformed or unusual code could cause the analyzer to panic:
- Line 70:
ce.Args[1]is used without checkinglen(ce.Args). A call likemgr.ParallelDeps(ctx)(missingself) would trigger an index‑out‑of‑range panic. - Line 67 /
nextBlock(stack): the result is used without checking whether a surrounding*ast.BlockStmtwas actually found (parentIndexmay be-1,parentBlockmay benil). - Line 75:
stack[parentIndex+1]assumes there is a child element after the block in the stack; that’s not guaranteed ifnextBlockever returns the last element. - Lines 77–83 and 94–98:
childIndexmay remain-1if the child statement isn’t found inparentBlock.List, but then it is used in slicing/index operations onparentBlock.Listandlist, which will panic at runtime.
For a linter that may run over arbitrary user code, it’s worth making this more defensive so it never crashes the tool. A minimal change set could look like:
@@ func run(pass *analysis.Pass) (interface{}, error) {
- parentIndex, parentBlock := nextBlock(stack)
+ parentIndex, parentBlock := nextBlock(stack)
+ if parentIndex < 0 || parentBlock == nil {
+ // No surrounding block; we can’t safely construct a SuggestedFix here.
+ return false
+ }
@@
- // second arg should be `self`
- if selfIdent, ok := ce.Args[1].(*ast.Ident); !ok {
+ if len(ce.Args) < 2 {
+ // Arity is off; don’t attempt a fix on malformed calls.
+ return false
+ }
+
+ // second arg should be `self`
+ if selfIdent, ok := ce.Args[1].(*ast.Ident); !ok {
@@
- childStmt := stack[parentIndex+1]
+ if parentIndex+1 >= len(stack) {
+ return false
+ }
+ childStmt := stack[parentIndex+1]
@@
- for i, stmt := range parentBlock.List {
- if reflect.DeepEqual(stmt, childStmt) {
- childIndex = i
- break
- }
- }
+ for i, stmt := range parentBlock.List {
+ if reflect.DeepEqual(stmt, childStmt) {
+ childIndex = i
+ break
+ }
+ }
+ if childIndex < 0 {
+ // Could not find the matching statement in this block.
+ return false
+ }This keeps the core behavior but prevents the analyzer from crashing on edge cases (odd placements of ParallelDeps/SerialDeps, bad arities, etc.).
Please re‑run the analyzer over a broader slice of your codebase (including intentionally malformed calls such as missing arguments or calls in global initializers) once these guards are in place, to confirm there are no remaining panics.
Also applies to: 87-103
I need to inspect the full analyzer code to verify the panic-safety concerns. Let me retrieve the file and analyze it.
---
🏁 Script executed:
```shell
#!/bin/bash
# Retrieve the full analyzer.go file to examine the complete function and identify panic vectors
fd -t f "analyzer\.go" -path "*/cardboardselfident/*" --exec cat {}
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Find the nextBlock function definition to understand its return semantics
rg -n "func nextBlock" --type=go -A 10Length of output: 723
🏁 Script executed:
#!/bin/bash
# Search for all uses of childIndex to see if it's guarded before use
rg -n "childIndex" --type=go -C 3 -g "*cardboard*"Length of output: 148
🏁 Script executed:
#!/bin/bash
# Check if there are any existing guards for ce.Args length
rg -n "len\(ce\.Args\)" --type=go -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Inspect the structure around lines 87-103 referenced as "Also applies to"
fd -t f "analyzer\.go" -path "*/cardboardselfident/*" --exec sed -n '87,103p' {}Length of output: 168
🏁 Script executed:
#!/bin/bash
# Get line count and broader context around the suspected problem areas
fd -t f "analyzer\.go" -path "*/cardboardselfident/*" --exec wc -l {}Length of output: 168
🏁 Script executed:
#!/bin/bash
# Retrieve full analyzer.go with line numbers for precise inspection
fd -t f "analyzer\.go" -path "*/cardboardselfident/*" --exec cat -n {}Length of output: 168
Let me correct the script syntax and retrieve the full file:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and retrieve the full analyzer.go file
find . -name "analyzer.go" -path "*/cardboardselfident/*" -type f -exec cat -n {} \;
Length of output: 5468
🏁 Script executed:
#!/bin/bash
# Get all uses of childIndex in the analyzer
rg -n "childIndex" internal/analyze/cardboardselfident/analyzer.go -C 3Length of output: 663
🏁 Script executed:
#!/bin/bash
# Search for ce.Args access patterns
rg -n "ce\.Args\[" internal/analyze/cardboardselfident/analyzer.go -C 2Length of output: 451
🏁 Script executed:
#!/bin/bash
# Show lines 36-110 to see the full run function and affected sections
sed -n '36,110p' internal/analyze/cardboardselfident/analyzer.goLength of output: 2626
🏁 Script executed:
#!/bin/bash
# Search for any existing guards: len checks before array access
rg -n "len\(" internal/analyze/cardboardselfident/analyzer.go | head -20Length of output: 99
🏁 Script executed:
#!/bin/bash
# Look for parentIndex usage to check for guards
rg -n "parentIndex" internal/analyze/cardboardselfident/analyzer.go -C 2Length of output: 272
All panic vectors are confirmed. The analyzer will crash on malformed or unusual code.
The verification confirms every issue raised in the review:
-
Line 70 (
ce.Args[1]): Nolen(ce.Args)guard. A call likemgr.ParallelDeps(ctx)crashes with index-out-of-range panic. -
Line 67 (
nextBlock): Can return(-1, nil)(line 148) but is used unconditionally at line 75 and 77 without checking. -
Line 75 (
stack[parentIndex+1]): IfparentIndex == -1orparentIndex+1 >= len(stack), this panics. -
Line 77 (
parentBlock.List): IfnextBlockreturnsnil, accessing.Listpanics. No nil check. -
Lines 95–97 (childIndex usage):
childIndexstays-1if the search loop (lines 77–82) finds no matching statement. Then:- Line 96:
list[childIndex]with-1index panics. - Line 97:
parentBlock.List[childIndex:]with-1index panics.
- Line 96:
The code lacks all suggested guards. A linter that crashes on edge cases is a critical failure.
🤖 Prompt for AI Agents
internal/analyze/cardboardselfident/analyzer.go around lines 36 to 83: the code
assumes ce.Args[1], nextBlock(...) and parentBlock are valid and that a matching
child statement will be found, which leads to panics on malformed code; add
defensive checks: verify len(ce.Args) > 1 before accessing ce.Args[1] and bail
out if not, check nextBlock's returned parentIndex != -1 and parentIndex+1 <
len(stack) before using stack[parentIndex+1], ensure parentBlock != nil before
accessing parentBlock.List, and after searching for childIndex handle childIndex
== -1 (e.g., skip/warn and return) instead of indexing with -1; make these
guards early returns so analyzer continues safely on unexpected AST shapes.
| 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 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Avoid mutating the shared AST when constructing SuggestedFixes
In the “non‑identifier second argument” branch you:
- Build an
AssignStmt(lines 87–91). - Inject it into
parentBlock.Listand replacece.Args[1]with a freshast.Ident("self")(lines 94–103). - Then call
render(pass.Fset, parentBlock)to produce the replacement text in the SuggestedFix (lines 112–115).
This is clever and works for a single analyzer, but it mutates the AST that is shared across all analyzers in the multichecker run. That can:
- Change what subsequent analyzers see (including your own
cardboardmeth), potentially hiding or altering diagnostics. - Interfere with any assumptions they make about position info or the original shape of the tree.
For something that may eventually live alongside other analyzers, it’s safer to treat the syntax tree as read‑only and either:
- Construct the replacement block as a separate AST subtree (e.g. clone
parentBlockusing an ast‑copy helper and modify the clone only), or - Skip modifying the AST entirely and build
NewTextfrom the original source ranges usingpass.Fset.Fileand string operations.
As a small nit, once you rely on pointer identity, reflect.DeepEqual for matching childStmt is overkill; comparing the interface values directly (if stmt == childStmt) is usually sufficient, since ast.Stmt implementations are pointers.
Given you’re already aware of the AST‑mutation caveat in the PR description, I’d recommend validating this by adding a tiny “tracer” analyzer to the same multichecker run that asserts it still sees the original tree shape before and after cardboardselfident runs.
Also applies to: 104-119, 130-149
I'll help you verify this review comment by examining the codebase to understand whether the AST mutation concern is valid.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the analyzer.go file to see full context around the mentioned lines
cat internal/analyze/cardboardselfident/analyzer.go | head -200
Length of output: 4425
🏁 Script executed:
#!/bin/bash
# Search for how render function is implemented
rg -n "func render" --type go -B2 -A8Length of output: 1564
🏁 Script executed:
#!/bin/bash
# Look for multichecker setup to understand how analyzers are composed
rg -n "multichecker\|NewMultiChecker" --type go -B2 -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for cardboardmeth analyzer to verify if it shares the AST
find . -name "*cardboardmeth*" -type f 2>/dev/null | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's any AST cloning pattern in the codebase
rg -n "ast\.(Clone|Copy)" --type go -B2 -A2Length of output: 0
I need to dig deeper into how these analyzers are actually used in the multichecker setup.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for how the analyzer is registered/used
rg -n "cardboardselfident\|cardboardmeth" --type go -B2 -A2 | grep -v "^--$"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for main functions or cmd packages
find . -type f -name "main.go" -o -name "cmd.go" | head -10Length of output: 69
🏁 Script executed:
#!/bin/bash
# Search for analysis.Pass or Analyzer registration patterns
rg -n "\.Analyzer\s*=|multichecker|composition" --type go -B1 -A3 | head -50Length of output: 591
🏁 Script executed:
#!/bin/bash
# Check go.mod for multichecker or analysis dependencies
cat go.mod 2>/dev/null | grep -i "multichecker\|analysis"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for test files that might show analyzer usage
find . -name "*_test.go" -path "*/analyze/*" -type f 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for pass.ResultOf pattern to understand analyzer dependencies/sharing
rg -n "pass\.ResultOf" --type go -B1 -A2Length of output: 1022
Now I need to examine the cardboardmeth analyzer to see if it would be affected by the mutations:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full cardboardmeth analyzer implementation
cat internal/analyze/cardboardmeth/analyzer.go
Length of output: 2760
🏁 Script executed:
#!/bin/bash
# Check if there's any cardboardself analyzer and its relation to these
cat internal/analyze/cardboardself/analyzer.go 2>/dev/null | head -100Length of output: 626
🏁 Script executed:
#!/bin/bash
# Look at how pass.Fset is used - understand if mutations affect positions
rg -n "pass\.Fset\|Pos\(\)\|End\(\)" internal/analyze/cardboardselfident/analyzer.go -A1 -B1Length of output: 0
Fix AST mutation to prevent interference with subsequent analyzers
The review comment is correct. AST mutations in cardboardselfident will affect cardboardmeth:
cardboardselfidentmutatesce.Args[1](line 100) andparentBlock.List(line 102) before callingrender()- Both analyzers run together via
multichecker.Main()in cmd/cardboard-lint/main.go cardboardmethlater examinesce.Args[1](line 71 in itsswitchstatement) and callsrender(pass.Fset, ce.Args[1])for diagnostics- When
cardboardmethruns after the mutation, it sees the modified tree and produces incorrect diagnostics based on the mutatedast.NewIdent("self")instead of the original problematic expression
Recommendations:
- Clone
parentBlockbefore mutations, or build the replacement text from original source ranges without mutating the shared AST - Regarding the
reflect.DeepEqualnit: direct pointer comparison (stmt == childStmt) is sufficient forast.Stmttypes
1 participant
This PR contains a PoC combination of 2 linters: one that identifies calls to manager.SerialDeps and manager.ParallelDeps and validates that
selfis 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:
PoC-Done:
go run ./cmd/cardboard-lint- behaves likes other linters)cardboardselfident: enforce self arg to be namedself- can propose a fix where an inline expression is factored into a prependedself := expressionstatementcardboardmeth: enforce valid receiver/method combinations in run.MethX argumentsMissing:
selfargument is correct, but named different. It would probably be better generated by something like gorename? essentially, this is "pls gorename the wrongly named identifier toself"cardboardself: self must actually be a dep wrapped version of the caller!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