From d07dbc51b8ebcb024b5fe7e7c972d3e0dd305520 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:49:17 -0600 Subject: [PATCH] Fix a panic on a field write through a non-identifier The nonmutating analyzer stopped the whole run with exit status 2 on a field write whose container was not an identifier. `containerStorageVisible` accepted only an identifier and panicked on every other expression, and `visibleMutationOrigin` called it for the container of each selector write. These shapes crashed: //constable:nonmutating func F(s []E) { s[0].N = 1 } // *ast.IndexExpr //constable:nonmutating func G(p *E) { (*p).N = 1 } // *ast.StarExpr //constable:nonmutating func H(ss [][]E) { ss[0][0].N = 1 } // nested *ast.IndexExpr A selector's container lives in caller-visible memory exactly when the container expression does, so `exprRefersToCallerVisibleMemory` subsumes `containerStorageVisible` for an identifier and answers for the other shapes without a panic. Delete the narrower function and call the general one at both sites. `rootOrigin` finds the parameter or receiver that starts a chain, so a write below an index or a dereference reports the identifier that the caller passed. The same change removes two silent misses that the old container check produced: `o.inner.data[0] = 0` on a value receiver now reports, and so does `(*p).inner.data[0] = 0` on a pointer parameter. Add regression cases for the three crashing shapes and for the value receiver chain, which passed silently before. Keep `o.inner = Inner{}` as a control: a field write on the receiver copy must stay unreported. Change-Id: Iade884a097fb704a54fc026e0c9d245f54989fdc --- internal/analysis/nonmutating/nonmutating.go | 50 ++++++++++--------- .../analysis/nonmutating/testdata/src/a/a.go | 31 ++++++++++++ 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/internal/analysis/nonmutating/nonmutating.go b/internal/analysis/nonmutating/nonmutating.go index 0f15fb0..0c9fd78 100644 --- a/internal/analysis/nonmutating/nonmutating.go +++ b/internal/analysis/nonmutating/nonmutating.go @@ -1,7 +1,6 @@ package nonmutating import ( - "fmt" "go/ast" "go/types" @@ -166,21 +165,6 @@ func (state *functionState) collectAssignAliases(stmt *ast.AssignStmt) bool { return changed } -func (state *functionState) containerStorageVisible(expr ast.Expr) (origin, bool) { - expr = unparen(expr) - switch expr := expr.(type) { - case *ast.Ident: - origin, ok := state.originOf(expr) - if !ok { - return origin, false - } - - return origin, isReferenceLike(state.typeOf(expr)) - default: - panic("unexpected expr type: " + fmt.Sprintf("%T", expr)) - } -} - func (state *functionState) exprRefersToCallerVisibleMemory(expr ast.Expr) (origin, bool) { expr = unparen(expr) switch expr := expr.(type) { @@ -192,18 +176,18 @@ func (state *functionState) exprRefersToCallerVisibleMemory(expr ast.Expr) (orig return origin, isReferenceLike(state.typeOf(expr)) case *ast.SelectorExpr: - origin, ok := state.originOf(expr.X) - if !ok { - return origin, false + if origin, ok := state.exprRefersToCallerVisibleMemory(expr.X); ok { + return origin, true } - if _, ok := state.containerStorageVisible(expr.X); ok { - return origin, true + origin, ok := state.rootOrigin(expr.X) + if !ok { + return origin, false } return origin, isReferenceLike(state.typeOf(expr)) case *ast.IndexExpr: - origin, ok := state.originOf(expr.X) + origin, ok := state.rootOrigin(expr.X) if !ok { return origin, false } @@ -296,6 +280,26 @@ func (state *functionState) reportMutations(body *ast.BlockStmt) { }) } +// rootOrigin returns the parameter or receiver that an expression starts at. +// The analyzer holds an origin for each identifier, so a chain that starts +// below an identifier, such as an element of a slice parameter, reports the +// identifier that the chain starts at. +func (state *functionState) rootOrigin(expr ast.Expr) (origin, bool) { + expr = unparen(expr) + switch expr := expr.(type) { + case *ast.Ident: + return state.originOf(expr) + case *ast.StarExpr: + return state.rootOrigin(expr.X) + case *ast.SelectorExpr: + return state.rootOrigin(expr.X) + case *ast.IndexExpr: + return state.rootOrigin(expr.X) + } + + return origin{}, false +} + func (state *functionState) typeOf(expr ast.Expr) types.Type { return state.pass.TypesInfo.TypeOf(expr) } @@ -308,7 +312,7 @@ func (state *functionState) visibleMutationOrigin(expr ast.Expr) (origin, bool) case *ast.StarExpr: return state.exprRefersToCallerVisibleMemory(expr.X) case *ast.SelectorExpr: - return state.containerStorageVisible(expr.X) + return state.exprRefersToCallerVisibleMemory(expr.X) case *ast.IndexExpr: return state.exprRefersToCallerVisibleMemory(expr.X) } diff --git a/internal/analysis/nonmutating/testdata/src/a/a.go b/internal/analysis/nonmutating/testdata/src/a/a.go index 53c1ed6..aa500f4 100644 --- a/internal/analysis/nonmutating/testdata/src/a/a.go +++ b/internal/analysis/nonmutating/testdata/src/a/a.go @@ -122,3 +122,34 @@ func DeletesLocalMap() { func MutatesDoublePointer(pp **int) { **pp = 1 // want "//constable:nonmutating function mutates pointer parameter pp" } + +type Element struct{ n int } + +//constable:nonmutating +func MutatesFieldOfSliceElement(elements []Element) { + elements[0].n = 1 // want "//constable:nonmutating function mutates parameter elements" +} + +//constable:nonmutating +func MutatesNestedSliceElementField(ss [][]Element) { + ss[0][0].n = 1 // want "//constable:nonmutating function mutates parameter ss" +} + +type Inner struct{ data []byte } + +type Outer struct{ inner Inner } + +//constable:nonmutating +func MutatesFieldThroughDeref(p *Outer) { + (*p).inner.data[0] = 0 // want "//constable:nonmutating function mutates pointer parameter p" +} + +//constable:nonmutating +func (o Outer) ClearNestedField() { + o.inner.data[0] = 0 // want "//constable:nonmutating method mutates receiver o" +} + +//constable:nonmutating +func (o Outer) ReplacesNestedField() { + o.inner = Inner{} +}