| index fde592418..ee2b561f9 100644 |
| --- a/approved_moves_copy_scan_test.go |
| +++ b/approved_moves_copy_scan_test.go |
| @@ -31,11 +31,30 @@ import ( |
| // A placeholder (an operand of a concatenation, an argument of a Sprintf) may |
| // be data, such as a host name: the row it is matched against must carry a |
| // <placeholder> at that spot, and the record is where a human vouches for it. |
| -// But a placeholder never carries an error's text. A call to Error(), a value |
| -// whose declared type is error or implements it, a value the gate cannot type |
| -// that is named err or ends in Err or err, a local or helper parameter that |
| -// holds any of these, and the argument of a %w verb are all violations there, |
| -// even when the format matches a row. |
| +// A placeholder must not carry an error's text that the gate can see. These |
| +// are violations there, even when the format matches a row (errorText): |
| +// |
| +// - a call to Error(); |
| +// - a value whose declared type is error or implements it; |
| +// - a value the gate cannot type that is named err or ends in Err or err; |
| +// - a local, or a helper parameter, that holds any of these; |
| +// - the argument of a %w verb; |
| +// - a call to a package helper with a string result, any of whose returns |
| +// carries an error's text, or that the gate cannot read (a return of a |
| +// shape it does not know, a receiver it cannot resolve, a call to |
| +// itself); |
| +// - a field, any of whose writes in the scanned files carries an error's |
| +// text or takes one result of a multi-value expression; |
| +// - any of these hitting the analysis depth limit. |
| +// |
| +// Data the gate cannot derive (a region read from a request, a list joined by |
| +// strings.Join, a host name in a field) may stand for a placeholder. That |
| +// includes a value made from a helper result the gate cannot read (a local |
| +// assigned it, a field written from it, an argument bound to it) when the |
| +// result carries no error's text the gate can see. The real tree's region |
| +// and host placeholders are read that way. The strict reading, where any |
| +// such value is a violation, refused 13 real Phase 1 sinks whose |
| +// placeholders are all data. |
| // |
| // What it is (ruling T15-scope): a lint against accidental drift into |
| // operator copy, not a boundary against code written to evade it. The |
| @@ -47,7 +66,14 @@ import ( |
| // A value of another package's type is an error only by its type's name |
| // (one ending in Error) or, when its type is unknown, by its own name. |
| // An error held in a variable of an unrelated name and an unknown type |
| -// (x := pkg.Do(); Sprintf("%v", x)) is read as data. |
| +// (x := pkg.Do(); Sprintf("%v", x)) is read as data. So is a helper |
| +// result of any type other than string or error (a Stringer formatted by |
| +// %v). |
| +// - A helper reads its parameters by their call sites only where it is |
| +// called as a placeholder. A field written from the writer's own |
| +// parameter (target{Name: n}) is read by n's type, not by what the |
| +// writer's callers pass, so mk(err.Error()) through such a field is not |
| +// seen. |
| // - Receivers are not lexically scoped. A method call whose receiver |
| // identifier is declared more than once in the function is refused; a |
| // receiver declared once is resolved by its one declaration. |
| @@ -341,8 +367,9 @@ type movesUniverse struct { |
| structFields map[string][]string |
| fieldTypeOf map[string]map[string]ast.Expr |
| results map[string]*fieldResult |
| - used map[string]bool // package functions evaluated as sentence helpers |
| - paramSinks map[string]bool // functions whose parameter reaches a sink |
| + fieldTexts map[string]string // fieldText's answer per field and struct |
| + used map[string]bool // package functions evaluated as sentence helpers |
| + paramSinks map[string]bool // functions whose parameter reaches a sink |
| files []string |
| steps int |
| } |
| @@ -357,7 +384,7 @@ func (u *movesUniverse) at(p string) string { |
| func newUniverse(t *testing.T, root string, files []string) *movesUniverse { |
| t.Helper() |
| u := &movesUniverse{t: t, root: root, fieldWrites: map[string][]*fieldWrite{}, fieldTypes: map[string]map[string]bool{}, |
| - structFields: map[string][]string{}, fieldTypeOf: map[string]map[string]ast.Expr{}, results: map[string]*fieldResult{}, used: map[string]bool{}, |
| + structFields: map[string][]string{}, fieldTypeOf: map[string]map[string]ast.Expr{}, results: map[string]*fieldResult{}, fieldTexts: map[string]string{}, used: map[string]bool{}, |
| paramSinks: map[string]bool{}, files: files} |
| parsed := map[string]*ast.File{} |
| for _, f := range files { |
| @@ -632,7 +659,12 @@ func isConcat(e ast.Expr) bool { |
| } |
| |
| func errorTextProblem(e ast.Expr, why string) problem { |
| - return problem{fmt.Sprintf("%s puts an error's text in a sentence (%s)", render(e), why), true} |
| + what := "an error's text" |
| + m, rest := whyClass(why) |
| + if m != "" { |
| + what = "text the gate cannot derive" |
| + } |
| + return problem{fmt.Sprintf("%s puts %s in a sentence (%s)", render(e), what, rest), true} |
| } |
| |
| // errName is the name an error usually goes by: err, or one ending in Err or |
| @@ -674,11 +706,49 @@ func (c *evalCtx) isErrorType(t ast.Expr) bool { |
| return false |
| } |
| |
| -// errorText says why e is an error's text, or "" when the gate has no reason to |
| -// think so. It reads the expression as written and the locals it names. |
| +// The classes of a reason errorText gives, carried as a mark at its front. |
| +// An unmarked reason is an error's text, and it counts wherever the value |
| +// goes. An unprovable one (an analysis limit, a multi-value field write) counts |
| +// wherever the value goes too: the gate does not know what the text is. An |
| +// underivable one (a helper result the gate cannot read: a return of a shape |
| +// it does not know, a receiver it cannot resolve, a call to itself) counts only |
| +// for the value that is that result. A value merely made from it (a local |
| +// assigned it, an argument bound to it, a field written from it) is data, as |
| +// a placeholder may be: the real tree's region and host names are read |
| +// through helpers the gate cannot fully derive. |
| +const ( |
| + underivable = "\x03" |
| + unprovable = "\x04" |
| +) |
| + |
| +func whyClass(why string) (mark, rest string) { |
| + if strings.HasPrefix(why, underivable) || strings.HasPrefix(why, unprovable) { |
| + return why[:1], why[1:] |
| + } |
| + return "", why |
| +} |
| + |
| +// rewrap wraps why in format (why is its last verb), keeping its class. |
| +func rewrap(why, format string, a ...any) string { |
| + m, rest := whyClass(why) |
| + return m + fmt.Sprintf(format, append(a, rest)...) |
| +} |
| + |
| +// dataWhy is why as it passes through data: an underivable result drops out. |
| +func dataWhy(why string) string { |
| + if m, _ := whyClass(why); m == underivable { |
| + return "" |
| + } |
| + return why |
| +} |
| + |
| +// errorText says why e is an error's text, or text the gate cannot derive |
| +// (see the classes above), or "" when the gate has no reason to think either. |
| +// It reads the expression as written, the locals it names, the bodies of the |
| +// package helpers it calls, and the writes of the fields it reads. |
| func (c *evalCtx) errorText(e ast.Expr, depth int) string { |
| if depth > maxEvalDepth { |
| - return "the gate ran out of depth deciding whether it is an error's text" |
| + return fmt.Sprintf("%sanalysis limit reached at %s: the gate ran out of depth reading what text it carries", unprovable, c.posOf(e)) |
| } |
| switch e := e.(type) { |
| case *ast.ParenExpr: |
| @@ -699,14 +769,20 @@ func (c *evalCtx) errorText(e ast.Expr, depth int) string { |
| return "its result is an error" |
| } |
| // A call out of the package (fmt.Sprint, strings.TrimSpace) carries the |
| - // text of its arguments. A package helper is read by eval instead. |
| - if _, p := c.resolveCallees(e); p != nil { |
| + // text of its arguments. A package helper is read through its body. |
| + decls, p := c.resolveCallees(e) |
| + if p != nil && p.hard { |
| + return underivable + p.msg // a receiver the gate cannot resolve |
| + } |
| + if p != nil { |
| for _, a := range e.Args { |
| - if why := c.errorText(a, depth+1); why != "" { |
| - return fmt.Sprintf("it is made from %s, and %s", render(a), why) |
| + if why := dataWhy(c.errorText(a, depth+1)); why != "" { |
| + return rewrap(why, "it is made from %s, and %s", render(a)) |
| } |
| } |
| + return "" |
| } |
| + return c.helperText(e, decls, 0, depth) |
| case *ast.Ident: |
| if why, ok := c.errParams[e.Name]; ok { |
| return why |
| @@ -722,14 +798,20 @@ func (c *evalCtx) errorText(e ast.Expr, depth int) string { |
| for _, a := range c.locals[e.Name] { |
| switch { |
| case a.rhs != nil: |
| - if why := c.errorText(a.rhs, depth+1); why != "" { |
| - return fmt.Sprintf("it is assigned %s at %s, and %s", render(a.rhs), c.posOf(a.rhs), why) |
| + if why := dataWhy(c.errorText(a.rhs, depth+1)); why != "" { |
| + if _, rest := whyClass(why); strings.HasPrefix(rest, "it is assigned ") { |
| + return why // an alias chain: its innermost step says it |
| + } |
| + return rewrap(why, "it is assigned %s at %s, and %s", render(a.rhs), c.posOf(a.rhs)) |
| } |
| case a.call != nil: |
| if decls, p := c.resolveCallees(a.call); p == nil && len(decls) > 0 && decls[0].Type != nil { |
| if rt := resultTypeAt(decls[0], a.pos); rt != nil && c.isErrorType(rt) { |
| return fmt.Sprintf("it is assigned an error result at %s", c.posOf(a.call)) |
| } |
| + if why := dataWhy(c.helperText(a.call, decls, a.pos, depth)); why != "" { |
| + return rewrap(why, "it is assigned %s at %s, and %s", render(a.call), c.posOf(a.call)) |
| + } |
| } |
| } |
| } |
| @@ -745,10 +827,162 @@ func (c *evalCtx) errorText(e ast.Expr, depth int) string { |
| if t == nil && errName(e.Sel.Name) { |
| return "the gate cannot type it, and it is named as an error" |
| } |
| + return c.fieldText(e) |
| + } |
| + return "" |
| +} |
| + |
| +func isStringType(t ast.Expr) bool { |
| + id, ok := t.(*ast.Ident) |
| + return ok && id.Name == "string" |
| +} |
| + |
| +// bindErrParams marks hc's parameters that call binds to an error's text (or |
| +// to text the gate cannot prove). They are refused where the helper speaks |
| +// them, not where it only compares them. |
| +func (c *evalCtx) bindErrParams(hc *evalCtx, d *ast.FuncDecl, call *ast.CallExpr, depth int) { |
| + for i, p := range paramNames(d.Type) { |
| + if i < len(call.Args) { |
| + if why := dataWhy(c.errorText(call.Args[i], depth)); why != "" { |
| + hc.errParams[p] = rewrap(why, "it is bound to %s at %s, and %s", render(call.Args[i]), c.posOf(call.Args[i])) |
| + } |
| + } |
| + } |
| +} |
| + |
| +// helperText reads a call to a package helper whose result at pos is a string, |
| +// used as a placeholder, through its body: each return is read with errorText |
| +// in the helper's own context, its parameters marked where the call binds |
| +// them to an error's text. A return that carries an error's text, or one the |
| +// gate cannot read, makes the call a violation. A return of data (a parameter, |
| +// a region read from a request, a list joined by strings.Join) may stand for |
| +// a <placeholder>, as any placeholder may. A helper with another result type |
| +// (an int for %d) is not text. |
| +func (c *evalCtx) helperText(e *ast.CallExpr, decls []*ast.FuncDecl, pos, depth int) string { |
| + if c.noCalls || len(decls) == 0 { |
| + return "" |
| + } |
| + for _, d := range decls { |
| + if d.Type == nil || d.Body == nil || !isStringType(resultTypeAt(d, pos)) { |
| + return "" |
| + } |
| + } |
| + if depth > maxEvalDepth { |
| + return fmt.Sprintf("%sanalysis limit reached at %s: helpers nested too deeply", unprovable, c.posOf(e)) |
| + } |
| + for _, d := range decls { |
| + key := fmt.Sprintf("errhelper %p %d", d, pos) |
| + if c.visiting[key] { |
| + return fmt.Sprintf("%shelper %s calls itself at %s", underivable, d.Name.Name, c.posOf(e)) |
| + } |
| + dfile := strings.TrimPrefix(movesFset.Position(d.Pos()).Filename, c.u.root+"/") |
| + hc := c.u.newCtx(dfile, d, c.noCalls) |
| + hc.visiting = c.visiting |
| + c.bindErrParams(hc, d, e, depth+1) |
| + c.visiting[key] = true |
| + why := hc.returnsText(d, pos, depth+1) |
| + delete(c.visiting, key) |
| + switch m, _ := whyClass(why); { |
| + case why == "": |
| + case m == "": |
| + return fmt.Sprintf("helper %s returns an error's text: %s", d.Name.Name, why) |
| + default: |
| + return rewrap(why, "helper %s returns text the gate cannot derive: %s", d.Name.Name) |
| + } |
| } |
| return "" |
| } |
| |
| +// returnsText reads each return of d at pos with errorText (c is d's own |
| +// context), as callDecl reads them for a sentence. |
| +func (c *evalCtx) returnsText(d *ast.FuncDecl, pos, depth int) string { |
| + nres := resultCount(d) |
| + named := namedResults(d) |
| + why := "" |
| + ast.Inspect(d.Body, func(n ast.Node) bool { |
| + if why != "" { |
| + return false |
| + } |
| + switch n := n.(type) { |
| + case *ast.FuncLit: |
| + return false |
| + case *ast.ReturnStmt: |
| + var r string |
| + switch { |
| + case len(n.Results) == nres && pos < len(n.Results): |
| + r = c.errorText(n.Results[pos], depth) |
| + case len(n.Results) == 1 && nres > 1: |
| + cl, ok := n.Results[0].(*ast.CallExpr) |
| + if !ok { |
| + r = fmt.Sprintf("%sreturn at %s has a shape the gate cannot read", underivable, c.posOf(n)) |
| + break |
| + } |
| + decls, p := c.resolveCallees(cl) |
| + switch { |
| + case p != nil && p.hard: |
| + r = underivable + p.msg |
| + case p != nil: |
| + r = fmt.Sprintf("%sreturn at %s forwards %s, which the gate cannot read", underivable, c.posOf(n), render(cl)) |
| + default: |
| + r = c.helperText(cl, decls, pos, depth) |
| + } |
| + case len(n.Results) == 0 && pos < len(named): |
| + r = c.errorText(&ast.Ident{Name: named[pos], NamePos: n.Pos()}, depth) |
| + default: |
| + r = fmt.Sprintf("%sreturn at %s has a shape the gate cannot read", underivable, c.posOf(n)) |
| + } |
| + if r != "" { |
| + why = rewrap(r, "at %s: %s", c.posOf(n)) |
| + } |
| + } |
| + return true |
| + }) |
| + return why |
| +} |
| + |
| +// fieldText reads a field used as a placeholder through its writes, as |
| +// fieldRead does for a Reason: a write that carries an error's text, or takes |
| +// one result of a multi-value expression, makes the read one too. A write of |
| +// data (a parameter, a variable, a helper result the gate cannot fully read) |
| +// may stand for a <placeholder>. Writes to a same-named field of another known |
| +// struct are not this field's. |
| +func (c *evalCtx) fieldText(e *ast.SelectorExpr) string { |
| + if c.noCalls { |
| + return "" |
| + } |
| + f := e.Sel.Name |
| + rt := lastName(baseTypeName(c.typeExprOf(e.X, 0))) |
| + key := f + "|" + rt |
| + if why, ok := c.u.fieldTexts[key]; ok { |
| + return why // a field read while its own writes are read is clean here |
| + } |
| + c.u.fieldTexts[key] = "" |
| + why := "" |
| + for _, w := range c.u.fieldWrites[f] { |
| + wc := c.u.newCtx(w.file, w.fd, c.noCalls) |
| + wt := lastName(w.ctype) |
| + if w.recv != nil { |
| + wt = lastName(baseTypeName(wc.typeExprOf(w.recv, 0))) |
| + } |
| + if rt != "" && wt != "" && wt != rt { |
| + continue |
| + } |
| + if w.multi { |
| + why = fmt.Sprintf("%sa write to field %s at %s takes one result of a multi-value expression, which the gate cannot prove is a sentence", unprovable, f, wc.posOf(w.expr)) |
| + break |
| + } |
| + // From depth 0: the answer is cached per field, so it must not depend |
| + // on how deep the first reader was. A field cycle reads as clean (the |
| + // entry above), and the step limit still bounds the work. |
| + if r := dataWhy(wc.errorText(w.expr, 0)); r != "" { |
| + why = rewrap(r, "a write to field %s at %s: %s", f, wc.posOf(w.expr)) |
| + break |
| + } |
| + } |
| + c.u.fieldTexts[key] = why |
| + return why |
| +} |
| + |
| // wrapArgs returns the argument indexes (into the call's Args, the format at |
| // 0) of a format's %w verbs. ok is false when the format's arguments cannot be |
| // counted (an explicit argument index). |
| @@ -829,6 +1063,16 @@ func (c *evalCtx) evalIdent(id *ast.Ident, depth int) ([]string, []problem) { |
| var alts []string |
| var probs []problem |
| known := false |
| + if why, ok := c.errParams[id.Name]; ok { |
| + // Spoken, not merely compared: the helper returns or builds text from a |
| + // parameter bound to an error's text. |
| + what := "an error's text" |
| + m, rest := whyClass(why) |
| + if m != "" { |
| + what = "text the gate cannot derive" |
| + } |
| + return nil, []problem{{fmt.Sprintf("%s is %s (%s)", id.Name, what, rest), true}} |
| + } |
| if b, ok := c.bind[id.Name]; ok { |
| known = true |
| alts = append(alts, b...) |
| @@ -1304,8 +1548,8 @@ func (c *evalCtx) callDecl(d *ast.FuncDecl, call *ast.CallExpr, pos, depth int) |
| probs = append(probs, pr...) |
| // An argument that is an error's text may still only select among |
| // sentences; it is refused where the helper puts it in one. |
| - if why := c.errorText(call.Args[i], 0); why != "" { |
| - hc.errParams[p] = fmt.Sprintf("it is bound to %s at %s, and %s", render(call.Args[i]), c.posOf(call.Args[i]), why) |
| + if why := dataWhy(c.errorText(call.Args[i], 0)); why != "" { |
| + hc.errParams[p] = rewrap(why, "it is bound to %s at %s, and %s", render(call.Args[i]), c.posOf(call.Args[i])) |
| } |
| } else { |
| hc.bind[p] = []string{wild} |
amadan