| index 15d3c3bf1..71275867a 100644 |
| --- a/approved_moves_copy_test.go |
| +++ b/approved_moves_copy_test.go |
| @@ -2,7 +2,6 @@ |
| package main |
| |
| import ( |
| - "bytes" |
| "fmt" |
| "go/ast" |
| "go/parser" |
| @@ -203,30 +202,16 @@ func checkMovesApplied(rows []copyRow, root string) []string { |
| return out |
| } |
| |
| +// movesTextApplied reports whether file's body speaks the row's text. In a Go |
| +// file the text must be one expression: a string literal, a concatenation of |
| +// literals, or one Sprintf or errors.New format. The row's <placeholders> |
| +// and the code's %s match each other, but fragments in unrelated literals are |
| +// never joined, and a comment never counts. Any other file is matched raw, |
| +// the fixed text between placeholders in order. |
| func movesTextApplied(file, text string, body []byte) bool { |
| - segs := copyPlaceholder.Split(text, -1) |
| - if len(segs) == 1 { |
| - if !strings.HasSuffix(file, ".go") { |
| - return bytes.Contains(body, []byte(text)) |
| - } |
| - // In a Go file the text must be inside a string literal's decoded |
| - // value (a whole literal when it is short), never in a comment. |
| - return goLiteralMatches(body, func(v string) bool { |
| - if len(text) >= shortCopyBytes { |
| - return strings.Contains(v, text) |
| - } |
| - return v == text |
| - }) |
| - } |
| - var fixed []string |
| - for _, s := range segs { |
| - if s != "" { |
| - fixed = append(fixed, s) |
| - } |
| - } |
| if !strings.HasSuffix(file, ".go") { |
| rest := string(body) |
| - for _, s := range fixed { |
| + for _, s := range copyPlaceholder.Split(text, -1) { |
| i := strings.Index(rest, s) |
| if i < 0 { |
| return false |
| @@ -235,19 +220,58 @@ func movesTextApplied(file, text string, body []byte) bool { |
| } |
| return true |
| } |
| - next := 0 |
| - goLiteralMatches(body, func(v string) bool { |
| - for next < len(fixed) { |
| - i := strings.Index(v, fixed[next]) |
| - if i < 0 { |
| - break |
| + f, err := parser.ParseFile(token.NewFileSet(), file, body, 0) |
| + if err != nil { |
| + return false |
| + } |
| + idx := &movesPkgIndex{vars: map[string]ast.Expr{}, funcs: map[string]*ast.FuncDecl{}} |
| + for _, d := range f.Decls { |
| + switch d := d.(type) { |
| + case *ast.FuncDecl: |
| + if d.Recv == nil { |
| + idx.funcs[d.Name.Name] = d |
| + } |
| + case *ast.GenDecl: |
| + for _, sp := range d.Specs { |
| + if vs, ok := sp.(*ast.ValueSpec); ok && len(vs.Names) == len(vs.Values) { |
| + for i, nm := range vs.Names { |
| + idx.vars[nm.Name] = vs.Values[i] |
| + } |
| + } |
| + } |
| + } |
| + } |
| + found := false |
| + ast.Inspect(f, func(n ast.Node) bool { |
| + if found { |
| + return false |
| + } |
| + var e ast.Expr |
| + switch n := n.(type) { |
| + case *ast.BasicLit: |
| + if n.Kind == token.STRING { |
| + e = n |
| + } |
| + case *ast.BinaryExpr: |
| + if n.Op == token.ADD { |
| + e = n |
| + } |
| + case *ast.CallExpr: |
| + if isFormatCall(n) { |
| + e = n |
| + } |
| + } |
| + if e == nil { |
| + return true |
| + } |
| + for _, p := range patternsOf(e, idx, 0) { |
| + if informative(p) && movesRowSpoken(text, p) { |
| + found = true |
| } |
| - v = v[i+len(fixed[next]):] |
| - next++ |
| } |
| - return next == len(fixed) |
| + return !found |
| }) |
| - return next == len(fixed) |
| + return found |
| } |
| |
| // checkPendingKeepsSwitchOff is D13: a pending row means the compiled default |
| @@ -558,10 +582,10 @@ const movesScanBase = "c17f39d5" |
| // (the third argument). A Check literal's Reason emits. A function in |
| // movesCopyTables is a table of sentences, and every value in it emits. In a |
| // file in movesLocalSinks, a call to the named local helper emits its |
| -// string argument (readiness.go's fail and reasonOr). In a file in |
| -// movesReturnedSentences, the string results of a function are sentences |
| -// (Ready and Passing return (bool, string), sourceRefusal (string, error)); |
| -// its error results are not. |
| +// string argument (readiness.go's fail and reasonOr). A function listed in |
| +// movesReturnedSentences returns sentences in its string results (Ready and |
| +// Passing return (bool, string), sourceRefusal (string, error)); its error |
| +// results are not. |
| var movesLocalSinks = map[string]map[string]int{ |
| "internal/moves/readiness.go": {"fail": 0, "reasonOr": 1}, |
| "internal/host/movesreport.go": {"fail": 0}, |
| @@ -574,11 +598,13 @@ var movesCopyTables = map[string][]string{ |
| "internal/console/apiinstancemove.go": {"phase1MoveCopy"}, |
| } |
| |
| -var movesReturnedSentences = map[string]bool{ |
| - "internal/moves/readiness.go": true, |
| - "internal/host/movesphase1.go": true, |
| - "internal/host/movesreport.go": true, |
| - "internal/host/movesstage.go": true, |
| +var movesReturnedSentences = map[string]map[string]bool{ |
| + "internal/moves/readiness.go": {"Passing": true, "Ready": true, "budgetEvidence": true, "reasonOr": true, "NoFreshRate": true}, |
| + "internal/host/movesphase1.go": {"sourceRefusal": true, "routeRefusal": true, "publishArchive": true, "archiveFailure": true, |
| + "s1NotExec": true, "s1Sidecar": true, "s1Column": true, "s1PrivateStorage": true, "s1SpecialFile": true, |
| + "s1OverCap": true, "s1LongPath": true, "s1NoAnswer": true}, |
| + "internal/host/movesreport.go": {"t1Below": true, "t3Differs": true, "t3Shadowed": true, "t4Failed": true, |
| + "t6NoRoom": true, "t8Count": true, "t12NotQuiet": true, "replicaReason": true}, |
| } |
| |
| // movesLiteralFuncs are functions whose every string literal is a sentence |
| @@ -636,6 +662,10 @@ type movesPkgIndex struct { |
| |
| var movesPkgCache = map[string]*movesPkgIndex{} |
| |
| +// movesFset positions every file the scan parses, so a violation inside a |
| +// helper in a sibling file can be named file:line. |
| +var movesFset = token.NewFileSet() |
| + |
| func loadMovesPkg(t *testing.T, dir string) *movesPkgIndex { |
| t.Helper() |
| if p, ok := movesPkgCache[dir]; ok { |
| @@ -652,7 +682,7 @@ func loadMovesPkg(t *testing.T, dir string) *movesPkgIndex { |
| if e.IsDir() || !strings.HasSuffix(n, ".go") || strings.HasSuffix(n, "_test.go") { |
| continue |
| } |
| - f, err := parser.ParseFile(token.NewFileSet(), filepath.Join(dir, n), nil, 0) |
| + f, err := parser.ParseFile(movesFset, filepath.Join(dir, n), nil, 0) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -748,12 +778,13 @@ func returnsOneString(d *ast.FuncDecl) bool { |
| } |
| |
| type movesEmission struct { |
| - file string |
| - line int // where the call or literal starts |
| - endLine int |
| - fn string // the enclosing function |
| - kind string |
| - pattern string |
| + file string |
| + line int // where the call or literal starts |
| + endLine int |
| + fn string // the enclosing function |
| + kind string |
| + pattern string |
| + fragment bool // a piece of a sentence, matched by containment |
| } |
| |
| func (e movesEmission) String() string { |
| @@ -785,6 +816,26 @@ func scanMovesFile(t *testing.T, file string) []movesEmission { |
| // functions as wildcards, so a legacy function's sentence is only the text |
| // written in that function. |
| func scanMovesFileWith(t *testing.T, root, file string, noCalls bool) []movesEmission { |
| + t.Helper() |
| + ems, _ := scanMovesFileFull(t, root, file, noCalls) |
| + return ems |
| +} |
| + |
| +// movesViolation is a sink that does not take a sentence the gate can read. |
| +type movesViolation struct { |
| + file string |
| + line int |
| + fn string |
| + kind string |
| + why string |
| +} |
| + |
| +func (v movesViolation) String() string { |
| + return fmt.Sprintf("%s:%d (%s in %s): %s", v.file, v.line, v.kind, v.fn, v.why) |
| +} |
| + |
| +// scanMovesFileFull returns a file's emissions and its sink violations. |
| +func scanMovesFileFull(t *testing.T, root, file string, noCalls bool) ([]movesEmission, []movesViolation) { |
| t.Helper() |
| at := func(p string) string { |
| if root == "." { |
| @@ -792,7 +843,7 @@ func scanMovesFileWith(t *testing.T, root, file string, noCalls bool) []movesEmi |
| } |
| return filepath.Join(root, p) |
| } |
| - fset := token.NewFileSet() |
| + fset := movesFset |
| f, err := parser.ParseFile(fset, at(file), nil, 0) |
| if err != nil { |
| t.Fatal(err) |
| @@ -801,11 +852,24 @@ func scanMovesFileWith(t *testing.T, root, file string, noCalls bool) []movesEmi |
| idx.noCalls = noCalls |
| pkg := &idx |
| var out []movesEmission |
| + var vios []movesViolation |
| + seenVio := map[string]bool{} |
| var fn string |
| - emit := func(pos token.Pos, kind string, e ast.Expr) { |
| + var ctx *sinkCtx |
| + emit := func(pos token.Pos, kind string, e ast.Expr, fragment bool) { |
| for _, p := range patternsOf(e, pkg, 0) { |
| if informative(p) { |
| - out = append(out, movesEmission{file, fset.Position(pos).Line, fset.Position(e.End()).Line, fn, kind, p}) |
| + out = append(out, movesEmission{file, fset.Position(pos).Line, fset.Position(e.End()).Line, fn, kind, p, fragment}) |
| + } |
| + } |
| + if fragment { |
| + return |
| + } |
| + if why := ctx.check(e, 0); why != "" { |
| + v := movesViolation{file, fset.Position(pos).Line, fn, kind, why} |
| + if !seenVio[v.String()] { |
| + seenVio[v.String()] = true |
| + vios = append(vios, v) |
| } |
| } |
| } |
| @@ -832,16 +896,30 @@ func scanMovesFileWith(t *testing.T, root, file string, noCalls bool) []movesEmi |
| continue |
| } |
| fn = fd.Name.Name |
| + ctx = newSinkCtx(file, fd, pkg) |
| inTable := tables[fn] |
| strs := stringResults(fd) |
| - returnsSentence := movesReturnedSentences[file] && len(strs) > 0 |
| + returnsSentence := movesReturnedSentences[file][fn] && len(strs) > 0 |
| + var stack []ast.Node |
| + litDepth := 0 |
| ast.Inspect(fd.Body, func(n ast.Node) bool { |
| + if n == nil { |
| + if _, ok := stack[len(stack)-1].(*ast.FuncLit); ok { |
| + litDepth-- |
| + } |
| + stack = stack[:len(stack)-1] |
| + return true |
| + } |
| + stack = append(stack, n) |
| switch n := n.(type) { |
| + case *ast.FuncLit: |
| + litDepth++ |
| case *ast.BasicLit: |
| // A literal with an em dash is main's wording, which phase1Conflict |
| - // matches and rewrites; it is never spoken. |
| + // matches and rewrites; it is never spoken. A literal here is a |
| + // piece of a sentence unless it is compared whole or returned. |
| if literals[fn] && n.Kind == token.STRING && !strings.Contains(n.Value, `\u2014`) && !strings.ContainsRune(n.Value, '\u2014') { |
| - emit(n.Pos(), fn, n) |
| + emit(n.Pos(), fn, n, true) |
| } |
| case *ast.CallExpr: |
| name := "" |
| @@ -856,14 +934,23 @@ func scanMovesFileWith(t *testing.T, root, file string, noCalls bool) []movesEmi |
| i, sink = 2, true |
| } |
| if sink && i < len(n.Args) { |
| - emit(n.Pos(), name, n.Args[i]) |
| + emit(n.Pos(), name, n.Args[i], false) |
| + } |
| + if j, fsink := movesFuncSinks[file][name]; fsink && j < len(n.Args) { |
| + if why := ctx.checkFuncArg(n.Args[j]); why != "" { |
| + v := movesViolation{file, fset.Position(n.Pos()).Line, fn, name, why} |
| + if !seenVio[v.String()] { |
| + seenVio[v.String()] = true |
| + vios = append(vios, v) |
| + } |
| + } |
| } |
| case *ast.CompositeLit: |
| if n.Type != nil && isCheck(n.Type) { |
| for _, el := range n.Elts { |
| if kv, ok := el.(*ast.KeyValueExpr); ok { |
| if k, ok := kv.Key.(*ast.Ident); ok && k.Name == "Reason" { |
| - emit(kv.Pos(), "Check.Reason", kv.Value) |
| + emit(kv.Pos(), "Check.Reason", kv.Value, false) |
| } |
| } |
| } |
| @@ -871,25 +958,38 @@ func scanMovesFileWith(t *testing.T, root, file string, noCalls bool) []movesEmi |
| if inTable { |
| for _, el := range n.Elts { |
| if kv, ok := el.(*ast.KeyValueExpr); ok { |
| - emit(kv.Pos(), fd.Name.Name, kv.Value) |
| + switch kv.Value.(type) { |
| + case *ast.FuncLit, *ast.CompositeLit: |
| + // closures are read through their returns; nested tables by descent |
| + default: |
| + emit(kv.Pos(), fn, kv.Value, false) |
| + } |
| } |
| } |
| } |
| case *ast.AssignStmt: |
| if len(n.Lhs) == 1 && len(n.Rhs) == 1 { |
| if s, ok := n.Lhs[0].(*ast.SelectorExpr); ok && s.Sel.Name == "Reason" && isCheck(s.X) { |
| - emit(n.Pos(), "Check.Reason", n.Rhs[0]) |
| + emit(n.Pos(), "Check.Reason", n.Rhs[0], false) |
| } |
| } |
| case *ast.ReturnStmt: |
| - if inTable { |
| + switch { |
| + case inTable: |
| for _, r := range n.Results { |
| - emit(r.Pos(), fd.Name.Name, r) |
| + if _, nested := r.(*ast.CompositeLit); !nested { |
| + emit(r.Pos(), fn, r, false) |
| + } |
| } |
| - } else if returnsSentence { |
| + case literals[fn]: |
| + // phase1Conflict's returns are sentences too. |
| + for _, r := range n.Results { |
| + emit(r.Pos(), fn, r, false) |
| + } |
| + case returnsSentence && litDepth == 0: |
| for i, r := range n.Results { |
| - if strs[i] && !isIdentOrCall(r) { |
| - emit(r.Pos(), "returned sentence", r) |
| + if strs[i] { |
| + emit(r.Pos(), "returned sentence", r, false) |
| } |
| } |
| } |
| @@ -897,68 +997,397 @@ func scanMovesFileWith(t *testing.T, root, file string, noCalls bool) []movesEmi |
| return true |
| }) |
| } |
| - return out |
| + return out, vios |
| } |
| |
| -// stringResults is the set of a function's result positions that are strings. |
| -func stringResults(d *ast.FuncDecl) map[int]bool { |
| - out := map[int]bool{} |
| - if d.Type.Results == nil { |
| +// ---- sinks must take sentences the gate can read ---- |
| + |
| +// movesFuncSinks are calls that take a sentence-producing function as an |
| +// argument (the index is where): the argument must be a recognised sentence |
| +// helper, and the parameter that receives it may be called inside the callee. |
| +var movesFuncSinks = map[string]map[string]int{ |
| + "internal/console/apiinstancemove.go": {"moveTransitionSaying": 7}, |
| +} |
| + |
| +// movesSinkAllow exempts a function's sinks from the readable-sentence rule. |
| +// An entry is only for a value that never reaches an operator, and says why. |
| +// A value that does reach an operator is mapped to a recorded sentence in the |
| +// code instead. TestMovesSinkAllowListIsHonest holds the list to this. |
| +var movesSinkAllow = map[string]map[string]string{} |
| + |
| +// sinkCtx classifies what a sink argument is, inside one function. |
| +type sinkCtx struct { |
| + file string |
| + fd *ast.FuncDecl |
| + pkg *movesPkgIndex |
| + fwd map[string]bool // string parameters whose values are sinks at the call sites |
| + fwdFn map[string]bool // function parameters that are sentence helpers at the call sites |
| +} |
| + |
| +func paramNames(ft *ast.FuncType) []string { |
| + var out []string |
| + if ft.Params == nil { |
| return out |
| } |
| - i := 0 |
| - for _, f := range d.Type.Results.List { |
| - n := len(f.Names) |
| - if n == 0 { |
| - n = 1 |
| + for _, f := range ft.Params.List { |
| + if len(f.Names) == 0 { |
| + out = append(out, "_") |
| } |
| - if id, ok := f.Type.(*ast.Ident); ok && id.Name == "string" { |
| - for k := 0; k < n; k++ { |
| - out[i+k] = true |
| - } |
| + for _, n := range f.Names { |
| + out = append(out, n.Name) |
| } |
| - i += n |
| } |
| return out |
| } |
| |
| -// isIdentOrCall is true for a returned value that is a variable or a call to |
| -// something other than fmt.Sprintf: those carry no sentence of their own. |
| -func isIdentOrCall(e ast.Expr) bool { |
| - switch e := e.(type) { |
| - case *ast.Ident, *ast.SelectorExpr: |
| +func newSinkCtx(file string, fd *ast.FuncDecl, pkg *movesPkgIndex) *sinkCtx { |
| + c := &sinkCtx{file: file, fd: fd, pkg: pkg, fwd: map[string]bool{}, fwdFn: map[string]bool{}} |
| + if fd == nil { |
| + return c |
| + } |
| + if i, ok := movesLocalSinks[file][fd.Name.Name]; ok { |
| + if ps := paramNames(fd.Type); i < len(ps) { |
| + c.fwd[ps[i]] = true |
| + } |
| + } |
| + if i, ok := movesFuncSinks[file][fd.Name.Name]; ok { |
| + if ps := paramNames(fd.Type); i < len(ps) { |
| + c.fwdFn[ps[i]] = true |
| + } |
| + } |
| + // A closure that is a local sink (fail := func(why string)) forwards its parameter. |
| + ast.Inspect(fd.Body, func(n ast.Node) bool { |
| + as, ok := n.(*ast.AssignStmt) |
| + if !ok || len(as.Lhs) != 1 || len(as.Rhs) != 1 { |
| + return true |
| + } |
| + id, ok := as.Lhs[0].(*ast.Ident) |
| + lit, isLit := as.Rhs[0].(*ast.FuncLit) |
| + if !ok || !isLit { |
| + return true |
| + } |
| + if i, ok := movesLocalSinks[file][id.Name]; ok { |
| + if ps := paramNames(lit.Type); i < len(ps) { |
| + c.fwd[ps[i]] = true |
| + } |
| + } |
| return true |
| + }) |
| + return c |
| +} |
| + |
| +// isConstLike reports whether an identifier names a package-level string. |
| +func (c *sinkCtx) pkgString(name string) (ast.Expr, bool) { |
| + x, ok := c.pkg.vars[name] |
| + return x, ok |
| +} |
| + |
| +// literalish reports whether e is, on its face, a piece of fixed text: a |
| +// string literal, a package const, a Sprintf or errors.New with a literal |
| +// format, or a call to a recognised sentence helper. |
| +func (c *sinkCtx) literalish(e ast.Expr, depth int) bool { |
| + switch e := e.(type) { |
| + case *ast.ParenExpr: |
| + return c.literalish(e.X, depth) |
| + case *ast.BasicLit: |
| + return e.Kind == token.STRING |
| + case *ast.Ident: |
| + _, ok := c.pkgString(e.Name) |
| + return ok |
| case *ast.CallExpr: |
| - if s, ok := e.Fun.(*ast.SelectorExpr); ok { |
| - if id, ok := s.X.(*ast.Ident); ok && id.Name == "fmt" { |
| - return false |
| + return isFormatCall(e) && c.formatIsLiteral(e) |
| + case *ast.BinaryExpr: |
| + if e.Op == token.ADD { |
| + return c.literalish(e.X, depth) || c.literalish(e.Y, depth) |
| + } |
| + } |
| + return false |
| +} |
| + |
| +func isFormatCall(e *ast.CallExpr) bool { |
| + sel, ok := e.Fun.(*ast.SelectorExpr) |
| + if !ok || len(e.Args) == 0 { |
| + return false |
| + } |
| + id, ok := sel.X.(*ast.Ident) |
| + return ok && ((id.Name == "fmt" && (sel.Sel.Name == "Sprintf" || sel.Sel.Name == "Errorf")) || |
| + (id.Name == "errors" && sel.Sel.Name == "New")) |
| +} |
| + |
| +func (c *sinkCtx) formatIsLiteral(e *ast.CallExpr) bool { |
| + switch a := e.Args[0].(type) { |
| + case *ast.BasicLit: |
| + return a.Kind == token.STRING |
| + case *ast.Ident: |
| + _, ok := c.pkgString(a.Name) |
| + return ok |
| + } |
| + return false |
| +} |
| + |
| +// localSentence reports whether a function assigns a string literal (or a |
| +// literal concatenation or Sprintf) to the local variable name. |
| +func (c *sinkCtx) localSentence(name string) bool { |
| + if c.fd == nil { |
| + return false |
| + } |
| + found := false |
| + ast.Inspect(c.fd.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 && i < len(n.Rhs) && isSentenceLiteral(n.Rhs[i]) { |
| + found = true |
| + } |
| + } |
| + case *ast.ValueSpec: |
| + for i, id := range n.Names { |
| + if id.Name == name && i < len(n.Values) && isSentenceLiteral(n.Values[i]) { |
| + found = true |
| + } |
| } |
| } |
| - return true |
| + return !found |
| + }) |
| + return found |
| +} |
| + |
| +// isSentenceLiteral is true for text written in place: a string literal, a |
| +// Sprintf or errors.New, or a concatenation with a string literal in it. A |
| +// reference to a const is not text written in place. |
| +func isSentenceLiteral(e ast.Expr) bool { |
| + switch e := e.(type) { |
| + case *ast.ParenExpr: |
| + return isSentenceLiteral(e.X) |
| + case *ast.BasicLit: |
| + return e.Kind == token.STRING |
| + case *ast.CallExpr: |
| + return isFormatCall(e) |
| + case *ast.BinaryExpr: |
| + return e.Op == token.ADD && (isSentenceLiteral(e.X) || isSentenceLiteral(e.Y)) |
| } |
| return false |
| } |
| |
| -// movesRowMatches reports whether a row carries an emitted sentence: the |
| -// sentence's fixed text appears in the row (the parts the code fills in match |
| -// anything), or the row's own <placeholders> are filled by the sentence (the |
| -// row says "a <role> does not move" and the code says "a replica does not |
| -// move"). A prefix a caller adds around a reason ("S2: ") is outside the |
| -// sentence, so containment is enough. |
| +// operandProblem is the one thing a placeholder operand may not be: a local |
| +// variable that was given a sentence. |
| +func (c *sinkCtx) operandProblem(e ast.Expr) string { |
| + if id, ok := e.(*ast.Ident); ok { |
| + if _, isPkg := c.pkgString(id.Name); !isPkg && c.localSentence(id.Name) { |
| + return fmt.Sprintf("%s holds a sentence assigned before the sink, so the sentence is never read where it is spoken", id.Name) |
| + } |
| + } |
| + return "" |
| +} |
| + |
| +func addOperands(e ast.Expr, out *[]ast.Expr) { |
| + if b, ok := e.(*ast.BinaryExpr); ok && b.Op == token.ADD { |
| + addOperands(b.X, out) |
| + addOperands(b.Y, out) |
| + return |
| + } |
| + if p, ok := e.(*ast.ParenExpr); ok { |
| + addOperands(p.X, out) |
| + return |
| + } |
| + *out = append(*out, e) |
| +} |
| + |
| +func render(e ast.Expr) string { |
| + var b strings.Builder |
| + ast.Inspect(e, func(n ast.Node) bool { |
| + switch n := n.(type) { |
| + case *ast.BinaryExpr: |
| + b.WriteString(render(n.X) + " " + n.Op.String() + " " + render(n.Y)) |
| + return false |
| + case *ast.Ident: |
| + b.WriteString(n.Name) |
| + case *ast.SelectorExpr: |
| + b.WriteString(render(n.X) + "." + n.Sel.Name) |
| + return false |
| + case *ast.CallExpr: |
| + b.WriteString(render(n.Fun) + "(...)") |
| + return false |
| + } |
| + return true |
| + }) |
| + return b.String() |
| +} |
| + |
| +// check returns why e is not a sentence the gate can read, or "". |
| +// |
| +// Taken: a string literal; a package const; a concatenation with a literal |
| +// part or a Sprintf with a literal format (the other operands are the |
| +// placeholders the record names); a call to a package function that returns |
| +// a string and whose own returns are taken; a parameter that is itself a |
| +// sink at its call sites; a Check's own Reason (every Check is made at a |
| +// sink). Everything else is refused: a variable, err.Error(), a method's |
| +// result, a field, an expression the gate cannot read. |
| +func (c *sinkCtx) check(e ast.Expr, depth int) string { |
| + switch e := e.(type) { |
| + case *ast.ParenExpr: |
| + return c.check(e.X, depth) |
| + case *ast.BasicLit: |
| + if e.Kind == token.STRING { |
| + return "" |
| + } |
| + return "not a string literal" |
| + case *ast.Ident: |
| + if c.fwd[e.Name] { |
| + return "" |
| + } |
| + if x, ok := c.pkgString(e.Name); ok { |
| + if depth >= 4 { |
| + return "" |
| + } |
| + return (&sinkCtx{pkg: c.pkg, fwd: map[string]bool{}, fwdFn: map[string]bool{}}).check(x, depth+1) |
| + } |
| + return fmt.Sprintf("%s is a variable, not a literal, a const or a sentence helper", e.Name) |
| + case *ast.SelectorExpr: |
| + if e.Sel.Name == "Reason" { |
| + return "" |
| + } |
| + return fmt.Sprintf("%s is a field the gate cannot read", render(e)) |
| + case *ast.BinaryExpr: |
| + if e.Op != token.ADD { |
| + return "an expression that is not a sentence" |
| + } |
| + var ops []ast.Expr |
| + addOperands(e, &ops) |
| + lit := false |
| + for _, o := range ops { |
| + if c.literalish(o, depth) { |
| + lit = true |
| + if why := c.check(o, depth); why != "" { |
| + return why |
| + } |
| + } else if why := c.operandProblem(o); why != "" { |
| + return why |
| + } |
| + } |
| + if !lit { |
| + return fmt.Sprintf("%s is a concatenation with no literal part", render(e)) |
| + } |
| + return "" |
| + case *ast.CallExpr: |
| + if isFormatCall(e) { |
| + if !c.formatIsLiteral(e) { |
| + return fmt.Sprintf("%s has a format that is not a literal", render(e)) |
| + } |
| + for _, a := range e.Args[1:] { |
| + if why := c.operandProblem(a); why != "" { |
| + return why |
| + } |
| + } |
| + return "" |
| + } |
| + if id, ok := e.Fun.(*ast.Ident); ok { |
| + if c.fwdFn[id.Name] { |
| + return "" |
| + } |
| + return c.helperProblem(id.Name, depth) |
| + } |
| + return fmt.Sprintf("%s forwards a call's result", render(e)) |
| + } |
| + return fmt.Sprintf("%s is not a sentence the gate can read", render(e)) |
| +} |
| + |
| +// helperProblem checks a package function used as a sentence helper: it must |
| +// return one string, and every return must itself be taken. |
| +func (c *sinkCtx) helperProblem(name string, depth int) string { |
| + d, ok := c.pkg.funcs[name] |
| + if !ok || d.Body == nil { |
| + return fmt.Sprintf("%s(...) is not a sentence helper of this package", name) |
| + } |
| + if !returnsOneString(d) { |
| + return fmt.Sprintf("%s(...) does not return a sentence", name) |
| + } |
| + if depth >= 4 { |
| + return "" |
| + } |
| + hc := &sinkCtx{fd: d, pkg: c.pkg, fwd: map[string]bool{}, fwdFn: map[string]bool{}} |
| + var why string |
| + ast.Inspect(d.Body, func(n ast.Node) bool { |
| + switch n := n.(type) { |
| + case *ast.FuncLit: |
| + return false |
| + case *ast.ReturnStmt: |
| + if why == "" && len(n.Results) == 1 { |
| + if w := hc.check(n.Results[0], depth+1); w != "" { |
| + why = fmt.Sprintf("helper %s, at %s: %s", name, movesFset.Position(n.Pos()), w) |
| + } |
| + } |
| + } |
| + return why == "" |
| + }) |
| + return why |
| +} |
| + |
| +// checkFuncArg checks the function a call hands to a sentence-function |
| +// parameter: a package function that is a sentence helper. |
| +func (c *sinkCtx) checkFuncArg(e ast.Expr) string { |
| + if id, ok := e.(*ast.Ident); ok { |
| + if _, isFn := c.pkg.funcs[id.Name]; isFn { |
| + return c.helperProblem(id.Name, 0) |
| + } |
| + } |
| + return fmt.Sprintf("%s is passed where a sentence helper is expected", render(e)) |
| +} |
| + |
| +// diagnosticPrefixes are the ids the readiness code puts before a reason |
| +// ("T11: ", "S2: ") and the record leaves on its row or off it. |
| +var diagnosticPrefixes = []string{"T11: ", "S1: ", "S2: "} |
| + |
| +// movesRowMatches reports whether a row carries an emitted sentence, whole: |
| +// the sentence equals the row's text, where the parts the code fills in |
| +// match anything, or the row's own <placeholders> are filled by the sentence |
| +// (the row says "a <role> does not move" and the code says "a replica does |
| +// not move"). A diagnostic prefix ("S2: ") may sit on either side. A |
| +// sentence that only appears inside a row does not match: "valid region" is |
| +// not "invalid region". |
| func movesRowMatches(text, pattern string) bool { |
| + return rowMatches(text, pattern, false) |
| +} |
| + |
| +// movesRowSpoken is the stricter match for an approved row: the code must say |
| +// all of the row's fixed text. The code's own fill-ins may only stand where |
| +// the row has a <placeholder>, never where the row has words. |
| +func movesRowSpoken(text, pattern string) bool { |
| + return rowMatches(text, pattern, true) |
| +} |
| + |
| +func rowMatches(text, pattern string, strict bool) bool { |
| + forms := []string{text} |
| + for _, p := range diagnosticPrefixes { |
| + if strings.HasPrefix(text, p) { |
| + forms = append(forms, strings.TrimPrefix(text, p)) |
| + } |
| + } |
| parts := strings.Split(pattern, wild) |
| for i := range parts { |
| parts[i] = regexp.QuoteMeta(parts[i]) |
| } |
| - if re, err := regexp.Compile(`(?s)` + strings.Join(parts, `.*`)); err == nil && re.MatchString(text) { |
| - return true |
| - } |
| - rowParts := copyPlaceholder.Split(text, -1) |
| - for i := range rowParts { |
| - rowParts[i] = regexp.QuoteMeta(rowParts[i]) |
| + code := regexp.MustCompile(`(?s)^` + strings.Join(parts, `.*`) + `$`) |
| + rendered := strings.ReplaceAll(pattern, wild, "%") |
| + for _, f := range forms { |
| + if !strict && code.MatchString(f) { |
| + return true |
| + } |
| + rowParts := copyPlaceholder.Split(f, -1) |
| + for i := range rowParts { |
| + rowParts[i] = regexp.QuoteMeta(rowParts[i]) |
| + } |
| + if regexp.MustCompile(`(?s)^` + strings.Join(rowParts, `.*`) + `$`).MatchString(rendered) { |
| + return true |
| + } |
| } |
| - re, err := regexp.Compile(`(?s)^` + strings.Join(rowParts, `.*`) + `$`) |
| - return err == nil && re.MatchString(strings.ReplaceAll(pattern, wild, "%")) |
| + return false |
| +} |
| + |
| +// movesFragmentMatches is the match for a piece of a sentence: a literal |
| +// phase1Conflict tests with HasPrefix or Contains. Some row must contain it. |
| +func movesFragmentMatches(text, pattern string) bool { |
| + return strings.Contains(text, strings.ReplaceAll(pattern, wild, "")) |
| } |
| |
| // movesLegacy reports whether an emission is main's own. |
| @@ -979,7 +1408,11 @@ func findUnrecorded(ems []movesEmission, rows []copyRow) []movesEmission { |
| } |
| found := false |
| for _, r := range rows { |
| - if movesRowMatches(r.Text, e.pattern) { |
| + match := movesRowMatches |
| + if e.fragment { |
| + match = movesFragmentMatches |
| + } |
| + if match(r.Text, e.pattern) { |
| found = true |
| break |
| } |
| @@ -1059,28 +1492,66 @@ func f(w int, c Check) { |
| } |
| } |
| |
| +// movesProductionGoFile reports whether a path from the diff is a production |
| +// Go file: not a test, and not under a testdata directory (the go tool |
| +// ignores those, and the negative fixtures live there). |
| +func movesProductionGoFile(path string) bool { |
| + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { |
| + return false |
| + } |
| + for _, el := range strings.Split(path, "/") { |
| + if el == "testdata" { |
| + return false |
| + } |
| + } |
| + return true |
| +} |
| + |
| +// movesUncovered returns the production files in changed that listed lacks. |
| +// exists says whether a path is still in the tree (a deleted file needs no |
| +// entry). |
| +func movesUncovered(changed, listed []string, exists func(string) bool) []string { |
| + have := map[string]bool{} |
| + for _, f := range listed { |
| + have[f] = true |
| + } |
| + var out []string |
| + for _, f := range changed { |
| + if movesProductionGoFile(f) && exists(f) && !have[f] { |
| + out = append(out, f) |
| + } |
| + } |
| + return out |
| +} |
| + |
| +func TestMovesScanFilesInventoryRules(t *testing.T) { |
| + exists := func(p string) bool { return p != "internal/gone.go" } |
| + changed := []string{ |
| + "internal/moves/new.go", // production: needs an entry |
| + "internal/moves/new_test.go", // a test |
| + "testdata/moves-copy/changed-text/applied.go", // a fixture |
| + "internal/host/testdata/fixture/emit.go", // a fixture below a package |
| + "internal/gone.go", // deleted |
| + "docs/superpowers/plans/2026-10-06-copy.md", // not Go |
| + "internal/moves/listed.go", // listed |
| + } |
| + got := movesUncovered(changed, []string{"internal/moves/listed.go"}, exists) |
| + if len(got) != 1 || got[0] != "internal/moves/new.go" { |
| + t.Errorf("uncovered = %q, want only internal/moves/new.go", got) |
| + } |
| +} |
| + |
| // The scan reads the files listed in movesScanFiles. When moves-phase1's diff |
| -// names a Go file the list does not, the scan has a blind spot: add it. The |
| -// check is skipped where git or the base commit is not available. |
| +// names a production Go file the list does not, the scan has a blind spot: |
| +// add it. The check is skipped where git or the base commit is not available. |
| func TestMovesScanFilesCoverTheDiff(t *testing.T) { |
| out, err := exec.Command("git", "diff", "--name-only", movesScanBase+"..HEAD", "--", "*.go").Output() |
| if err != nil { |
| t.Skipf("git diff %s..HEAD: %v", movesScanBase, err) |
| } |
| - listed := map[string]bool{} |
| - for _, f := range movesScanFiles { |
| - listed[f] = true |
| - } |
| - for _, f := range strings.Fields(string(out)) { |
| - if strings.HasSuffix(f, "_test.go") { |
| - continue |
| - } |
| - if _, err := os.Stat(f); err != nil { |
| - continue // deleted |
| - } |
| - if !listed[f] { |
| - t.Errorf("%s changed since %s and is not in movesScanFiles", f, movesScanBase) |
| - } |
| + exists := func(p string) bool { _, err := os.Stat(p); return err == nil } |
| + for _, f := range movesUncovered(strings.Fields(string(out)), movesScanFiles, exists) { |
| + t.Errorf("%s changed since %s and is not in movesScanFiles", f, movesScanBase) |
| } |
| for _, f := range movesScanFiles { |
| if _, err := os.Stat(f); err != nil { |
| @@ -1202,3 +1673,190 @@ func TestMovesGateBitesOnTheRealRecord(t *testing.T) { |
| t.Error("a default that is on beside pending rows must fail") |
| } |
| } |
| + |
| +// stringResults is the set of a function's result positions that are strings. |
| +func stringResults(d *ast.FuncDecl) map[int]bool { |
| + out := map[int]bool{} |
| + if d.Type.Results == nil { |
| + return out |
| + } |
| + i := 0 |
| + for _, f := range d.Type.Results.List { |
| + n := len(f.Names) |
| + if n == 0 { |
| + n = 1 |
| + } |
| + if id, ok := f.Type.(*ast.Ident); ok && id.Name == "string" { |
| + for k := 0; k < n; k++ { |
| + out[i+k] = true |
| + } |
| + } |
| + i += n |
| + } |
| + return out |
| +} |
| + |
| +// movesViolationExempt reports whether a violation is main's own (legacy) or |
| +// is allow-listed with a reason. |
| +func movesViolationExempt(v movesViolation) bool { |
| + if _, ok := movesLegacyFiles[v.file]; ok { |
| + return true |
| + } |
| + if _, ok := movesLegacyFuncs[v.file][v.fn]; ok { |
| + return true |
| + } |
| + _, ok := movesSinkAllow[v.file][v.fn] |
| + return ok |
| +} |
| + |
| +// Every value that reaches an operator sink in the scanned files is a |
| +// sentence the gate can read: a literal, a const, a concatenation or Sprintf |
| +// with a literal format, or a call to a sentence helper. A forwarded |
| +// err.Error(), a variable holding text, or an expression the gate cannot read |
| +// fails here, naming file:line, because the record cannot vouch for text the |
| +// gate never sees. |
| +func TestMovesSinksAreSentences(t *testing.T) { |
| + for _, f := range movesScanFiles { |
| + _, vios := scanMovesFileFull(t, ".", f, false) |
| + for _, v := range vios { |
| + if !movesViolationExempt(v) { |
| + t.Errorf("%s", v) |
| + } |
| + } |
| + } |
| +} |
| + |
| +// ---- negative fixtures for the sink rule, the matcher and the placeholder check ---- |
| + |
| +func sinkFixture(t *testing.T, dir string) []movesViolation { |
| + t.Helper() |
| + _, v := scanMovesFileFull(t, ".", filepath.Join("testdata", "moves-copy", "sinks", dir, "emit.go"), false) |
| + return v |
| +} |
| + |
| +func wantViolations(t *testing.T, name string, got []movesViolation, wants ...string) { |
| + t.Helper() |
| + if len(got) != len(wants) { |
| + t.Errorf("%s: %d violations %v, want %d", name, len(got), got, len(wants)) |
| + return |
| + } |
| + for i, w := range wants { |
| + if !strings.Contains(got[i].String(), w) { |
| + t.Errorf("%s: violation %d = %q, want one containing %q", name, i, got[i], w) |
| + } |
| + } |
| +} |
| + |
| +func TestMovesSinkFixtures(t *testing.T) { |
| + wantViolations(t, "a forwarded error", sinkFixture(t, "forwarded-error"), |
| + "emit.go:7 (jsonError in refuse): err.Error(...) forwards a call's result", |
| + "emit.go:18 (Check.Reason in report): helper why, at ", |
| + ) |
| + wantViolations(t, "a message assigned to a local before the sink", sinkFixture(t, "local-variable"), |
| + "emit.go:8 (jsonError in refuse): msg is a variable", |
| + "emit.go:16 (jsonError in refuseLate): text is a variable", |
| + "emit.go:21 (jsonError in inside): gone holds a sentence assigned before the sink", |
| + ) |
| + wantViolations(t, "values the gate cannot read", sinkFixture(t, "unresolved"), |
| + "emit.go:6 (jsonError in methodResult): s.render(...) forwards a call's result", |
| + "emit.go:10 (jsonError in indexed): names is not a sentence the gate can read", |
| + "emit.go:14 (Check.Reason in bare): a + b is a concatenation with no literal part", |
| + "emit.go:18 (jsonError in dynamicFormat): fmt.Sprintf(...) has a format that is not a literal", |
| + "emit.go:22 (jsonError in field): r.Message is a field the gate cannot read", |
| + ) |
| + wantViolations(t, "sentences the gate can read", sinkFixture(t, "ok")) |
| +} |
| + |
| +// An allow-list entry exempts a sink, but it needs a reason and must still |
| +// exempt something. |
| +func checkSinkAllow(allow map[string]map[string]string, vios []movesViolation) []string { |
| + used := map[string]bool{} |
| + for _, v := range vios { |
| + used[v.file+" "+v.fn] = true |
| + } |
| + var out []string |
| + for file, fns := range allow { |
| + for fn, why := range fns { |
| + if len(strings.Fields(why)) < 5 { |
| + out = append(out, fmt.Sprintf("%s %s: the allow-list entry needs a comment saying why the value never reaches an operator", file, fn)) |
| + } |
| + if !used[file+" "+fn] { |
| + out = append(out, fmt.Sprintf("%s %s: the allow-list entry exempts nothing; drop it", file, fn)) |
| + } |
| + } |
| + } |
| + sort.Strings(out) |
| + return out |
| +} |
| + |
| +func TestMovesSinkAllowListIsHonest(t *testing.T) { |
| + var all []movesViolation |
| + for _, f := range movesScanFiles { |
| + _, v := scanMovesFileFull(t, ".", f, false) |
| + all = append(all, v...) |
| + } |
| + reportAll(t, checkSinkAllow(movesSinkAllow, all)) |
| + |
| + vios := []movesViolation{{file: "a.go", fn: "f"}} |
| + if got := checkSinkAllow(map[string]map[string]string{"a.go": {"f": "because"}}, vios); len(got) != 1 || !strings.Contains(got[0], "needs a comment") { |
| + t.Errorf("an entry with no reason: %q", got) |
| + } |
| + if got := checkSinkAllow(map[string]map[string]string{"a.go": {"g": "this value is only ever logged"}}, vios); len(got) != 1 || !strings.Contains(got[0], "exempts nothing") { |
| + t.Errorf("an entry that exempts nothing: %q", got) |
| + } |
| + if got := checkSinkAllow(map[string]map[string]string{"a.go": {"f": "this value is only ever logged"}}, vios); len(got) != 0 { |
| + t.Errorf("a good entry: %q", got) |
| + } |
| +} |
| + |
| +// "valid region" is not "invalid region", though the record's text contains it. |
| +func TestMovesWholeSentencesOnly(t *testing.T) { |
| + dir := filepath.Join("testdata", "moves-copy", "substring") |
| + rows, err := parseMovesCopyRecord(filepath.Join(dir, "record.md")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + ems := scanMovesFile(t, filepath.Join(dir, "emit.go")) |
| + un := findUnrecorded(ems, rows) |
| + if len(un) != 1 || un[0].pattern != "valid region" { |
| + t.Errorf("unrecorded = %v, want only \"valid region\"", un) |
| + } |
| + |
| + cases := []struct { |
| + row, pattern string |
| + want bool |
| + }{ |
| + {"invalid region", "invalid region", true}, |
| + {"invalid region", "valid region", false}, |
| + {"invalid region", "invalid", false}, |
| + {"region <to> is gone", "region " + wild + " is gone", true}, |
| + {"a <role> does not move", "a replica does not move", true}, |
| + {"S2: the next spawn of this host is not strict", "the next spawn of this host is not strict", true}, |
| + {"T11: the cut budget admits no probe attempt", "T11: " + wild, true}, |
| + {"S2: the next spawn is not strict", "the next spawn is not strict now", false}, |
| + {"the source did not answer", "S1: the source did not answer", false}, |
| + } |
| + for _, c := range cases { |
| + if got := movesRowMatches(c.row, c.pattern); got != c.want { |
| + t.Errorf("movesRowMatches(%q, %q) = %v, want %v", c.row, c.pattern, got, c.want) |
| + } |
| + } |
| +} |
| + |
| +// Approved placeholder text is matched inside one expression. Fragments in |
| +// unrelated literals are never joined. |
| +func TestMovesApprovedPlaceholderTextIsOneExpression(t *testing.T) { |
| + root := filepath.Join("testdata", "moves-copy", "assembled") |
| + rows, err := parseMovesCopyRecord(filepath.Join(root, "record.md")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + got := checkMovesApplied(rows, root) |
| + if len(got) != 2 { |
| + t.Fatalf("problems = %q, want exactly MV-N1 (unrelated fragments) and MV-N4 (only the start of the sentence)", got) |
| + } |
| + sort.Strings(got) |
| + if !strings.HasPrefix(got[0], "MV-N1:") || !strings.HasPrefix(got[1], "MV-N4:") { |
| + t.Errorf("problems = %q", got) |
| + } |
| +} |
amadan