diff --git a/README.md b/README.md index bc2a91d..4cc4366 100644 --- a/README.md +++ b/README.md @@ -126,12 +126,14 @@ renders nothing by `Fake`, and is compiled only so the tree's fences still run. The columns are the point, and their facts stay together: two columns that reference one category — `{/currency.code}` and `{/currency.symbol}` — share one draw of it, so the record is internally consistent. That one draw is also why two -columns may not read overlapping reference paths — `{/cat.a}` beside `{/cat.a.b}` -is refused, naming the fields to write instead, exactly as -[One draw, one spelling](#one-draw-one-spelling) refuses the pair inside a single -format. A field hold, transform or operand ties fields together within one column -as always (see [Correlated fields](#correlated-fields) and -[Decisions](#decisions)). +columns may not read overlapping reference *paths* — `{/cat.a}` beside +`{/cat.a.b}` is refused, naming the fields to write instead, as +[One draw, one spelling](#one-draw-one-spelling) refuses that pair inside a single +format. A column may not reference the record it belongs to either: `{/users.first}` +inside `users` would contradict the `first` column beside it, so put a value two +columns share in its own category and reference that. A field hold, transform or +operand ties fields together within one column as always (see +[Correlated fields](#correlated-fields) and [Decisions](#decisions)). ## Library diff --git a/fejkdata.go b/fejkdata.go index 4ede2f1..c558d9b 100644 --- a/fejkdata.go +++ b/fejkdata.go @@ -45,6 +45,7 @@ type Generator struct { mu sync.Mutex rand *session categories map[string]node + records map[node]recordShape } // session is one generator's mutable render state: the seeded rng plus the {seq()} diff --git a/hold.go b/hold.go index 0c7d6b5..05099df 100644 --- a/hold.go +++ b/hold.go @@ -274,10 +274,10 @@ func readField(s *session, t *template, held, refScope *draws, a arm) string { } return render(s, t.fields[a.key], refScope) } - // A reference reads the caller's scope, so its draw outlives this expansion; a - // sibling stays local to it. + // A reference that reads a path reads the caller's scope, so its draw outlives + // this expansion; a sibling, and a reference read whole, stay local to it. d := held - if isRef(a.key) && refScope != nil { + if isRef(a.key) && refScope != nil && len(a.tail) > 0 { d = refScope } if v, read := d.value[a.path]; read { diff --git a/perf_test.go b/perf_test.go index 765525a..8536cac 100644 --- a/perf_test.go +++ b/perf_test.go @@ -51,6 +51,25 @@ func TestNoRenderAllocRegression(t *testing.T) { } } +// A record's fences read the compiled tree, so they belong to New, not to a draw. +// A per-draw walk costs allocations in proportion to the tree; this pins that the +// count does not move with the column count. +func TestNoRecordAllocRegression(t *testing.T) { + for _, s := range []struct{ name, json string }{ + {"record 3 columns", `{"format":"","a":"x","b":"y","c":"z"}`}, + {"record 50 columns", wideTokenJSON(50)}, + } { + f, err := New(WithoutShippedData(), WithDataFS(fstest.MapFS{"x.json": {Data: []byte(s.json)}})) + if err != nil { + t.Fatalf("New(%s): %v", s.name, err) + } + const base = 4.0 + if allocs := testing.AllocsPerRun(10000, func() { f.Record("x") }); allocs > base*1.10 { + t.Errorf("%s: %.1f allocs/op regressed past %.1f (baseline %.1f + 10%%); a record fence running per draw is the usual cause", s.name, allocs, base*1.10, base) + } + } +} + func BenchmarkNestedDepth25(b *testing.B) { benchPath(b, tmpData(b, "deep", nestedJSON(25)), "deep") } func BenchmarkNestedDepth100(b *testing.B) { benchPath(b, tmpData(b, "deep", nestedJSON(100)), "deep") } func BenchmarkWideTokens100(b *testing.B) { diff --git a/record.go b/record.go index 583b358..2798901 100644 --- a/record.go +++ b/record.go @@ -17,7 +17,8 @@ type Column struct { // Record is one record rendered from a template: every direct field is a column, // listed in name order. Each column is its own expansion, so a sibling field is -// local to it, while a reference is drawn once for the whole record. +// local to it, while a reference that reads a path is drawn once for the whole +// record. type Record struct { columns []Column } @@ -104,11 +105,35 @@ func (f *Generator) Record(path string) (*Record, error) { if len(tail) > 0 { return nil, fmt.Errorf("fejkdata: %s descends into %q, a field; only a category-level template is a record", path, tail[0]) } - t, columns, err := recordOf(n) - if err != nil { - return nil, fmt.Errorf("fejkdata: %s %w", path, err) + shape := f.recordShapeOf(n) + if shape.err != nil { + return nil, fmt.Errorf("fejkdata: %s %w", path, shape.err) } - return renderRecord(f.rand, t, columns), nil + return renderRecord(f.rand, shape.t, shape.columns), nil +} + +// recordShape is what recordOf settled about a node: the template to project, its +// columns, or why it is not a record. +type recordShape struct { + t *template + columns []string + err error +} + +// recordShapeOf fences a node once and remembers the answer. The fences read the +// compiled tree, which New fixed, so a repeated Record call on one path pays them +// once rather than per draw. Callers hold the generator's lock. +func (f *Generator) recordShapeOf(n node) recordShape { + if shape, done := f.records[n]; done { + return shape + } + t, columns, err := recordOf(n) + shape := recordShape{t: t, columns: columns, err: err} + if f.records == nil { + f.records = map[node]recordShape{} + } + f.records[n] = shape + return shape } // RecordTemplate is an inline record compiled, referenced and validated once, @@ -169,14 +194,14 @@ func recordOf(n node) (*template, []string, error) { return t, columns, nil } -// checkColumnRefs rejects two reference reads that overlap across a record's -// columns: one renders a level the other reads a path into, and the record's one -// draw of that level cannot answer for both. checkNoOverlap settles the pair -// within a single format; the record's shared scope is what carries it across -// columns, so the same pair is settled here. A bare reference holds nothing, so it -// is not collected and keeps drawing on its own. +// checkColumnRefs rejects the reference reads a record's shared draw cannot answer +// for: one column rendering a level another reads a path into, and a column +// reading the record back through its own path. func checkColumnRefs(t *template, columns []string) error { - reads := columnRefs(t, columns) + reads, err := columnRefs(t, columns) + if err != nil { + return err + } sort.Slice(reads, func(i, j int) bool { if reads[i].a.path != reads[j].a.path { return reads[i].a.path < reads[j].a.path @@ -200,21 +225,32 @@ type columnRef struct { a arm } -// columnRefs lists every held reference read anywhere a column renders, following -// the same edges expand does. -func columnRefs(t *template, columns []string) []columnRef { +// columnRefs lists every reference read that reads a path, anywhere a column +// renders, following the same edges expand does. A read that lands back on the +// record itself names a sibling column, which no draw of the record can answer +// for, so it is reported here rather than collected. +func columnRefs(t *template, columns []string) ([]columnRef, error) { var out []columnRef + var err error for _, name := range columns { seen := map[node]bool{} var walk func(n node) walk = func(n node) { - if n == nil || seen[n] { + if n == nil || seen[n] || err != nil { return } seen[n] = true if tm, ok := n.(*template); ok { for _, r := range boundReaders(tm.format, tm.bound, tm.refs) { - if a := splitArm(r.name, tm.refs); isRef(a.key) && len(a.tail) > 0 { + a := splitArm(r.name, tm.refs) + if !isRef(a.key) { + continue + } + if tm.fields[a.key] == node(t) { + err = fmt.Errorf("column %q reads {%s}, which points back at this record; a column cannot read another column — move the shared value into its own category and reference that", name, a.name) + return + } + if len(a.tail) > 0 { out = append(out, columnRef{name, a}) } } @@ -225,12 +261,11 @@ func columnRefs(t *template, columns []string) []columnRef { } walk(t.fields[name]) } - return out + return out, err } -// renderRecord draws each column once, in the name order recordOf fixed. The -// columns share one reference scope, so two columns that reference one category -// read one draw of it. +// renderRecord draws each column once, in the name order recordOf fixed, over one +// reference scope shared across them. func renderRecord(s *session, t *template, columns []string) *Record { scope := &draws{variant: map[string]node{}, value: map[string]string{}} r := &Record{columns: make([]Column, len(columns))} diff --git a/record_test.go b/record_test.go index ae0e6a9..0de914e 100644 --- a/record_test.go +++ b/record_test.go @@ -152,9 +152,9 @@ func TestRecordRejectsOverlappingReferenceColumns(t *testing.T) { cat := `{"format":"","a":[{"format":"A={b}","b":"1"},{"format":"A={b}","b":"2"}]}` for _, c := range []struct{ name, row string }{ {"sibling columns", `{"format":"","whole":"{/cat.a}","inner":"{/cat.a.b}"}`}, - {"through a nested template", `{"format":"","whole":"{/cat.a}","inner":{"format":"{/cat.a.b}"}}`}, + {"through a nested template", `{"format":"","whole":"{/cat.a}","inner":{"format":"{/cat.a.b} {x}","x":"1"}}`}, {"through a column repeat", `{"format":"","whole":"{/cat.a}","inner":{"format":"{/cat.a.b}","repeat":2,"separator":"-"}}`}, - {"through a choice variant", `{"format":"","whole":"{/cat.a}","inner":[{"format":"{/cat.a.b}"},{"format":"{/cat.a.b}!"}]}`}, + {"through a choice variant", `{"format":"","whole":"{/cat.a}","inner":[{"format":"{/cat.a.b} {x}","x":"1"},{"format":"{/cat.a.b}! {x}","x":"2"}]}`}, {"as a builtin operand", `{"format":"","whole":"{uppercase(/cat.a)}","inner":"{/cat.a.b}"}`}, } { f := newGenerator(t, writeData(t, map[string]string{"cat": cat, "row": c.row}), WithSeed(1)) @@ -172,6 +172,17 @@ func TestRecordRejectsOverlappingReferenceColumns(t *testing.T) { } } +func TestInlineRecordRejectsOverlappingColumns(t *testing.T) { + dir := writeData(t, map[string]string{ + "cat": `{"format":"","a":[{"format":"A={b}","b":"1"},{"format":"A={b}","b":"2"}]}`, + }) + f := newGenerator(t, dir, WithSeed(1)) + _, err := f.FakeRecord(`{"format":"","whole":"{/cat.a}","inner":"{/cat.a.b}"}`) + if err == nil || !strings.Contains(err.Error(), "reads a path into") { + t.Fatalf("inline record over an overlapping pair = %v, want the inline entry point to refuse it too", err) + } +} + func TestRecordRejectsAColumnReadingItsOwnRecord(t *testing.T) { dir := writeData(t, map[string]string{ "person": `{"format":"{first} {last}","first":["Ada","Bo"],"last":["Lovelace","Ek"],"full":"{/person.first} {/person.last}"}`, @@ -361,7 +372,6 @@ func TestInlineRecordErrors(t *testing.T) { {`"hello"`, "has no fields, so no columns"}, {`["a","b"]`, "a record is a template whose fields are its columns"}, {`{"format":"{a}-","repeat":3,"separator":"|","a":["x","y"]}`, "carries repeat 3"}, - {`{"format":"","whole":"{/cur.code}","inner":"{/cur.code.x}"}`, "reads a path into"}, } { if _, err := f.FakeRecord(c.input); err == nil || !strings.Contains(err.Error(), c.want) { t.Errorf("FakeRecord(%q) = %v, want an error naming %q", c.input, err, c.want)