amadan

carlos / platform Public

copy gate: a list's join is its own, traced from the head's fill-in

Fix round 1 of Task 7. joinsWithComma passed any strings.Join(_, ", ") in
the element file, so a list joined with " and " passed beside an unrelated
comma join. The gate now follows the head's final fill-in (the last operand
of a concatenation, or the last argument of a format call) to its
strings.Join: directly, through one local variable assigned once, or from a
parameter of the enclosing function to its callers in the head's and
element files (every caller must join with ", ", and there must be one). A
producer it cannot find refuses the row with the reason.

MV-N54 traces to apicli.go's c.ambiguous(seg, strings.Join(alts, ", ")),
MV-N129 to t3Differs's own join; both still pass. New fixtures: an
unrelated correct join beside a wrong target join, and unfindable, uncalled
and mixed-caller producers. The reviewed code itself, restored as a
mutation, is caught.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 338023e2a1e54ac49ec069187167517541604d10 parent 57effa5
6 files changed, +452 −36
  • approved_moves_copy_test.go +364 −33
  • testdata/moves-copy/list-join-two/head.go +15 −0
  • testdata/moves-copy/list-join-two/record.md +9 −0
  • testdata/moves-copy/list-producer/head.go +34 −0
  • testdata/moves-copy/list-producer/record.md +16 −0
  • testdata/moves-copy/list-template/elem.go +14 −3
diff --git a/approved_moves_copy_test.go b/approved_moves_copy_test.go
index e74242707..ebaf2c325 100644
--- a/approved_moves_copy_test.go
+++ b/approved_moves_copy_test.go
@@ -254,8 +254,10 @@ func checkMovesAppliedWith(rows []copyRow, root string, elementFiles map[string]
// spoken by one expression of the row's file, whose final part is the
// code's own fill-in (the list is never fixed text);
// - the element is built where the code builds it (the row's file, or the
-// file movesListElementFiles names), and that file joins the list with
-// ", ", which is what the row's ", ..." shows.
+// file movesListElementFiles names);
+// - the list's own join, traced from the head's fill-in (listProducers),
+// is strings.Join with ", ", which is what the row's ", ..." shows. Any
+// other join in the file is another list's and does not count.
//
// The list follows the head's last ": ", as both template rows do; a row
// that ends in ", ..." with no such colon, or whose element does not start
@@ -302,14 +304,13 @@ func checkMovesListApplied(r copyRow, root string, body []byte, elementFiles map
return []string{fmt.Sprintf("%s: the gate cannot read the list template in %q: %v", r.ID, r.Text, err)}
}
var out []string
- headSpoken := false
- for _, p := range goTextPatterns(r.Path(), body) {
- if informative(p) && strings.HasSuffix(p, wild) && movesRowSpoken(head+"<list>", p) {
- headSpoken = true
- break
+ var heads []textExpr
+ for _, te := range goTextExprs(r.Path(), body) {
+ if informative(te.pattern) && strings.HasSuffix(te.pattern, wild) && movesRowSpoken(head+"<list>", te.pattern) {
+ heads = append(heads, te)
}
}
- if !headSpoken {
+ if len(heads) == 0 {
out = append(out, fmt.Sprintf("%s: %s does not say %q with the list as its final fill-in", r.ID, r.Path(), head+"<list>"))
}
elemFile := r.Path()
@@ -332,40 +333,323 @@ func checkMovesListApplied(r copyRow, root string, body []byte, elementFiles map
out = append(out, fmt.Sprintf("%s: %s does not build the list's element %q", r.ID, elemFile, element))
}
}
- if !joinsWithComma(elemFile, elemBody) {
- out = append(out, fmt.Sprintf("%s: %s does not join the list with \", \"", r.ID, elemFile))
+ // Every expression that says the head must make its list with a comma
+ // join; the callers a parameter hop reaches are looked for in the head's
+ // file and the element file.
+ callFiles := []goFile{{r.Path(), body}}
+ if elemFile != r.Path() {
+ callFiles = append(callFiles, goFile{elemFile, elemBody})
+ }
+ seen := map[string]bool{}
+ for _, h := range heads {
+ for _, p := range listProducers(h, r.Path(), callFiles) {
+ var msg string
+ switch {
+ case p.why != "":
+ msg = fmt.Sprintf("%s: the gate cannot find the strings.Join that makes the list in %s: %s", r.ID, p.file, p.why)
+ case p.sep != ", ":
+ msg = fmt.Sprintf("%s: %s does not join the list with \", \" (it joins with %q)", r.ID, p.file, p.sep)
+ }
+ if msg != "" && !seen[msg] {
+ seen[msg] = true
+ out = append(out, msg)
+ }
+ }
}
return out
}
-// joinsWithComma reports whether a Go file calls strings.Join with the
-// literal separator ", ".
-func joinsWithComma(file string, body []byte) bool {
- f, err := parser.ParseFile(token.NewFileSet(), file, body, 0)
- if err != nil {
+// goFile is a Go file's name and body.
+type goFile struct {
+ name string
+ body []byte
+}
+
+// funcScope is a function as far as the list trace needs it: the name its
+// callers use (a declaration's name, or the field or variable a function
+// literal is bound to), its parameter names in order, and its body.
+type funcScope struct {
+ name string
+ params []string
+ body *ast.BlockStmt
+}
+
+// textExpr is one expression that could hold a sentence, with its pattern and
+// the innermost function it sits in (nil at package level).
+type textExpr struct {
+ pattern string
+ expr ast.Expr
+ scope *funcScope
+}
+
+// listProducer is one join that makes a list: its separator, or why it cannot
+// be found.
+type listProducer struct {
+ file, sep, why string
+}
+
+// listProducers traces the list in a head expression to the joins that make
+// it. The list is the head's final fill-in: the last operand of a
+// concatenation, or the last argument of a format call. That must be a call
+// to strings.Join with a literal separator, directly or through one local
+// variable assigned once. A parameter of the enclosing function is followed
+// to its callers in files (the call's argument at the parameter's place,
+// traced the same way, with no further parameter hop), and there must be at
+// least one. Anything else is a producer the gate cannot find, and refused.
+// A drift lint, not a dataflow engine.
+func listProducers(h textExpr, headFile string, files []goFile) []listProducer {
+ fill, why := listFill(h.expr)
+ if why != "" {
+ return []listProducer{{file: headFile, why: why}}
+ }
+ sep, param, why := joinOf(fill, h.scope, true)
+ switch {
+ case why != "":
+ return []listProducer{{file: headFile, why: why}}
+ case param < 0:
+ return []listProducer{{file: headFile, sep: sep}}
+ }
+ var out []listProducer
+ for _, f := range files {
+ for _, c := range callsTo(f, h.scope.name) {
+ if param >= len(c.call.Args) {
+ out = append(out, listProducer{file: f.name, why: fmt.Sprintf("a call to %s has no argument for its parameter %s", h.scope.name, h.scope.params[param])})
+ continue
+ }
+ sep, _, why := joinOf(c.call.Args[param], c.scope, false)
+ out = append(out, listProducer{file: f.name, sep: sep, why: why})
+ }
+ }
+ if len(out) == 0 {
+ return []listProducer{{file: headFile, why: fmt.Sprintf("the list is %s's parameter %s, and nothing calls %s", h.scope.name, h.scope.params[param], h.scope.name)}}
+ }
+ return out
+}
+
+// listFill is the expression that fills a head's final placeholder.
+func listFill(e ast.Expr) (ast.Expr, string) {
+ switch e := e.(type) {
+ case *ast.ParenExpr:
+ return listFill(e.X)
+ case *ast.BinaryExpr:
+ return e.Y, ""
+ case *ast.CallExpr:
+ lit, ok := e.Args[0].(*ast.BasicLit)
+ if !ok || lit.Kind != token.STRING {
+ return nil, "the head's format is not a literal"
+ }
+ f, err := strconv.Unquote(lit.Value)
+ if err != nil {
+ return nil, "the head's format is not a literal"
+ }
+ verbs := 0
+ for _, v := range formatVerb.FindAllString(f, -1) {
+ if strings.Contains(v, "*") || strings.Contains(v, "[") {
+ return nil, "the head's format takes a width or an index argument"
+ }
+ if v != "%%" {
+ verbs++
+ }
+ }
+ if verbs != len(e.Args)-1 {
+ return nil, "the head's format has a different number of verbs than arguments"
+ }
+ return e.Args[len(e.Args)-1], ""
+ }
+ return nil, "the head is not a concatenation or a format call"
+}
+
+// joinOf reads e as a strings.Join call and returns its separator. An
+// identifier assigned once in scope is followed one hop. With params, an
+// identifier that is one of scope's parameters returns its index (param >= 0)
+// for the caller to follow; otherwise param is -1. why says what could not be
+// read.
+func joinOf(e ast.Expr, scope *funcScope, params bool) (sep string, param int, why string) {
+ for hop := 0; ; hop++ {
+ for p, ok := e.(*ast.ParenExpr); ok; p, ok = e.(*ast.ParenExpr) {
+ e = p.X
+ }
+ if c, ok := e.(*ast.CallExpr); ok && isStringsJoin(c) {
+ lit, ok := c.Args[1].(*ast.BasicLit)
+ if !ok || lit.Kind != token.STRING {
+ return "", -1, "the separator of its strings.Join is not a literal"
+ }
+ v, err := strconv.Unquote(lit.Value)
+ if err != nil {
+ return "", -1, "the separator of its strings.Join is not a literal"
+ }
+ return v, -1, ""
+ }
+ id, ok := e.(*ast.Ident)
+ if !ok {
+ return "", -1, fmt.Sprintf("the list is %s, not a strings.Join", render(e))
+ }
+ if scope == nil {
+ return "", -1, fmt.Sprintf("the list is %s, outside any function", id.Name)
+ }
+ vals := assignmentsTo(scope.body, id.Name)
+ pi := -1
+ for i, p := range scope.params {
+ if p == id.Name {
+ pi = i
+ }
+ }
+ switch {
+ case pi >= 0 && len(vals) == 0 && params:
+ return "", pi, ""
+ case pi >= 0 && len(vals) == 0:
+ return "", -1, fmt.Sprintf("the list is %s, a parameter of the caller's own function (one hop only)", id.Name)
+ case len(vals) != 1 || pi >= 0:
+ return "", -1, fmt.Sprintf("the list is %s, which is not assigned exactly once", id.Name)
+ case hop > 0:
+ return "", -1, fmt.Sprintf("the list is %s, more than one local variable away from its join", id.Name)
+ }
+ e = vals[0]
+ }
+}
+
+func isStringsJoin(c *ast.CallExpr) bool {
+ sel, ok := c.Fun.(*ast.SelectorExpr)
+ if !ok || sel.Sel.Name != "Join" || len(c.Args) != 2 {
return false
}
- found := false
- ast.Inspect(f, func(n ast.Node) bool {
+ id, ok := sel.X.(*ast.Ident)
+ return ok && id.Name == "strings"
+}
+
+// assignmentsTo returns the values a function body assigns to name with =,
+// := or var.
+func assignmentsTo(body *ast.BlockStmt, name string) []ast.Expr {
+ var out []ast.Expr
+ if body == nil {
+ return nil
+ }
+ ast.Inspect(body, func(n ast.Node) bool {
+ switch n := n.(type) {
+ case *ast.AssignStmt:
+ for i, l := range n.Lhs {
+ if id, ok := l.(*ast.Ident); ok && id.Name == name {
+ if len(n.Lhs) == len(n.Rhs) {
+ out = append(out, n.Rhs[i])
+ } else {
+ out = append(out, nil, nil) // a multi-value assignment: not read
+ }
+ }
+ }
+ case *ast.ValueSpec:
+ for i, nm := range n.Names {
+ if nm.Name == name && i < len(n.Values) {
+ out = append(out, n.Values[i])
+ }
+ }
+ }
+ return true
+ })
+ return out
+}
+
+// scopedCall is a call and the function it sits in.
+type scopedCall struct {
+ call *ast.CallExpr
+ scope *funcScope
+}
+
+// callsTo returns the calls in f to a function or field called name.
+func callsTo(f goFile, name string) []scopedCall {
+ var out []scopedCall
+ walkScoped(f, func(n ast.Node, scope *funcScope) {
c, ok := n.(*ast.CallExpr)
- if !ok || found || len(c.Args) != 2 {
- return !found
+ if !ok {
+ return
}
- sel, ok := c.Fun.(*ast.SelectorExpr)
- if !ok || sel.Sel.Name != "Join" {
- return true
+ switch fn := c.Fun.(type) {
+ case *ast.Ident:
+ if fn.Name == name {
+ out = append(out, scopedCall{c, scope})
+ }
+ case *ast.SelectorExpr:
+ if fn.Sel.Name == name {
+ out = append(out, scopedCall{c, scope})
+ }
}
- if id, ok := sel.X.(*ast.Ident); !ok || id.Name != "strings" {
- return true
+ })
+ return out
+}
+
+// walkScoped visits every node of a Go file with the innermost function it
+// sits in. A function literal takes the name of the field or variable it is
+// bound to.
+func walkScoped(f goFile, visit func(n ast.Node, scope *funcScope)) {
+ file, err := parser.ParseFile(token.NewFileSet(), f.name, f.body, 0)
+ if err != nil {
+ return
+ }
+ litNames := map[*ast.FuncLit]string{}
+ ast.Inspect(file, func(n ast.Node) bool {
+ switch n := n.(type) {
+ case *ast.KeyValueExpr:
+ if k, ok := n.Key.(*ast.Ident); ok {
+ if fl, ok := n.Value.(*ast.FuncLit); ok {
+ litNames[fl] = k.Name
+ }
+ }
+ case *ast.AssignStmt:
+ for i, l := range n.Lhs {
+ if id, ok := l.(*ast.Ident); ok && i < len(n.Rhs) {
+ if fl, ok := n.Rhs[i].(*ast.FuncLit); ok {
+ litNames[fl] = id.Name
+ }
+ }
+ }
+ case *ast.ValueSpec:
+ for i, nm := range n.Names {
+ if i < len(n.Values) {
+ if fl, ok := n.Values[i].(*ast.FuncLit); ok {
+ litNames[fl] = nm.Name
+ }
+ }
+ }
}
- if lit, ok := c.Args[1].(*ast.BasicLit); ok && lit.Kind == token.STRING {
- if v, err := strconv.Unquote(lit.Value); err == nil && v == ", " {
- found = true
+ return true
+ })
+ paramsOf := func(ft *ast.FuncType) []string {
+ var ps []string
+ for _, fld := range ft.Params.List {
+ if len(fld.Names) == 0 {
+ ps = append(ps, "_")
+ }
+ for _, nm := range fld.Names {
+ ps = append(ps, nm.Name)
+ }
+ }
+ return ps
+ }
+ var nodes []ast.Node
+ var scopes []*funcScope
+ ast.Inspect(file, func(n ast.Node) bool {
+ if n == nil {
+ last := nodes[len(nodes)-1]
+ nodes = nodes[:len(nodes)-1]
+ switch last.(type) {
+ case *ast.FuncDecl, *ast.FuncLit:
+ scopes = scopes[:len(scopes)-1]
}
+ return true
+ }
+ nodes = append(nodes, n)
+ switch n := n.(type) {
+ case *ast.FuncDecl:
+ scopes = append(scopes, &funcScope{name: n.Name.Name, params: paramsOf(n.Type), body: n.Body})
+ case *ast.FuncLit:
+ scopes = append(scopes, &funcScope{name: litNames[n], params: paramsOf(n.Type), body: n.Body})
}
- return !found
+ var scope *funcScope
+ if len(scopes) > 0 {
+ scope = scopes[len(scopes)-1]
+ }
+ visit(n, scope)
+ return true
})
- return found
}
// movesTextApplied reports whether file's body speaks the row's text. In a Go
@@ -399,6 +683,15 @@ func movesTextApplied(file, text string, body []byte) bool {
// Sprintf or errors.New format, with the file's consts and vars read in
// place. A file that does not parse has none.
func goTextPatterns(file string, body []byte) []string {
+ var out []string
+ for _, te := range goTextExprs(file, body) {
+ out = append(out, te.pattern)
+ }
+ return out
+}
+
+// goTextExprs is goTextPatterns with each pattern's expression and function.
+func goTextExprs(file string, body []byte) []textExpr {
f, err := parser.ParseFile(token.NewFileSet(), file, body, 0)
if err != nil {
return nil
@@ -415,8 +708,8 @@ func goTextPatterns(file string, body []byte) []string {
}
}
}
- var out []string
- ast.Inspect(f, func(n ast.Node) bool {
+ var out []textExpr
+ walkScoped(goFile{file, body}, func(n ast.Node, scope *funcScope) {
var e ast.Expr
switch n := n.(type) {
case *ast.BasicLit:
@@ -433,9 +726,10 @@ func goTextPatterns(file string, body []byte) []string {
}
}
if e != nil {
- out = append(out, literalPatterns(e, vars, 0)...)
+ for _, p := range literalPatterns(e, vars, 0) {
+ out = append(out, textExpr{p, e, scope})
+ }
}
- return true
})
return out
}
@@ -703,6 +997,43 @@ func TestMovesCopyGateRefusesTheNegativeFixtures(t *testing.T) {
}
})
+ // Fix round 1: the join is the list's own, traced from the head's fill-in,
+ // never any join in the file.
+ t.Run("an unrelated comma join does not answer for the list's own join", func(t *testing.T) {
+ root := filepath.Join(dir, "list-join-two")
+ rows, err := parseMovesCopyRecord(filepath.Join(root, "record.md"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := checkMovesAppliedWith(rows, root, nil)
+ if len(got) != 1 || !strings.HasPrefix(got[0], "MV-N1:") || !strings.Contains(got[0], `does not join the list with ", " (it joins with " and ")`) {
+ t.Errorf("problems = %q, want exactly MV-N1, for its own join (MV-N2's comma join is another list's)", got)
+ }
+ })
+
+ t.Run("the join that makes the list must be found", func(t *testing.T) {
+ root := filepath.Join(dir, "list-producer")
+ rows, err := parseMovesCopyRecord(filepath.Join(root, "record.md"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ got := checkMovesAppliedWith(rows, root, nil)
+ sort.Strings(got)
+ want := []struct{ id, why string }{
+ {"MV-N1", "cannot find the strings.Join that makes the list"},
+ {"MV-N2", "cannot find the strings.Join that makes the list"},
+ {"MV-N4", `does not join the list with ", " (it joins with " or ")`},
+ }
+ if len(got) != len(want) {
+ t.Fatalf("problems = %q, want exactly MV-N1, MV-N2 and MV-N4 (MV-N3 is spoken)", got)
+ }
+ for i, w := range want {
+ if !strings.HasPrefix(got[i], w.id+":") || !strings.Contains(got[i], w.why) {
+ t.Errorf("problem %d = %q, want %s refused because it %s", i, got[i], w.id, w.why)
+ }
+ }
+ })
+
t.Run("%q prints quotes, so a row shows them and a %s does not satisfy them", func(t *testing.T) {
root := filepath.Join(dir, "quoted-verb")
rows, err := parseMovesCopyRecord(filepath.Join(root, "record.md"))
diff --git a/testdata/moves-copy/list-join-two/head.go b/testdata/moves-copy/list-join-two/head.go
new file mode 100644
index 000000000..1a2876c52
--- /dev/null
+++ b/testdata/moves-copy/list-join-two/head.go
@@ -0,0 +1,15 @@
+package fixture
+
+// Not compiled: testdata is ignored by the go tool.
+
+import "strings"
+
+// MV-N1: the target list, joined with the wrong separator.
+func differs(names []string) string {
+ return "the environment differs from the source's: " + strings.Join(names, " and ")
+}
+
+// MV-N2: an unrelated list in the same file, joined correctly.
+func answered(boxes []string) string {
+ return "the boxes that answered: " + strings.Join(boxes, ", ")
+}
diff --git a/testdata/moves-copy/list-join-two/record.md b/testdata/moves-copy/list-join-two/record.md
new file mode 100644
index 000000000..ea6a6e9b0
--- /dev/null
+++ b/testdata/moves-copy/list-join-two/record.md
@@ -0,0 +1,9 @@
+# Negative fixture: one file, two lists; only the list's own join counts (Task 7 fix round 1)
+
+MV-N1's list is joined with " and ". MV-N2's unrelated list in the same file
+is joined with ", ", which must not answer for MV-N1.
+
+| id | file | text | status |
+|---|---|---|---|
+| MV-N1 | head.go | the environment differs from the source's: <NAME (kind)>, ... | approved |
+| MV-N2 | head.go | the boxes that answered: <box>, ... | approved |
diff --git a/testdata/moves-copy/list-producer/head.go b/testdata/moves-copy/list-producer/head.go
new file mode 100644
index 000000000..c783be94f
--- /dev/null
+++ b/testdata/moves-copy/list-producer/head.go
@@ -0,0 +1,34 @@
+package fixture
+
+// Not compiled: testdata is ignored by the go tool.
+
+import "strings"
+
+// render is a helper the gate does not read. A correct join elsewhere in
+// this file must not answer for it.
+func render(names []string) string { return strings.Join(names, ", ") }
+
+// MV-N1
+func differs(names []string) string {
+ return "the environment differs from the source's: " + render(names)
+}
+
+// MV-N2: nothing calls it.
+func storeDiffers(list string) string {
+ return "the store differs from the source's: " + list
+}
+
+// MV-N3: one caller, through a local variable.
+func zoneLine(list string) string { return "the zones that answered: " + list }
+
+func zones(zs []string) string {
+ joined := strings.Join(zs, ", ")
+ return zoneLine(joined)
+}
+
+// MV-N4: two callers; the second joins with " or ".
+func regionLine(list string) string { return "the regions that answered: " + list }
+
+func regions(rs []string) string { return regionLine(strings.Join(rs, ", ")) }
+
+func regionsOr(rs []string) string { return regionLine(strings.Join(rs, " or ")) }
diff --git a/testdata/moves-copy/list-producer/record.md b/testdata/moves-copy/list-producer/record.md
new file mode 100644
index 000000000..31766b474
--- /dev/null
+++ b/testdata/moves-copy/list-producer/record.md
@@ -0,0 +1,16 @@
+# Negative fixture: the join that makes the list must be found (Task 7 fix round 1)
+
+The gate follows the list's fill-in to its strings.Join: directly, through
+one local variable, or from a parameter to the function's callers.
+
+- MV-N1: the list comes from a helper the gate does not read, so its join cannot be found.
+- MV-N2: the list is a parameter of a function nothing calls.
+- MV-N3: a parameter whose one caller joins with ", " through a local variable (spoken).
+- MV-N4: a parameter with two callers, one joining with " or ".
+
+| id | file | text | status |
+|---|---|---|---|
+| MV-N1 | head.go | the environment differs from the source's: <NAME>, ... | approved |
+| MV-N2 | head.go | the store differs from the source's: <NAME>, ... | approved |
+| MV-N3 | head.go | the zones that answered: <zone>, ... | approved |
+| MV-N4 | head.go | the regions that answered: <region>, ... | approved |
diff --git a/testdata/moves-copy/list-template/elem.go b/testdata/moves-copy/list-template/elem.go
index 876bc3012..b1ac972d8 100644
--- a/testdata/moves-copy/list-template/elem.go
+++ b/testdata/moves-copy/list-template/elem.go
@@ -7,11 +7,22 @@ import (
"strings"
)
-// The elements of MV-N2 and MV-N3, and the join both lists use.
-func alts(sqids, names []string) string {
+// The elements of MV-N2 and MV-N3, built here, and the joins that make
+// their lists, passed to the head's function (one directly, one through a
+// local variable).
+func refuseAmbiguous(seg string, sqids, names []string) string {
out := make([]string, len(sqids))
for i := range sqids {
out[i] = fmt.Sprintf("%s (%q)", sqids[i], names[i])
}
- return strings.Join(out, ", ")
+ return ambiguous(seg, strings.Join(out, ", "))
+}
+
+func refuseSeveral(region string, sqids, names []string) string {
+ out := make([]string, len(sqids))
+ for i := range sqids {
+ out[i] = fmt.Sprintf("%s (%q)", sqids[i], names[i])
+ }
+ joined := strings.Join(out, ", ")
+ return several(region, joined)
}