diff --git a/.claude/skills/morphic-measuring-performance/references/cost-tests.md b/.claude/skills/morphic-measuring-performance/references/cost-tests.md index 0b82b6d4a..f674c04a0 100644 --- a/.claude/skills/morphic-measuring-performance/references/cost-tests.md +++ b/.claude/skills/morphic-measuring-performance/references/cost-tests.md @@ -81,14 +81,16 @@ gives. A clock does none of that reliably; a counter does. ## Measure a render through a real walk, not a zero path `allocs-miss-a-free-render` -- **Rule.** `testing.AllocsPerRun` cannot see a render the compiler turns into a no-copy map lookup, - nor one of the zero `ir.WalkPath`, which renders "" without allocating. Measure each visitor in a - call of its own to `irtest.AllocatingVisits`, which visits inside a real walk with the walk's own - path, so no other visitor's pruning hides a visit, and assert the exact set of allocating paths. - Check `go build -gcflags=-m` before trusting a count to see a render at all. -- **Why.** An eager-render mutant survived a test indexing a map with the rendered path (#1125), - and four survived visit tests run on the zero path (#1133). -- **Probe.** Plant an eager render in the visitor; the test reddens. +- **Rule.** `testing.AllocsPerRun` sees neither a render the compiler turns into a no-copy map + lookup nor one of the zero `ir.WalkPath`, which renders "" without allocating. Measure visitors + through `irtest.AllocatingVisits`, which walks each as it would walk alone with the walk's own + path, and assert the exact allocating paths. It measures only a visit's repeat, so hold a first + meeting with a test that doubles the document. Check `go build -gcflags=-m` before trusting a + count. +- **Why.** Eager-render mutants survived a test indexing a map with the rendered path (#1125) and + visit tests on the zero path (#1133); a path kept on each first meeting survived every visit test + (#1147). +- **Probe.** Plant an eager render, and a path kept on a first meeting; each test reddens. ## Remove a process-wide confound from the measured window `process-counter-window` diff --git a/ir/bigval.go b/ir/bigval.go index 2ad4b73e6..1a775e470 100644 --- a/ir/bigval.go +++ b/ir/bigval.go @@ -22,15 +22,29 @@ type BigVal string // value whose leading digit carries a power of ten past [MaxDecimalExp] with // [ErrExponentRange]. It never rounds or reformats the significant digits: a // high-precision decimal and an exponential literal are preserved verbatim, -// and only JSON-invalid affixes are normalized. +// and only JSON-invalid affixes are normalized. A literal in JSON form already +// is returned as it is, allocating nothing. func NewBigVal(s string) (BigVal, error) { l, _, err := readDecimal(s) if err != nil { return "", fmt.Errorf("bigval: %q: %w", s, err) } + if l.spelledForJSON(s) { + return BigVal(s), nil + } return BigVal(jsonSpelling(l)), nil } +// spelledForJSON reports whether s, the text l was scanned from, is what +// jsonSpelling makes of l already: no leading "+", an integer part that is "0" +// or opens with another digit, and a point only where digits follow it. Those +// are the only affixes jsonSpelling rewrites, and it keeps every other part as +// written. +func (l decimalLiteral) spelledForJSON(s string) bool { + whole := l.intPart == "0" || (l.intPart != "" && l.intPart[0] != '0') + return whole && s[0] != '+' && l.point == (l.frac != "") +} + // jsonSpelling rewrites the JSON-invalid affixes of a scanned literal into JSON // form without altering its value: it drops a leading "+", gives the integer // part its one leading digit (".5" → "0.5", "09" → "9"), and drops a trailing diff --git a/ir/bigval_test.go b/ir/bigval_test.go index 18db3501f..ec0fab1f9 100644 --- a/ir/bigval_test.go +++ b/ir/bigval_test.go @@ -139,6 +139,72 @@ func TestNewBigVal_CanonicalizesToJSONForm(t *testing.T) { } } +// TestNewBigVal_ReturnsAJSONSpelledLiteralAsItIs holds the shortcut that keeps +// a literal already in JSON form to exactly the literals jsonSpelling leaves +// unchanged, over every string of up to six runes drawn from the runes the +// grammar treats differently: a sign, a zero, another digit, a point and an +// exponent marker. A literal it takes wrongly would be stored un-canonicalized, +// and one it misses only costs a string. +func TestNewBigVal_ReturnsAJSONSpelledLiteralAsItIs(t *testing.T) { + t.Parallel() + const runes, longest = "+-01.e", 6 + size, width := 1, 1 + for range longest { + width *= len(runes) + size += width + } + literals, shorter := make([]string, 1, size), []string{""} + for range longest { + longer := make([]string, 0, len(runes)*len(shorter)) + for _, s := range shorter { + for _, r := range runes { + longer = append(longer, s+string(r)) + } + } + literals, shorter = append(literals, longer...), longer + } + var read, asIs int + for _, s := range literals { + spelling, spelled, ok := ir.JSONSpelling(s) + if !ok { + continue + } + read++ + if spelled { + asIs++ + } + require.Equal(t, spelling == s, spelled, "%q spells as %q", s, spelling) + v, err := ir.NewBigVal(s) + require.NoError(t, err, s) + require.Equal(t, spelling, v.String(), s) + } + assert.Positive(t, asIs, "some literal is in JSON form already") + assert.Less(t, asIs, read, "some literal is rewritten") +} + +// TestNewBigVal_AllocatesOnlyToRewrite pins NewBigVal's cost: nothing for a +// literal in JSON form, whatever its parts, and the one string a rewrite +// builds otherwise. Reading the range joins no digits, so a literal with digits +// either side of its point costs no more, even past the length a join could be +// built on the stack. It is not run in parallel: AllocsPerRun measures the +// process. +func TestNewBigVal_AllocatesOnlyToRewrite(t *testing.T) { + for _, tc := range []struct { + in string + allocs float64 + }{ + {"0", 0}, {"-0", 0}, {"42", 0}, {"3.14", 0}, {"-0.5", 0}, {"0.0", 0}, {"1.5e+3", 0}, + {"2.5E-3", 0}, {"12345678901234567890.12345678901234567890", 0}, + {"+3.14", 1}, {".5", 1}, {"5.", 1}, {"007.5", 1}, {"-00.5e3", 1}, + } { + var err error + allocs := testing.AllocsPerRun(10, func() { _, err = ir.NewBigVal(tc.in) }) + + require.NoError(t, err, tc.in) + assert.Equal(t, tc.allocs, allocs, tc.in) + } +} + func TestBigVal_JSONRoundTrip(t *testing.T) { t.Parallel() v, err := ir.NewBigVal("123456789012345678901234567890.5") diff --git a/ir/decimal.go b/ir/decimal.go index 80d69a2ce..571d0a01d 100644 --- a/ir/decimal.go +++ b/ir/decimal.go @@ -74,11 +74,14 @@ type Decimal struct { } // decimalLiteral is a numeric literal split as written: its sign, the digit -// runs either side of the point, and the exponent with its e or E and sign, or -// "" when it has none. It keeps what [BigVal] spells and [Decimal] normalizes. +// runs either side of the point, whether a point was written at all, which a +// point with no digits after it leaves no other trace of, and the exponent with +// its e or E and sign, or "" when it has none. It keeps what [BigVal] spells and +// [Decimal] normalizes. type decimalLiteral struct { neg bool intPart, frac string + point bool exponent string } @@ -88,26 +91,27 @@ type decimalLiteral struct { // Zero is accepted at any written exponent. The result is the zero Decimal on // error, which reads as the number zero, so a caller must check the error. func ParseDecimal(s string) (Decimal, error) { - _, d, err := readDecimal(s) + l, exp, err := readDecimal(s) if err != nil { return Decimal{}, fmt.Errorf("decimal: %q: %w", s, err) } - return d, nil + return l.decimal(exp), nil } -// readDecimal splits and reads s, so [NewBigVal] spells the literal from the -// split that [ParseDecimal] reads the value from. Its error is bare, for each -// of them to name itself in. -func readDecimal(s string) (decimalLiteral, Decimal, error) { +// readDecimal splits s and reads the power of ten its leading digit carries, so +// [NewBigVal] spells the literal from the split that [ParseDecimal] reads the +// value from. The split is substrings of s, so reading it allocates nothing. +// Its error is bare, for each of them to name itself in. +func readDecimal(s string) (decimalLiteral, int64, error) { l, ok := scanDecimal(s) if !ok { - return decimalLiteral{}, Decimal{}, ErrNotDecimal + return decimalLiteral{}, 0, ErrNotDecimal } - d, err := l.decimal() + exp, err := l.leadExponent() if err != nil { - return decimalLiteral{}, Decimal{}, err + return decimalLiteral{}, 0, err } - return l, d, nil + return l, exp, nil } // scanDecimal splits s by the grammar sign? (digits ("." digits*)? | "." digits) @@ -125,7 +129,7 @@ func scanDecimal(s string) (decimalLiteral, bool) { l.intPart, i = s[i:end], end if i < len(s) && s[i] == '.' { end = scanDigits(s, i+1) - l.frac, i = s[i+1:end], end + l.frac, l.point, i = s[i+1:end], true, end } if l.intPart == "" && l.frac == "" { return decimalLiteral{}, false // no digit anywhere: "", "+", ".", "Inf" @@ -188,24 +192,46 @@ func exponentValue(exponent string) int64 { return e } -// decimal reads l as a [Decimal]. Zero is accepted at any written exponent; any -// other value must carry its leading digit's power of ten within ±[MaxDecimalExp]. +// leadExponent returns the power of ten l's leading significant digit carries, +// or 0 for zero, which is accepted at any written exponent; any other value +// must carry it within ±[MaxDecimalExp]. It counts the digits where they lie +// rather than joining the two runs, so it allocates nothing. // // The sum below stays inside int64: exponentValue is at most 4·10¹⁸ in // magnitude and the digit term at most len(s), which is below Go's largest // allocation. Saturating an exponent never changes a verdict, since a saturated // exponent plus any string's digit count is still past the bound. -func (l decimalLiteral) decimal() (Decimal, error) { - digits := l.intPart + l.frac - lead := strings.TrimLeft(digits, "0") - if lead == "" { - return Decimal{}, nil // zero, at any written exponent +func (l decimalLiteral) leadExponent() (int64, error) { + zeros := l.leadingZeros() + if zeros == len(l.intPart)+len(l.frac) { + return 0, nil // zero, at any written exponent } - exp := exponentValue(l.exponent) + int64(len(lead)-len(l.frac)-1) + exp := exponentValue(l.exponent) + int64(len(l.intPart)-zeros-1) if exp > MaxDecimalExp || exp < -MaxDecimalExp { - return Decimal{}, &ExponentRangeError{neg: l.neg, small: exp < 0} + return 0, &ExponentRangeError{neg: l.neg, small: exp < 0} + } + return exp, nil +} + +// leadingZeros returns how many zeros open l's digits: the integer part's, and +// when those are all zeros, the fraction's after them. +func (l decimalLiteral) leadingZeros() int { + zeros := len(l.intPart) - len(strings.TrimLeft(l.intPart, "0")) + if zeros == len(l.intPart) { + zeros += len(l.frac) - len(strings.TrimLeft(l.frac, "0")) + } + return zeros +} + +// decimal reads l, whose leading digit carries the power of ten exp, as a +// [Decimal]. Its digits are the two runs joined, which costs an allocation when +// both hold one, so only [ParseDecimal] asks for them. +func (l decimalLiteral) decimal(exp int64) Decimal { + lead := strings.TrimLeft(l.intPart+l.frac, "0") + if lead == "" { + return Decimal{} // zero, at any written exponent } - return Decimal{neg: l.neg, digits: strings.TrimRight(lead, "0"), exp: exp}, nil + return Decimal{neg: l.neg, digits: strings.TrimRight(lead, "0"), exp: exp} } // Sign returns -1, 0 or +1 as d is negative, zero or positive. Having no digits diff --git a/ir/declarations.go b/ir/declarations.go index 5bad04537..d6e56c80f 100644 --- a/ir/declarations.go +++ b/ir/declarations.go @@ -1,6 +1,7 @@ package ir import ( + "iter" "reflect" ) @@ -9,37 +10,37 @@ import ( // the Go compiler cannot check. const idFieldName = "ID" -// IDDeclaration is one node's declaration of its own identity: the class of ID, -// the ID itself, and the path of the node that declares it. +// IDDeclaration is one node's declaration of its own identity: the class of ID +// and the ID itself. Where the node sits is [IDDeclarations.Paths]'s to spell. type IDDeclaration struct { // Class is the Go type of the ID, which names the reference class the // declaration settles. Class reflect.Type // ID is the declared identity. ID string - // Path locates the declaring node, as [WalkValues] spells it. - Path string } // IDDeclarations collects, as a [Visitor], the identities the values its walk -// reaches declare, in walk order and each with its path, for a check reporting -// where one is declared, such as twice. Reading them off the value graph covers -// a new ID-bearing node the moment it exists. [IDReferences] reads those it -// resolves references against, rendering no path. +// reaches declare, in walk order, for a check reporting where one is declared, +// such as twice, each with its path kept unspelled for the check to spell +// only those it reports. Reading them off the value graph covers a new +// ID-bearing node the moment it exists. // // An empty ID is skipped, since nothing can reference one and several nodes // carrying one are not duplicates. Reporting it is irverify.checkRegistryKeys's // job for a map-keyed class and irverify.checkDeclaredIDs's for the rest. The // zero IDDeclarations is ready to walk with. type IDDeclarations struct { - ids IDReader - list []IDDeclaration + ids IDReader + list []IDDeclaration + paths PathLog } // Visit records the identity v declares, if any, and asks for v's children. func (d *IDDeclarations) Visit(v reflect.Value, at WalkPath) bool { if class, id, declares := d.ids.DeclaredIDOf(v); declares && id != "" { - d.list = append(d.list, IDDeclaration{Class: class, ID: id, Path: at.String()}) + d.list = appendGrowing(d.list, IDDeclaration{Class: class, ID: id}) + d.paths.Record(at) } return true } @@ -50,6 +51,13 @@ func (d *IDDeclarations) List() []IDDeclaration { return d.list } +// Paths yields the path of each declaration List holds, with its position +// there, in walk order, as a WalkPath valid until the next is yielded: a check +// keeps the String of those it reports and spells no other. +func (d *IDDeclarations) Paths() iter.Seq2[int, WalkPath] { + return d.paths.All() +} + // IDReader reads the identity a value declares for itself, reading where a // struct type declares one only the first time it meets the type: a visitor // shown every value of a walk meets a few dozen struct types many times over, diff --git a/ir/declarations_test.go b/ir/declarations_test.go index 48b9f2ee8..5cd86abd6 100644 --- a/ir/declarations_test.go +++ b/ir/declarations_test.go @@ -1,7 +1,9 @@ package ir_test import ( + "math/bits" "reflect" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -32,8 +34,8 @@ func declaringDoc() *ir.Document { // declaredBy indexes declarations by ID class name, so a case can assert on one // class without depending on walk order. -func declaredBy(decls []ir.IDDeclaration) map[string][]ir.IDDeclaration { - out := map[string][]ir.IDDeclaration{} +func declaredBy(decls []declaredAt) map[string][]declaredAt { + out := map[string][]declaredAt{} for _, d := range decls { out[d.Class.Name()] = append(out[d.Class.Name()], d) } @@ -53,15 +55,15 @@ func TestIDDeclarations_ReachesEveryIDBearingNodeOnce(t *testing.T) { require.False(t, truncated, "a shallow document must not exhaust the walk") byClass := declaredBy(decls) - want := map[string]ir.IDDeclaration{ - "TypeID": {ID: "t/x/M", Path: "doc.Types[t/x/M]"}, - "PropID": {ID: "p/x/M/f", Path: "doc.Types[t/x/M].Properties[0]"}, - "ChannelID": {ID: "c/x", Path: "doc.Channels[c/x]"}, - "MessageID": {ID: "m/x", Path: "doc.Messages[m/x]"}, - "AuthID": {ID: "auth/x", Path: "doc.Auth[auth/x]"}, - "ServiceID": {ID: "s/x", Path: "doc.Services[0]"}, - "GroupID": {ID: "g/x", Path: "doc.Services[0].Groups[0]"}, - "OpID": {ID: "op/x", Path: "doc.Services[0].Groups[0].Operations[0]"}, + want := map[string]declaredAt{ + "TypeID": {IDDeclaration: ir.IDDeclaration{ID: "t/x/M"}, Path: "doc.Types[t/x/M]"}, + "PropID": {IDDeclaration: ir.IDDeclaration{ID: "p/x/M/f"}, Path: "doc.Types[t/x/M].Properties[0]"}, + "ChannelID": {IDDeclaration: ir.IDDeclaration{ID: "c/x"}, Path: "doc.Channels[c/x]"}, + "MessageID": {IDDeclaration: ir.IDDeclaration{ID: "m/x"}, Path: "doc.Messages[m/x]"}, + "AuthID": {IDDeclaration: ir.IDDeclaration{ID: "auth/x"}, Path: "doc.Auth[auth/x]"}, + "ServiceID": {IDDeclaration: ir.IDDeclaration{ID: "s/x"}, Path: "doc.Services[0]"}, + "GroupID": {IDDeclaration: ir.IDDeclaration{ID: "g/x"}, Path: "doc.Services[0].Groups[0]"}, + "OpID": {IDDeclaration: ir.IDDeclaration{ID: "op/x"}, Path: "doc.Services[0].Groups[0].Operations[0]"}, } for class, w := range want { got := byClass[class] @@ -252,19 +254,46 @@ func TestDeclaredIDField_CostsNoAllocation(t *testing.T) { } } -// TestIDDeclarations_AllocatesOnlyForADeclaration holds a walk's per-value cost -// for its declarations to the record a declaration adds: a value declaring -// nothing, of a type met before, allocates nothing. Each is handed the walk's -// own path, so rendering one for a value declaring nothing would count. It is -// not run in parallel: AllocsPerRun measures the process. -func TestIDDeclarations_AllocatesOnlyForADeclaration(t *testing.T) { +// TestIDDeclarations_AllocatesNothingPerDeclaration holds a walk's per-value +// cost for its declarations at nothing, a declaration included: each keeps its +// path unspelled, where spelling it costs a string per declaration. Each visit +// is handed the walk's own path, so spelling one would count. It is not run in +// parallel: AllocsPerRun measures the process. +func TestIDDeclarations_AllocatesNothingPerDeclaration(t *testing.T) { doc := declaringDoc() decls, _ := declaredIDs(doc) - want := make([]string, 0, len(decls)) - for _, d := range decls { - want = append(want, d.Path) - } + require.NotEmpty(t, decls, "some value declares, or no declaration is measured") var visitor ir.IDDeclarations - assert.Equal(t, want, irtest.AllocatingVisits(doc, &visitor)) + assert.Empty(t, irtest.AllocatingVisits(doc, &visitor)) +} + +// TestIDDeclarations_GrowsTheListGeometrically holds the list of declarations +// to a growth factor well past append's, as TestPathLog_GrowsGeometrically holds +// the log its paths are kept in: append grows a large slice by a quarter at a +// time, so a list built one declaration at a time is copied several times +// over. Beside a log recording the same paths, the list may grow fewer times +// than doubling would take, log2(n). It is not run in parallel: AllocsPerRun +// measures the process. +func TestIDDeclarations_GrowsTheListGeometrically(t *testing.T) { + const declarations = 1 << 13 + enum := &ir.Enum{ID: "t/e"} + for i := range declarations { + enum.Members = append(enum.Members, ir.EnumMember{ID: ir.EnumMemberID("e/e/" + strconv.Itoa(i))}) + } + doc := &ir.Document{Types: ir.TypeRegistry{enum.ID: enum}} + declaring := testing.AllocsPerRun(3, func() { + ir.WalkValues(doc, ir.DocumentPath, &ir.IDDeclarations{}) + }) + recording := testing.AllocsPerRun(3, func() { + var ids ir.IDReader + var log ir.PathLog + ir.WalkValues(doc, ir.DocumentPath, ir.VisitFunc(func(v reflect.Value, at ir.WalkPath) bool { + if _, id, declares := ids.DeclaredIDOf(v); declares && id != "" { + log.Record(at) + } + return true + })) + }) + assert.Less(t, declaring-recording, float64(bits.Len(declarations)-1), "%v allocations to list", declaring-recording) } diff --git a/ir/export_test.go b/ir/export_test.go index b33d27f8b..0631f9fa3 100644 --- a/ir/export_test.go +++ b/ir/export_test.go @@ -1,13 +1,23 @@ package ir -import "reflect" +import ( + "reflect" + "slices" +) // DomainParents exposes domainParents to ir's external tests. func (k PrimKind) DomainParents() []PrimKind { return k.domainParents() } // Kept returns the references r keeps for Unresolved to settle, in walk order, -// so ir's external tests can hold the walk to settling what it can as it goes. -func (r *IDReferences) Kept() []IDReference { return r.sites } +// each with its path spelled, so ir's external tests can hold the walk to +// settling what it can as it goes. +func (r *IDReferences) Kept() []IDReference { + kept := slices.Clone(r.sites) + for i, at := range r.paths.All() { + kept[i].Path = at.String() + } + return kept +} // CountingIDReader returns an IDReader that counts in reads each time it reads // where a struct type declares its identity. @@ -18,6 +28,16 @@ func CountingIDReader(reads *int) *IDReader { }} } +// JSONSpelling returns what jsonSpelling makes of s and whether spelledForJSON +// reads s as that already, or false for ok when s is no decimal literal. +func JSONSpelling(s string) (spelling string, spelled, ok bool) { + l, ok := scanDecimal(s) + if !ok { + return "", false, false + } + return jsonSpelling(l), l.spelledForJSON(s), true +} + // IDClasses is idClasses, for the external tests. var IDClasses = idClasses @@ -36,3 +56,7 @@ func FlattenSteps(types TypeRegistry, root TypeID) int { f.flatten(root) return f.steps } + +// StoredSteps returns how many steps l stores for the paths it holds, so ir's +// external tests can hold it to keeping each step a run of paths shares once. +func (l *PathLog) StoredSteps() int { return len(l.tails) } diff --git a/ir/helpers_test.go b/ir/helpers_test.go index 367e9beb3..326cecca0 100644 --- a/ir/helpers_test.go +++ b/ir/helpers_test.go @@ -485,12 +485,23 @@ func populatedProperty() ir.Property { } } +// declaredAt is one declaration an ir.IDDeclarations collected, with the path +// it spells for it. +type declaredAt struct { + ir.IDDeclaration + Path string +} + // declaredIDs walks doc with an ir.IDDeclarations and returns what it -// collected, with whether the walk was cut short. -func declaredIDs(doc *ir.Document) ([]ir.IDDeclaration, bool) { +// collected, each with its path, and whether the walk was cut short. +func declaredIDs(doc *ir.Document) ([]declaredAt, bool) { var found ir.IDDeclarations truncated := ir.WalkValues(doc, ir.DocumentPath, &found) - return found.List(), truncated + out := make([]declaredAt, len(found.List())) + for i, at := range found.Paths() { + out[i] = declaredAt{IDDeclaration: found.List()[i], Path: at.String()} + } + return out, truncated } // flatLines renders each member of f on one line: the declaration it takes as diff --git a/ir/irtest/export_test.go b/ir/irtest/export_test.go index 02e940a23..e0872067d 100644 --- a/ir/irtest/export_test.go +++ b/ir/irtest/export_test.go @@ -14,3 +14,6 @@ var RootFrom = rootFrom // UpdateFlag is update, for the external tests. var UpdateFlag = update + +// VisitMeasurements is visitMeasurements, for the external tests. +const VisitMeasurements = visitMeasurements diff --git a/ir/irtest/visits.go b/ir/irtest/visits.go index e2614232e..2bdbe00cb 100644 --- a/ir/irtest/visits.go +++ b/ir/irtest/visits.go @@ -8,51 +8,73 @@ import ( "github.com/dexpace/morphic/ir" ) -// AllocatingVisits walks doc with visitors and returns, in walk order, the path -// of each value whose visit allocates however often it is repeated, handed the -// walk's own path, which the zero ir.WalkPath cannot stand in for: it renders +// AllocatingVisits walks doc with visitors and returns the path of each visit +// that must allocate, once per visitor allocating there, in walk order, a walk +// per 64 visitors as ir.WalkValues makes. Each visitor is shown the walk it +// would be shown alone, with the walk's own path: the zero ir.WalkPath renders // empty without allocating. // -// Each value is visited once before it is measured, so what a first meeting -// builds, such as a memo entry, is not counted, and then measured one visit at -// a time, the fewest counting (fewestAllocs). What one visitor prunes is pruned -// for all. AllocsPerRun measures the process, so a caller is not run in -// parallel. +// Only a repeat of a visit is measured (fewestAllocs), so what a visitor builds +// on first meeting a value goes uncounted, whatever it keeps per value, which +// a test growing the document finds. AllocsPerRun measures the process, so a +// caller is not run in parallel. func AllocatingVisits(doc *ir.Document, visitors ...ir.Visitor) []string { var paths []string - ir.WalkValues(doc, ir.DocumentPath, ir.VisitFunc(func(v reflect.Value, at ir.WalkPath) bool { - visit := func() bool { - children := true - for _, visitor := range visitors { - children = visitor.Visit(v, at) && children - } - return children + measured := make([]ir.Visitor, len(visitors)) + for i, visitor := range visitors { + if visitor != nil { // a nil one stays nil, which the walk skips + measured[i] = measuredVisitor{visitor: visitor, paths: &paths} } - children := visit() - if fewestAllocs(visit) > 0 { - paths = append(paths, at.String()) - } - return children - })) + } + ir.WalkValues(doc, ir.DocumentPath, measured...) return paths } +// measuredVisitor shows visitor what the walk shows it and appends the path of +// each visit that allocates to paths. +type measuredVisitor struct { + visitor ir.Visitor + paths *[]string +} + +// Visit shows v to the visitor, then measures the visit, and asks for v's +// children as the visitor does. +func (m measuredVisitor) Visit(v reflect.Value, at ir.WalkPath) bool { + children := m.visitor.Visit(v, at) + if fewestAllocs(func() { m.visitor.Visit(v, at) }) > 0 { + *m.paths = append(*m.paths, at.String()) + } + return children +} + +// Cycle tells the visitor of a cycle if it asks to be told, so a visitor +// gathering cycles gathers what it would in a walk of its own. +func (m measuredVisitor) Cycle(at, entered ir.WalkPath, ref reflect.Type) { + if cycles, asks := m.visitor.(ir.CycleVisitor); asks { + cycles.Cycle(at, entered, ref) + } +} + // visitMeasurements is how many times AllocatingVisits measures one visit. A // race build's sync.Pool drops one in four of the items it is handed, so a // visit handing k of them back allocates in a measurement with a chance of -// about 1-(3/4)^k, and in every one of this many with a negligible chance for -// the handful of pools a visit reaches. +// about 1-(3/4)^k, and in every one of this many with a negligible chance +// while k is a handful. A visit handing back dozens allocates in nearly every +// one, so code measured here reads its values through nothing pooled per value. const visitMeasurements = 20 // fewestAllocs returns the fewest allocations one call of visit makes in -// visitMeasurements measurements. The fewest, because under the race detector +// visitMeasurements measurements, stopping at the first that makes none, as no +// later one can make fewer. The fewest, because under the race detector // sync.Pool drops a share of what it is handed, so a visit reaching a pool, as // jsontext.Value.IsValid does, allocates a random amount more than it must, // and never less: an average counts the drops. -func fewestAllocs(visit func() bool) float64 { +func fewestAllocs(visit func()) float64 { fewest := math.Inf(1) for range visitMeasurements { - fewest = min(fewest, testing.AllocsPerRun(1, func() { visit() })) + if fewest = min(fewest, testing.AllocsPerRun(1, visit)); fewest == 0 { + break + } } return fewest } diff --git a/ir/irtest/visits_test.go b/ir/irtest/visits_test.go index 709dadafd..0404df5d0 100644 --- a/ir/irtest/visits_test.go +++ b/ir/irtest/visits_test.go @@ -38,28 +38,107 @@ func TestAllocatingVisits_FindsEachVisitRenderingItsPath(t *testing.T) { })), "a visitor doing nothing allocates nothing") } -// TestAllocatingVisits_PrunesForAllWhatOnePrunes holds the walk to never -// showing a visitor a value it pruned: what one visitor prunes, the others are -// not shown either. It is not run in parallel: AllocsPerRun measures the -// process. -func TestAllocatingVisits_PrunesForAllWhatOnePrunes(t *testing.T) { +// TestAllocatingVisits_PrunesForEachVisitorAlone holds the walk to the one each +// visitor would be shown alone: never what it pruned, and still what another +// pruned, so a visitor pruning at the root, as one visiting nothing does, hides +// nothing from the others. A walk pruning for all what one prunes measured no +// value below the root, and reported a document clean. It is not run in +// parallel: AllocsPerRun measures the process. +func TestAllocatingVisits_PrunesForEachVisitorAlone(t *testing.T) { doc := &ir.Document{Services: []ir.Service{{ID: "s/x"}}} + var pruned, recorded []string pruneServices := ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + pruned = append(pruned, at.String()) return at.String() != "doc.Services" }) - var seen []string record := ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { - seen = append(seen, at.String()) + recorded = append(recorded, at.String()) return true }) - irtest.AllocatingVisits(doc, pruneServices, record) - assert.Contains(t, seen, "doc.Services") - for _, at := range seen { - assert.False(t, strings.HasPrefix(at, "doc.Services["), "%s lies under a pruned value", at) + irtest.AllocatingVisits(doc, ir.VisitFunc(nil), pruneServices, nil, record) + assert.Contains(t, pruned, "doc.Services") + for _, at := range pruned { + assert.False(t, strings.HasPrefix(at, "doc.Services["), "%s lies under a value this visitor pruned", at) } + assert.Contains(t, recorded, "doc.Services[0].ID", "what other visitors prune is shown to this one") } +// TestAllocatingVisits_ListsAValueForEachVisitorAllocating holds the report to +// one path per allocating visit: a value two visitors allocate for is listed +// twice, and one only a visitor that does not prune it allocates for, once. +// It is not run in parallel: AllocsPerRun measures the process. +func TestAllocatingVisits_ListsAValueForEachVisitorAllocating(t *testing.T) { + doc := &ir.Document{Services: []ir.Service{{ID: "s/x"}}} + var kept []string + rendering := func(prune bool) ir.Visitor { + return ir.VisitFunc(func(v reflect.Value, at ir.WalkPath) bool { + if v.Type() == reflect.TypeFor[ir.ServiceID]() || v.Type() == reflect.TypeFor[ir.Service]() { + kept = append(kept[:0], at.String()) + } + return !prune || v.Type() != reflect.TypeFor[ir.Service]() + }) + } + + assert.Equal(t, []string{"doc.Services[0]", "doc.Services[0]", "doc.Services[0].ID"}, + irtest.AllocatingVisits(doc, rendering(false), rendering(true))) +} + +// TestAllocatingVisits_MeasuresACleanVisitOnce pins what a measurement costs: +// a visit allocating nothing is measured once, since no later measurement can +// find fewer, and one that allocates as often as it takes to find its fewest. +// Each measurement makes the visit twice, a warm-up and the one measured, after +// the walk's own. It is not run in parallel: AllocsPerRun measures the process. +func TestAllocatingVisits_MeasuresACleanVisitOnce(t *testing.T) { + doc := &ir.Document{Name: "api"} + var kept []byte + counting := func(allocate bool) (ir.Visitor, *int) { + calls := 0 + return ir.VisitFunc(func(v reflect.Value, _ ir.WalkPath) bool { + if v.Kind() == reflect.String && v.String() == "api" { + calls++ + if allocate { + kept = make([]byte, 64) + } + } + return true + }), &calls + } + clean, cleanCalls := counting(false) + allocating, allocatingCalls := counting(true) + + assert.Empty(t, irtest.AllocatingVisits(doc, clean)) + assert.Equal(t, []string{"doc.Name"}, irtest.AllocatingVisits(doc, allocating)) + assert.Equal(t, 1+2, *cleanCalls, "the walk's visit, then one measurement") + assert.Equal(t, 1+2*irtest.VisitMeasurements, *allocatingCalls, "the walk's visit, then every measurement") + assert.Len(t, kept, 64, "the allocating visit kept what it made") +} + +// TestAllocatingVisits_TellsACycleVisitorOfACycle holds the walk to telling a +// visitor gathering cycles of each one, as a walk of its own would: a value +// holding itself is met again where the walk is inside it. It is not run in +// parallel: AllocsPerRun measures the process. +func TestAllocatingVisits_TellsACycleVisitorOfACycle(t *testing.T) { + list := make([]ir.Value, 1) + list[0] = ir.Value{Kind: ir.ValueList, List: list} + doc := &ir.Document{Types: ir.TypeRegistry{"t/x/M": &ir.Model{ + ID: "t/x/M", Examples: []ir.Example{{Value: &list[0]}}, + }}} + cycles := &cycleCounter{} + + irtest.AllocatingVisits(doc, cycles) + assert.Positive(t, cycles.met, "the self-holding list closes a cycle") +} + +// cycleCounter visits everything and counts the cycles it is told of. +type cycleCounter struct{ met int } + +// Visit asks for every value's children. +func (*cycleCounter) Visit(reflect.Value, ir.WalkPath) bool { return true } + +// Cycle counts one cycle. +func (c *cycleCounter) Cycle(ir.WalkPath, ir.WalkPath, reflect.Type) { c.met++ } + // TestAllocatingVisits_CountsTheFewestAVisitAllocates holds the measurement to // what a visit must allocate, not what it happens to: a visit allocating on // every third call, as one reaching a pool a race build drops from does at diff --git a/ir/irverify/bigval_internal_test.go b/ir/irverify/bigval_internal_test.go new file mode 100644 index 000000000..5ce8e1aa0 --- /dev/null +++ b/ir/irverify/bigval_internal_test.go @@ -0,0 +1,33 @@ +package irverify + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irtest" +) + +// TestCheckBigVals_VisitingCostsNoAllocation holds the check's cost for a +// numeric literal in canonical form at nothing, whatever its parts: it asks +// ir.NewBigVal for the canonical form, which hands such a literal back as it +// is, where rendering the canonical form to compare costs a string per +// literal. Both carriers are reached, a Value's Num and a bound held by +// pointer. It is not run in parallel: AllocsPerRun measures the process. +func TestCheckBigVals_VisitingCostsNoAllocation(t *testing.T) { + bound := func(s ir.BigVal) *ir.BigVal { return &s } + nums := []ir.BigVal{"0", "-0", "42", "-1.5", "2.5E-3", "1e+10", "12345678901234567890.12345678901234567890"} + values := make([]ir.Value, 0, len(nums)) + for _, num := range nums { + values = append(values, ir.Value{Kind: ir.ValueNumber, Num: num}) + } + doc := &ir.Document{Types: ir.TypeRegistry{"t/x/S": &ir.Scalar{ + ID: "t/x/S", + Examples: []ir.Example{{Value: &ir.Value{Kind: ir.ValueList, List: values}}}, + Constraints: &ir.Constraints{Min: bound("-0.5"), Max: bound("1e400"), MultipleOf: bound("0.25")}, + }}} + check := checkBigVals(doc) + assert.Empty(t, irtest.AllocatingVisits(doc, check)) + assert.Empty(t, check.report(declarations{}), "every literal is a canonical decimal") +} diff --git a/ir/irverify/declarations_internal_test.go b/ir/irverify/declarations_internal_test.go new file mode 100644 index 000000000..98a8ef9fc --- /dev/null +++ b/ir/irverify/declarations_internal_test.go @@ -0,0 +1,19 @@ +package irverify + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestDeclarations_ZeroValueDeclaresNothing holds the zero declarations to +// what its doc says, a document declaring none: it lists none and spells no +// path, whatever position a check asks for, rather than dereferencing a +// collector that is not there. +func TestDeclarations_ZeroValueDeclaresNothing(t *testing.T) { + t.Parallel() + var none declarations + assert.Nil(t, none.list()) + assert.Nil(t, none.pathsAt(map[int]bool{0: true})) + assert.False(t, none.truncated) +} diff --git a/ir/irverify/declared_internal_test.go b/ir/irverify/declared_internal_test.go index b49ae3749..8558ff1b3 100644 --- a/ir/irverify/declared_internal_test.go +++ b/ir/irverify/declared_internal_test.go @@ -80,11 +80,10 @@ func TestCheckDeclaredIDs_ReachesEveryIDDeclaringNode(t *testing.T) { keyed := ir.KeyedClasses() var want []string - for _, d := range decls { - if keyed[d.Class] { - continue + for i, path := range found.Paths() { + if d := decls[i]; !keyed[d.Class] { + want = append(want, "ir/empty-"+ir.RefNoun(d.Class)+"-id "+path.String()) } - want = append(want, "ir/empty-"+ir.RefNoun(d.Class)+"-id "+d.Path) } require.NotEmpty(t, want, "the fixture must declare a class Document keys no map by") diff --git a/ir/irverify/document.go b/ir/irverify/document.go index 1bde61df5..910a774ab 100644 --- a/ir/irverify/document.go +++ b/ir/irverify/document.go @@ -42,25 +42,32 @@ func checkDocument(doc *ir.Document) []Violation { }) } for i, src := range doc.Sources { - at := sourcesPath + "[" + strconv.Itoa(i) + "]" if !isFormat(src.Format) { vs = append(vs, Violation{ Code: "ir/source-format-malformed", Message: "source format " + strconv.Quote(src.Format) + " is not a name@version pair", - Path: at + ".Format", + Path: sourcePath(i, ".Format"), }) } if !isSHA256Hex(src.Hash) { vs = append(vs, Violation{ Code: "ir/source-hash-malformed", Message: "source hash " + strconv.Quote(src.Hash) + " is not a lowercase hex SHA-256 digest", - Path: at + ".Hash", + Path: sourcePath(i, ".Hash"), }) } } return vs } +// sourcePath spells where field of source i sits, for a violation to be placed +// there. It is spelled only then, since strconv.Itoa builds the string for an +// index of 100 or more: spelled for every source, it would cost a large source +// table an allocation per source with nothing wrong. +func sourcePath(i int, field string) string { + return sourcesPath + "[" + strconv.Itoa(i) + "]" + field +} + // isFormat reports whether format reads as name@version, each part non-empty // and the name free of '@', as compilers.SourceFormat.String spells one. func isFormat(format string) bool { diff --git a/ir/irverify/duplicates.go b/ir/irverify/duplicates.go index 6194ffcda..66b1cb0ed 100644 --- a/ir/irverify/duplicates.go +++ b/ir/irverify/duplicates.go @@ -3,7 +3,6 @@ package irverify import ( "reflect" "strconv" - "strings" "github.com/dexpace/morphic/ir" ) @@ -25,12 +24,12 @@ type identity struct { id string } -// declaredAt is where an identity was first declared, and the fingerprint of the -// node that declared it, so a later declaration of the same identity can be -// compared against it rather than only counted. -type declaredAt struct { - path string - fingerprint string +// fingerprint is what separates one property from another (see +// checkDuplicateIDs): its source name, wire name and TypeRef target. It holds +// the property's own strings, so taking one copies no bytes, and two compare +// equal exactly when each part does. +type fingerprint struct { + source, wire, target string } // checkDuplicateIDs asserts no two nodes declare the same identity (invariant @@ -44,59 +43,114 @@ type declaredAt struct { // the defect (#280). The fingerprint is source name, wire name and TypeRef // target; a wider one flags every reused component. func checkDuplicateIDs(*ir.Document) walkCheck { - // Keyed by path, which is how the declarations locate the same node. - fingerprints := map[string]string{} + // The first property met on each PropID, and the later ones whose + // fingerprint differs from it, each named by its PropID and how many met on + // that PropID before it. The walk meets properties in the order the + // declarations list them, so the first met is the PropID's first + // declaration, and the n-th met its n-th. + first := map[string]firstProperty{} + differs := map[occurrence]bool{} return walkCheck{ - visit: func(v reflect.Value, at ir.WalkPath) bool { - if v.Kind() == reflect.Struct && v.Type() == propertyType { - fingerprints[at.String()] = fingerprintOf(v) + visit: func(v reflect.Value, _ ir.WalkPath) bool { + if v.Kind() != reflect.Struct || v.Type() != propertyType { + return true } + id, got := v.FieldByName("ID").String(), fingerprintOf(v) + met, seen := first[id] + if !seen { + first[id] = firstProperty{fingerprint: got, met: 1} + return true + } + if met.fingerprint != got { + differs[occurrence{id: id, n: met.met}] = true + } + met.met++ + first[id] = met return true }, report: func(decls declarations) []Violation { - return duplicateIDs(decls.ids, fingerprints) + return duplicateIDs(decls, differs) }, } } +// firstProperty is the first property the walk met on a PropID, by its +// fingerprint, and how many it has met on that PropID so far. +type firstProperty struct { + fingerprint fingerprint + met int +} + +// occurrence names one declaration of an identity: the ID, and how many +// declarations of it came before this one. +type occurrence struct { + id string + n int +} + +// redeclaration is a declaration of an identity declared before it, by its +// position among the declarations and the first one's. +type redeclaration struct { + at, first int +} + // duplicateIDs reports each declaration in decls of an identity declared before -// it, unless both are one property, by the fingerprints of the properties -// declaring them. -func duplicateIDs(decls []ir.IDDeclaration, fingerprints map[string]string) []Violation { - first := make(map[identity]declaredAt, len(decls)) - var vs []Violation - for _, d := range decls { +// it, unless it is a property that is the first one's copy: differs holds the +// occurrences of the properties that are not. Only what it reports has its +// path spelled. +func duplicateIDs(decls declarations, differs map[occurrence]bool) []Violation { + list := decls.list() + type declared struct{ first, met int } + seen := make(map[identity]declared, len(list)) + var again []redeclaration + for i, d := range list { key := identity{class: d.Class, id: d.ID} - at, taken := first[key] + before, taken := seen[key] if !taken { - first[key] = declaredAt{path: d.Path, fingerprint: fingerprints[d.Path]} + seen[key] = declared{first: i, met: 1} continue } - if d.Class == propIDType && at.fingerprint == fingerprints[d.Path] { + seen[key] = declared{first: before.first, met: before.met + 1} + if d.Class == propIDType && !differs[occurrence{id: d.ID, n: before.met}] { continue // one property materialized at several paths, not two properties } + again = append(again, redeclaration{at: i, first: before.first}) + } + return redeclarationViolations(list, again, decls) +} + +// redeclarationViolations reports each of again, spelling the paths of the +// declarations it names and no other. +func redeclarationViolations(list []ir.IDDeclaration, again []redeclaration, decls declarations) []Violation { + if len(again) == 0 { + return nil + } + at := make(map[int]bool, 2*len(again)) + for _, r := range again { + at[r.at], at[r.first] = true, true + } + paths := decls.pathsAt(at) + vs := make([]Violation, 0, len(again)) + for _, r := range again { + d := list[r.at] vs = append(vs, Violation{ Code: "ir/duplicate-" + ir.RefNoun(d.Class) + "-id", - Message: "id " + strconv.Quote(d.ID) + " is declared here and at " + strconv.Quote(at.path), - Path: d.Path, + Message: "id " + strconv.Quote(d.ID) + " is declared here and at " + strconv.Quote(paths[r.first]), + Path: paths[r.at], }) } return vs } -// fingerprintOf renders what separates one property from another (see +// fingerprintOf reads what separates one property from another (see // checkDuplicateIDs). It reads fields off the walked value rather than -// converting it back to an ir.Property, because a value the walk reached through -// an unexported field cannot be converted (see ir.WalkValues); checkNaming's -// namingChannels reads its channels the same way. -// -// The parts are joined on NUL, which no source name, wire name or ID contains, -// so no two properties can agree on the rendering while disagreeing on the -// parts. -func fingerprintOf(prop reflect.Value) string { - return strings.Join([]string{ - prop.FieldByName("Name").FieldByName("Source").String(), - prop.FieldByName("WireName").String(), - prop.FieldByName("Type").FieldByName("Target").String(), - }, "\x00") +// converting it back to an ir.Property, because a value the walk reached +// through an unexported field cannot be converted (see ir.WalkValues); +// checkNaming's namingChannels reads its channels the same way. +func fingerprintOf(prop reflect.Value) fingerprint { + return fingerprint{ + source: prop.FieldByName("Name").FieldByName("Source").String(), + wire: prop.FieldByName("WireName").String(), + target: prop.FieldByName("Type").FieldByName("Target").String(), + } } diff --git a/ir/irverify/duplicates_internal_test.go b/ir/irverify/duplicates_internal_test.go index dd2c9fa07..57aa88c6f 100644 --- a/ir/irverify/duplicates_internal_test.go +++ b/ir/irverify/duplicates_internal_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irtest" ) // duplicateViolations runs the duplicate check and drops the truncation flag, @@ -188,6 +189,85 @@ func TestCheckDuplicateIDs_CopiesDifferingOutsideTheFingerprintAreClean(t *testi assert.Empty(t, duplicateViolations(doc)) } +// TestCheckDuplicateIDs_EachRepeatIsHeldToTheFirstProperty pins what a later +// property on a PropID is compared with: the first one the walk meets, and not +// the one met just before it, nor the PropID as a whole. Each copy and each +// difference is placed by its own path, in the walk's order of the models. +func TestCheckDuplicateIDs_EachRepeatIsHeldToTheFirstProperty(t *testing.T) { + t.Parallel() + const dup ir.PropID = "p/x/dup" + alpha := ir.Property{ID: dup, Name: ir.Naming{Source: "alpha"}, WireName: "alpha", Type: ir.TypeRef{Target: "t/x/A"}} + beta := alpha + beta.WireName = "beta" + for _, tc := range []struct { + name string + props []ir.Property // one per model, met in this order + want []string + }{ + {"a copy, then a difference", []ir.Property{alpha, alpha, beta}, []string{"doc.Types[t/x/C].Properties[0]"}}, + {"a difference, then its copy", []ir.Property{alpha, beta, beta}, + []string{"doc.Types[t/x/B].Properties[0]", "doc.Types[t/x/C].Properties[0]"}}, + {"copies only", []ir.Property{alpha, alpha, alpha}, []string{}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + doc := &ir.Document{Types: ir.TypeRegistry{}} + for i, prop := range tc.props { + id := ir.TypeID("t/x/" + string(rune('A'+i))) + doc.Types[id] = modelWithProp(id, prop) + } + got := duplicateViolations(doc) + paths := make([]string, 0, len(got)) + for _, v := range got { + assert.Equal(t, "ir/duplicate-prop-id", v.Code) + assert.Contains(t, v.Message, "doc.Types[t/x/A].Properties[0]", "held to the first property") + paths = append(paths, v.Path) + } + assert.Equal(t, tc.want, paths) + }) + } +} + +// TestCheckDuplicateIDs_VisitingCostsNoAllocation holds the check's cost for a +// property at nothing however often its PropID repeats, as every copy of a +// reused component does: a fingerprint holds the property's own strings, and a +// path is rendered only for a property whose fingerprint is not the first's. +// It is not run in parallel: AllocsPerRun measures the process. +func TestCheckDuplicateIDs_VisitingCostsNoAllocation(t *testing.T) { + header := ir.Property{ID: "p/x/Listed/headers/X-Rate", Name: ir.Naming{Source: "X-Rate"}, WireName: "X-Rate", + Type: ir.TypeRef{Target: "t/x/Rate"}} + shared := ir.Response{Name: ir.Naming{Source: "Listed"}, Headers: []ir.Property{header}} + doc := docWithOperations( + ir.Operation{ID: "op/a", Responses: []ir.Response{shared}}, + ir.Operation{ID: "op/b", Responses: []ir.Response{shared}}, + ir.Operation{ID: "op/c", Responses: []ir.Response{shared}}, + ) + doc.Types = ir.TypeRegistry{"t/x/M": &ir.Model{ID: "t/x/M", Properties: []ir.Property{ + {ID: "p/x/M/a", Name: ir.Naming{Source: "a"}, WireName: "a", Type: ir.TypeRef{Target: "t/x/A"}}, + {ID: "p/x/M/b", Name: ir.Naming{Source: "b"}, WireName: "b", Type: ir.TypeRef{Target: "t/x/B"}}, + }}} + check := checkDuplicateIDs(doc) + assert.Empty(t, irtest.AllocatingVisits(doc, check)) + assert.Empty(t, duplicateViolations(doc), "the header's copies are one property") +} + +// TestPropIDs_AreDeclaredByPropertiesAlone pins what checkDuplicateIDs leans on +// when it lets a repeated PropID through: every node declaring one is an +// ir.Property, the one node it fingerprints. A second node declaring a PropID +// would repeat it unfingerprinted and pass for a copy, so adding one reddens +// this, for the check to decide what tells two of them apart. +func TestPropIDs_AreDeclaredByPropertiesAlone(t *testing.T) { + t.Parallel() + var declaring []string + for _, f := range declaredFields(t) { + named, isNamed := types.Unalias(f.field.Type()).(*types.Named) + if f.field.Name() == "ID" && isNamed && named.Obj().Name() == propIDType.Name() { + declaring = append(declaring, f.owner) + } + } + assert.Equal(t, []string{propertyType.Name()}, declaring) +} + // TestVerify_ReportsDuplicateIDs pins that Verify runs the check, not just the // test: an ambiguous identity has to reach a caller that only ever calls Verify. func TestVerify_ReportsDuplicateIDs(t *testing.T) { diff --git a/ir/irverify/export_test.go b/ir/irverify/export_test.go index 864bc243f..0f53d36c5 100644 --- a/ir/irverify/export_test.go +++ b/ir/irverify/export_test.go @@ -4,6 +4,8 @@ import ( "maps" "reflect" "testing" + + "github.com/dexpace/morphic/ir" ) // EntryReader is entryReader, for this package's external tests. @@ -48,3 +50,11 @@ func HeldLists() map[string]bool { } return held } + +// WalkVisitors builds every walking check for doc and returns the visitors +// Verify's one walk carries, the declarations' collector among them, for this +// package's external tests. +func WalkVisitors(doc *ir.Document) []ir.Visitor { + _, _, visitors := walkVisitors(doc, walkChecks()) + return visitors +} diff --git a/ir/irverify/ids.go b/ir/irverify/ids.go index 1714a01e7..bda4c2808 100644 --- a/ir/irverify/ids.go +++ b/ir/irverify/ids.go @@ -27,7 +27,7 @@ func checkIDs(doc *ir.Document) []Violation { continue // checkRegistryKeys reports the nil entry itself } if v, bad := idDefect(ir.IDKindType, string(id), td.Common().Provenance); bad { - v.Path = "types[" + string(id) + "]" + v.Path = typePath(id) vs = append(vs, v) } } @@ -128,14 +128,13 @@ func checkPrimIDs(doc *ir.Document) []Violation { if ir.IsNilTypeDef(td) { continue // checkRegistryKeys reports the nil entry itself } - path := "types[" + string(id) + "]" prim, isPrim := td.(*ir.Primitive) if !isPrim { - vs = appendReservedSpace(vs, id, td.Kind(), path) + vs = appendReservedSpace(vs, id, td.Kind()) continue } if want := ir.PrimTypeID(prim.Prim); id != want { - vs = append(vs, primIDViolation(id, prim.Prim, want, path)) + vs = append(vs, primIDViolation(id, prim.Prim, want)) } } return vs @@ -147,13 +146,13 @@ func checkPrimIDs(doc *ir.Document) []Violation { // destination. ir.PrimTypeID derives t/prim/ from it, which is not an ID at all // — checkIDs reports it malformed wherever it is used — so offering it as the // place the node belongs would send a reader to fix the wrong end. -func primIDViolation(id ir.TypeID, kind ir.PrimKind, want ir.TypeID, path string) Violation { +func primIDViolation(id ir.TypeID, kind ir.PrimKind, want ir.TypeID) Violation { msg := "primitive of kind " + strconv.Quote(string(kind)) + " is interned at " + strconv.Quote(string(id)) + " rather than the shared " + strconv.Quote(string(want)) if kind == "" { msg = "primitive at " + strconv.Quote(string(id)) + " carries no kind, so no shared ID derives from it" } - return Violation{Code: "ir/prim-id-not-derived", Message: msg, Path: path} + return Violation{Code: "ir/prim-id-not-derived", Message: msg, Path: typePath(id)} } // appendReservedSpace reports a type that is not a primitive addressing the @@ -163,7 +162,7 @@ func primIDViolation(id ir.TypeID, kind ir.PrimKind, want ir.TypeID, path string // collides with the primitive of that kind outright, or squats a name the next // PrimKind takes. Both make the node reached for a kind depend on which // declaration lowered first, which is invariant 3's corollary. -func appendReservedSpace(vs []Violation, id ir.TypeID, kind ir.TypeKind, path string) []Violation { +func appendReservedSpace(vs []Violation, id ir.TypeID, kind ir.TypeKind) []Violation { space, ok := ir.IDSpace(ir.IDKindType, string(id)) if !ok || space != ir.IDSpacePrim { return vs @@ -172,7 +171,7 @@ func appendReservedSpace(vs []Violation, id ir.TypeID, kind ir.TypeKind, path st Code: "ir/prim-space-reserved", Message: "id " + strconv.Quote(string(id)) + " addresses the reserved primitive space but names a " + string(kind), - Path: path, + Path: typePath(id), }) } diff --git a/ir/irverify/idspaces.go b/ir/irverify/idspaces.go index fc321c016..8151a70e4 100644 --- a/ir/irverify/idspaces.go +++ b/ir/irverify/idspaces.go @@ -26,16 +26,19 @@ const idSpacesPath = ir.DocumentPath + ".IDSpaces" func checkIDSpaces(doc *ir.Document) walkCheck { // The declarations are all it reads, so it visits nothing itself. return walkCheck{report: func(decls declarations) []Violation { - return idSpaceViolations(doc, decls.ids) + return idSpaceViolations(doc, decls) }} } // idSpaceViolations is checkIDSpaces' finding over the identities decls. -func idSpaceViolations(doc *ir.Document, decls []ir.IDDeclaration) []Violation { +// Only a declaration outside every declared namespace has its path spelled. +func idSpaceViolations(doc *ir.Document, decls declarations) []Violation { vs := declarationViolations(doc.IDSpaces) declared := declaredSpaces(doc.IDSpaces) + list := decls.list() var carried int - for _, d := range decls { + var outside []int + for i, d := range list { prefix, held := kindPrefixes[d.Class] if !held { continue @@ -46,14 +49,10 @@ func idSpaceViolations(doc *ir.Document, decls []ir.IDDeclaration) []Violation { } carried++ if len(doc.IDSpaces) > 0 && !declared[prefix][space] { - vs = append(vs, Violation{ - Code: "ir/id-space-undeclared", - Message: "id " + strconv.Quote(d.ID) + " lives in namespace " + strconv.Quote(space) + - ", which the document does not declare for kind " + strconv.Quote(prefix), - Path: d.Path, - }) + outside = append(outside, i) } } + vs = appendUndeclaredSpaces(vs, list, outside, decls) if carried > 0 && len(doc.IDSpaces) == 0 { vs = append(vs, Violation{ Code: "ir/id-spaces-absent", @@ -64,6 +63,31 @@ func idSpaceViolations(doc *ir.Document, decls []ir.IDDeclaration) []Violation { return vs } +// appendUndeclaredSpaces reports each declaration of list at a position in +// outside as living in a namespace its kind does not declare. +func appendUndeclaredSpaces(vs []Violation, list []ir.IDDeclaration, outside []int, decls declarations) []Violation { + if len(outside) == 0 { + return vs + } + at := make(map[int]bool, len(outside)) + for _, i := range outside { + at[i] = true + } + paths := decls.pathsAt(at) + for _, i := range outside { + d := list[i] + prefix := kindPrefixes[d.Class] + space, _ := ir.IDSpace(prefix, d.ID) + vs = append(vs, Violation{ + Code: "ir/id-space-undeclared", + Message: "id " + strconv.Quote(d.ID) + " lives in namespace " + strconv.Quote(space) + + ", which the document does not declare for kind " + strconv.Quote(prefix), + Path: paths[i], + }) + } + return vs +} + // declaredSpaces returns the declaration as sets, for membership. func declaredSpaces(declaration map[string][]string) map[string]map[string]bool { out := make(map[string]map[string]bool, len(declaration)) @@ -110,21 +134,26 @@ func declarationViolations(declaration map[string][]string) []Violation { // appendSpaceViolations reports the unusable entries of one kind's list. func appendSpaceViolations(vs []Violation, kind string, spaces []string) []Violation { for i, space := range spaces { - path := idSpacesPath + "[" + kind + "][" + strconv.Itoa(i) + "]" if space == "" || strings.Contains(space, ir.IDSeparator) { vs = append(vs, Violation{ Code: "ir/id-space-invalid", Message: "namespace " + strconv.Quote(space) + " is empty or carries the id separator, so no id can be in it", - Path: path, + Path: spacePath(kind, i), }) } if i > 0 && space <= spaces[i-1] { vs = append(vs, Violation{ Code: "ir/id-spaces-not-canonical", Message: "namespace " + strconv.Quote(space) + " does not follow " + strconv.Quote(spaces[i-1]) + " in sorted order without repeats", - Path: path, + Path: spacePath(kind, i), }) } } return vs } + +// spacePath spells where namespace i of the kind's list sits, for a violation +// to be placed there. +func spacePath(kind string, i int) string { + return idSpacesPath + "[" + kind + "][" + strconv.Itoa(i) + "]" +} diff --git a/ir/irverify/idspaces_test.go b/ir/irverify/idspaces_test.go index a541e827f..5b417bab9 100644 --- a/ir/irverify/idspaces_test.go +++ b/ir/irverify/idspaces_test.go @@ -94,6 +94,27 @@ func TestVerify_IDInAnUndeclaredNamespaceIsAViolation(t *testing.T) { } } +// TestVerify_EachIDOutsideTheDeclarationIsReportedAtItsOwnNode pins where each +// ir/id-space-undeclared lands when several IDs miss the declaration: at the +// node declaring that ID, not at another's. A declared ID sits between the two, +// so a report placed by the wrong position shows too. +func TestVerify_EachIDOutsideTheDeclarationIsReportedAtItsOwnNode(t *testing.T) { + t.Parallel() + doc := &ir.Document{IRVersion: ir.IRVersion, Types: ir.TypeRegistry{}} + for _, id := range []ir.TypeID{"t/openapipaths/x/a", "t/openapi/x/b", "t/openapipaths/x/c"} { + doc.Types[id] = &ir.Model{ID: id, Name: ir.Naming{Source: "M", Canonical: "m"}} + } + declaration := map[string][]string{} + for _, kind := range ir.IDKinds() { + declaration[kind] = []string{"x"} + } + declaration[ir.IDKindType] = []string{"openapi"} + + got := irverify.Verify(declaring(doc, declaration)) + assert.Equal(t, []string{"doc.Types[t/openapipaths/x/a]", "doc.Types[t/openapipaths/x/c]"}, + violationPaths(got, "ir/id-space-undeclared")) +} + // TestVerify_NamespaceDeclaredForAnotherKindDoesNotCount pins that the // declaration is per kind. A namespace some other kind declares is not one this // kind may use, which a flat set could not tell. diff --git a/ir/irverify/indices_internal_test.go b/ir/irverify/indices_internal_test.go index c4b79e426..011a2d633 100644 --- a/ir/irverify/indices_internal_test.go +++ b/ir/irverify/indices_internal_test.go @@ -207,11 +207,13 @@ var integerFields = map[string]string{ "flattener.index": "where a wire name sits among a flattening's own members, not a position in any IR slice", "flattener.aliasOf": "where an alias scalar's chain end sits in a flattening's own aliasEnds, " + "not a position in any IR slice", - "flattener.steps": "how much a flattening has read, not a position", - "frame.next": "the next composition entry a flattening follows, counted across Base, Implements and Mixins", - "frame.num": "the order a flattening met a model in, not a position in any IR slice", - "frame.low": "Tarjan's low-link of a model a flattening met, not a position in any IR slice", - "frame.since": "how many models a flattening had finished, a length of its own list, not a position", + "flattener.steps": "how much a flattening has read, not a position", + "frame.next": "the next composition entry a flattening follows, counted across Base, Implements and Mixins", + "frame.num": "the order a flattening met a model in, not a position in any IR slice", + "frame.low": "Tarjan's low-link of a model a flattening met, not a position in any IR slice", + "frame.since": "how many models a flattening had finished, a length of its own list, not a position", + "loggedPath.shared": "how many steps a kept path shares with the one kept before it, not a position in any IR slice", + "loggedPath.end": "where a kept path's own steps end in its log's storage, not a position in any IR slice", // The ir package's reading of its own types, which is not IR data either. "idField.index": "the field through which a struct type declares its identity, read off the type", diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 13301635f..8246d791a 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -26,6 +26,12 @@ type Violation struct { // Verify runs every structural invariant check over doc and returns the // violations, sorted by (Code, Path) for deterministic output. A structurally // sound document yields nil. +// +// A check builds a path, or any other string, only for a violation, so a value +// with nothing wrong costs it no allocation but a step a map or a log it keeps +// may grow by, beyond what its readers make: the set ir.Inferred.IsCanonical +// makes of a long list, and the names a jsontext decoder keeps of a large +// object to refuse a duplicate. func Verify(doc *ir.Document) []Violation { return verify(doc, ir.WalkValues) } @@ -64,8 +70,9 @@ func verify(doc *ir.Document, walk walkFunc) []Violation { return vs } -// declarations is the identities a document's nodes declare, plus whether the -// walk that read them was cut short. +// declarations is the identities a document's nodes declare, each with its +// path kept unspelled for pathsAt, plus whether the walk that read them was cut +// short. // // The walk every check shares reads them, and two checks need them once it is // over: checkDuplicateIDs to hold each to being declared once, and @@ -75,10 +82,33 @@ func verify(doc *ir.Document, walk walkFunc) []Violation { // // The zero value says the document declares none and the walk ran to its end. type declarations struct { - ids []ir.IDDeclaration + collected *ir.IDDeclarations truncated bool } +// list returns the declarations, in walk order. +func (d declarations) list() []ir.IDDeclaration { + if d.collected == nil { + return nil + } + return d.collected.List() +} + +// pathsAt spells the path of each declaration at a position in at, keyed by +// that position, so a check spells only the paths it reports. +func (d declarations) pathsAt(at map[int]bool) map[int]string { + if d.collected == nil { + return nil + } + paths := make(map[int]string, len(at)) + for i, path := range d.collected.Paths() { + if at[i] { + paths[i] = path.String() + } + } + return paths +} + // walkCheck is one check's part in the walk every check shares: visit is shown // each value the walk reaches, as an ir.Visitor's Visit is, and report returns // what the check found once the walk is over, given the declarations the same @@ -156,16 +186,8 @@ func runWalkChecks(doc *ir.Document, walk walkFunc) []Violation { // reading the declarations in the same walk. It returns what they report, in // the order given, and whether the walk was cut short. func walkWith(doc *ir.Document, walk walkFunc, checks []func(*ir.Document) walkCheck) ([]Violation, bool) { - built := make([]walkCheck, len(checks)) - var ids ir.IDDeclarations - visitors := make([]ir.Visitor, 0, 1+len(checks)) - visitors = append(visitors, &ids) - for i, newCheck := range checks { - built[i] = newCheck(doc) - visitors = append(visitors, built[i]) - } - decls := declarations{truncated: walk(doc, ir.DocumentPath, visitors...)} - decls.ids = ids.List() + ids, built, visitors := walkVisitors(doc, checks) + decls := declarations{collected: ids, truncated: walk(doc, ir.DocumentPath, visitors...)} reports := make([][]Violation, len(built)) for i, c := range built { @@ -174,6 +196,21 @@ func walkWith(doc *ir.Document, walk walkFunc, checks []func(*ir.Document) walkC return slices.Concat(reports...), decls.truncated } +// walkVisitors builds each of checks for doc and returns them, the collector +// of the document's declarations, and the visitors one walk carries for all of +// them, the collector first. +func walkVisitors(doc *ir.Document, checks []func(*ir.Document) walkCheck) (*ir.IDDeclarations, []walkCheck, []ir.Visitor) { + ids := &ir.IDDeclarations{} + built := make([]walkCheck, len(checks)) + visitors := make([]ir.Visitor, 0, 1+len(checks)) + visitors = append(visitors, ids) + for i, newCheck := range checks { + built[i] = newCheck(doc) + visitors = append(visitors, built[i]) + } + return ids, built, visitors +} + // checkRegistryKeys asserts every entry of each flat, ID-keyed registry // (Types, Channels, Messages, Auth) is keyed by its own node ID and that the key // is non-empty (invariant #3). Each registry contributes symmetric empty-*-id and @@ -189,7 +226,7 @@ func checkRegistryKeys(doc *ir.Document) []Violation { vs = append(vs, Violation{ Code: "ir/nil-type", Message: "types registry has a nil type definition", - Path: "types[" + string(id) + "]", + Path: typePath(id), }) continue } @@ -207,6 +244,12 @@ func checkRegistryKeys(doc *ir.Document) []Violation { return vs } +// typePath spells where a check that ranges over the type registry reports the +// entry id. A check spells it only once it has a violation to place there. +func typePath(id ir.TypeID) string { + return "types[" + string(id) + "]" +} + // registryKey checks one registry entry: key must be non-empty and equal the // node's own ID. noun is the diagnostic-code singular ("type", "channel", …) and // reg is the path/message registry label ("types", "channels", …). diff --git a/ir/irverify/irverify_corpus_test.go b/ir/irverify/irverify_corpus_test.go index ebefb93e9..e8c090e75 100644 --- a/ir/irverify/irverify_corpus_test.go +++ b/ir/irverify/irverify_corpus_test.go @@ -1,10 +1,13 @@ package irverify_test // external test package — importing across layers is legal in tests import ( + "fmt" "maps" "os" "path/filepath" + "reflect" "slices" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -14,6 +17,7 @@ import ( "github.com/dexpace/morphic/compilers/openapi" "github.com/dexpace/morphic/internal/harness" "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irtest" "github.com/dexpace/morphic/ir/irverify" ) @@ -71,6 +75,89 @@ func TestVerify_Corpus(t *testing.T) { } } +// TestWalkChecks_ACleanValueCostsNothing holds what Verify's walk carries, the +// declarations and every walking check, together, to costing nothing for a +// value none reports on, in a compiled document. extensions-x.yaml writes an +// extension on every object OpenAPI admits one on, so its document reaches +// nearly every node a compile produces, and holds none of the costs Verify's +// doc excepts. It is not run in parallel: AllocsPerRun measures the process. +func TestWalkChecks_ACleanValueCostsNothing(t *testing.T) { + data, err := os.ReadFile("../../testdata/conformance/openapi/extensions-x.yaml") + require.NoError(t, err) + doc := compile(t, "extensions-x.yaml", data) + require.NotNil(t, doc) + require.Empty(t, irverify.Verify(doc), "the document is clean, or what is measured is a report") + + assert.Empty(t, irtest.AllocatingVisits(doc, irverify.WalkVisitors(doc)...)) +} + +// TestVerify_ACleanComponentCostsNothing holds all Verify does to costing +// nothing for a value it finds clean, including what irtest.AllocatingVisits +// never measures: a check's first meeting with a value, and its report once +// the walk is over. A check keeping a path per PropID it meets, or spelling +// every declaration's path to report a few, passes the test above. Twice the +// components must cost less than one allocation more per component added, the +// walk's growth aside; a map or a log a check keeps grows in steps. It is not +// run in parallel: AllocsPerRun measures the process. +func TestVerify_ACleanComponentCostsNothing(t *testing.T) { + const components = 100 + few, many := scaledDocument(t, components), scaledDocument(t, 2*components) + + added := verifyingAllocs(many) - verifyingAllocs(few) + assert.Less(t, added, float64(components), "%v allocations more for %d components more", added, components) +} + +// scaledDocument compiles a spec of n components, each an operation and the +// schema it returns, which reach every walking check, and requires the +// document to be clean. +func scaledDocument(t *testing.T, n int) *ir.Document { + t.Helper() + var spec strings.Builder + spec.WriteString("openapi: 3.1.0\ninfo: {title: scaled, version: '1'}\npaths:\n") + for i := range n { + fmt.Fprintf(&spec, ` /pets%[1]d/{id}: + get: + operationId: getPet%[1]d + tags: [pets%[1]d] + x-kept: {a: [1, b]} + parameters: + - {name: id, in: path, required: true, schema: {type: string}} + - {name: q, in: query, schema: {type: integer, minimum: 1, maximum: 2.5e3}} + responses: + '200': {description: ok, content: {application/json: {schema: {$ref: '#/components/schemas/Pet%[1]d'}}}} + '404': {$ref: '#/components/responses/Missing'} +`, i) + } + spec.WriteString("components:\n responses:\n Missing:\n description: missing\n" + + " headers: {X-Rate: {schema: {type: integer}}}\n schemas:\n") + for i := range n { + fmt.Fprintf(&spec, ` Pet%d: + type: object + x-kept: {a: [1, b]} + required: [name] + properties: + name: {type: string, maxLength: 10, example: rex} + age: {type: number, minimum: 0, maximum: 1.5e3, multipleOf: 0.5} + kind: {type: string, enum: [cat, dog]} + tags: {type: array, items: {type: string}} +`, i) + } + doc := compile(t, "scaled.yaml", []byte(spec.String())) + require.NotNil(t, doc) + require.Empty(t, irverify.Verify(doc), "the document is clean, or what is measured is a report") + return doc +} + +// verifyingAllocs returns what one Verify of doc allocates, less what a walk of +// doc allocates visiting nothing. +func verifyingAllocs(doc *ir.Document) float64 { + verifying := testing.AllocsPerRun(5, func() { _ = irverify.Verify(doc) }) + walking := testing.AllocsPerRun(5, func() { + ir.WalkValues(doc, ir.DocumentPath, ir.VisitFunc(func(reflect.Value, ir.WalkPath) bool { return true })) + }) + return verifying - walking +} + // uncorpusedUnmodeled exercises the Unmodeled writes no committed fixture // reaches: path-item servers and an error response's own x-* extension. The // parameter's xml hints and the `items` an object schema cannot hold are diff --git a/ir/irverify/kinds.go b/ir/irverify/kinds.go index 4e1a352b1..cf575b27d 100644 --- a/ir/irverify/kinds.go +++ b/ir/irverify/kinds.go @@ -30,7 +30,7 @@ func checkPrimKinds(doc *ir.Document) []Violation { vs = append(vs, Violation{ Code: "ir/unknown-prim-kind", Message: "primitive carries undeclared kind " + strconv.Quote(string(prim.Prim)), - Path: "types[" + string(id) + "]", + Path: typePath(id), }) } return vs diff --git a/ir/irverify/maps.go b/ir/irverify/maps.go index d2503da44..f3b12bbdc 100644 --- a/ir/irverify/maps.go +++ b/ir/irverify/maps.go @@ -22,7 +22,7 @@ func checkMaps(doc *ir.Document) []Violation { vs = append(vs, Violation{ Code: "ir/map-as-model", Message: "model is a map and nothing else, which is a MapT", - Path: "types[" + string(id) + "]", + Path: typePath(id), }) } return vs @@ -43,7 +43,7 @@ func checkMapKeys(doc *ir.Document) []Violation { vs = append(vs, Violation{ Code: "ir/map-key-string", Message: "key names the string primitive, which a nil key already states", - Path: "types[" + string(id) + "]." + field, + Path: typePath(id) + "." + field, }) } return vs diff --git a/ir/irverify/memberids.go b/ir/irverify/memberids.go index 18212f1a4..d0be6212e 100644 --- a/ir/irverify/memberids.go +++ b/ir/irverify/memberids.go @@ -1,8 +1,6 @@ package irverify import ( - "maps" - "slices" "strconv" "strings" @@ -16,11 +14,12 @@ import ( // only check tying a member's ID to its enum's. // // A malformed or empty member ID is skipped: checkDeclaredIDs reports those once. -// Enums are visited in ID order, so the report does not follow map iteration. +// Each violation's path names its enum and member, so Verify's sort orders the +// report whatever order the registry is ranged in. func checkMemberIDs(doc *ir.Document) []Violation { var vs []Violation - for _, id := range slices.Sorted(maps.Keys(doc.Types)) { - enum, isEnum := doc.Types[id].(*ir.Enum) + for id, td := range doc.Types { + enum, isEnum := td.(*ir.Enum) if !isEnum || enum == nil { continue } @@ -43,8 +42,7 @@ func memberScopeViolation(enumID ir.TypeID, member ir.EnumMemberID, i int) (Viol if !spaceOK || !hasPath || !enumSpaceOK || !enumHasPath { return Violation{}, false } - rest, under := strings.CutPrefix(path, enumPath+ir.IDSeparator) - if space == enumSpace && under && rest != "" { + if space == enumSpace && beneath(path, enumPath) { return Violation{}, false } return Violation{ @@ -55,3 +53,12 @@ func memberScopeViolation(enumID ir.TypeID, member ir.EnumMemberID, i int) (Viol Path: ir.DocumentPath + ".Types[" + string(enumID) + "].Members[" + strconv.Itoa(i) + "]", }, true } + +// beneath reports whether the ID path path lies strictly below parent: parent, +// then the ID separator, then at least one more byte. It compares the parts +// where they lie rather than joining parent and the separator to compare. +func beneath(path, parent string) bool { + rest, under := strings.CutPrefix(path, parent) + rest, separated := strings.CutPrefix(rest, ir.IDSeparator) + return under && separated && rest != "" +} diff --git a/ir/irverify/memberids_test.go b/ir/irverify/memberids_test.go index 437264b21..ea143a8cc 100644 --- a/ir/irverify/memberids_test.go +++ b/ir/irverify/memberids_test.go @@ -67,6 +67,7 @@ func TestVerify_MemberIDOutsideItsEnumIsAViolation(t *testing.T) { {"another namespace", "e/anon/components/schemas/E/s:a"}, {"the enum's own path with no key", "e/openapi/components/schemas/E"}, {"the enum's own path with an empty key", "e/openapi/components/schemas/E/"}, + {"the enum's path behind an empty segment", "e/openapi//components/schemas/E/s:a"}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 7519f150b..e151ff2bf 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -12,16 +12,15 @@ import ( var namingType = reflect.TypeFor[ir.Naming]() -// nameField is how a node spells the ir.Naming that names it, and so the last -// segment of the path the walk reaches that one by. A node in nameOptional -// renamed out of this spelling stops matching, which reports a violation on a -// document that has none rather than going silent — +// nameField is the field through which a node holds the ir.Naming that names +// it. A node in nameOptional renamed out of this spelling stops matching, which +// reports a violation on a document that has none rather than going silent — // TestVerify_OptionalNameOwnersAreClean is what reddens. // // Only the exemptions are addressed this way. The rule itself holds every // ir.Naming the walk reaches, including the ones no Name field owns, such as the // values of Service.Renames. -const nameField = ".Name" +const nameField = "Name" // nameOptional are the nodes that carry no name of their own, so an empty // Naming on one is expected rather than the missing-name defect below. @@ -51,22 +50,15 @@ var nameOptional = map[reflect.Type]bool{ // Naming.Aliases has rules of its own: see appendAliasViolations. func checkNaming(*ir.Document) walkCheck { var vs []Violation - optional := map[string]bool{} + // Every alias list is read through this one, so a list costs no map of its + // own however long it is. + var aliases reusedMap[string, int] return walkCheck{ visit: func(v reflect.Value, at ir.WalkPath) bool { - if v.Kind() != reflect.Struct { + if v.Kind() != reflect.Struct || v.Type() != namingType { return true } - if nameOptional[v.Type()] { - // The walk visits a struct before its fields, so this is recorded - // before the Naming it exempts is reached. - optional[at.String()+nameField] = true - return true - } - if v.Type() != namingType { - return true - } - vs = appendNamingChecks(vs, v, at, optional) + vs = appendNamingChecks(vs, v, at, &aliases) return false // a Naming holds no Naming nested in it }, report: reporting(&vs), @@ -74,33 +66,27 @@ func checkNaming(*ir.Document) walkCheck { } // appendNamingChecks reports what is wrong with the one Naming naming, at at. -// optional holds the paths of the Namings whose nodes carry no name of their -// own. -func appendNamingChecks(vs []Violation, naming reflect.Value, at ir.WalkPath, optional map[string]bool) []Violation { +// read is what appendAliasViolations reads the aliases through. +func appendNamingChecks(vs []Violation, naming reflect.Value, at ir.WalkPath, read *reusedMap[string, int]) []Violation { source, canon, hint, aliases := namingChannels(naming) - vs = appendAbsentViolation(vs, source, canon, hint, at, optional) + vs = appendAbsentViolation(vs, source, canon, hint, at) vs = appendNamingViolations(vs, source, canon, hint, at) - return appendAliasViolations(vs, source, aliases, at) + return appendAliasViolations(vs, source, aliases, at, read) } // namingChannels reads every name channel off one Naming. It reads fields // rather than converting back to an ir.Naming because a value the walk reached // through an unexported field cannot be (see ir.WalkValues), which is also why -// aliases are copied element by element. +// aliases come back as the list itself, for each entry to be read in turn. // // Nothing guards the field lookups: renaming a field compiles clean and reddens // the naming tests. A guard would be a statement no test can cover, since // checkNaming calls this only for an ir.Naming. -func namingChannels(naming reflect.Value) (source, canon, hint string, aliases []string) { - list := naming.FieldByName("Aliases") - aliases = make([]string, list.Len()) - for i := range list.Len() { - aliases[i] = list.Index(i).String() - } +func namingChannels(naming reflect.Value) (source, canon, hint string, aliases reflect.Value) { return naming.FieldByName("Source").String(), naming.FieldByName("Canonical").String(), naming.FieldByName("Hint").String(), - aliases + naming.FieldByName("Aliases") } // appendAliasViolations holds one alias list to the rules ir.Naming.Aliases @@ -114,9 +100,12 @@ func namingChannels(naming reflect.Value) (source, canon, hint string, aliases [ // // Collisions with other entities' names are no rule of the list (ir-design // §3.2). -func appendAliasViolations(vs []Violation, source string, aliases []string, at ir.WalkPath) []Violation { - seen := make(map[string]int, len(aliases)) - for i, alias := range aliases { +func appendAliasViolations(vs []Violation, source string, aliases reflect.Value, at ir.WalkPath, + read *reusedMap[string, int], +) []Violation { + seen := read.emptied() // what another entity's list held collides with nothing here + for i := range aliases.Len() { + alias := aliases.Index(i).String() switch first, repeated := seen[alias]; { case isBlankName(alias): // First, so a repeated blank is reported blank: the repair is to @@ -162,12 +151,10 @@ func aliasPath(at ir.WalkPath, i int) string { // hold whichever is there. This asks only that one of them is, because nothing // downstream can render an identifier from three empty strings. // -// optional is read last, since reading it renders the path, so only a Naming -// that names nothing pays for one. -func appendAbsentViolation(vs []Violation, source, canon, hint string, at ir.WalkPath, - optional map[string]bool, -) []Violation { - if source != "" || canon != "" || hint != "" || optional[at.String()] { +// The exemption is asked last, as it reads the path's steps, which only a +// Naming that names nothing needs. +func appendAbsentViolation(vs []Violation, source, canon, hint string, at ir.WalkPath) []Violation { + if source != "" || canon != "" || hint != "" || nameExempt(at, nameOptional) { return vs } return append(vs, Violation{ @@ -177,6 +164,15 @@ func appendAbsentViolation(vs []Violation, source, canon, hint string, at ir.Wal }) } +// nameExempt reports whether the Naming at at names a node optional lists: +// whether the walk reached it as that node's own name, the field its path ends +// in, which for a Primitive's is the Name its embedded ir.TypeCommon promotes. +// Any other Naming such a node holds is held to the rule. +func nameExempt(at ir.WalkPath, optional map[reflect.Type]bool) bool { + owner, field, ok := at.Field() + return ok && field == nameField && optional[owner] +} + // appendNamingViolations reports the ways one Naming can break neutrality: the // grammar rule over the canonical, and the content rules over each channel that // carries a name for an emitter to render. diff --git a/ir/irverify/naming_internal_test.go b/ir/irverify/naming_internal_test.go index 3c15a535e..cbe7f772c 100644 --- a/ir/irverify/naming_internal_test.go +++ b/ir/irverify/naming_internal_test.go @@ -1,6 +1,8 @@ package irverify import ( + "reflect" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -10,15 +12,20 @@ import ( ) // TestCheckNaming_VisitingCostsNoAllocation holds the check's cost for a Naming -// whose canonical the grammar derives from its source at nothing, a source -// already canonical or not: it compares the two as it reads the source, where -// rendering the canonical to compare costs a string, or one per word before -// that. A Primitive is left out, as its exemption is recorded by path. It is -// not run in parallel: AllocsPerRun measures the process. +// it has nothing to report on at nothing: a canonical the grammar derives from +// its source, compared as the source is read rather than rendered to compare; a +// primitive's empty name, exempt by the field the walk reached it through +// rather than by a path recorded for every primitive; and an alias list longer +// than a map holds without allocating, held through the one map the check +// keeps. It is not run in parallel: AllocsPerRun measures the process. func TestCheckNaming_VisitingCostsNoAllocation(t *testing.T) { named := func(source string) ir.Naming { return ir.Naming{Source: source, Canonical: ir.CanonicalWords(source)} } + aliased := named("Aliased") + for i := range 20 { + aliased.Aliases = append(aliased.Aliases, "com.example.Aliased"+strconv.Itoa(i)) + } doc := &ir.Document{Types: ir.TypeRegistry{ "t/pet": &ir.Model{ID: "t/pet", Name: named("PetStore"), Properties: []ir.Property{ {ID: "p/pet/a", Name: named("userID")}, @@ -26,9 +33,80 @@ func TestCheckNaming_VisitingCostsNoAllocation(t *testing.T) { {ID: "p/pet/c", Name: named("already_canonical")}, {ID: "p/pet/d", Name: named("com.example.User")}, }}, - "t/anon": &ir.Model{ID: "t/anon", Name: ir.Naming{Hint: "anon_pet"}}, + "t/anon": &ir.Model{ID: "t/anon", Name: ir.Naming{Hint: "anon_pet"}}, + "t/aliased": &ir.Model{ID: "t/aliased", Name: aliased}, + "t/prim/str": &ir.Primitive{ID: "t/prim/str", Prim: ir.PrimString}, }} check := checkNaming(doc) assert.Empty(t, irtest.AllocatingVisits(doc, check)) - assert.Empty(t, check.report(declarations{}), "every name is neutral and derived") + assert.Empty(t, check.report(declarations{}), "every name is neutral and derived, and the primitive exempt") +} + +// TestNameExempt_IsTheOptionalNodesOwnNameAlone holds the exemption to the one +// Naming a listed node is named by. No ir type tells the parts apart, since a +// primitive holds no Naming but its name, so the node here is the test's own: +// one carrying a second Naming, which is not its name, and a node of its own +// inside it, held beside an unlisted node. No Name but the listed node's is +// exempt. +func TestNameExempt_IsTheOptionalNodesOwnNameAlone(t *testing.T) { + t.Parallel() + type inner struct{ Name ir.Naming } + type twoNamings struct { + Name, Other ir.Naming + Inner inner + } + type holder struct { + Listed twoNamings + Unlisted struct{ Name ir.Naming } + } + optional := map[reflect.Type]bool{reflect.TypeFor[twoNamings](): true} + got := map[string]bool{} + ir.WalkValues(holder{}, ir.DocumentPath, ir.VisitFunc(func(v reflect.Value, at ir.WalkPath) bool { + if v.Type() == namingType { + got[at.String()] = nameExempt(at, optional) + } + return true + })) + assert.Equal(t, map[string]bool{ + "doc.Listed.Name": true, + "doc.Listed.Other": false, + "doc.Listed.Inner.Name": false, + "doc.Unlisted.Name": false, + }, got) +} + +// TestCheckNaming_ExemptsAPrimitiveInAFieldNotOneEmbedded holds the exemption +// to the field a Naming is reached through, as its path spells it: a primitive +// held in a field of any name keeps its own Name exempt, while a struct +// embedding one beside a Name of its own spells the primitive's Name, which +// that Name shadows, on itself, so neither is exempt, though both share a +// path. An exemption recorded by path silenced both there. +func TestCheckNaming_ExemptsAPrimitiveInAFieldNotOneEmbedded(t *testing.T) { + t.Parallel() + type held struct{ Kept ir.Primitive } + type embedding struct { + ir.Primitive + Name ir.Naming + } + for _, tc := range []struct { + name string + root any + want []string + }{ + {"held in a field", held{}, nil}, + {"embedded beside a Name", embedding{}, []string{"doc.Name", "doc.Name"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + check := checkNaming(nil) + ir.WalkValues(tc.root, ir.DocumentPath, check) + var absent []string + for _, v := range check.report(declarations{}) { + if v.Code == "ir/naming-absent" { + absent = append(absent, v.Path) + } + } + assert.Equal(t, tc.want, absent) + }) + } } diff --git a/ir/irverify/naming_test.go b/ir/irverify/naming_test.go index 0f1957517..452fb2cba 100644 --- a/ir/irverify/naming_test.go +++ b/ir/irverify/naming_test.go @@ -316,8 +316,9 @@ func primitiveDoc() *ir.Document { // is the shape the IR expects. Without this, the rule would fail every document // in the corpus, all of which intern primitives. // -// It is also what holds the ".Name" path the exemption is recorded under: a node -// that spelled its Naming as any other field would stop matching and redden here. +// It is also what holds the field the exemption is keyed on: a primitive that +// held its Naming in a field of any other name would stop matching and redden +// here. func TestVerify_OptionalNameOwnersAreClean(t *testing.T) { t.Parallel() assert.Empty(t, verifyDeclared(primitiveDoc())) @@ -358,14 +359,14 @@ func TestVerify_NamelessServerAndResponseAreViolations(t *testing.T) { } // TestVerify_OptionalOwnerExemptsOnlyItsOwnName is the overreach guard: the -// exemption is recorded against one path, so it silences that node's own Naming -// and no other. A document-wide "an exempt node was seen" flag would pass every -// test that measures one node at a time and silence the whole corpus. +// exemption is keyed on the field the walk reaches a Naming through, so it +// silences that node's own Naming and no other. A document-wide "an exempt node +// was seen" flag would pass every test that measures one node at a time and +// silence the whole corpus. // -// It reaches the sibling axis only. The descendant axis, a nameless node inside -// an exempt one, has no pair to measure with while ir.Primitive is the only -// exemption, since nothing under a primitive carries a Naming. Whoever adds the -// next exemption owns that half again. +// It reaches the sibling axis only. Nothing under a primitive carries a Naming, +// so TestNameExempt_IsTheOptionalNodesOwnNameAlone measures the descendant +// axis, a nameless node inside an exempt one, through a node of its own. func TestVerify_OptionalOwnerExemptsOnlyItsOwnName(t *testing.T) { t.Parallel() doc := primitiveDoc() @@ -380,7 +381,7 @@ func TestVerify_OptionalOwnerExemptsOnlyItsOwnName(t *testing.T) { // every Naming the walk reaches, not only the ones a node calls its name. // Service.Renames is a map[TypeID]Naming — the rename target is the value, and // no ".Name" path addresses it — so a rename to nothing would slip through a -// rule written against name fields. The exemptions are addressed by path; the +// rule written against name fields. The exemptions are addressed by field; the // rule is not. func TestVerify_PresenceReachesANamingNoNameFieldOwns(t *testing.T) { t.Parallel() diff --git a/ir/irverify/rawpayloads.go b/ir/irverify/rawpayloads.go index 0376cc8fd..128af74f8 100644 --- a/ir/irverify/rawpayloads.go +++ b/ir/irverify/rawpayloads.go @@ -1,7 +1,10 @@ package irverify import ( + "bytes" "encoding/json/jsontext" + "errors" + "io" "reflect" "strconv" @@ -26,6 +29,7 @@ var ( func checkRawPayloads(*ir.Document) walkCheck { var vs []Violation var reader entryReader + var payloads payloadReader return walkCheck{ visit: func(v reflect.Value, at ir.WalkPath) bool { if v.Kind() != reflect.Map { @@ -33,9 +37,9 @@ func checkRawPayloads(*ir.Document) walkCheck { } switch v.Type() { case unmodeledType: - vs = appendEntries(vs, &reader, v, at, unmodeledEntry) + vs = appendEntries(vs, &reader, &payloads, v, at, unmodeledEntry) case rawConfigType: - vs = appendEntries(vs, &reader, v, at, rawConfigEntry) + vs = appendEntries(vs, &reader, &payloads, v, at, rawConfigEntry) default: return true // any other map may still hold one of these below it } @@ -46,15 +50,15 @@ func checkRawPayloads(*ir.Document) walkCheck { } // appendEntries appends check's verdict on every entry of the payload map m, at -// at, to vs, reading the entries through r. Both callers key on a plain string, -// so the key spells the entry's path. -func appendEntries(vs []Violation, r *entryReader, m reflect.Value, at ir.WalkPath, - check func(key string, entry reflect.Value, at ir.WalkPath) []Violation, +// at, to vs, reading the entries through r and their payloads through p. Both +// callers key on a plain string, so the key spells the entry's path. +func appendEntries(vs []Violation, r *entryReader, p *payloadReader, m reflect.Value, at ir.WalkPath, + check func(p *payloadReader, key string, entry reflect.Value, at ir.WalkPath) []Violation, ) []Violation { held := r.heldFor(m) for iter := m.MapRange(); iter.Next(); { key, entry := held.read(iter) - vs = append(vs, check(key.String(), entry, at)...) + vs = append(vs, check(p, key.String(), entry, at)...) } return vs } @@ -68,7 +72,7 @@ func appendEntries(vs []Violation, r *entryReader, m reflect.Value, at ir.WalkPa // // The three are checked independently so a doubly-broken entry names each of its // defects rather than stopping at the first. -func unmodeledEntry(key string, entry reflect.Value, at ir.WalkPath) []Violation { +func unmodeledEntry(p *payloadReader, key string, entry reflect.Value, at ir.WalkPath) []Violation { var vs []Violation if key == "" { vs = append(vs, Violation{ @@ -76,7 +80,7 @@ func unmodeledEntry(key string, entry reflect.Value, at ir.WalkPath) []Violation Message: "unmodeled map has an empty key", }) } - vs = appendRawValue(vs, entry.FieldByName("Value"), "unmodeled entry") + vs = appendRawValue(vs, p, entry.FieldByName("Value"), "unmodeled entry") switch reason := ir.UnmodeledReason(entry.FieldByName("Reason").String()); { case reason == "": @@ -96,8 +100,8 @@ func unmodeledEntry(key string, entry reflect.Value, at ir.WalkPath) []Violation // rawConfigEntry checks one entry of a RawConfig map. Unlike an Unmodeled entry // it carries no reason to check — the source declared it where the IR expects it // — so the payload is all there is to hold it to. -func rawConfigEntry(key string, entry reflect.Value, at ir.WalkPath) []Violation { - return placeEntry(appendRawValue(nil, entry, "protocol config"), at, key, "[]") +func rawConfigEntry(p *payloadReader, key string, entry reflect.Value, at ir.WalkPath) []Violation { + return placeEntry(appendRawValue(nil, p, entry, "protocol config"), at, key, "[]") } // placeEntry gives each of one entry's violations vs the entry's path: the map @@ -118,7 +122,7 @@ func placeEntry(vs []Violation, at ir.WalkPath, key, empty string) []Violation { } // appendRawValue appends a violation to vs when the payload at value is not a -// JSON value, naming its carrier in what. +// JSON value, as p reads it, naming its carrier in what. // // Reading through reflect.Value.Bytes rather than Interface() keeps Verify a // report-only oracle: the caller has matched the map's exact type, so Bytes @@ -127,9 +131,9 @@ func placeEntry(vs []Violation, at ir.WalkPath, key, empty string) []Violation { // Nil is reported with malformed and empty bytes: a nil jsontext.Value encodes // as null but decodes back as the four bytes "null", so the document stops // round-tripping to an equal one. -func appendRawValue(vs []Violation, value reflect.Value, what string) []Violation { +func appendRawValue(vs []Violation, p *payloadReader, value reflect.Value, what string) []Violation { raw := value.Bytes() - if jsontext.Value(raw).IsValid() { + if p.valid(raw) { return vs } msg := what + " value is not a JSON value, so the document cannot be marshaled" @@ -138,3 +142,28 @@ func appendRawValue(vs []Violation, value reflect.Value, what string) []Violatio } return append(vs, Violation{Code: "ir/invalid-raw-value", Message: msg}) } + +// payloadReader reads raw payloads through one decoder and one reader it resets +// for each, so a payload costs nothing once the decoder's buffer has grown to +// the largest met. jsontext.Value.IsValid borrows a decoder from a pool for +// each instead, and a race build drops a share of what a pool is handed, so +// there each payload read that way costs an allocation at random. The zero +// payloadReader is ready to use. +type payloadReader struct { + src bytes.Reader + dec jsontext.Decoder +} + +// valid reports whether raw is one JSON value, by the rules +// jsontext.Value.IsValid holds a value to by default: valid UTF-8, escapes that +// decode, no name twice in one object, and nothing after the value but +// whitespace. +func (p *payloadReader) valid(raw []byte) bool { + p.src.Reset(raw) + p.dec.Reset(&p.src) + if p.dec.SkipValue() != nil { + return false + } + _, err := p.dec.ReadToken() + return errors.Is(err, io.EOF) +} diff --git a/ir/irverify/rawpayloads_internal_test.go b/ir/irverify/rawpayloads_internal_test.go index c94640662..7082a219e 100644 --- a/ir/irverify/rawpayloads_internal_test.go +++ b/ir/irverify/rawpayloads_internal_test.go @@ -1,9 +1,13 @@ package irverify import ( + "encoding/json/jsontext" + "strconv" + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/dexpace/morphic/ir" "github.com/dexpace/morphic/ir/irtest" @@ -12,18 +16,25 @@ import ( // TestCheckRawPayloads_VisitingCostsNoAllocation holds the check's cost per // payload map at nothing once it has met the map's type: it reads entries into // what it keeps, where reflect.MapIter.Key and Value allocate a copy of each, -// two per entry of every Unmodeled map a document holds. It is not run in -// parallel: AllocsPerRun measures the process. +// two per entry of every Unmodeled map a document holds, and reads every +// payload through one decoder. A pooled decoder per payload costs one +// allocation for each a race build's pool drops, so a map of many entries +// almost always costs some. It is not run in parallel: AllocsPerRun measures +// the process. func TestCheckRawPayloads_VisitingCostsNoAllocation(t *testing.T) { kept := func(keys ...string) ir.Unmodeled { u := ir.Unmodeled{} for _, key := range keys { - u[key] = ir.UnmodeledEntry{Reason: ir.ReasonVendorExtension, Value: ir.RawValue(`{"kept":true}`)} + u[key] = ir.UnmodeledEntry{Reason: ir.ReasonVendorExtension, Value: ir.RawValue(`{"kept":[1,"a"]}`)} } return u } + many := make([]string, 0, 40) + for i := range 40 { + many = append(many, "openapi:x-"+strconv.Itoa(i)) + } doc := &ir.Document{ - Unmodeled: kept("openapi:x-a", "openapi:x-b", "openapi:x-c"), + Unmodeled: kept(many...), Types: ir.TypeRegistry{ "t/a": &ir.Model{ID: "t/a", Unmodeled: kept("openapi:x-d")}, "t/b": &ir.Model{ID: "t/b", Unmodeled: kept("openapi:x-e", "openapi:x-f")}, @@ -37,3 +48,45 @@ func TestCheckRawPayloads_VisitingCostsNoAllocation(t *testing.T) { assert.Empty(t, irtest.AllocatingVisits(doc, check)) assert.Empty(t, check.report(declarations{}), "every payload is a JSON value and every entry routable") } + +// TestPayloadReader_ReadsAsIsValidDoes holds payloadReader to the rules +// jsontext.Value.IsValid applies by default, over every string of up to five +// bytes drawn from the ones JSON's grammar turns on, an ill-formed byte among +// them, and over the shapes no short string reaches, nesting either side of the +// limit a decoder holds a value to among them. One reader reads them all, so +// what one payload leaves in it reaching the next would show too. +func TestPayloadReader_ReadsAsIsValidDoes(t *testing.T) { + t.Parallel() + const alphabet, longest = "{}[]\",:1a \xff", 5 + size, width := 1, 1 + for range longest { + width *= len(alphabet) + size += width + } + payloads, shorter := make([]string, 1, size), []string{""} + for range longest { + longer := make([]string, 0, len(alphabet)*len(shorter)) + for _, s := range shorter { + for i := range len(alphabet) { + longer = append(longer, s+alphabet[i:i+1]) + } + } + payloads, shorter = append(payloads, longer...), longer + } + payloads = append(payloads, `{"a":1,"a":2}`, `{"a":{"b":1,"b":2}}`, `"\ud800"`, `"\u00e9"`, " 1 ", "1 2", + `{"a":1}x`, `[1,]`, "01", "-0", "1e999", "\t\n1\r\n") + for _, depth := range []int{10000, 10001} { // either side of the nesting limit + payloads = append(payloads, strings.Repeat("[", depth)+strings.Repeat("]", depth), + strings.Repeat(`{"a":`, depth)+"1"+strings.Repeat("}", depth)) + } + var reader payloadReader + var valid int + for _, p := range payloads { + want := jsontext.Value(p).IsValid() + require.Equal(t, want, reader.valid([]byte(p)), "%q", p[:min(len(p), 40)]) + if want { + valid++ + } + } + assert.Positive(t, valid, "some payload is a JSON value") +} diff --git a/ir/irverify/rawpayloads_test.go b/ir/irverify/rawpayloads_test.go index 6f22ce952..6da0d9e17 100644 --- a/ir/irverify/rawpayloads_test.go +++ b/ir/irverify/rawpayloads_test.go @@ -112,8 +112,8 @@ func TestVerify_UnmodeledIsCheckedBelowTheTopLevel(t *testing.T) { // badPayloads is every shape of ir.RawValue that is not a JSON value. Empty is // the one a nil guard lets through, nil is the one that marshals but does not // come back equal, and the duplicate-name and invalid-UTF-8 entries are the -// two classes jsontext.Value.IsValid() catches that the byte-level json.Valid -// it replaced did not. +// two classes a jsontext reading refuses that the byte-level json.Valid it +// replaced did not. var badPayloads = map[string]ir.RawValue{ "malformed": ir.RawValue(`{not json}`), "empty": {}, // non-nil and zero length: the state a nil guard admits diff --git a/ir/irverify/registrychecks_internal_test.go b/ir/irverify/registrychecks_internal_test.go new file mode 100644 index 000000000..ec4ad3ebb --- /dev/null +++ b/ir/irverify/registrychecks_internal_test.go @@ -0,0 +1,132 @@ +package irverify + +import ( + "fmt" + "go/types" + "reflect" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" +) + +// registryChecks are the checks that range over a document's registries and +// tables themselves rather than through the walk. +var registryChecks = []func(*ir.Document) []Violation{ + checkRegistryKeys, checkIDs, checkMemberIDs, checkPrimIDs, checkPrimKinds, checkAuthKinds, + checkOAuthScopes, checkUnions, checkMaps, checkMapKeys, checkRequires, checkVersion, checkDocument, +} + +// registryCheckName is a registryChecks entry's function name, without its +// package path. +func registryCheckName(check func(*ir.Document) []Violation) string { + full := runtime.FuncForPC(reflect.ValueOf(check).Pointer()).Name() + _, name, _ := strings.CutLast(full, ".") + return name +} + +// TestRegistryChecks_ListsEveryRegistryCheck holds registryChecks to every +// function shaped like one, Verify aside, so a check written later is held to +// the cost the test below pins. +func TestRegistryChecks_ListsEveryRegistryCheck(t *testing.T) { + t.Parallel() + listed := map[string]bool{} + for _, check := range registryChecks { + listed[registryCheckName(check)] = true + } + shape, err := verifyDecls(t).Named("Violation") + require.NoError(t, err) + var shaped []string + for _, fn := range packageFuncs(t) { + params, results := fn.sig.Params(), fn.sig.Results() + if fn.name == "Verify" || fn.sig.Recv() != nil || params.Len() != 1 || results.Len() != 1 { + continue + } + doc, isPtr := params.At(0).Type().(*types.Pointer) + list, isSlice := results.At(0).Type().(*types.Slice) + if isPtr && types.TypeString(doc.Elem(), nil) == irPath+".Document" && + isSlice && types.Identical(list.Elem(), shape) { + shaped = append(shaped, fn.name) + } + } + for _, name := range shaped { + assert.True(t, listed[name], "%s is shaped like a registry check but is not in registryChecks", name) + } + assert.Len(t, listed, len(shaped), "registryChecks lists each check once, and only functions shaped like one") +} + +// TestRegistryChecks_ACleanEntryCostsNothing holds every registry check to a +// cost that does not grow with the entries it ranges over when none has +// anything to report: no path is spelled for an entry before there is a +// violation to place there, and a model's or a flow's collection is read into +// one map the check empties for each rather than one of its own. Twice the +// entries must cost no more, past the index of 100 from which strconv builds +// the string an index spells. It is not run in parallel: AllocsPerRun measures +// the process. +func TestRegistryChecks_ACleanEntryCostsNothing(t *testing.T) { + few, many := cleanRegistry(100), cleanRegistry(200) + checks := map[string]func(*ir.Document) []Violation{ + "declarationViolations": func(doc *ir.Document) []Violation { return declarationViolations(doc.IDSpaces) }, + } + for _, check := range registryChecks { + checks[registryCheckName(check)] = check + } + for name, check := range checks { + require.Empty(t, check(many), "%s: the fixture is clean, or what is measured is a report", name) + assert.Equal(t, + testing.AllocsPerRun(10, func() { _ = check(few) }), + testing.AllocsPerRun(10, func() { _ = check(many) }), + "%s: twice the entries cost more", name) + } +} + +// cleanRegistry returns a sound document holding n of each entry a registry +// check ranges over, beside a primitive of each of a few kinds: a model with +// more properties and Requires entries than a map holds unallocated, a union, a +// map, an enum whose members sit beneath a path longer than a string built on +// the stack holds, an OAuth scheme with more scopes than a map holds +// unallocated, a declared namespace and a source. +func cleanRegistry(n int) *ir.Document { + doc := &ir.Document{ + IRVersion: ir.IRVersion, + Types: ir.TypeRegistry{}, + Auth: map[ir.AuthID]ir.AuthScheme{}, + IDSpaces: map[string][]string{}, + Services: []ir.Service{{ID: "s/x"}}, + } + for _, kind := range []ir.PrimKind{ir.PrimString, ir.PrimInt32, ir.PrimBool} { + doc.Types[ir.PrimTypeID(kind)] = &ir.Primitive{ID: ir.PrimTypeID(kind), Prim: kind} + } + for i := range n { + suffix := fmt.Sprintf("%04d", i) + model := &ir.Model{ID: ir.TypeID("t/x/M" + suffix)} + for j := range 12 { + model.Properties = append(model.Properties, ir.Property{WireName: fmt.Sprintf("own%d", j)}) + model.Requires = append(model.Requires, fmt.Sprintf("inherited%d", j)) + } + enumPath := "components/schemas/Enumeration" + suffix + enum := &ir.Enum{ID: ir.TypeID("t/x/" + enumPath), Members: []ir.EnumMember{ + {ID: ir.EnumMemberID("e/x/" + enumPath + "/a")}, {ID: ir.EnumMemberID("e/x/" + enumPath + "/b")}, + }} + for _, td := range []ir.TypeDef{ + model, enum, + &ir.Union{ID: ir.TypeID("t/x/U" + suffix), Variants: []ir.Variant{{}}}, + &ir.MapT{ID: ir.TypeID("t/x/Map" + suffix)}, + } { + doc.Types[td.Common().ID] = td + } + flow := ir.OAuthFlow{Kind: "client_credentials"} + for j := range 12 { + flow.Scopes = append(flow.Scopes, ir.OAuthScope{Name: fmt.Sprintf("scope%d", j)}) + } + scheme := ir.AuthID("auth/x/S" + suffix) + doc.Auth[scheme] = ir.AuthScheme{ID: scheme, Kind: ir.AuthKindOAuth2, Flows: []ir.OAuthFlow{flow}} + doc.IDSpaces[ir.IDKindType] = append(doc.IDSpaces[ir.IDKindType], "s"+suffix) + doc.Sources = append(doc.Sources, ir.SourceInfo{Format: "openapi@3.1", Hash: strings.Repeat("0", sha256HexLen)}) + } + return doc +} diff --git a/ir/irverify/requires.go b/ir/irverify/requires.go index 2dda23cea..1c54a2545 100644 --- a/ir/irverify/requires.go +++ b/ir/irverify/requires.go @@ -12,30 +12,33 @@ import ( // a repeat admits nothing more. A repeat is reported at its later entry. func checkRequires(doc *ir.Document) []Violation { var vs []Violation + // Every model is read through these two, so a model costs no map of its + // own however many properties and entries it has. + var owned reusedMap[string, bool] + var listed reusedMap[string, int] for id, td := range doc.Types { m, isModel := td.(*ir.Model) if !isModel || m == nil || len(m.Requires) == 0 { continue // a nil entry is checkRegistryKeys's to report } - own := make(map[string]bool, len(m.Properties)) + own := owned.emptied() for _, p := range m.Properties { own[p.EffectiveWireName()] = true } - first := make(map[string]int, len(m.Requires)) + first := listed.emptied() for i, name := range m.Requires { - at := "types[" + string(id) + "].Requires[" + strconv.Itoa(i) + "]" if own[name] { vs = append(vs, Violation{ Code: "ir/requires-own-property", Message: "requires " + strconv.Quote(name) + ", which an own property declares and holds as Required", - Path: at, + Path: requiresPath(id, i), }) } if earlier, repeated := first[name]; repeated { vs = append(vs, Violation{ Code: "ir/requires-duplicate", Message: "requires " + strconv.Quote(name) + " here and at index " + strconv.Itoa(earlier), - Path: at, + Path: requiresPath(id, i), }) continue } @@ -44,3 +47,9 @@ func checkRequires(doc *ir.Document) []Violation { } return vs } + +// requiresPath spells where entry i of the Requires of the model id sits, for +// a violation to be placed there. +func requiresPath(id ir.TypeID, i int) string { + return typePath(id) + ".Requires[" + strconv.Itoa(i) + "]" +} diff --git a/ir/irverify/requires_test.go b/ir/irverify/requires_test.go index 0a2fe6a9a..c4c908e5e 100644 --- a/ir/irverify/requires_test.go +++ b/ir/irverify/requires_test.go @@ -146,3 +146,23 @@ func TestVerify_RequiresReadsTheEffectiveWireName(t *testing.T) { }) } } + +// TestVerify_RequiresIsHeldModelByModel pins that a model's Requires answers to +// its own properties and entries alone: a name another model declares or also +// requires is no defect there. Each of the two models declares what the other +// requires and both require one name, so whichever the check meets first, what +// it read of that model must not linger into the other. +func TestVerify_RequiresIsHeldModelByModel(t *testing.T) { + t.Parallel() + str := &ir.Primitive{ID: ir.PrimTypeID(ir.PrimString), Prim: ir.PrimString} + model := func(id ir.TypeID, own string, requires ...string) *ir.Model { + return &ir.Model{ID: id, Name: named(own), Requires: requires, Properties: []ir.Property{{ + ID: ir.PropID("p" + string(id[1:]) + "/" + own), Name: named(own), WireName: own, + Type: ir.TypeRef{Target: str.ID}, + }}} + } + a, b := model("t/x/A", "a", "b", "shared"), model("t/x/B", "b", "a", "shared") + doc := &ir.Document{IRVersion: ir.IRVersion, Types: ir.TypeRegistry{str.ID: str, a.ID: a, b.ID: b}} + + assert.Empty(t, verifyDeclared(doc)) +} diff --git a/ir/irverify/reuse.go b/ir/irverify/reuse.go new file mode 100644 index 000000000..796788176 --- /dev/null +++ b/ir/irverify/reuse.go @@ -0,0 +1,26 @@ +package irverify + +// reuseLimit is how many entries a reusedMap may have held for it to be +// emptied in place rather than replaced. Emptying a map costs in proportion to +// the most it has ever held, as Go never shrinks one, so a map kept after one +// huge collection would tax every collection read through it after. +const reuseLimit = 1024 + +// reusedMap is a map a check reads one collection after another through, so a +// collection costs no map of its own. The zero reusedMap is ready to use; pass +// keeps the same type for its checks. +type reusedMap[K comparable, V any] struct { + m map[K]V +} + +// emptied returns the map empty for the next collection: the one r holds, +// cleared, while the last collection left it at most reuseLimit entries, and a +// fresh one past that, which r holds from then on. +func (r *reusedMap[K, V]) emptied() map[K]V { + if r.m == nil || len(r.m) > reuseLimit { + r.m = make(map[K]V) + return r.m + } + clear(r.m) + return r.m +} diff --git a/ir/irverify/reuse_internal_test.go b/ir/irverify/reuse_internal_test.go new file mode 100644 index 000000000..339346f91 --- /dev/null +++ b/ir/irverify/reuse_internal_test.go @@ -0,0 +1,51 @@ +package irverify + +import ( + "go/types" + "reflect" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestReusedMap_EmptiesInPlaceUpToTheLimit holds emptied to reusing the map it +// holds while the last collection left it at most reuseLimit entries, and to +// handing a fresh one past that: Go never shrinks a map, so emptying one that +// held a huge collection costs in proportion to it, at every collection after. +func TestReusedMap_EmptiesInPlaceUpToTheLimit(t *testing.T) { + t.Parallel() + var r reusedMap[string, int] + first := r.emptied() + require.NotNil(t, first, "the zero reusedMap hands a map to write to") + fill := func(m map[string]int, n int) { + for i := range n { + m[strconv.Itoa(i)] = i + } + } + + fill(first, reuseLimit) + atLimit := r.emptied() + assert.Empty(t, atLimit) + assert.Equal(t, reflect.ValueOf(first).UnsafePointer(), reflect.ValueOf(atLimit).UnsafePointer(), + "a map left at the limit is emptied in place") + + fill(atLimit, reuseLimit+1) + past := r.emptied() + assert.Empty(t, past) + assert.NotEqual(t, reflect.ValueOf(atLimit).UnsafePointer(), reflect.ValueOf(past).UnsafePointer(), + "a map left past the limit is replaced") + assert.Equal(t, reflect.ValueOf(past).UnsafePointer(), reflect.ValueOf(r.emptied()).UnsafePointer(), + "the fresh map is the one reused from then on") +} + +// TestReusedMaps_AreEmptiedThroughReusedMapAlone holds every map the package +// empties for another collection to reusedMap, whose limit bounds what +// emptying costs: a bare clear of a map kept across collections makes one huge +// collection tax every later one. +func TestReusedMaps_AreEmptiedThroughReusedMapAlone(t *testing.T) { + t.Parallel() + clearing := verifyDecls(t).FuncsUsing(types.Universe.Lookup("clear")) + assert.Equal(t, []string{"emptied"}, clearing) +} diff --git a/ir/irverify/scopes.go b/ir/irverify/scopes.go index 535955faa..bc9c0b70a 100644 --- a/ir/irverify/scopes.go +++ b/ir/irverify/scopes.go @@ -15,9 +15,12 @@ import ( // is the compiler's. It is reported at the later entry. func checkOAuthScopes(doc *ir.Document) []Violation { var vs []Violation + // Every flow is read through this one, so a flow costs no map of its own + // however many scopes it offers. + var offered reusedMap[string, bool] for id, scheme := range doc.Auth { for i, flow := range scheme.Flows { - seen := make(map[string]bool, len(flow.Scopes)) + seen := offered.emptied() for j, scope := range flow.Scopes { if seen[scope.Name] { vs = append(vs, Violation{ diff --git a/ir/irverify/stringlists.go b/ir/irverify/stringlists.go index 9b0030d53..037e7d300 100644 --- a/ir/irverify/stringlists.go +++ b/ir/irverify/stringlists.go @@ -98,6 +98,9 @@ type heldList struct { func checkStringLists(*ir.Document) walkCheck { var vs []Violation fields := map[reflect.Type][]heldList{} + // Every held list is read through this one, so a list costs no map of its + // own however long it is. + var entries reusedMap[string, int] return walkCheck{ visit: func(v reflect.Value, at ir.WalkPath) bool { if v.Kind() != reflect.Struct { @@ -109,7 +112,7 @@ func checkStringLists(*ir.Document) walkCheck { fields[v.Type()] = byType } for _, f := range byType { - vs = appendListViolations(vs, v.Field(f.index), at, f) + vs = appendListViolations(vs, v.Field(f.index), at, f, &entries) } return true }, @@ -134,9 +137,12 @@ func heldListsOf(t reflect.Type) []heldList { } // appendListViolations reports each entry of the held list f that breaks its -// rule, at the entry itself; owner is where the struct holding it sits. -func appendListViolations(vs []Violation, list reflect.Value, owner ir.WalkPath, f heldList) []Violation { - first := map[string]int{} +// rule, at the entry itself; owner is where the struct holding it sits, and +// entries is what the list is read through. +func appendListViolations(vs []Violation, list reflect.Value, owner ir.WalkPath, f heldList, + entries *reusedMap[string, int], +) []Violation { + first := entries.emptied() for i := range list.Len() { entry := list.Index(i).String() switch earlier, repeated := first[entry]; { diff --git a/ir/irverify/stringlists_internal_test.go b/ir/irverify/stringlists_internal_test.go index 00e15f1d3..580edb4d4 100644 --- a/ir/irverify/stringlists_internal_test.go +++ b/ir/irverify/stringlists_internal_test.go @@ -3,10 +3,14 @@ package irverify import ( "go/types" "reflect" + "strconv" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irtest" ) // TestStringLists_AreAllClassified fails when the ir package declares a @@ -44,3 +48,25 @@ func TestHeldListsOf_HoldsAnUnlistedFieldAsAPath(t *testing.T) { assert.Equal(t, []heldList{{index: 1, name: "Segments", rule: listPath}}, heldListsOf(reflect.TypeFor[unlisted]())) } + +// TestCheckStringLists_VisitingCostsNoAllocation holds the check's cost for a +// list it has nothing to report on at nothing, however long: every list is +// read into the one map the check keeps, emptied for each, where a map of its +// own allocates once a list holds more than eight entries. A path and a set, +// on two nodes, share entries, which only a map left unemptied would report. +// It is not run in parallel: AllocsPerRun measures the process. +func TestCheckStringLists_VisitingCostsNoAllocation(t *testing.T) { + entries := make([]string, 0, 12) + for i := range 12 { + entries = append(entries, "entry"+strconv.Itoa(i)) + } + doc := &ir.Document{ + Types: ir.TypeRegistry{"t/x/M": &ir.Model{ID: "t/x/M", Namespace: entries}}, + Auth: map[ir.AuthID]ir.AuthScheme{"auth/x/S": {ID: "auth/x/S", Flows: []ir.OAuthFlow{ + {TokenEndpointAuthMethods: entries}, + }}}, + } + check := checkStringLists(doc) + assert.Empty(t, irtest.AllocatingVisits(doc, check)) + assert.Empty(t, check.report(declarations{}), "no entry is blank, and no set entry repeats within its list") +} diff --git a/ir/irverify/unions.go b/ir/irverify/unions.go index e1871f90d..425a2ef14 100644 --- a/ir/irverify/unions.go +++ b/ir/irverify/unions.go @@ -24,19 +24,18 @@ func checkUnions(doc *ir.Document) []Violation { if !isUnion { continue } - path := "types[" + string(id) + "]" if len(u.Variants) == 0 { vs = append(vs, Violation{ Code: "ir/union-no-variants", Message: "union declares no variants, so no value inhabits it", - Path: path, + Path: typePath(id), }) } if u.Discriminator != nil && !u.WireTagged { vs = append(vs, Violation{ Code: "ir/union-discriminator-untagged", Message: "union carries a discriminator but is not WireTagged, a tag mode ir-design §4.4 does not define", - Path: path, + Path: typePath(id), }) } } diff --git a/ir/irverify/utf8_test.go b/ir/irverify/utf8_test.go index 9cdb9c4a1..6ed584919 100644 --- a/ir/irverify/utf8_test.go +++ b/ir/irverify/utf8_test.go @@ -220,8 +220,8 @@ func TestVerify_InvalidUTF8IsReported(t *testing.T) { // TestVerify_RawPayloadUTF8IsNotDoublyReported pins the division of labour // checkUTF8's own doc states: the walk skips byte sequences, so an ill-formed // byte inside an Unmodeled or RawConfig payload is never reached by it. It is -// ir/invalid-raw-value's to report — jsontext.Value.IsValid already rejects -// invalid UTF-8 inside a payload — and reported once, not twice. +// ir/invalid-raw-value's to report — the payload's jsontext reading already +// rejects invalid UTF-8 inside it — and reported once, not twice. func TestVerify_RawPayloadUTF8IsNotDoublyReported(t *testing.T) { t.Parallel() doc := docWithUnmodeled(ir.Unmodeled{ diff --git a/ir/pathlog.go b/ir/pathlog.go new file mode 100644 index 000000000..e4054f7bf --- /dev/null +++ b/ir/pathlog.go @@ -0,0 +1,93 @@ +package ir + +import "iter" + +// PathLog keeps the paths a walk reaches values by, in the order recorded, +// without spelling any. Each is kept as the steps it shares with the path +// recorded before it and the steps after those, so recording one copies the +// steps that differ and allocates nothing once the log has grown, where keeping +// a path's String costs a string. A path is spelled only when [PathLog.All] +// replays it and its String is asked for. The zero PathLog is ready to record +// into; one is not safe for concurrent use. +type PathLog struct { + // last holds the steps of the path recorded last, whole. + last []pathStep + // tails holds each path's steps past the ones it shares with the path + // recorded before it, back to back. + tails []pathStep + entries []loggedPath +} + +// loggedPath is one path a PathLog holds: the walk's root, how many steps it +// shares with the path recorded before it, and where its own steps end in +// tails. A step is shared whatever root it was reached from, since a path +// spells its root and then each step. +type loggedPath struct { + root string + shared int + end int +} + +// Record keeps at, the path to the value a visitor is being shown, at the next +// position of the log: the position Len reports before the call. The zero +// WalkPath is kept as the empty path it renders. +func (l *PathLog) Record(at WalkPath) { + var root string + var steps []pathStep + if at.walk != nil { + root, steps = at.walk.root, at.walk.steps[:at.depth] + } + shared := sharedSteps(l.last, steps) + l.tails = appendGrowing(l.tails, steps[shared:]...) + l.entries = appendGrowing(l.entries, loggedPath{root: root, shared: shared, end: len(l.tails)}) + l.last = append(l.last[:shared], steps[shared:]...) +} + +// Len returns how many paths the log holds. +func (l *PathLog) Len() int { + return len(l.entries) +} + +// All replays the log: it yields each path in the order recorded, with its +// position, as a WalkPath that is valid until the next is yielded, so a caller +// spells only the paths it keeps. The log's own storage is not copied, but +// replaying a path costs a copy of its steps. +func (l *PathLog) All() iter.Seq2[int, WalkPath] { + return func(yield func(int, WalkPath) bool) { + replay := &valueWalk{} + start := 0 + for i, e := range l.entries { + replay.root = e.root + replay.steps = append(replay.steps[:e.shared], l.tails[start:e.end]...) + start = e.end + if !yield(i, WalkPath{walk: replay, depth: len(replay.steps)}) { + return + } + } + } +} + +// sharedSteps returns how many steps a and b open with alike. +func sharedSteps(a, b []pathStep) int { + n := min(len(a), len(b)) + for i := range n { + if a[i] != b[i] { + return i + } + } + return n +} + +// appendGrowing appends vs to s, growing s's capacity five halves whenever it +// fills. append grows a large slice by a quarter at a time, so a slice built +// one entry at a time is copied several times over, where this copies each +// entry less than once on average. slices.Grow grows about as far, but a race +// build makes two allocations for each of its growths. +func appendGrowing[T any](s []T, vs ...T) []T { + if need := len(s) + len(vs); need > cap(s) { + grown := make([]T, len(s), max(need, cap(s)*5/2)) + copy(grown, s) + s = grown + } + return append(s, vs...) +} diff --git a/ir/pathlog_test.go b/ir/pathlog_test.go new file mode 100644 index 000000000..3096e56c8 --- /dev/null +++ b/ir/pathlog_test.go @@ -0,0 +1,205 @@ +package ir_test + +import ( + "math/bits" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irtest" +) + +// loggedDoc reaches every kind of step a path takes: fields, a promoted one +// among them, elements, map keys and map values, nested and side by side. +func loggedDoc() *ir.Document { + doc := declaringDoc() + m := &ir.Model{TypeCommon: populatedTypeCommon("t/x/P"), Properties: []ir.Property{populatedProperty(), {ID: "p/x/P/b"}}} + doc.Types[m.ID] = m + doc.Unmodeled = populatedUnmodeled() + doc.Services[0].Renames = map[ir.TypeID]ir.Naming{"t/x/M": populatedNaming(), "t/x/P": {Source: "p"}} + return doc +} + +// recorded is what a walk showed a visitor at one value: the path's spelling, +// the field it ends in, and whether it ends in a map key. +type recorded struct { + path string + owner reflect.Type + field string + ok bool + mapKey bool +} + +// recordingOf returns what at shows of itself. +func recordingOf(at ir.WalkPath) recorded { + owner, field, ok := at.Field() + return recorded{path: at.String(), owner: owner, field: field, ok: ok, mapKey: at.IsMapKey()} +} + +// TestPathLog_ReplaysEachPathAsTheWalkShowedIt holds a replayed path to the one +// the walk showed when it was recorded, at every value of a document whose +// paths share prefixes of every length and part ways at every kind of step, +// spelled alike and reading alike through Field and IsMapKey. +func TestPathLog_ReplaysEachPathAsTheWalkShowedIt(t *testing.T) { + t.Parallel() + var log ir.PathLog + var shown []recorded + ir.WalkValues(loggedDoc(), ir.DocumentPath, ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + shown = append(shown, recordingOf(at)) + log.Record(at) + return true + })) + require.Equal(t, len(shown), log.Len()) + require.Contains(t, shown, recorded{path: "doc.Unmodeled[openapi:a-ext].key", mapKey: true}, "a map key is reached") + + var replayed []recorded + for i, at := range log.All() { + require.Len(t, replayed, i, "positions run in the order recorded") + replayed = append(replayed, recordingOf(at)) + } + assert.Equal(t, shown, replayed) +} + +// TestPathLog_KeepsEachWalksRoot holds a log fed by two walks to the root each +// path was reached from, though the second walk's path shares its steps with +// the first's. +func TestPathLog_KeepsEachWalksRoot(t *testing.T) { + t.Parallel() + var log ir.PathLog + var shown []string + for _, root := range []string{"a", "b"} { + ir.WalkValues(&ir.Document{Name: "x"}, root, ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + if at.String() == root+".Name" { + shown = append(shown, at.String()) + log.Record(at) + } + return true + })) + } + var replayed []string + for _, at := range log.All() { + replayed = append(replayed, at.String()) + } + assert.Equal(t, []string{"a.Name", "b.Name"}, shown) + assert.Equal(t, shown, replayed) +} + +// TestPathLog_KeepsTheTypeEachFieldIsSpelledOn holds a log fed by walks of two +// types to the struct each step names its field on: the second path ends in +// the field at the same position of its struct as the first path's does, so +// only the struct's type tells the two steps apart. +func TestPathLog_KeepsTheTypeEachFieldIsSpelledOn(t *testing.T) { + t.Parallel() + version, _ := reflect.TypeFor[ir.Document]().FieldByName("IRVersion") + id, _ := reflect.TypeFor[ir.Channel]().FieldByName("ID") + require.Equal(t, version.Index, id.Index, "the two fields share a position, or the owner decides nothing") + + var log ir.PathLog + for _, walk := range []struct { + root any + want string + }{{&ir.Document{IRVersion: "x"}, "a.IRVersion"}, {&ir.Channel{ID: "c/x"}, "a.ID"}} { + ir.WalkValues(walk.root, "a", ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + if at.String() == walk.want { + log.Record(at) + } + return true + })) + } + var replayed []string + for _, at := range log.All() { + replayed = append(replayed, at.String()) + } + assert.Equal(t, []string{"a.IRVersion", "a.ID"}, replayed) +} + +// TestPathLog_KeepsTheZeroPathAsEmpty pins the zero WalkPath, which no walk is +// behind: it is kept as the empty path it renders, beside a path a walk showed. +func TestPathLog_KeepsTheZeroPathAsEmpty(t *testing.T) { + t.Parallel() + var log ir.PathLog + log.Record(ir.WalkPath{}) + ir.WalkValues(&ir.Document{Name: "x"}, ir.DocumentPath, ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + if at.String() == "doc.Name" { + log.Record(at) + } + return true + })) + var replayed []string + for _, at := range log.All() { + replayed = append(replayed, at.String()) + } + assert.Equal(t, []string{"", "doc.Name"}, replayed) +} + +// TestPathLog_StopsWhereTheCallerStops holds a replay to yielding nothing after +// the caller breaks off. +func TestPathLog_StopsWhereTheCallerStops(t *testing.T) { + t.Parallel() + var log ir.PathLog + ir.WalkValues(&ir.Document{Name: "x"}, ir.DocumentPath, ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + log.Record(at) + return true + })) + require.Greater(t, log.Len(), 1) + var seen []int + for i := range log.All() { + seen = append(seen, i) + break + } + assert.Equal(t, []int{0}, seen) +} + +// TestPathLog_RecordingCostsNoAllocation holds keeping a path at nothing once +// the log has grown, where keeping its String costs a string per value. Each +// visit is handed the walk's own path, so spelling one would count. It is not +// run in parallel: AllocsPerRun measures the process. +func TestPathLog_RecordingCostsNoAllocation(t *testing.T) { + var log ir.PathLog + recording := ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + log.Record(at) + return true + }) + assert.Empty(t, irtest.AllocatingVisits(loggedDoc(), recording)) + assert.Positive(t, log.Len(), "the visits recorded what they were shown") +} + +// TestPathLog_KeepsASharedStepOnce holds the log to storing the steps a path +// shares with the one recorded before it once: a walk's paths share all but +// their last step or two, so storing each whole would hold several steps per +// path where this holds about one. +func TestPathLog_KeepsASharedStepOnce(t *testing.T) { + t.Parallel() + var log ir.PathLog + ir.WalkValues(listOf(1000), ir.DocumentPath, ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + log.Record(at) + return true + })) + require.Greater(t, log.Len(), 1000, "the walk reaches every element, or nothing is shared") + assert.Less(t, log.StoredSteps(), 2*log.Len(), "%d steps for %d paths", log.StoredSteps(), log.Len()) +} + +// TestPathLog_GrowsGeometrically holds the log's growth to a factor well past +// append's: append grows a large slice by a quarter at a time, so a log built +// one path at a time would be copied several times over, which on a +// golden-spec document cost tens of megabytes. Recording n paths may grow each +// of its slices about log2(n) times. It is not run in parallel: AllocsPerRun +// measures the process. +func TestPathLog_GrowsGeometrically(t *testing.T) { + const paths = 1 << 14 + doc := listOf(paths) + recording := testing.AllocsPerRun(3, func() { + var log ir.PathLog + ir.WalkValues(doc, ir.DocumentPath, ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + log.Record(at) + return true + })) + }) + walking := testing.AllocsPerRun(3, func() { + ir.WalkValues(doc, ir.DocumentPath, ir.VisitFunc(func(reflect.Value, ir.WalkPath) bool { return true })) + }) + assert.Less(t, recording-walking, float64(3*(bits.Len(paths)+2)), "%v allocations to record", recording-walking) +} diff --git a/ir/references.go b/ir/references.go index 733a1459f..4d3d8b9a7 100644 --- a/ir/references.go +++ b/ir/references.go @@ -2,6 +2,7 @@ package ir import ( "reflect" + "slices" "strings" ) @@ -40,8 +41,10 @@ type IDReferences struct { // read through ids. declared map[reflect.Type]map[string]bool ids IDReader - // sites holds the references that may not resolve, in walk order. + // sites holds the references that may not resolve, in walk order, each + // with its path unspelled at the same position of paths. sites []IDReference + paths PathLog } // NewIDReferences returns the IDReferences of a walk of doc, holding the IDs @@ -73,8 +76,8 @@ func NewIDReferences(doc *Document) *IDReferences { // // A reference resolving as the walk reaches it, in a map-keyed registry or to // an identity already declared, is not kept: declarations only accumulate. Any -// other is kept, path and all, for [IDReferences.Unresolved], since a declared -// class's registry is complete only once the walk is over. A node's own ID is +// other is kept for [IDReferences.Unresolved], with its path unspelled, since a +// declared class's registry is complete only once the walk is over. A node's own ID is // reached just after the node declares it. An empty ID is no reference: some // positions spell "none" with one (Discriminator.Default). func (r *IDReferences) Visit(v reflect.Value, at WalkPath) bool { @@ -125,26 +128,39 @@ func (r *IDReferences) refer(v reflect.Value, at WalkPath) { if ids[id] { return } - r.sites = append(r.sites, IDReference{Class: t, ID: id, Path: at.String()}) + r.sites = append(r.sites, IDReference{Class: t, ID: id}) + r.paths.Record(at) } // Unresolved returns, in walk order, the references the walk reached that -// resolve in no registry. +// resolve in no registry, each with its path. Only these have their paths +// spelled, so a kept reference that resolves once the walk is over costs none. // // A declared class's registry is complete only when the walk read every // declaration. A walk cut short, as truncated says, would report a reference to // a node past the cap as dangling, so nothing is claimed for such a class then; // a map-keyed registry is read off the document and still answers. func (r *IDReferences) Unresolved(truncated bool) []IDReference { + if !slices.ContainsFunc(r.sites, func(s IDReference) bool { return r.dangles(s, truncated) }) { + return nil + } var out []IDReference - for _, s := range r.sites { - if _, keyed := r.keyed[s.Class]; keyed || (!truncated && !r.declared[s.Class][s.ID]) { + for i, at := range r.paths.All() { + if s := r.sites[i]; r.dangles(s, truncated) { + s.Path = at.String() out = append(out, s) } } return out } +// dangles reports whether the kept reference s resolves in no registry, as +// Unresolved says, truncated saying whether the walk was cut short. +func (r *IDReferences) dangles(s IDReference, truncated bool) bool { + _, keyed := r.keyed[s.Class] + return keyed || (!truncated && !r.declared[s.Class][s.ID]) +} + // Declares reports whether a node the walk reached declares id as its identity // of class, which is what a reference of a class no Document map keys resolves // against. PropID is answered too, so a check holding one to a declared property diff --git a/ir/references_test.go b/ir/references_test.go index a1f487212..ef29470cf 100644 --- a/ir/references_test.go +++ b/ir/references_test.go @@ -394,3 +394,21 @@ func TestRefNoun_NamesTheReferenceClass(t *testing.T) { assert.Equal(t, want, ir.RefNoun(idType)) } } + +// TestIDReferences_KeepingCostsNoAllocation holds a kept reference's cost at +// nothing: one to an identity the walk has yet to declare keeps its path +// unspelled, and spells it only if it still resolves nowhere once the walk is +// over, where keeping its String cost a string per forward reference. It is not +// run in parallel: AllocsPerRun measures the process. +func TestIDReferences_KeepingCostsNoAllocation(t *testing.T) { + doc := declaringDoc() + group := &doc.Services[0].Groups[0] + group.Operations = append([]ir.Operation{{ID: "op/y", OverloadOf: new(ir.OpID("op/x"))}}, group.Operations...) + refs := ir.NewIDReferences(doc) + + assert.Empty(t, irtest.AllocatingVisits(doc, refs), "a forward reference keeps no string") + assert.Contains(t, refIDs(refs.Kept()), "op/x", "the forward reference was kept, once per visit made") + assert.Empty(t, refs.Unresolved(false), "and resolved once the walk was over") + assert.Zero(t, testing.AllocsPerRun(5, func() { _ = refs.Unresolved(false) }), + "settling a kept reference that resolved spells nothing") +} diff --git a/ir/walk.go b/ir/walk.go index 987f80dcc..58eebcedf 100644 --- a/ir/walk.go +++ b/ir/walk.go @@ -130,8 +130,9 @@ type visitorSet uint64 // // A WalkPath reads the walk's own state, and nothing renders it until String or // AppendTo is called, so a visit that never reads its path pays nothing for it. -// It is valid only until the call it was passed to returns; a visitor that keeps -// a path keeps its String. The zero WalkPath is empty. +// It is valid until the call it was passed to returns, or, replayed by a +// [PathLog], until the next is yielded; a visitor keeps its String, or records +// it in a PathLog. The zero WalkPath is empty. type WalkPath struct { walk *valueWalk // depth is how many of the walk's steps lead to the value. @@ -160,6 +161,39 @@ func (p WalkPath) AppendTo(b []byte) []byte { return b } +// Field reports the struct field the path ends in, as String spells it: the +// struct type the field is spelled on and the field's name, or false when the +// path ends in an element, a map entry or the root. A field reached through an +// embedded struct, promoted or shadowed, is spelled on the outermost struct +// embedding it, so a Primitive's Name is reported as Primitive's, and an +// embedded struct, which the path does not spell, reports what reached the +// struct embedding it. +func (p WalkPath) Field() (owner reflect.Type, name string, ok bool) { + if p.walk == nil { + return nil, "", false + } + steps := p.walk.steps[:p.depth] + last := len(steps) - 1 + for last >= 0 && steps[last].embedded() { + last-- + } + if last < 0 || steps[last].kind != stepField { + return nil, "", false + } + owner = steps[last].owner + for i := last - 1; i >= 0 && steps[i].embedded(); i-- { + owner = steps[i].owner + } + return owner, steps[last].owner.Field(steps[last].index).Name, true +} + +// IsMapKey reports whether the path ends in a map entry's key, which String +// spells with [MapKeySuffix], so a caller can tell a key from the entry's value +// without spelling the path. +func (p WalkPath) IsMapKey() bool { + return p.walk != nil && p.depth > 0 && p.walk.steps[p.depth-1].kind == stepMapKey +} + // stepKind is how a value is reached from its parent. type stepKind uint8 @@ -199,6 +233,12 @@ func (s pathStep) appendTo(b []byte) []byte { return b } +// embedded reports whether s is a step into an embedded struct, which a path +// does not spell. +func (s pathStep) embedded() bool { + return s.kind == stepField && s.owner.Field(s.index).Anonymous +} + // refKey identifies a reference the way the JSON encoder does, so that what a // walk calls a cycle is what the encoder refuses. A pointer to a struct and a // pointer to its first field share an address and differ in type; a prefix of a diff --git a/ir/walk_path_test.go b/ir/walk_path_test.go index ef11caf09..2379dfa36 100644 --- a/ir/walk_path_test.go +++ b/ir/walk_path_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irtest" ) // listOf returns a document whose one example is a list of n string values, so @@ -74,6 +75,112 @@ func TestWalkPath_ZeroValueIsEmpty(t *testing.T) { assert.Equal(t, []byte("kept"), p.AppendTo([]byte("kept"))) } +// Roots that embed a primitive: Outer through Middle, so a primitive's Name is +// promoted through three embedded structs, and Shadowed beside a Name of its +// own, which hides the promoted one from Go but not from a walk. +type ( + Outer struct{ Middle } + Middle struct{ ir.Primitive } + Shadowed struct { + ir.Primitive + Name ir.Naming + } +) + +// fieldAt is what WalkPath.Field reports at one path. +type fieldAt struct { + owner reflect.Type + name string + ok bool +} + +// fieldsOf walks root and returns what WalkPath.Field reports at each path the +// walk reaches, the reports at one path in walk order. +func fieldsOf(root any) map[string][]fieldAt { + got := map[string][]fieldAt{} + ir.WalkValues(root, "doc", ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + owner, name, ok := at.Field() + got[at.String()] = append(got[at.String()], fieldAt{owner, name, ok}) + return true + })) + return got +} + +// TestWalkPath_FieldIsTheFieldThePathEndsIn holds Field to the last segment +// String spells: the field and the struct it is spelled on, a promoted field on +// the struct embedding it however deep, and none for a path ending in an +// element, a map entry, the root or an embedded struct, which spells nothing of +// its own. +func TestWalkPath_FieldIsTheFieldThePathEndsIn(t *testing.T) { + t.Parallel() + document, model := reflect.TypeFor[ir.Document](), reflect.TypeFor[ir.Model]() + primitive, naming := reflect.TypeFor[ir.Primitive](), reflect.TypeFor[ir.Naming]() + doc := fieldsOf(&ir.Document{Types: ir.TypeRegistry{ + "t/p": &ir.Primitive{ID: "t/p"}, + "t/m": &ir.Model{ID: "t/m", Deprecation: &ir.Deprecation{}, Properties: []ir.Property{{ID: "p/m/a"}}}, + }}) + none := fieldAt{} + for _, tc := range []struct { + path string + want []fieldAt + }{ + {"doc", []fieldAt{none, none}}, + {"doc.Types", []fieldAt{{document, "Types", true}}}, + {"doc.Types[t/p].key", []fieldAt{none}}, + {"doc.Types[t/p]", []fieldAt{none, none, none, none}}, // interface, pointer, Primitive, TypeCommon + {"doc.Types[t/p].Name", []fieldAt{{primitive, "Name", true}}}, + {"doc.Types[t/p].Name.Source", []fieldAt{{naming, "Source", true}}}, + {"doc.Types[t/p].Prim", []fieldAt{{primitive, "Prim", true}}}, + {"doc.Types[t/m].Deprecation", []fieldAt{{model, "Deprecation", true}, {model, "Deprecation", true}}}, + {"doc.Types[t/m].Properties", []fieldAt{{model, "Properties", true}}}, + {"doc.Types[t/m].Properties[0]", []fieldAt{none}}, + {"doc.Types[t/m].Properties[0].Name", []fieldAt{{reflect.TypeFor[ir.Property](), "Name", true}}}, + } { + assert.Equal(t, tc.want, doc[tc.path], tc.path) + } + + outer := reflect.TypeFor[Outer]() + assert.Equal(t, []fieldAt{none, none, none, none}, fieldsOf(Outer{})["doc"], + "Outer and the three structs it embeds, each spelled as the root") + assert.Equal(t, []fieldAt{{outer, "Name", true}}, fieldsOf(Outer{})["doc.Name"], + "a field promoted through three embedded structs is spelled on the outermost") + + shadowed := reflect.TypeFor[Shadowed]() + assert.Equal(t, []fieldAt{{shadowed, "Name", true}, {shadowed, "Name", true}}, fieldsOf(Shadowed{})["doc.Name"], + "the promoted Name and the one beside it are both spelled on Shadowed") +} + +// TestWalkPath_FieldOfTheZeroPathIsNone pins the zero WalkPath, which no walk +// is behind: it ends in no field. +func TestWalkPath_FieldOfTheZeroPathIsNone(t *testing.T) { + t.Parallel() + owner, name, ok := ir.WalkPath{}.Field() + assert.Nil(t, owner) + assert.Empty(t, name) + assert.False(t, ok) +} + +// TestWalkPath_FieldAllocatesNothing holds Field to reading the walk's own +// steps, so a visitor may ask it of every value. It is not run in parallel: +// AllocsPerRun measures the process. +func TestWalkPath_FieldAllocatesNothing(t *testing.T) { + doc := &ir.Document{Types: ir.TypeRegistry{ + "t/p": &ir.Primitive{ID: "t/p"}, + "t/m": &ir.Model{ID: "t/m", Properties: []ir.Property{{ID: "p/m/a"}}}, + }} + var fields int + asking := ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + if _, _, ok := at.Field(); ok { + fields++ + } + return true + }) + ir.WalkValues(doc, "doc", asking) + require.Positive(t, fields, "some path must end in a field, or nothing is measured") + + assert.Empty(t, irtest.AllocatingVisits(doc, asking)) +} + // TestWalkPath_LongPathsRenderWhole holds a path longer than String's stack // buffer to rendering whole: one past it only costs an allocation more. func TestWalkPath_LongPathsRenderWhole(t *testing.T) { @@ -166,3 +273,29 @@ func TestWalkPath_EntriesAreOrderedByTheirSpelling(t *testing.T) { })) assert.Equal(t, []string{"doc[98]", "doc[99]"}, got) } + +// TestWalkPath_IsMapKeyIsTheKeyAlone holds IsMapKey to the step String spells +// with MapKeySuffix: a map entry's key, and not its value, a field, an element, +// the root or the zero path. +func TestWalkPath_IsMapKeyIsTheKeyAlone(t *testing.T) { + t.Parallel() + doc := &ir.Document{Services: []ir.Service{{Renames: map[ir.TypeID]ir.Naming{"t/x/M": {Source: "m"}}}}} + got := map[string]bool{} + ir.WalkValues(doc, "doc", ir.VisitFunc(func(_ reflect.Value, at ir.WalkPath) bool { + got[at.String()] = at.IsMapKey() + return true + })) + for path, want := range map[string]bool{ + "doc": false, + "doc.Services": false, + "doc.Services[0]": false, + "doc.Services[0].Renames": false, + "doc.Services[0].Renames[t/x/M].key": true, + "doc.Services[0].Renames[t/x/M]": false, + "doc.Services[0].Renames[t/x/M].Source": false, + } { + require.Contains(t, got, path) + assert.Equal(t, want, got[path], path) + } + assert.False(t, ir.WalkPath{}.IsMapKey(), "the zero path ends in nothing") +} diff --git a/pass/export_test.go b/pass/export_test.go index 710a1f006..e561a1f37 100644 --- a/pass/export_test.go +++ b/pass/export_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irfacts" ) // DanglingRefs is checkDanglingRefs over what one walk of doc reads, as validate @@ -21,10 +22,10 @@ func DanglingRefs(tb testing.TB, doc *ir.Document) []ir.Diagnostic { // checkPropIDRefs to settle sit, in walk order, for the external tests. func KeptPropIDs(tb testing.TB, doc *ir.Document) []string { tb.Helper() - sites := walkedFor(tb, doc).props.sites - paths := make([]string, 0, len(sites)) - for _, s := range sites { - paths = append(paths, s.Path) + props := walkedFor(tb, doc).props + paths := make([]string, 0, len(props.sites)) + for _, at := range props.paths.All() { + paths = append(paths, at.String()) } return paths } @@ -61,3 +62,48 @@ var ValidateWith = validate // Families is how many rule families Validate runs, each after an ask of its // context. func Families() int { return len(families(nil, walked{}, nil)) } + +// FamilyAllocs returns the allocations one run of each rule family Validate +// runs over doc makes, in the order it runs them, as testing.AllocsPerRun +// counts. The walk they read is taken beforehand, and so is the index they +// ask, its hierarchy and its exposure built in full, so each family is +// measured at its own cost and not at the index's. +func FamilyAllocs(tb testing.TB, doc *ir.Document) []float64 { + tb.Helper() + read := walkedFor(tb, doc) + facts := irfacts.New(doc) + facts.Subtypes("") // builds the hierarchy + for range len(doc.Types) + 1 { + facts.Exposes("", "") // each takes a step, so the walks run out and the exposure is built + } + run := families(doc, read, facts) + costs := make([]float64, 0, len(run)) + for _, family := range run { + costs = append(costs, testing.AllocsPerRun(5, func() { _ = family() })) + } + return costs +} + +// WalkAllocs returns the allocations the one walk Validate takes of doc makes, +// as testing.AllocsPerRun counts. +func WalkAllocs(tb testing.TB, doc *ir.Document) float64 { + tb.Helper() + return testing.AllocsPerRun(5, func() { _ = walkedFor(tb, doc) }) +} + +// ReusedMap is reusedMap, for the external tests. +type ReusedMap[K comparable, V any] = reusedMap[K, V] + +// Emptied is r.emptied, for the external tests. +func Emptied[K comparable, V any](r *ReusedMap[K, V]) map[K]V { return r.emptied() } + +// ReuseLimit is reuseLimit, for the external tests. +const ReuseLimit = reuseLimit + +// PropIDRefs returns checkPropIDRefs over what one walk of doc reads, as +// validate runs it, for the external tests to measure. +func PropIDRefs(tb testing.TB, doc *ir.Document) func() []ir.Diagnostic { + tb.Helper() + w := walkedFor(tb, doc) + return func() []ir.Diagnostic { return checkPropIDRefs(w.props, w.refs) } +} diff --git a/pass/reuse.go b/pass/reuse.go new file mode 100644 index 000000000..df9939c6d --- /dev/null +++ b/pass/reuse.go @@ -0,0 +1,26 @@ +package pass + +// reuseLimit is how many entries a reusedMap may have held for it to be +// emptied in place rather than replaced. Emptying a map costs in proportion to +// the most it has ever held, as Go never shrinks one, so a map kept after one +// huge collection would tax every collection read through it after. +const reuseLimit = 1024 + +// reusedMap is a map a check reads one collection after another through, so a +// collection costs no map of its own. The zero reusedMap is ready to use; +// irverify keeps the same type for its checks. +type reusedMap[K comparable, V any] struct { + m map[K]V +} + +// emptied returns the map empty for the next collection: the one r holds, +// cleared, while the last collection left it at most reuseLimit entries, and a +// fresh one past that, which r holds from then on. +func (r *reusedMap[K, V]) emptied() map[K]V { + if r.m == nil || len(r.m) > reuseLimit { + r.m = make(map[K]V) + return r.m + } + clear(r.m) + return r.m +} diff --git a/pass/reuse_test.go b/pass/reuse_test.go new file mode 100644 index 000000000..7e1ef58b5 --- /dev/null +++ b/pass/reuse_test.go @@ -0,0 +1,55 @@ +package pass_test + +import ( + "go/types" + "reflect" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/internal/decls" + "github.com/dexpace/morphic/pass" +) + +// TestReusedMap_EmptiesInPlaceUpToTheLimit holds emptied to reusing the map it +// holds while the last collection left it at most the limit's entries, and to +// handing a fresh one past that: Go never shrinks a map, so emptying one that +// held a huge collection costs in proportion to it, at every collection after. +func TestReusedMap_EmptiesInPlaceUpToTheLimit(t *testing.T) { + t.Parallel() + var r pass.ReusedMap[string, int] + first := pass.Emptied(&r) + require.NotNil(t, first, "the zero reusedMap hands a map to write to") + fill := func(m map[string]int, n int) { + for i := range n { + m[strconv.Itoa(i)] = i + } + } + + fill(first, pass.ReuseLimit) + atLimit := pass.Emptied(&r) + assert.Empty(t, atLimit) + assert.Equal(t, reflect.ValueOf(first).UnsafePointer(), reflect.ValueOf(atLimit).UnsafePointer(), + "a map left at the limit is emptied in place") + + fill(atLimit, pass.ReuseLimit+1) + past := pass.Emptied(&r) + assert.Empty(t, past) + assert.NotEqual(t, reflect.ValueOf(atLimit).UnsafePointer(), reflect.ValueOf(past).UnsafePointer(), + "a map left past the limit is replaced") + assert.Equal(t, reflect.ValueOf(past).UnsafePointer(), reflect.ValueOf(pass.Emptied(&r)).UnsafePointer(), + "the fresh map is the one reused from then on") +} + +// TestReusedMaps_AreEmptiedThroughReusedMapAlone holds every map the pass +// empties for another collection to reusedMap, whose limit bounds what +// emptying costs: a bare clear of a map kept across collections makes one huge +// collection tax every later one. +func TestReusedMaps_AreEmptiedThroughReusedMapAlone(t *testing.T) { + t.Parallel() + p, err := decls.Load(reflect.TypeFor[pass.Validator]().PkgPath()) + require.NoError(t, err) + assert.Equal(t, []string{"emptied"}, p.FuncsUsing(types.Universe.Lookup("clear"))) +} diff --git a/pass/validate.go b/pass/validate.go index 0355b5637..756172cad 100644 --- a/pass/validate.go +++ b/pass/validate.go @@ -78,6 +78,12 @@ func validate(ctx context.Context, doc *ir.Document, walk walkFunc) ([]ir.Diagno // families returns validate's rule families over doc and what its walk read, // in the order they report. Validate asks its context between two, so a cancel // waits out one family at most. +// +// A family allocates only for a diagnostic. It sorts and keys collections in a +// list and a map it keeps for the run, so a value with nothing wrong costs it +// nothing but a step those or a log may grow by. The index it asks, facts, is +// not yet held to that: it allocates per type it indexes and per question it +// walks (GitHub #1150). func families(doc *ir.Document, read walked, facts *irfacts.Index) []func() []ir.Diagnostic { return []func() []ir.Diagnostic{ func() []ir.Diagnostic { return checkNilTypes(doc) }, diff --git a/pass/validate_cost_test.go b/pass/validate_cost_test.go new file mode 100644 index 000000000..25829d148 --- /dev/null +++ b/pass/validate_cost_test.go @@ -0,0 +1,165 @@ +package pass_test + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irtest" + "github.com/dexpace/morphic/pass" +) + +// wide is how many entries each collection of soundDocument holds, past the +// eight a map holds before it allocates. +const wide = 12 + +// soundDocument returns a document Validate finds nothing wrong with, of n +// entities, each reaching every rule family with collections wider than a map +// holds unallocated: a model with properties, aliases and a discriminator +// routing to its subtypes, a union routing to its variants, an enum, a channel +// on a server with its messages, and an operation binding parameters, +// paginating to an item property the walk meets before its model, keyed by a +// token, with multipart request, response, error and default payloads and a +// message binding with a reply. +func soundDocument(n int) *ir.Document { + doc := &ir.Document{ + IRVersion: ir.IRVersion, Name: "t", Version: "1", + Types: ir.TypeRegistry{"t/prim/string": &ir.Primitive{ID: "t/prim/string", Prim: ir.PrimString}}, + Channels: map[ir.ChannelID]ir.Channel{}, + Messages: map[ir.MessageID]ir.Message{}, + Servers: []ir.Server{{Name: ir.Naming{Source: "api"}, URLTemplate: "https://api.example.com"}}, + } + group := ir.OperationGroup{ID: "g/x"} + commonErrors := make([]ir.ErrorCase, 0, n) + for i := range n { + model := addEntityTypes(doc, i) + messages := addEntityMessages(doc, i) + group.Operations = append(group.Operations, entityOperation(i, model, messages)) + commonErrors = append(commonErrors, ir.ErrorCase{Payload: jsonPayload(model.ID)}) + } + doc.Services = []ir.Service{{ID: "s/x", Servers: []int{0}, CommonErrors: commonErrors, Groups: []ir.OperationGroup{group}}} + return doc +} + +// addEntityTypes adds entity i's model, its subtypes, a union over them and an +// enum to doc, and returns the model. +func addEntityTypes(doc *ir.Document, i int) *ir.Model { + str := ir.TypeRef{Target: "t/prim/string"} + model := &ir.Model{ID: ir.TypeID(fmt.Sprintf("t/x/M%d", i)), Discriminator: &ir.Discriminator{Mapping: map[string]ir.TypeID{}}} + union := &ir.Union{ID: ir.TypeID(fmt.Sprintf("t/x/U%d", i)), Discriminator: &ir.Discriminator{Mapping: map[string]ir.TypeID{}}} + enum := &ir.Enum{ID: ir.TypeID(fmt.Sprintf("t/x/E%d", i))} + for j := range wide { + wire := fmt.Sprintf("w%d", j) + model.Properties = append(model.Properties, ir.Property{ + ID: ir.PropID(fmt.Sprintf("p/x/M%d/%s", i, wire)), WireName: wire, Type: str, + Name: ir.Naming{Source: wire, Aliases: []string{wire + "a", wire + "b"}}, + }) + sub := &ir.Model{ID: ir.TypeID(fmt.Sprintf("t/x/M%d/S%d", i, j)), Base: &ir.TypeRef{Target: model.ID}} + doc.Types[sub.ID] = sub + model.Discriminator.Mapping[wire] = sub.ID + union.Variants = append(union.Variants, ir.Variant{Type: ir.TypeRef{Target: sub.ID}}) + union.Discriminator.Mapping[wire] = sub.ID + enum.Members = append(enum.Members, ir.EnumMember{ID: ir.EnumMemberID(fmt.Sprintf("e/x/E%d/%s", i, wire))}) + } + model.Discriminator.Property = model.Properties[0].ID + model.Discriminator.Default = model.Discriminator.Mapping["w0"] + for _, td := range []ir.TypeDef{model, union, enum} { + doc.Types[td.Common().ID] = td + } + return model +} + +// addEntityMessages adds entity i's channel and the messages it carries to doc, +// and returns the channel's ID and the messages'. +func addEntityMessages(doc *ir.Document, i int) entityMessages { + ch := ir.Channel{ID: ir.ChannelID(fmt.Sprintf("c/x/C%d", i)), Servers: []int{0}} + for j := range wide { + id := ir.MessageID(fmt.Sprintf("m/x/C%d/M%d", i, j)) + doc.Messages[id] = ir.Message{ID: id, Payload: *jsonPayload("t/prim/string")} + ch.Messages = append(ch.Messages, id) + } + doc.Channels[ch.ID] = ch + return entityMessages{channel: ch.ID, messages: ch.Messages} +} + +// entityMessages is the channel an entity's operation binds and the messages +// it carries. +type entityMessages struct { + channel ir.ChannelID + messages []ir.MessageID +} + +// entityOperation returns entity i's operation over model and the channel of +// messages. +func entityOperation(i int, model *ir.Model, messages entityMessages) ir.Operation { + op := ir.Operation{ + ID: ir.OpID(fmt.Sprintf("op/x/O%d", i)), + Request: multipartBody(model), + Responses: []ir.Response{{Payload: jsonPayload(model.ID)}}, + Errors: []ir.ErrorCase{{Payload: jsonPayload(model.ID)}}, + Default: &ir.Response{Payload: jsonPayload(model.ID), Conditions: ir.ResponseConditions{StatusCodes: []ir.StatusRange{{}}}}, + } + binding := ir.HTTPBinding{SuccessStatus: map[int]int{0: 200}} + for j := range wide { + id := ir.ParamID(fmt.Sprintf("op/x/O%d/q%d", i, j)) + op.Params = append(op.Params, ir.Parameter{ID: id, Name: ir.Naming{Source: fmt.Sprintf("q%d", j)}}) + binding.ParamBindings = append(binding.ParamBindings, ir.HTTPParamBinding{Param: id, Location: ir.HTTPLocationQuery}) + } + op.Bindings.HTTP = []ir.HTTPBinding{binding} + op.Pagination = &ir.Pagination{ + InputCursor: &ir.ParamPath{Param: op.Params[0].ID}, InputLimit: &ir.ParamPath{Param: op.Params[1].ID}, + Items: &ir.PropPath{Segments: []ir.PropID{model.Properties[0].ID}}, + } + op.Idempotency = ir.Idempotency{Kind: ir.IdempotencyToken, TokenParam: op.Params[2].ID} + reply := messages.channel + op.Bindings.Message = &ir.MessageBinding{ + Channel: messages.channel, Messages: messages.messages, + Reply: &ir.Reply{Channel: &reply, Messages: messages.messages}, + } + return op +} + +// multipartBody returns a request body sending model as multipart parts, +// one encoding per property. +func multipartBody(model *ir.Model) *ir.Payload { + content := ir.Content{MediaType: "multipart/form-data", Type: ir.TypeRef{Target: model.ID}, Encoding: map[ir.PropID]ir.PartEncoding{}} + for _, p := range model.Properties { + content.Encoding[p.ID] = ir.PartEncoding{} + } + return &ir.Payload{Contents: []ir.Content{content}} +} + +// jsonPayload returns a JSON body of the type target. +func jsonPayload(target ir.TypeID) *ir.Payload { + return &ir.Payload{Contents: []ir.Content{{MediaType: "application/json", Type: ir.TypeRef{Target: target}}}} +} + +// TestValidate_ASoundValueCostsNothing holds the walk Validate takes, and each +// rule family once the run's index is built, to costing less than one +// allocation more per entity added when the document doubles, so a location +// or a kept path is spelled, and a list or a map made, only for a diagnostic: +// each entity reaches every family with collections wider than a map holds +// unallocated. A map or a log the run keeps grows in steps, far fewer. +// Building the index allocates per type it indexes, which this leaves out +// (GitHub #1150). It is not run in parallel: AllocsPerRun measures the +// process. +func TestValidate_ASoundValueCostsNothing(t *testing.T) { + const entities = 50 + few, many := soundDocument(entities), soundDocument(2*entities) + require.Empty(t, validated(t, many), "the document is sound, or what is measured is a report") + + pair := soundDocument(2) // every kind of value the walk meets + require.NotEmpty(t, pass.KeptPropIDs(t, pair), "an item property is kept for its model to settle") + assert.Empty(t, irtest.AllocatingVisits(pair, pass.WalkVisitors(t, pair)...), "the walk's visits") + walked := pass.WalkAllocs(t, many) - pass.WalkAllocs(t, few) + assert.Less(t, walked, float64(entities), "the walk: %v allocations more", walked) + fewCosts, manyCosts := pass.FamilyAllocs(t, few), pass.FamilyAllocs(t, many) + require.Len(t, manyCosts, pass.Families()) + for i := range manyCosts { + assert.Less(t, manyCosts[i]-fewCosts[i], float64(entities), "family %d: %v allocations for %v", + i, manyCosts[i], fewCosts[i]) + } +} diff --git a/pass/validate_discriminators.go b/pass/validate_discriminators.go index dd574a906..d220214e1 100644 --- a/pass/validate_discriminators.go +++ b/pass/validate_discriminators.go @@ -2,7 +2,6 @@ package pass import ( "fmt" - "maps" "slices" "github.com/dexpace/morphic/ir" @@ -14,12 +13,13 @@ import ( // tag property the model does not expose. facts indexes doc. func checkDiscriminators(doc *ir.Document, facts *irfacts.Index) []ir.Diagnostic { var diags []ir.Diagnostic + var read mappingReads for _, id := range liveTypeIDs(doc) { switch t := doc.Types[id].(type) { case *ir.Union: - diags = append(diags, checkUnionDiscriminator(doc, t)...) + diags = append(diags, checkUnionDiscriminator(doc, t, &read)...) case *ir.Model: - diags = append(diags, checkModelDiscriminator(doc, facts, t)...) + diags = append(diags, checkModelDiscriminator(doc, facts, t, &read)...) default: // Only Union and Model can declare a discriminator; every other // TypeDef kind has nothing to check. A new discriminator-bearing @@ -29,17 +29,25 @@ func checkDiscriminators(doc *ir.Document, facts *irfacts.Index) []ir.Diagnostic return diags } +// mappingReads is what checkDiscriminators reads each discriminator through, +// so none costs a collection of its own: a union's variant targets, and a +// mapping's keys, sorted. +type mappingReads struct { + variants reusedMap[ir.TypeID, bool] + keys []string +} + // checkUnionDiscriminator requires each mapping target to be one of the union's // variant targets. -func checkUnionDiscriminator(doc *ir.Document, u *ir.Union) []ir.Diagnostic { +func checkUnionDiscriminator(doc *ir.Document, u *ir.Union, read *mappingReads) []ir.Diagnostic { if u.Discriminator == nil { return nil } - variants := make(map[ir.TypeID]bool, len(u.Variants)) + variants := read.variants.emptied() for _, v := range u.Variants { variants[v.Type.Target] = true } - return checkMapping(doc, u.Discriminator, string(u.ID), func(target ir.TypeID) bool { + return checkMapping(doc, u.Discriminator, string(u.ID), read, func(target ir.TypeID) bool { return variants[target] }) } @@ -47,12 +55,12 @@ func checkUnionDiscriminator(doc *ir.Document, u *ir.Union) []ir.Diagnostic { // checkModelDiscriminator requires each mapping target to be a declared subtype // of the discriminated base model, and the tag property to be one this model // exposes. Its routes and tag are asked of facts, the one index of the run. -func checkModelDiscriminator(doc *ir.Document, facts *irfacts.Index, m *ir.Model) []ir.Diagnostic { +func checkModelDiscriminator(doc *ir.Document, facts *irfacts.Index, m *ir.Model, read *mappingReads) []ir.Diagnostic { if m.Discriminator == nil { return nil } diags := checkDiscriminatorProperty(facts, m) - return append(diags, checkMapping(doc, m.Discriminator, string(m.ID), func(target ir.TypeID) bool { + return append(diags, checkMapping(doc, m.Discriminator, string(m.ID), read, func(target ir.TypeID) bool { return facts.IsSubtype(target, m.ID) })...) } @@ -88,26 +96,40 @@ func checkDiscriminatorProperty(facts *irfacts.Index, m *ir.Model) []ir.Diagnost // not referentially closed, while pass/discriminator-missing-variant says this // discriminator cannot route that wire value, which a polymorphic decoder's // emitter subscribes to. Neither consumer should need the other's code. -func checkMapping(doc *ir.Document, d *ir.Discriminator, where string, member func(ir.TypeID) bool) []ir.Diagnostic { +func checkMapping(doc *ir.Document, d *ir.Discriminator, where string, read *mappingReads, + member func(ir.TypeID) bool, +) []ir.Diagnostic { var diags []ir.Diagnostic - report := func(target ir.TypeID, position string) { - legal, why := mappingTarget(doc, target, member) - if legal { - return + // Asked in ascending key order, sorted in read's list, so map iteration + // reaches neither the output nor the questions member asks. + read.keys = read.keys[:0] + for key := range d.Mapping { + read.keys = append(read.keys, key) + } + slices.Sort(read.keys) + for _, key := range read.keys { + target := d.Mapping[key] + if legal, why := mappingTarget(doc, target, member); !legal { + diags = append(diags, missingVariant(fmt.Sprintf("mapping %q", key), where, target, why)) } - diags = append(diags, diag(ir.SeverityError, "pass/discriminator-missing-variant", - fmt.Sprintf("discriminator %s on %s references %q, which %s", position, where, target, why), - where)) } - for _, key := range slices.Sorted(maps.Keys(d.Mapping)) { - report(d.Mapping[key], fmt.Sprintf("mapping %q", key)) + if d.Default == "" { + return diags } - if d.Default != "" { - report(d.Default, "default") + if legal, why := mappingTarget(doc, d.Default, member); !legal { + diags = append(diags, missingVariant("default", where, d.Default, why)) } return diags } +// missingVariant reports that the discriminator on where routes position to +// target, which why says is no legal target. +func missingVariant(position, where string, target ir.TypeID, why string) ir.Diagnostic { + return diag(ir.SeverityError, "pass/discriminator-missing-variant", + fmt.Sprintf("discriminator %s on %s references %q, which %s", position, where, target, why), + where) +} + // mappingTarget reports whether target is a legal mapping target and, when it is // not, the clause naming why. The two failures read differently on purpose: a // target no type declares is a broken reference, while a declared one that fails diff --git a/pass/validate_discriminators_test.go b/pass/validate_discriminators_test.go index 840a966f3..a63711e73 100644 --- a/pass/validate_discriminators_test.go +++ b/pass/validate_discriminators_test.go @@ -1,6 +1,7 @@ package pass_test import ( + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -311,3 +312,52 @@ func TestValidate_DanglingDiscriminatorDefaultIsReportedTwice(t *testing.T) { }) } } + +// TestValidate_MappingDiagnosticsAreDeterministic pins invariant 7 for a +// discriminator's Mapping, whose keys Go ranges in randomized order: they are +// asked in sorted order, so the reports within one discriminator, all placed at +// the union, follow the wire values their messages name. +func TestValidate_MappingDiagnosticsAreDeterministic(t *testing.T) { + t.Parallel() + doc := discriminatedUnion("t/m") + u, ok := doc.Types["t/u"].(*ir.Union) + require.True(t, ok) + for _, key := range []string{"z", "b", "q", "c", "m"} { + u.Discriminator.Mapping[key] = "t/none" + } + messages := func() []string { + found := withCode(validated(t, doc), "pass/discriminator-missing-variant") + out := make([]string, 0, len(found)) + for _, d := range found { + out = append(out, d.Message) + } + return out + } + want := messages() + require.Len(t, want, 5) + for i, key := range []string{"b", "c", "m", "q", "z"} { + assert.Contains(t, want[i], fmt.Sprintf("mapping %q", key)) + } + for range 8 { + assert.Equal(t, want, messages()) + } +} + +// TestValidate_UnionMappingIsHeldToItsOwnVariants pins that a union's mapping +// answers to its own variants alone. The union read just before it lists the +// target as a variant, so a check carrying what it read of one union into the +// next would let the mapping through. +func TestValidate_UnionMappingIsHeldToItsOwnVariants(t *testing.T) { + t.Parallel() + doc := discriminatedUnion("t/n") + doc.Types["t/n"] = &ir.Model{ID: "t/n"} + doc.Types["t/a"] = &ir.Union{ // sorts before t/u, so it is read first + ID: "t/a", + Variants: []ir.Variant{{Type: ir.TypeRef{Target: "t/n"}}}, + Discriminator: &ir.Discriminator{PropertyName: "kind", Mapping: map[string]ir.TypeID{"n": "t/n"}}, + } + + found := withCode(validated(t, doc), "pass/discriminator-missing-variant") + require.Len(t, found, 1, "the union listing t/n routes to it, and the other does not") + assert.Equal(t, `discriminator mapping "a" on t/u references "t/n", which is not a variant of it`, found[0].Message) +} diff --git a/pass/validate_enummembers.go b/pass/validate_enummembers.go index 394e56514..57c5cef5e 100644 --- a/pass/validate_enummembers.go +++ b/pass/validate_enummembers.go @@ -12,15 +12,17 @@ import ( // in its occurrence number, so a compiler following ir-design §4.5 never trips // this. A document that does has two members an emitter cannot tell apart, // which is what keying collision resolution by ID (emitter-design §4.12) rests -// on not happening. +// on not happening. Every enum is read through the same map, so none costs one +// of its own. func checkDuplicateMemberIDs(doc *ir.Document) []ir.Diagnostic { var diags []ir.Diagnostic + var members reusedMap[ir.EnumMemberID, bool] for _, id := range liveTypeIDs(doc) { enum, ok := doc.Types[id].(*ir.Enum) if !ok { continue } - seen := make(map[ir.EnumMemberID]bool, len(enum.Members)) + seen := members.emptied() for i, m := range enum.Members { if m.ID == "" { continue diff --git a/pass/validate_indices.go b/pass/validate_indices.go index 4c12eeea8..c89deaf63 100644 --- a/pass/validate_indices.go +++ b/pass/validate_indices.go @@ -53,21 +53,29 @@ func checkResponseIndices(doc *ir.Document) []ir.Diagnostic { var diags []ir.Diagnostic ir.ForEachOperation(doc, func(op ir.Operation) { for i, b := range op.Bindings.HTTP { - where := fmt.Sprintf("%s/bindings/http/%d", op.ID, i) - diags = appendSuccessStatusDiags(diags, b.SuccessStatus, len(op.Responses), where) + diags = appendSuccessStatusDiags(diags, b.SuccessStatus, len(op.Responses), op.ID, i) } }) return diags } -// appendSuccessStatusDiags appends to dst a diagnostic per SuccessStatus key that -// addresses none of the declared responses, in ascending key order so map -// iteration cannot reach the output. -func appendSuccessStatusDiags(dst []ir.Diagnostic, status map[int]int, declared int, where string) []ir.Diagnostic { - for _, index := range slices.Sorted(maps.Keys(status)) { - if index >= 0 && index < declared { - continue +// appendSuccessStatusDiags appends to dst a diagnostic per SuccessStatus key of +// HTTP binding i of operation op that addresses none of the declared responses, +// in ascending key order so map iteration cannot reach the output. Only the +// keys reported are sorted, and only their location spelled. +func appendSuccessStatusDiags(dst []ir.Diagnostic, status map[int]int, declared int, op ir.OpID, i int) []ir.Diagnostic { + var outside []int + for index := range status { + if index < 0 || index >= declared { + outside = append(outside, index) } + } + if len(outside) == 0 { + return dst + } + slices.Sort(outside) + where := fmt.Sprintf("%s/bindings/http/%d", op, i) + for _, index := range outside { at := fmt.Sprintf("%s/successStatus/%d", where, index) dst = append(dst, diag(ir.SeverityError, "ir/response-index-out-of-range", fmt.Sprintf("response index %d at %s addresses none of the %d declared responses", index, at, declared), diff --git a/pass/validate_indices_test.go b/pass/validate_indices_test.go index 7c3d93328..51916e093 100644 --- a/pass/validate_indices_test.go +++ b/pass/validate_indices_test.go @@ -163,3 +163,17 @@ func TestValidate_ResponseIndexDiagnosticsAreDeterministic(t *testing.T) { assert.Equal(t, want, pointers(withCode(validated(t, doc), "ir/response-index-out-of-range"))) } } + +// TestValidate_ResponseIndexOutOfRangeIsPlacedAtItsBinding pins that an index +// out of range is reported at the binding holding it, the second here, after a +// first whose index is in range. +func TestValidate_ResponseIndexOutOfRangeIsPlacedAtItsBinding(t *testing.T) { + t.Parallel() + doc := opWithSuccessStatus(map[int]int{0: 200}) + op := firstOp(doc) + op.Bindings.HTTP = append(op.Bindings.HTTP, + ir.HTTPBinding{Method: "POST", URITemplate: "/x", SuccessStatus: map[int]int{1: 202}}) + + assert.Equal(t, []string{"op/bindings/http/1/successStatus/1"}, + pointers(withCode(validated(t, doc), "ir/response-index-out-of-range"))) +} diff --git a/pass/validate_messages.go b/pass/validate_messages.go index 88496e240..9f56271e5 100644 --- a/pass/validate_messages.go +++ b/pass/validate_messages.go @@ -20,8 +20,9 @@ import ( // checkGroupWalkTruncated reports. func checkMessageBindings(doc *ir.Document) []ir.Diagnostic { var diags []ir.Diagnostic + var carried reusedMap[ir.MessageID, bool] ir.ForEachOperation(doc, func(op ir.Operation) { - diags = append(diags, checkBoundMessages(doc, op)...) + diags = append(diags, checkBoundMessages(doc, op, &carried)...) }) return diags } @@ -33,24 +34,31 @@ func checkMessageBindings(doc *ir.Document) []ir.Diagnostic { // request the operation's channel does not — so neither is checked alone. // // A reply with no channel of its own is left alone: its address is dynamic, so -// there is no declared message set to hold it to (ir-design §8.3). -func checkBoundMessages(doc *ir.Document, op ir.Operation) []ir.Diagnostic { +// there is no declared message set to hold it to (ir-design §8.3). Each +// channel's messages are read through carried. +func checkBoundMessages(doc *ir.Document, op ir.Operation, carried *reusedMap[ir.MessageID, bool]) []ir.Diagnostic { b := op.Bindings.Message if b == nil { return nil } - at := string(op.ID) + "/bindings/message" - diags := appendUncarriedMessageDiags(nil, doc, b.Channel, b.Messages, at+"/messages") + diags := appendUncarriedMessageDiags(nil, doc, b.Channel, b.Messages, messagesAt{op.ID, "messages"}, carried) if b.Reply == nil || b.Reply.Channel == nil { return diags } return appendUncarriedMessageDiags(diags, doc, *b.Reply.Channel, b.Reply.Messages, - at+"/reply/messages") + messagesAt{op.ID, "reply/messages"}, carried) +} + +// messagesAt is where a binding's message list sits: the operation, and the +// list's field below its message binding. It is spelled only for a diagnostic. +type messagesAt struct { + op ir.OpID + field string } // appendUncarriedMessageDiags appends to dst a diagnostic per message in used -// that channel does not carry, in declaration order; at is the pointer prefix the -// index is appended to. +// that channel does not carry, in declaration order; at is where used sits, and +// carried what the channel's messages are read through. // // A channel that resolves nowhere is left alone: there is no set to compare // against. One that names no declared channel is ir/dangling-channel-ref @@ -59,20 +67,20 @@ func checkBoundMessages(doc *ir.Document, op ir.Operation) []ir.Diagnostic { // irverify alone, as ir/empty-channel-ref: this pass does not report an empty // channel yet (GitHub #575). func appendUncarriedMessageDiags(dst []ir.Diagnostic, doc *ir.Document, - channel ir.ChannelID, used []ir.MessageID, at string) []ir.Diagnostic { + channel ir.ChannelID, used []ir.MessageID, at messagesAt, carried *reusedMap[ir.MessageID, bool]) []ir.Diagnostic { ch, declared := doc.Channels[channel] if !declared { return dst } - carried := make(map[ir.MessageID]bool, len(ch.Messages)) + carries := carried.emptied() for _, id := range ch.Messages { - carried[id] = true + carries[id] = true } for i, id := range used { - if carried[id] { + if carries[id] { continue } - site := fmt.Sprintf("%s/%d", at, i) + site := fmt.Sprintf("%s/bindings/message/%s/%d", at.op, at.field, i) dst = append(dst, diag(ir.SeverityError, "pass/message-not-in-channel", fmt.Sprintf("message %q at %s is not carried by channel %q", id, site, channel), site)) } diff --git a/pass/validate_messages_test.go b/pass/validate_messages_test.go index 336a4ce89..18637d14f 100644 --- a/pass/validate_messages_test.go +++ b/pass/validate_messages_test.go @@ -37,6 +37,17 @@ func TestValidate_MessageNotCarriedByChannel(t *testing.T) { assert.Contains(t, messageForCode(t, diags, "pass/message-not-in-channel"), "msg/b") } +// TestValidate_MessageNotCarriedIsPlacedAtItsPosition pins that a message its +// channel does not carry is reported at its own position in the binding's +// list, the second here, after one the channel carries. +func TestValidate_MessageNotCarriedIsPlacedAtItsPosition(t *testing.T) { + t.Parallel() + doc := messageBindingDoc("chan/a", []ir.MessageID{"msg/a"}, []ir.MessageID{"msg/a", "msg/b"}) + + assert.Equal(t, []string{"op/bindings/message/messages/1"}, + pointers(withCode(validated(t, doc), "pass/message-not-in-channel"))) +} + // TestValidate_MessagesCarriedByChannelAreClean is the silent half. A binding // using a subset of the channel's messages, and one naming none at all, both // report nothing — a check that cannot stay silent is no better than one that diff --git a/pass/validate_params.go b/pass/validate_params.go index 3476f4ba5..9c6545c56 100644 --- a/pass/validate_params.go +++ b/pass/validate_params.go @@ -30,17 +30,31 @@ func danglingWalkSkips(id ir.ParamID, refs *ir.IDReferences) bool { } // checkParamBindings validates each HTTP binding's parameter bindings against -// the operation's logical parameters. +// the operation's logical parameters. Every operation and binding is read +// through the same maps, so none costs a map of its own. func checkParamBindings(doc *ir.Document, refs *ir.IDReferences) []ir.Diagnostic { var diags []ir.Diagnostic + var read bindingMaps ir.ForEachOperation(doc, func(op ir.Operation) { + names := read.names.emptied() + for _, p := range op.Params { + names[p.ID] = p.Name.Source + } for i := range op.Bindings.HTTP { - diags = append(diags, checkHTTPParamBinding(op, i, refs)...) + diags = append(diags, checkHTTPParamBinding(op, i, names, refs, &read)...) } }) return diags } +// bindingMaps are what checkParamBindings reads an operation and each of its +// HTTP bindings through. +type bindingMaps struct { + names reusedMap[ir.ParamID, string] + bound reusedMap[ir.ParamID, int] + perLocation reusedMap[paramLocation, int] +} + // paramLocation keys the per-location bound-once count: a parameter is // identified by its ID, not its name, so two parameters sharing a name never // count against each other. @@ -49,25 +63,23 @@ type paramLocation struct { location ir.HTTPLocation } -// checkHTTPParamBinding validates one HTTP binding: a parameter this operation -// does not declare is an error unless the dangling walk reports it -// (danglingWalkSkips), a param bound twice in one non-host location is an -// error, and an unbound parameter is a warning (body-carried operations bind -// nothing). -func checkHTTPParamBinding(op ir.Operation, idx int, refs *ir.IDReferences) []ir.Diagnostic { +// checkHTTPParamBinding validates HTTP binding idx of op, whose parameters' +// names names holds by ID: a parameter this operation does not declare is an +// error unless the dangling walk reports it (danglingWalkSkips), a param bound +// twice in one non-host location is an error, and an unbound parameter is a +// warning (body-carried operations bind nothing). The binding's location is +// spelled only for a diagnostic. +func checkHTTPParamBinding(op ir.Operation, idx int, names map[ir.ParamID]string, refs *ir.IDReferences, + read *bindingMaps, +) []ir.Diagnostic { b := op.Bindings.HTTP[idx] - where := fmt.Sprintf("%s/bindings/http/%d", op.ID, idx) - names := make(map[ir.ParamID]string, len(op.Params)) - for _, p := range op.Params { - names[p.ID] = p.Name.Source - } var diags []ir.Diagnostic - bound := make(map[ir.ParamID]int, len(op.Params)) - perLocation := make(map[paramLocation]int, len(b.ParamBindings)) + bound, perLocation := read.bound.emptied(), read.perLocation.emptied() for _, pb := range b.ParamBindings { name, own := names[pb.Param] if !own { if danglingWalkSkips(pb.Param, refs) { + where := bindingWhere(op.ID, idx) diags = append(diags, diag(ir.SeverityError, "pass/param-binding-mismatch", fmt.Sprintf("binding on %s names parameter %q, which this operation does not declare", where, pb.Param), where)) } @@ -79,20 +91,28 @@ func checkHTTPParamBinding(op ir.Operation, idx int, refs *ir.IDReferences) []ir } key := paramLocation{param: pb.Param, location: pb.Location} if perLocation[key]++; perLocation[key] == 2 { + where := bindingWhere(op.ID, idx) diags = append(diags, diag(ir.SeverityError, "pass/param-binding-mismatch", fmt.Sprintf("parameter %q is bound more than once in location %q on %s", name, pb.Location, where), where)) } } - return append(diags, unboundParamWarnings(op, bound, where)...) + return append(diags, unboundParamWarnings(op, idx, bound)...) +} + +// bindingWhere spells where HTTP binding idx of operation op sits, for a +// diagnostic to be placed there. +func bindingWhere(op ir.OpID, idx int) string { + return fmt.Sprintf("%s/bindings/http/%d", op, idx) } -// unboundParamWarnings reports each logical parameter that no binding placed on -// the wire. -func unboundParamWarnings(op ir.Operation, bound map[ir.ParamID]int, where string) []ir.Diagnostic { +// unboundParamWarnings reports each logical parameter that HTTP binding idx of +// op placed nowhere on the wire. +func unboundParamWarnings(op ir.Operation, idx int, bound map[ir.ParamID]int) []ir.Diagnostic { var diags []ir.Diagnostic for _, p := range op.Params { if bound[p.ID] == 0 { + where := bindingWhere(op.ID, idx) diags = append(diags, diag(ir.SeverityWarning, "pass/param-binding-mismatch", fmt.Sprintf("parameter %q on %s is not bound to any HTTP location", p.Name.Source, where), where)) @@ -102,30 +122,31 @@ func unboundParamWarnings(op ir.Operation, bound map[ir.ParamID]int, where strin } // paramRef is one reference an operation makes to a parameter outside its HTTP -// bindings, with the location it is reported at. +// bindings, with the field of the operation it sits in, which spells where it +// is reported. type paramRef struct { id ir.ParamID - where string + field string } -// operationParamRefs lists the parameter references op carries besides its -// bindings: the pagination inputs and the idempotency token. An absent input is -// no reference, and neither is an empty token, which is how an operation that -// is not token-keyed spells having none. An input with an empty ID is one. -func operationParamRefs(op ir.Operation) []paramRef { - var refs []paramRef +// appendParamRefs appends to dst the parameter references op carries besides +// its bindings: the pagination inputs and the idempotency token. An absent +// input is no reference, and neither is an empty token, which is how an +// operation that is not token-keyed spells having none. An input with an empty +// ID is one. +func appendParamRefs(dst []paramRef, op ir.Operation) []paramRef { if pg := op.Pagination; pg != nil { if pg.InputCursor != nil { - refs = append(refs, paramRef{pg.InputCursor.Param, fmt.Sprintf("%s/pagination/inputCursor", op.ID)}) + dst = append(dst, paramRef{pg.InputCursor.Param, "pagination/inputCursor"}) } if pg.InputLimit != nil { - refs = append(refs, paramRef{pg.InputLimit.Param, fmt.Sprintf("%s/pagination/inputLimit", op.ID)}) + dst = append(dst, paramRef{pg.InputLimit.Param, "pagination/inputLimit"}) } } if token := op.Idempotency.TokenParam; token != "" { - refs = append(refs, paramRef{token, fmt.Sprintf("%s/idempotency/tokenParam", op.ID)}) + dst = append(dst, paramRef{token, "idempotency/tokenParam"}) } - return refs + return dst } // checkParamReferences holds an operation's pagination inputs and idempotency @@ -134,21 +155,26 @@ func operationParamRefs(op ir.Operation) []paramRef { // Another operation's parameter is a well-formed reference, so the registry walk // has nothing to say about it, while an emitter reads the pagination cursor off // the wrong operation's parameter. What the dangling walk reports is left to it -// (danglingWalkSkips). +// (danglingWalkSkips). Every operation is read through the same map and list, +// so none costs one of its own. func checkParamReferences(doc *ir.Document, refs *ir.IDReferences) []ir.Diagnostic { var diags []ir.Diagnostic + var declared reusedMap[ir.ParamID, bool] + var carried []paramRef ir.ForEachOperation(doc, func(op ir.Operation) { - own := make(map[ir.ParamID]bool, len(op.Params)) + own := declared.emptied() for _, p := range op.Params { own[p.ID] = true } - for _, ref := range operationParamRefs(op) { + carried = appendParamRefs(carried[:0], op) + for _, ref := range carried { if own[ref.id] || !danglingWalkSkips(ref.id, refs) { continue } + where := string(op.ID) + "/" + ref.field diags = append(diags, diag(ir.SeverityError, "pass/param-binding-mismatch", - fmt.Sprintf("%s names parameter %q, which this operation does not declare", ref.where, ref.id), - ref.where)) + fmt.Sprintf("%s names parameter %q, which this operation does not declare", where, ref.id), + where)) } }) return diags diff --git a/pass/validate_params_test.go b/pass/validate_params_test.go index cf7e8b587..67c2bf241 100644 --- a/pass/validate_params_test.go +++ b/pass/validate_params_test.go @@ -108,6 +108,50 @@ func TestValidate_ParamBinding_OtherOperationsParamIsRejected(t *testing.T) { assert.Contains(t, mismatches[0], "does not declare") } +// TestValidate_ParamBinding_EachBindingIsJudgedAlone pins that an operation's +// HTTP bindings are judged one at a time, each report placed at its binding. +// The first binding places the parameter once: a second placing nothing +// leaves it unbound there, one placing it once binds it once there too, and +// the second's repeat and foreign parameter are the second's. +func TestValidate_ParamBinding_EachBindingIsJudgedAlone(t *testing.T) { + t.Parallel() + own := ir.HTTPParamBinding{Param: "param/x/op/id/query", Location: ir.HTTPLocationQuery} + foreign := ir.HTTPParamBinding{Param: "param/x/other/id/query", Location: ir.HTTPLocationQuery} + type report struct { + severity ir.Severity + message string + } + for _, tc := range []struct { + name string + second []ir.HTTPParamBinding + want []report + }{ + {"placing nothing", nil, []report{{ir.SeverityWarning, + `parameter "id" on op/bindings/http/1 is not bound to any HTTP location`}}}, + {"placing it once", []ir.HTTPParamBinding{own}, []report{}}, + {"placing it twice", []ir.HTTPParamBinding{own, own}, []report{{ir.SeverityError, + `parameter "id" is bound more than once in location "query" on op/bindings/http/1`}}}, + {"placing another operation's parameter", []ir.HTTPParamBinding{own, foreign}, []report{{ir.SeverityError, + `binding on op/bindings/http/1 names parameter "param/x/other/id/query", which this operation does not declare`}}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + op := paramBindingOp("op", ir.Parameter{ID: own.Param, Name: ir.Naming{Source: "id"}}) + op.Bindings.HTTP = append(op.Bindings.HTTP, + ir.HTTPBinding{Method: "POST", URITemplate: "/x", ParamBindings: tc.second}) + other := paramBindingOp("other", ir.Parameter{ID: foreign.Param, Name: ir.Naming{Source: "id"}}) + + found := withCode(validated(t, docWithOperations(op, other)), "pass/param-binding-mismatch") + got := make([]report, 0, len(found)) + for _, d := range found { + assert.Equal(t, "op/bindings/http/1", d.Provenance.Node, d.Message) + got = append(got, report{d.Severity, d.Message}) + } + assert.Equal(t, tc.want, got) + }) + } +} + // TestValidate_HostParamBoundMultipleTimesIsLegal covers the host-location // continue: a host label may be filled by a param several times without a // double-bind error. diff --git a/pass/validate_payloads.go b/pass/validate_payloads.go index 29dae2c16..21ecf9e92 100644 --- a/pass/validate_payloads.go +++ b/pass/validate_payloads.go @@ -4,6 +4,7 @@ import ( "fmt" "maps" "slices" + "strconv" "github.com/dexpace/morphic/ir" "github.com/dexpace/morphic/ir/irfacts" @@ -22,8 +23,9 @@ import ( // it. Only this pass reports it today. facts indexes doc. func checkEncodingKeys(doc *ir.Document, facts *irfacts.Index) []ir.Diagnostic { var diags []ir.Diagnostic + var keys []ir.PropID // every content's keys are sorted in this one list forEachPayload(doc, func(site payloadSite) { - diags = appendEncodingKeyDiags(diags, facts, site.payload, site.where) + diags = appendEncodingKeyDiags(diags, facts, site, &keys) }) return diags } @@ -42,23 +44,42 @@ func checkEncodingKeys(doc *ir.Document, facts *irfacts.Index) []ir.Diagnostic { func checkPayloadRequired(doc *ir.Document) []ir.Diagnostic { var diags []ir.Diagnostic forEachPayload(doc, func(site payloadSite) { - if site.request || site.payload.Required == nil { + if site.request || site.required == nil { return } + where := site.where() diags = append(diags, diag(ir.SeverityError, "ir/payload-required-outside-request", - fmt.Sprintf("payload at %s sets required, which only a request body can state", site.where), - site.where)) + fmt.Sprintf("payload at %s sets required, which only a request body can state", where), + where)) }) return diags } -// payloadSite is one Payload a document carries: the node, where it hangs, and -// whether that position is a request — the one place Payload.Required is -// defined. +// payloadSite is one Payload a document carries: what the checks read of it, +// where it hangs, and whether that position is a request, the one place +// Payload.Required is defined. Where it hangs is kept as its parts, the ID of +// the node holding it, the field it hangs from and its index there, or -1 for +// none, and spelled only for a diagnostic. type payloadSite struct { - payload *ir.Payload - where string - request bool + contents []ir.Content + required *bool + owner string + field string + index int + request bool +} + +// where spells where the payload hangs: its owner's ID, then its field and its +// index there, each after a "/". +func (s payloadSite) where() string { + where := s.owner + if s.field != "" { + where += "/" + s.field + } + if s.index >= 0 { + where += "/" + strconv.Itoa(s.index) + } + return where } // forEachPayload calls fn once per Payload the document carries, skipping the @@ -75,67 +96,79 @@ type payloadSite struct { // leave the other unjudged. func forEachPayload(doc *ir.Document, fn func(payloadSite)) { for _, svc := range doc.Services { - forEachErrorPayload(svc.CommonErrors, string(svc.ID)+"/commonErrors", fn) + forEachErrorPayload(svc.CommonErrors, string(svc.ID), "commonErrors", fn) } ir.ForEachOperation(doc, func(op ir.Operation) { + owner := string(op.ID) if op.Request != nil { - fn(payloadSite{payload: op.Request, where: string(op.ID) + "/request", request: true}) + fn(siteOf(*op.Request, owner, "request", -1, true)) } for i, r := range op.Responses { if r.Payload != nil { - fn(payloadSite{payload: r.Payload, where: fmt.Sprintf("%s/responses/%d", op.ID, i)}) + fn(siteOf(*r.Payload, owner, "responses", i, false)) } } - forEachErrorPayload(op.Errors, string(op.ID)+"/errors", fn) + forEachErrorPayload(op.Errors, owner, "errors", fn) if op.Default != nil && op.Default.Payload != nil { - fn(payloadSite{payload: op.Default.Payload, where: string(op.ID) + "/default"}) + fn(siteOf(*op.Default.Payload, owner, "default", -1, false)) } }) for _, id := range slices.Sorted(maps.Keys(doc.Messages)) { - msg := doc.Messages[id] - fn(payloadSite{payload: &msg.Payload, where: string(id)}) + fn(siteOf(doc.Messages[id].Payload, string(id), "", -1, false)) } } -// forEachErrorPayload calls fn once per error case carrying a payload; where -// locates the list the cases hang from. -func forEachErrorPayload(errs []ir.ErrorCase, where string, fn func(payloadSite)) { +// forEachErrorPayload calls fn once per error case carrying a payload; owner +// and field locate the list the cases hang from. +func forEachErrorPayload(errs []ir.ErrorCase, owner, field string, fn func(payloadSite)) { for i, ec := range errs { if ec.Payload != nil { - fn(payloadSite{payload: ec.Payload, where: fmt.Sprintf("%s/%d", where, i)}) + fn(siteOf(*ec.Payload, owner, field, i, false)) } } } +// siteOf is the payloadSite of p, hanging from field of owner at index, or at +// none for -1, request saying whether that position is a request. +func siteOf(p ir.Payload, owner, field string, index int, request bool) payloadSite { + return payloadSite{contents: p.Contents, required: p.Required, owner: owner, field: field, index: index, request: request} +} + // appendEncodingKeyDiags appends to dst a diagnostic per unresolvable encoding -// key in each of the payload's contents; where locates the payload's owner. +// key in each of the contents of the payload at site, sorting each content's +// keys in keys. // // A content whose type no entry declares exposes no part, so each of its keys // is reported beside checkDanglingRefs's finding on the type. The claims // differ, as with checkMapping. -func appendEncodingKeyDiags(dst []ir.Diagnostic, facts *irfacts.Index, payload *ir.Payload, where string) []ir.Diagnostic { - for i, c := range payload.Contents { - if len(c.Encoding) == 0 { - continue - } - at := fmt.Sprintf("%s/contents/%d", where, i) - dst = appendUnknownPartDiags(dst, facts, c, at) +func appendEncodingKeyDiags(dst []ir.Diagnostic, facts *irfacts.Index, site payloadSite, keys *[]ir.PropID) []ir.Diagnostic { + for i, c := range site.contents { + dst = appendUnknownPartDiags(dst, facts, c, site, i, keys) } return dst } -// appendUnknownPartDiags appends to dst a diagnostic per encoding key of one -// content that names no property the content's type exposes, in ascending key -// order so map iteration cannot reach the output. A key naming a declaration -// the type's flattening overrides passes, though ir-design §4.3 has it name the -// one taken: flattening every type asked about grows quadratically down a deep +// appendUnknownPartDiags appends to dst a diagnostic per encoding key of +// content i of the payload at site that names no property the content's type +// exposes. The keys are asked in ascending order, sorted in keys, so map +// iteration reaches neither the output nor the questions facts is asked, and +// only a key reported has its location spelled. A key naming a declaration the +// type's flattening overrides passes, though ir-design §4.3 has it name the one +// taken: flattening every type asked about grows quadratically down a deep // hierarchy. -func appendUnknownPartDiags(dst []ir.Diagnostic, facts *irfacts.Index, c ir.Content, where string) []ir.Diagnostic { - for _, key := range slices.Sorted(maps.Keys(c.Encoding)) { +func appendUnknownPartDiags(dst []ir.Diagnostic, facts *irfacts.Index, c ir.Content, site payloadSite, i int, + keys *[]ir.PropID, +) []ir.Diagnostic { + *keys = (*keys)[:0] + for key := range c.Encoding { + *keys = append(*keys, key) + } + slices.Sort(*keys) + for _, key := range *keys { if facts.Exposes(c.Type.Target, key) { continue } - at := where + "/encoding/" + string(key) + at := site.where() + "/contents/" + strconv.Itoa(i) + "/encoding/" + string(key) dst = append(dst, diag(ir.SeverityError, "ir/encoding-key-unknown-property", fmt.Sprintf("encoding key %q at %s addresses no property of the content's type %q", key, at, c.Type.Target), at)) diff --git a/pass/validate_payloads_test.go b/pass/validate_payloads_test.go index 9ef55030a..363199c79 100644 --- a/pass/validate_payloads_test.go +++ b/pass/validate_payloads_test.go @@ -129,6 +129,34 @@ func TestValidate_EncodingKeysNamingRealPropertiesAreClean(t *testing.T) { assert.Empty(t, validated(t, doc)) } +// TestValidate_EncodingKeyIsPlacedWhereItSits pins that an encoding key no +// property answers to is reported at its own position past the first: the +// second content of a request, the second response and the second error case, +// each after one carrying no such key. +func TestValidate_EncodingKeyIsPlacedWhereItSits(t *testing.T) { + t.Parallel() + ghost := multipartPayload(map[ir.PropID]ir.PartEncoding{"p/m/ghost": {Multi: true}}) + twoContents := ir.Payload{Contents: append( + []ir.Content{{MediaType: "application/json", Type: ir.TypeRef{Target: "t/m"}}}, ghost.Contents...)} + for _, tc := range []struct { + name string + op ir.Operation + at string + }{ + {"the second content", ir.Operation{ID: "op", Request: &twoContents}, "op/request/contents/1"}, + {"the second response", ir.Operation{ID: "op", Responses: []ir.Response{{}, {Payload: &ghost}}}, + "op/responses/1/contents/0"}, + {"the second error case", ir.Operation{ID: "op", Errors: []ir.ErrorCase{{}, {Payload: &ghost}}}, + "op/errors/1/contents/0"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, []string{tc.at + "/encoding/p/m/ghost"}, + pointers(withCode(validated(t, docWithOperation(tc.op)), "ir/encoding-key-unknown-property"))) + }) + } +} + // TestValidate_EncodingKeyThroughServiceCommonErrors covers the second position // an ir.ErrorCase hangs from. The carrier table above plants into an operation's // own Errors, which is one of two lists of the same node: a check reaching only diff --git a/pass/validate_refs.go b/pass/validate_refs.go index 5837b7659..c5dca5471 100644 --- a/pass/validate_refs.go +++ b/pass/validate_refs.go @@ -50,11 +50,9 @@ func checkDanglingRefs(refs *ir.IDReferences, truncated bool) []ir.Diagnostic { // a token parser, and a false positive in prose is noisier than a missing // check. func checkPropIDRefs(props *propIDSites, refs *ir.IDReferences) []ir.Diagnostic { - var diags []ir.Diagnostic - for _, ref := range byLocation(props.sites) { - if refs.Declares(propIDType, ref.ID) { - continue - } + dangling := byLocation(props.dangling(refs)) + diags := make([]ir.Diagnostic, 0, len(dangling)) + for _, ref := range dangling { diags = append(diags, diag(ir.SeverityError, "ir/dangling-prop-ref", fmt.Sprintf("prop reference %q at %s resolves to no property declared in the document", ref.ID, ref.Path), ref.Path)) @@ -84,9 +82,9 @@ func byLocation(sites []ir.IDReference) []ir.IDReference { var propIDType = reflect.TypeFor[ir.PropID]() // propIDSites gathers, as an ir.Visitor, the ir.PropID references a walk -// reaches that may not resolve. One naming a property refs has already read, -// the property's own ID among them, resolves, so it is not kept and its path -// never rendered. +// reaches that may not resolve, each with its path kept unspelled. One naming +// a property refs has already read, the property's own ID among them, +// resolves, so it is not kept. // // A PropID reached as a map key is left out. The one PropID-keyed map the IR // declares is Content.Encoding, and checkEncodingKeys resolves those keys @@ -96,19 +94,35 @@ var propIDType = reflect.TypeFor[ir.PropID]() type propIDSites struct { refs *ir.IDReferences sites []ir.IDReference + paths ir.PathLog } // Visit keeps v if it is a PropID reference that may not resolve. func (p *propIDSites) Visit(v reflect.Value, at ir.WalkPath) bool { - if v.Kind() != reflect.String || v.Type() != propIDType || v.String() == "" { + if v.Kind() != reflect.String || v.Type() != propIDType || v.String() == "" || at.IsMapKey() { return true } if p.refs.Declares(propIDType, v.String()) { return true } - where := at.String() - if !strings.HasSuffix(where, ir.MapKeySuffix) { - p.sites = append(p.sites, ir.IDReference{Class: propIDType, ID: v.String(), Path: where}) - } + p.sites = append(p.sites, ir.IDReference{Class: propIDType, ID: v.String()}) + p.paths.Record(at) return true } + +// dangling returns, in walk order, the references p kept that name no +// property refs declares now the walk is over, each with its path. Only these +// have their paths spelled. +func (p *propIDSites) dangling(refs *ir.IDReferences) []ir.IDReference { + if !slices.ContainsFunc(p.sites, func(s ir.IDReference) bool { return !refs.Declares(propIDType, s.ID) }) { + return nil + } + var out []ir.IDReference + for i, at := range p.paths.All() { + if s := p.sites[i]; !refs.Declares(propIDType, s.ID) { + s.Path = at.String() + out = append(out, s) + } + } + return out +} diff --git a/pass/validate_refs_test.go b/pass/validate_refs_test.go index f56e9634d..327874748 100644 --- a/pass/validate_refs_test.go +++ b/pass/validate_refs_test.go @@ -865,10 +865,11 @@ func TestCheckDanglingRefs_NilTypeDefIsNotFollowed(t *testing.T) { // TestPropIDSites_KeepOnlyWhatIsNotYetDeclared holds the PropID walk to // settling what it can as it goes. A model's discriminator follows its -// properties, so naming one is settled on the spot and its path never rendered; -// a pagination path sits in a service, which the walk reaches before any type, -// so it is kept until every property is read, and then only the ghost dangles. -// It is not run in parallel: AllocsPerRun measures the process. +// properties, so naming one is settled on the spot; a pagination path sits in a +// service, which the walk reaches before any type, so it is kept until every +// property is read, and then only the ghost dangles. Keeping one costs no +// allocation, as its path is spelled only once it dangles. It is not run in +// parallel: AllocsPerRun measures the process. func TestPropIDSites_KeepOnlyWhatIsNotYetDeclared(t *testing.T) { doc := docWithOperation(ir.Operation{ID: "op/o", Pagination: &ir.Pagination{ Items: &ir.PropPath{Segments: []ir.PropID{"p/m/a", "p/ghost"}}, @@ -882,8 +883,8 @@ func TestPropIDSites_KeepOnlyWhatIsNotYetDeclared(t *testing.T) { "doc.Services[0].Groups[0].Operations[0].Pagination.Items.Segments[1]", } assert.Equal(t, kept, pass.KeptPropIDs(t, doc)) - assert.Equal(t, kept, irtest.AllocatingVisits(doc, pass.WalkVisitors(t, doc)...), - "a path is rendered only for what is kept") + assert.Empty(t, irtest.AllocatingVisits(doc, pass.WalkVisitors(t, doc)...), + "keeping a reference spells no path") var dangling []string for _, d := range validated(t, doc) { if d.Code == "ir/dangling-prop-ref" { @@ -893,6 +894,20 @@ func TestPropIDSites_KeepOnlyWhatIsNotYetDeclared(t *testing.T) { assert.Equal(t, []string{"doc.Services[0].Groups[0].Operations[0].Pagination.Items.Segments[1]"}, dangling) } +// TestCheckPropIDRefs_SettlingAKeptReferenceCostsNothing holds settling a +// PropID the walk kept to spelling nothing once it resolves: a pagination path +// sits in a service, which the walk reaches before the property it names. It +// is not run in parallel: AllocsPerRun measures the process. +func TestCheckPropIDRefs_SettlingAKeptReferenceCostsNothing(t *testing.T) { + doc := docWithOperation(ir.Operation{ID: "op/o", Pagination: &ir.Pagination{ + Items: &ir.PropPath{Segments: []ir.PropID{"p/m/a"}}, + }}) + require.NotEmpty(t, pass.KeptPropIDs(t, doc), "the reference is kept, or nothing is settled") + settle := pass.PropIDRefs(t, doc) + require.Empty(t, settle(), "the kept reference resolves once every property is read") + assert.Zero(t, testing.AllocsPerRun(5, func() { _ = settle() })) +} + // danglingRefs runs checkDanglingRefs over what one walk of doc gathers, as // Validate does. func danglingRefs(t *testing.T, doc *ir.Document) []ir.Diagnostic { diff --git a/pass/validate_wirenames.go b/pass/validate_wirenames.go index 6cd64c80a..b18bba97d 100644 --- a/pass/validate_wirenames.go +++ b/pass/validate_wirenames.go @@ -7,15 +7,17 @@ import ( ) // checkDuplicateWireNames reports wire-name collisions within a single model's -// own properties. +// own properties. Every model is read through the same map, so none costs one +// of its own. func checkDuplicateWireNames(doc *ir.Document) []ir.Diagnostic { var diags []ir.Diagnostic + var names reusedMap[string, bool] for _, id := range liveTypeIDs(doc) { m, ok := doc.Types[id].(*ir.Model) if !ok { continue } - seen := make(map[string]bool, len(m.Properties)) + seen := names.emptied() for _, p := range m.Properties { name := p.EffectiveWireName() if name == "" { @@ -44,9 +46,10 @@ func checkDuplicateWireNames(doc *ir.Document) []ir.Diagnostic { // (ir/naming-alias-duplicate, ir/naming-alias-redundant). func checkAmbiguousAliases(doc *ir.Document) []ir.Diagnostic { var diags []ir.Diagnostic + var claims reusedMap[string, int] for _, id := range liveTypeIDs(doc) { if m, ok := doc.Types[id].(*ir.Model); ok { - diags = append(diags, ambiguousAliases(id, m.Properties)...) + diags = append(diags, ambiguousAliases(id, m.Properties, &claims)...) } } return diags @@ -56,9 +59,10 @@ func checkAmbiguousAliases(doc *ir.Document) []ir.Diagnostic { // wire name is claimed before any alias, so an alias meeting a sibling's wire // name is reported whichever comes first; an alias meeting a sibling's alias is // reported at the later one. Either way the alias is reported, since it is the -// name a fix renames or removes, and the message names the sibling. -func ambiguousAliases(id ir.TypeID, props []ir.Property) []ir.Diagnostic { - claimant := make(map[string]int, len(props)) +// name a fix renames or removes, and the message names the sibling. The names +// are read through claims. +func ambiguousAliases(id ir.TypeID, props []ir.Property, claims *reusedMap[string, int]) []ir.Diagnostic { + claimant := claims.emptied() for i, p := range props { if name := p.EffectiveWireName(); name != "" { if _, taken := claimant[name]; !taken { diff --git a/pass/validate_wirenames_test.go b/pass/validate_wirenames_test.go index c8b50c7eb..39a1d3d32 100644 --- a/pass/validate_wirenames_test.go +++ b/pass/validate_wirenames_test.go @@ -135,8 +135,10 @@ func TestValidate_AmbiguousAliasIsReported(t *testing.T) { // TestValidate_UnambiguousAliasIsClean pins what the check leaves alone. An // alias repeating its own property's name is irverify's, as is a repeat within -// one list; an empty alias answers to nothing; and the same alias on two -// models is two scopes, since a field alias is resolved within its record. +// one list; an empty alias answers to nothing; and two models are two scopes, +// since a field alias is resolved within its record, whether the other model +// holds the same alias or, read just before, a property of that name at +// another position. func TestValidate_UnambiguousAliasIsClean(t *testing.T) { t.Parallel() twoModels := aliasDoc(aliasProperty("b", "x")) @@ -144,12 +146,15 @@ func TestValidate_UnambiguousAliasIsClean(t *testing.T) { {ID: "p/n/c", Name: ir.Naming{Source: "c", Canonical: "c", Aliases: []string{"x"}}, Type: ir.TypeRef{Target: "t/prim/string"}}, }} + claimedBefore := aliasDoc(aliasProperty("b", "x"), aliasProperty("c")) + claimedBefore.Types["t/a"] = &ir.Model{ID: "t/a", Properties: []ir.Property{aliasProperty("a"), aliasProperty("x")}} for name, doc := range map[string]*ir.Document{ - "distinct aliases": aliasDoc(aliasProperty("a", "x"), aliasProperty("b", "y")), - "its own name": aliasDoc(aliasProperty("a", "a")), - "repeated in one list": aliasDoc(aliasProperty("a", "x", "x")), - "empty on two siblings": aliasDoc(aliasProperty("a", ""), aliasProperty("b", "")), - "same alias, two models": twoModels, + "distinct aliases": aliasDoc(aliasProperty("a", "x"), aliasProperty("b", "y")), + "its own name": aliasDoc(aliasProperty("a", "a")), + "repeated in one list": aliasDoc(aliasProperty("a", "x", "x")), + "empty on two siblings": aliasDoc(aliasProperty("a", ""), aliasProperty("b", "")), + "same alias, two models": twoModels, + "a name another model reads first": claimedBefore, } { t.Run(name, func(t *testing.T) { t.Parallel()