From f39d08e731c06762992bd334dd182e8bd714dffa Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 14:24:36 +0300 Subject: [PATCH 01/31] perf(ir): return a BigVal already in JSON form as it is NewBigVal built a new string for every literal, through a strings.Builder, even when the result spelled the literal it was given, and read the literal's range through a Decimal whose digits joined the runs either side of the point. A literal in JSON form already now comes back as it is: the scanner records whether a point was written, and the three affixes the JSON spelling rewrites are checked where they lie. The range is read by counting digits rather than joining them, so only ParseDecimal, which keeps the digits, pays for the join. A canonical literal now costs NewBigVal nothing, and a rewrite the one string it builds. irverify asks NewBigVal of every numeric literal a document holds, and the compilers of every literal a spec writes. --- ir/bigval.go | 16 ++++++++++- ir/bigval_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++ ir/decimal.go | 72 ++++++++++++++++++++++++++++++++--------------- ir/export_test.go | 10 +++++++ 4 files changed, 140 insertions(+), 24 deletions(-) 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/export_test.go b/ir/export_test.go index 3a80a2e18..d9d506a46 100644 --- a/ir/export_test.go +++ b/ir/export_test.go @@ -18,6 +18,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 From 820f07dbdbcb9851bff821ed40ee5f5727c7966e Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 14:35:24 +0300 Subject: [PATCH 02/31] feat(ir): tell a visitor which field the walk reached a value by irverify exempted a primitive's empty name by rendering the path of every primitive it met, with ".Name" appended, and looking each Naming that names nothing up among them. The record cost a string per primitive, leaned on the walk visiting a struct before its fields, and matched by spelling: a struct embedding a primitive beside a Name of its own shares that path, so its own missing name was exempted too. WalkPath.Field reports the struct field a path ends in, as String spells it: the field's name and the struct it is spelled on, which for a field promoted through embedded structs is the outermost embedding one. It reads the walk's own steps and allocates nothing. checkNaming now asks it of a Naming that names nothing, keeps no state for the exemption, and exempts exactly the Name of a node nameOptional lists. Alias lists are read entry by entry rather than copied into a slice, and held through one map the check keeps and empties for each list, so a list longer than a map holds unallocated no longer costs a map of its own. --- ir/irverify/naming.go | 74 +++++++++---------- ir/irverify/naming_internal_test.go | 85 ++++++++++++++++++++-- ir/irverify/naming_test.go | 11 +-- ir/walk.go | 31 ++++++++ ir/walk_path_test.go | 107 ++++++++++++++++++++++++++++ 5 files changed, 257 insertions(+), 51 deletions(-) diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 7519f150b..ff5726c58 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 held through this one map, emptied for each, so a + // list costs no map of its own however long it is. + seen := map[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, seen) 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 { +// seen is the map appendAliasViolations holds the aliases through. +func appendNamingChecks(vs []Violation, naming reflect.Value, at ir.WalkPath, seen map[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, seen) } // 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, + seen map[string]int, +) []Violation { + clear(seen) // 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..bf2173959 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,73 @@ 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, held inside an unlisted +// node whose Name is not exempt either. +func TestNameExempt_IsTheOptionalNodesOwnNameAlone(t *testing.T) { + t.Parallel() + type twoNamings struct{ Name, Other ir.Naming } + 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.Unlisted.Name": false, + }, got) +} + +// TestCheckNaming_ExemptsAPrimitivesOwnNameWhereverItSits 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, and a struct embedding +// one does not inherit the exemption, though its own Name and the primitive's +// promoted one share a path. An exemption recorded by path silenced both there. +func TestCheckNaming_ExemptsAPrimitivesOwnNameWhereverItSits(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..1e0f47a57 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,8 +359,8 @@ 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 +// 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 @@ -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/walk.go b/ir/walk.go index 987f80dcc..a8805fc2a 100644 --- a/ir/walk.go +++ b/ir/walk.go @@ -160,6 +160,31 @@ 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 Go promotes through +// an embedded struct is spelled on the embedding struct, 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 +} + // stepKind is how a value is reached from its parent. type stepKind uint8 @@ -199,6 +224,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..8f4d8e7f8 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 two embedded structs, 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 two 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) { From 12df418c4a5bd85ed54e5a7beabeecf7c5388638 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 14:39:33 +0300 Subject: [PATCH 03/31] perf(irverify): fingerprint a property without building a string checkDuplicateIDs rendered the path of every property the walk reached and joined each property's three fingerprint parts into a string, so that the report could look up a declaration's fingerprint by its path. Both strings were thrown away for every property a document holds once, and for every copy a reused component makes that matches the first. A fingerprint is now a struct of the property's own strings, kept per PropID for the first property the walk meets on it. A later property is compared with it as the walk reaches it, and only one that differs, which is the defect, has its path rendered. The walk meets properties in the order the declarations list them, so the first property is the first declaration; a test pins that ir.Property is the only node declaring a PropID, since another would repeat one unfingerprinted. --- ir/irverify/duplicates.go | 73 ++++++++++++---------- ir/irverify/duplicates_internal_test.go | 80 +++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 33 deletions(-) diff --git a/ir/irverify/duplicates.go b/ir/irverify/duplicates.go index 6194ffcda..a3410eb70 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,67 @@ 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 fingerprint of the first property met on each PropID, and the paths + // of the properties met later whose fingerprint differs from it. The walk + // meets properties in the order the declarations list them, so the first + // met is the PropID's first declaration, and only a property that differs, + // which is a defect, has its path rendered. + first := map[string]fingerprint{} + differs := map[string]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) + if v.Kind() != reflect.Struct || v.Type() != propertyType { + return true + } + id, got := v.FieldByName("ID").String(), fingerprintOf(v) + switch met, seen := first[id]; { + case !seen: + first[id] = got + case met != got: + differs[at.String()] = true } return true }, report: func(decls declarations) []Violation { - return duplicateIDs(decls.ids, fingerprints) + return duplicateIDs(decls.ids, differs) }, } } // 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)) +// it, unless it is a property that is the first one's copy: of the properties +// declared again, differs holds the paths of those that are not. +func duplicateIDs(decls []ir.IDDeclaration, differs map[string]bool) []Violation { + first := make(map[identity]string, len(decls)) var vs []Violation for _, d := range decls { key := identity{class: d.Class, id: d.ID} at, taken := first[key] if !taken { - first[key] = declaredAt{path: d.Path, fingerprint: fingerprints[d.Path]} + first[key] = d.Path continue } - if d.Class == propIDType && at.fingerprint == fingerprints[d.Path] { + if d.Class == propIDType && !differs[d.Path] { continue // one property materialized at several paths, not two properties } 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), + Message: "id " + strconv.Quote(d.ID) + " is declared here and at " + strconv.Quote(at), Path: d.Path, }) } 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 ad2e473e6..70acd1f4d 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) { From 444ea724181dd962f4d0943f60eb9697fe0185e9 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 14:50:08 +0300 Subject: [PATCH 04/31] perf(irverify): spell a registry entry's path only to report it The checks that range over the type registry, the auth schemes and the namespace declaration built a path for every entry before knowing there was anything wrong with it: checkPrimIDs and checkUnions for every type or union, checkRequires for every entry, and appendSpaceViolations for every namespace. Each now spells it in the branch that reports, through one typePath helper for the type registry's spelling. memberScopeViolation joined an enum's path and the separator to compare each member against; beneath compares the two in place. checkMemberIDs also sorted every type ID to visit enums in ID order, but each violation's path names its enum and member, so Verify's own sort already orders them. checkRequires made two maps per model and checkOAuthScopes one per flow, which allocate once a model holds more than eight properties or a flow more than eight scopes. Each check now holds one map of each and empties it for the next model or flow. A test holds every function shaped like a registry check to a cost that does not grow with the entries it ranges over, and a second one holds the list it measures to every such function the package declares. --- ir/irverify/ids.go | 15 ++- ir/irverify/idspaces.go | 11 +- ir/irverify/irverify.go | 8 +- ir/irverify/kinds.go | 2 +- ir/irverify/maps.go | 4 +- ir/irverify/memberids.go | 21 ++-- ir/irverify/registrychecks_internal_test.go | 131 ++++++++++++++++++++ ir/irverify/requires.go | 18 ++- ir/irverify/requires_test.go | 20 +++ ir/irverify/scopes.go | 5 +- ir/irverify/unions.go | 5 +- 11 files changed, 209 insertions(+), 31 deletions(-) create mode 100644 ir/irverify/registrychecks_internal_test.go 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..9b876c512 100644 --- a/ir/irverify/idspaces.go +++ b/ir/irverify/idspaces.go @@ -110,21 +110,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/irverify.go b/ir/irverify/irverify.go index cf33d8c23..53a04cd90 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -188,7 +188,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 } @@ -206,6 +206,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/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/registrychecks_internal_test.go b/ir/irverify/registrychecks_internal_test.go new file mode 100644 index 000000000..0bcce5491 --- /dev/null +++ b/ir/irverify/registrychecks_internal_test.go @@ -0,0 +1,131 @@ +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. It is not run in parallel: AllocsPerRun measures +// the process. +func TestRegistryChecks_ACleanEntryCostsNothing(t *testing.T) { + few, many := cleanRegistry(10), cleanRegistry(20) + 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 7d3983c8c..f16f5e37b 100644 --- a/ir/irverify/requires.go +++ b/ir/irverify/requires.go @@ -12,30 +12,32 @@ import ( // a repeat admits nothing more. A repeat is reported at its later entry. func checkRequires(doc *ir.Document) []Violation { var vs []Violation + // Emptied for each model rather than made afresh, so a model costs no map + // of its own however many properties and entries it has. + own, first := map[string]bool{}, map[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)) + clear(own) for _, p := range m.Properties { own[p.WireName] = true } - first := make(map[string]int, len(m.Requires)) + clear(first) 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 +46,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 d2a912aaf..45c184559 100644 --- a/ir/irverify/requires_test.go +++ b/ir/irverify/requires_test.go @@ -120,3 +120,23 @@ func TestVerify_RequiresHoldsNoOwnPropertyAndNoRepeat(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/scopes.go b/ir/irverify/scopes.go index 535955faa..eeabfc3c9 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 + // Emptied for each flow rather than made afresh, so a flow costs no map of + // its own however many scopes it offers. + seen := map[string]bool{} for id, scheme := range doc.Auth { for i, flow := range scheme.Flows { - seen := make(map[string]bool, len(flow.Scopes)) + clear(seen) for j, scope := range flow.Scopes { if seen[scope.Name] { vs = append(vs, Violation{ 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), }) } } From a6c43f3ff7adfaa8c56be05c7c17b3fc1f83f2e3 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 14:53:47 +0300 Subject: [PATCH 05/31] perf(irverify): read every string list through one map checkStringLists made a map for each []string field it holds to a rule, which allocates once a list holds more than eight entries. The check now keeps one map and empties it for each list, as checkNaming does for alias lists. A test also holds checkBigVals to costing nothing for a literal in canonical form, which ir.NewBigVal now hands back as it is, through both carriers: a Value's Num and a bound held by pointer. --- ir/irverify/bigval_internal_test.go | 33 ++++++++++++++++++++++++ ir/irverify/stringlists.go | 14 +++++++--- ir/irverify/stringlists_internal_test.go | 26 +++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 ir/irverify/bigval_internal_test.go 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/stringlists.go b/ir/irverify/stringlists.go index 1136e7c59..8f7cdecda 100644 --- a/ir/irverify/stringlists.go +++ b/ir/irverify/stringlists.go @@ -94,6 +94,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 map, emptied for each, so a + // list costs no map of its own however long it is. + first := map[string]int{} return walkCheck{ visit: func(v reflect.Value, at ir.WalkPath) bool { if v.Kind() != reflect.Struct { @@ -105,7 +108,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, first) } return true }, @@ -130,9 +133,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 +// first is the map the entries are read into, emptied before them. +func appendListViolations(vs []Violation, list reflect.Value, owner ir.WalkPath, f heldList, + first map[string]int, +) []Violation { + clear(first) 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") +} From 3099dd7d85970281d94d07024056290385a07084 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 15:23:10 +0300 Subject: [PATCH 06/31] test(ir/irtest): show each visitor the walk it would be shown alone AllocatingVisits walked a document with one visitor standing for all of them, so what any one pruned was pruned for every one. A visitor that prunes at the root, as one visiting nothing does, then hid the whole document from the others: handed every irverify walking check, the measurement reached no value below the root and reported a document clean with a path rendered for every property. Each visitor is now wrapped on its own and walked as ir.WalkValues walks it, so it prunes for itself alone, and a visitor gathering cycles is told of them. A value is listed once for each visitor allocating there. A visit allocating nothing is measured once rather than twenty times, as no later measurement can find fewer, which keeps a walk with many visitors affordable. --- ir/irtest/export_test.go | 3 ++ ir/irtest/visits.go | 68 ++++++++++++++++---------- ir/irtest/visits_test.go | 101 ++++++++++++++++++++++++++++++++++----- 3 files changed, 137 insertions(+), 35 deletions(-) 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..e4142312c 100644 --- a/ir/irtest/visits.go +++ b/ir/irtest/visits.go @@ -9,34 +9,51 @@ import ( ) // 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 -// empty without allocating. +// of each visit that must allocate, once for each visitor allocating there. +// Each visit is handed the walk's own path, which the zero ir.WalkPath cannot +// stand in for: it 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. +// Each visitor is shown the walk it would be shown alone, so one pruning even +// at the root hides nothing from the others. A visit is made once before it is +// measured, so what a first meeting builds is not counted (fewestAllocs). +// 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 @@ -45,14 +62,17 @@ func AllocatingVisits(doc *ir.Document, visitors ...ir.Visitor) []string { 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 From 93fd0f5cdfcc046cc2d819558c5700e8c95c7144 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 15:23:36 +0300 Subject: [PATCH 07/31] test(irverify): hold every walking check to costing nothing per value Each check that fixed a per-value allocation pins its own cost, but a check with no such test, or one written later, could still build a string for every value it reaches. One test now walks a document the OpenAPI compiler produces from extensions-x.yaml, which writes an extension on every object OpenAPI admits one on, and holds the walking checks together to no allocating visit. checkReferentialIntegrity is left out of it: ir.IDReferences keeps a reference whose target the walk has yet to declare with its path, until the walk is over, which is that type's design and pinned in ir. Verify's doc now states what a value costs a check and that exception. --- ir/irverify/export_test.go | 12 ++++++++++++ ir/irverify/irverify.go | 5 +++++ ir/irverify/irverify_corpus_test.go | 22 ++++++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/ir/irverify/export_test.go b/ir/irverify/export_test.go index 864bc243f..7ae5c4ae5 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,13 @@ func HeldLists() map[string]bool { } return held } + +// WalkChecks builds every walking check for doc, keyed by its function's name, +// as Verify's walk is shown them, for this package's external tests. +func WalkChecks(doc *ir.Document) map[string]ir.Visitor { + checks := map[string]ir.Visitor{} + for _, newCheck := range walkChecks() { + checks[checkName(newCheck)] = newCheck(doc) + } + return checks +} diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 53a04cd90..302e74e12 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -26,6 +26,11 @@ 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 nothing. What ir reads with a path is the +// exception: each declaration, and each reference whose target the walk has +// yet to reach. func Verify(doc *ir.Document) []Violation { return verify(doc, ir.WalkValues) } diff --git a/ir/irverify/irverify_corpus_test.go b/ir/irverify/irverify_corpus_test.go index ebefb93e9..a6153a2c5 100644 --- a/ir/irverify/irverify_corpus_test.go +++ b/ir/irverify/irverify_corpus_test.go @@ -14,6 +14,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 +72,27 @@ func TestVerify_Corpus(t *testing.T) { } } +// TestWalkChecks_ACleanValueCostsNothing holds the walking checks, together, to +// costing nothing for a value none has anything to report on, in a document the +// OpenAPI compiler produces: each builds a path, or any other string, only once +// it has a violation to place. extensions-x.yaml writes an extension on every +// object OpenAPI admits one on, so its document reaches nearly every node a +// compile produces. checkReferentialIntegrity is left out: a reference to what +// the walk has yet to declare is kept with its path, ir.IDReferences' design, +// which ir pins. 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") + + checks := irverify.WalkChecks(doc) + require.Contains(t, checks, "checkReferentialIntegrity") + delete(checks, "checkReferentialIntegrity") + assert.Empty(t, irtest.AllocatingVisits(doc, slices.Collect(maps.Values(checks))...)) +} + // 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 From 0dd9a89348319ac58fb25c146ae12036eaf08911 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 15:58:12 +0300 Subject: [PATCH 08/31] docs(irverify): name the inferred-marker set among a value's costs checkInferred, which main added beside this branch's changes, asks ir.Inferred.IsCanonical of every marker list, and that reads a list through a map of its own: free for the lists a compiler writes, one allocation for a list of more than eight names, as its doc chooses. Verify's doc said a value with nothing wrong costs a check nothing beyond the paths ir reads, which that list contradicts; it now names the set as the other exception. --- ir/irverify/irverify.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 26e824707..507f9a624 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -28,9 +28,10 @@ type Violation struct { // 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 nothing. What ir reads with a path is the -// exception: each declaration, and each reference whose target the walk has -// yet to reach. +// with nothing wrong costs it nothing. The exceptions are what ir reads for it: +// the path of each declaration and of each reference whose target the walk has +// yet to reach, and the set ir.Inferred.IsCanonical makes for a list longer +// than a map holds unallocated. func Verify(doc *ir.Document) []Violation { return verify(doc, ir.WalkValues) } From 4e223fd3b13cdd695d9916eeca3e46534e64dc0c Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 16:29:43 +0300 Subject: [PATCH 09/31] perf(irverify): read every raw payload through one decoder checkRawPayloads asked jsontext.Value.IsValid of each Unmodeled and RawConfig payload, which borrows a decoder from a sync.Pool and hands it back. A race build drops a share of what a pool is handed, so there a payload cost an allocation at random. The walking checks' cost test flagged an Unmodeled map of many entries for it under make coverage's -race build, while every other build measured nothing. The check now reads payloads through one jsontext.Decoder and one bytes.Reader it resets for each, which costs nothing once the decoder's buffer holds the largest payload, in every build. A test holds it to IsValid's verdict over every string of up to five bytes JSON's grammar turns on, and the shapes no short string reaches; a fuzz comparison of the two found no disagreement in five million inputs. A payload object with many members still costs the decoder an allocation per name it keeps to refuse a duplicate, as IsValid did, and Verify's doc names it beside ir's paths and the inferred-marker set. --- ir/irverify/irverify.go | 8 ++-- ir/irverify/rawpayloads.go | 57 ++++++++++++++++++------ ir/irverify/rawpayloads_internal_test.go | 56 +++++++++++++++++++++-- ir/irverify/rawpayloads_test.go | 4 +- ir/irverify/utf8_test.go | 4 +- 5 files changed, 103 insertions(+), 26 deletions(-) diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 507f9a624..3680ad2c3 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -28,10 +28,10 @@ type Violation struct { // 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 nothing. The exceptions are what ir reads for it: -// the path of each declaration and of each reference whose target the walk has -// yet to reach, and the set ir.Inferred.IsCanonical makes for a list longer -// than a map holds unallocated. +// with nothing wrong costs it nothing beyond what its readers keep: ir's path +// for each declaration and each reference met before its target, 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) } diff --git a/ir/irverify/rawpayloads.go b/ir/irverify/rawpayloads.go index 0376cc8fd..7baa23261 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 a payload cost 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..016dbb2d0 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 cost 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,40 @@ 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. 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", strings.Repeat("[", 20000)+strings.Repeat("]", 20000)) + 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/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{ From 1aa8777b1de2cb97cf351a464b3e39febdc08a96 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 18:07:02 +0300 Subject: [PATCH 10/31] perf(irverify): spell a source's path only to report it checkDocument built the path of every source in the table before holding it to either rule. The issue counted it free, since the string never escapes and stays short enough to build on the stack, but strconv.Itoa builds the string for an index of 100 or more, so a document read from more than 100 sources paid an allocation for each source past the hundredth. The path is now spelled in the branch that reports, as every other registry check spells its own. The registry checks' cost test measured 10 entries against 20, below the index at which that starts, so it could not see this; it now measures 100 against 200. --- ir/irverify/document.go | 13 ++++++++++--- ir/irverify/registrychecks_internal_test.go | 5 +++-- 2 files changed, 13 insertions(+), 5 deletions(-) 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/registrychecks_internal_test.go b/ir/irverify/registrychecks_internal_test.go index 0bcce5491..ec4ad3ebb 100644 --- a/ir/irverify/registrychecks_internal_test.go +++ b/ir/irverify/registrychecks_internal_test.go @@ -64,10 +64,11 @@ func TestRegistryChecks_ListsEveryRegistryCheck(t *testing.T) { // 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. It is not run in parallel: AllocsPerRun measures +// 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(10), cleanRegistry(20) + few, many := cleanRegistry(100), cleanRegistry(200) checks := map[string]func(*ir.Document) []Violation{ "declarationViolations": func(doc *ir.Document) []Violation { return declarationViolations(doc.IDSpaces) }, } From 731be69b6157808757f225d3c93adb74892fe2da Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 18:07:02 +0300 Subject: [PATCH 11/31] test(irverify): hold the name exemption off a node inside a listed one TestVerify_OptionalOwnerExemptsOnlyItsOwnName said the descendant axis, a nameless node inside an exempt one, had no pair to measure with while ir.Primitive is the only exemption. nameExempt takes its table, so TestNameExempt_IsTheOptionalNodesOwnNameAlone now holds a node of its own inside the listed one to the rule, and the comment points there. --- ir/irverify/naming_internal_test.go | 18 ++++++++++++------ ir/irverify/naming_test.go | 12 ++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/ir/irverify/naming_internal_test.go b/ir/irverify/naming_internal_test.go index bf2173959..f99ef0d11 100644 --- a/ir/irverify/naming_internal_test.go +++ b/ir/irverify/naming_internal_test.go @@ -45,11 +45,16 @@ func TestCheckNaming_VisitingCostsNoAllocation(t *testing.T) { // 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, held inside an unlisted -// node whose Name is not exempt either. +// 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 twoNamings struct{ Name, Other ir.Naming } + 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 } @@ -63,9 +68,10 @@ func TestNameExempt_IsTheOptionalNodesOwnNameAlone(t *testing.T) { return true })) assert.Equal(t, map[string]bool{ - "doc.Listed.Name": true, - "doc.Listed.Other": false, - "doc.Unlisted.Name": false, + "doc.Listed.Name": true, + "doc.Listed.Other": false, + "doc.Listed.Inner.Name": false, + "doc.Unlisted.Name": false, }, got) } diff --git a/ir/irverify/naming_test.go b/ir/irverify/naming_test.go index 1e0f47a57..452fb2cba 100644 --- a/ir/irverify/naming_test.go +++ b/ir/irverify/naming_test.go @@ -360,13 +360,13 @@ func TestVerify_NamelessServerAndResponseAreViolations(t *testing.T) { // TestVerify_OptionalOwnerExemptsOnlyItsOwnName is the overreach guard: the // 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. +// 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() From eeff0e48a2815f4b5460e00495ad9c4a3d586dda Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 18:07:02 +0300 Subject: [PATCH 12/31] docs(ir): say when a race build's pool drops defeat a measurement visitMeasurements' comment held the chance of a pool drop in every measurement negligible for the handful of pools a visit reaches, which leaves out a visit handing back dozens: checkRawPayloads' Unmodeled maps were one until it stopped pooling. It now says so, and two comments describing that cost read in the present. --- ir/irtest/visits.go | 5 +++-- ir/irverify/rawpayloads.go | 4 ++-- ir/irverify/rawpayloads_internal_test.go | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/ir/irtest/visits.go b/ir/irtest/visits.go index e4142312c..623553827 100644 --- a/ir/irtest/visits.go +++ b/ir/irtest/visits.go @@ -57,8 +57,9 @@ func (m measuredVisitor) Cycle(at, entered ir.WalkPath, ref reflect.Type) { // 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 diff --git a/ir/irverify/rawpayloads.go b/ir/irverify/rawpayloads.go index 7baa23261..128af74f8 100644 --- a/ir/irverify/rawpayloads.go +++ b/ir/irverify/rawpayloads.go @@ -147,8 +147,8 @@ func appendRawValue(vs []Violation, p *payloadReader, value reflect.Value, what // 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 a payload cost an allocation at random. The zero payloadReader is ready -// to use. +// 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 diff --git a/ir/irverify/rawpayloads_internal_test.go b/ir/irverify/rawpayloads_internal_test.go index 016dbb2d0..7d0e425a3 100644 --- a/ir/irverify/rawpayloads_internal_test.go +++ b/ir/irverify/rawpayloads_internal_test.go @@ -19,7 +19,7 @@ import ( // 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 cost some. It is not run in parallel: AllocsPerRun measures +// 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 { From ffa0ea5488e45c015a67916c931bc9fdef888676 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 20:13:32 +0300 Subject: [PATCH 13/31] docs(ir): state what the cost tests' own claims leave out AllocatingVisits said it lists paths in walk order, which holds for up to 64 visitors: past that ir.WalkValues walks again, so the list runs walk by walk. A walk-path test said a primitive's Name is promoted through two embedded structs where its own table counts three. The walking checks' cost test leaned on extensions-x.yaml holding none of the costs Verify's doc excepts, the one it leaves out aside, without saying so; a later edit to the spec adding a large extension object would redden it for a documented cost. --- ir/irtest/visits.go | 8 ++++---- ir/irverify/irverify_corpus_test.go | 14 +++++++------- ir/walk_path_test.go | 8 ++++---- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/ir/irtest/visits.go b/ir/irtest/visits.go index 623553827..fe2c606d3 100644 --- a/ir/irtest/visits.go +++ b/ir/irtest/visits.go @@ -8,10 +8,10 @@ import ( "github.com/dexpace/morphic/ir" ) -// AllocatingVisits walks doc with visitors and returns, in walk order, the path -// of each visit that must allocate, once for each visitor allocating there. -// Each visit is handed the walk's own path, which the zero ir.WalkPath cannot -// stand in for: it renders empty without allocating. +// AllocatingVisits walks doc with visitors and returns the path of each visit +// that must allocate, once for each visitor allocating there, in walk order, +// one walk per 64 visitors as ir.WalkValues makes. A visit is handed the walk's +// own path: the zero ir.WalkPath renders empty. // // Each visitor is shown the walk it would be shown alone, so one pruning even // at the root hides nothing from the others. A visit is made once before it is diff --git a/ir/irverify/irverify_corpus_test.go b/ir/irverify/irverify_corpus_test.go index a6153a2c5..dfe0e7159 100644 --- a/ir/irverify/irverify_corpus_test.go +++ b/ir/irverify/irverify_corpus_test.go @@ -73,13 +73,13 @@ func TestVerify_Corpus(t *testing.T) { } // TestWalkChecks_ACleanValueCostsNothing holds the walking checks, together, to -// costing nothing for a value none has anything to report on, in a document the -// OpenAPI compiler produces: each builds a path, or any other string, only once -// it has a violation to place. extensions-x.yaml writes an extension on every -// object OpenAPI admits one on, so its document reaches nearly every node a -// compile produces. checkReferentialIntegrity is left out: a reference to what -// the walk has yet to declare is kept with its path, ir.IDReferences' design, -// which ir pins. It is not run in parallel: AllocsPerRun measures the process. +// 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 but the one left out here: +// checkReferentialIntegrity keeps a reference to what the walk has yet to +// declare with its path, ir.IDReferences' design, which ir pins. 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) diff --git a/ir/walk_path_test.go b/ir/walk_path_test.go index 8f4d8e7f8..d34cfbaeb 100644 --- a/ir/walk_path_test.go +++ b/ir/walk_path_test.go @@ -75,9 +75,9 @@ func TestWalkPath_ZeroValueIsEmpty(t *testing.T) { assert.Equal(t, []byte("kept"), p.AppendTo([]byte("kept"))) } -// Roots that embed a primitive: Outer through two embedded structs, Shadowed -// beside a Name of its own, which hides the promoted one from Go but not from a -// walk. +// 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 } @@ -143,7 +143,7 @@ func TestWalkPath_FieldIsTheFieldThePathEndsIn(t *testing.T) { 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 two embedded structs is spelled on the outermost") + "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"], From 75f538f689ca88eefd4723c03ee06d1daba4446f Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 20:47:43 +0300 Subject: [PATCH 14/31] test(ir): hold a check to costing nothing on a first meeting AllocatingVisits measures a visit only once the walk has made it, so a cost a check pays on its first meeting with a value goes unmeasured. That is meant for a memo kept per type, but it hides state kept per value too: checkDuplicateIDs rendering and keeping a path for each PropID it meets, the cost this branch removed, passed every test in the package. TestWalkChecks_AFirstMeetingCostsNothing compiles a spec of 100 and of 200 components, each an operation and the schema it returns, and requires the walking checks to allocate less than one more per component added, with the walk's own allocations taken out. The checks add 2 in the plain build and in the race and coverage build; the planted path adds about 400. AllocatingVisits' doc now says what a first meeting hides. Verify's no longer says a sound value costs a check nothing while checkDuplicateIDs keeps a fingerprint per PropID: what it may cost is the step a map the check keeps grows by. --- ir/irtest/visits.go | 15 +++--- ir/irverify/irverify.go | 5 +- ir/irverify/irverify_corpus_test.go | 75 +++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 9 deletions(-) diff --git a/ir/irtest/visits.go b/ir/irtest/visits.go index fe2c606d3..294d5ecbe 100644 --- a/ir/irtest/visits.go +++ b/ir/irtest/visits.go @@ -9,14 +9,15 @@ import ( ) // AllocatingVisits walks doc with visitors and returns the path of each visit -// that must allocate, once for each visitor allocating there, in walk order, -// one walk per 64 visitors as ir.WalkValues makes. A visit is handed the walk's -// own path: the zero ir.WalkPath renders empty. +// 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. // -// Each visitor is shown the walk it would be shown alone, so one pruning even -// at the root hides nothing from the others. A visit is made once before it is -// measured, so what a first meeting builds is not counted (fewestAllocs). -// 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: a memo, but also anything 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 measured := make([]ir.Visitor, len(visitors)) diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 3680ad2c3..56bfd7c7d 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -28,8 +28,9 @@ type Violation struct { // 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 nothing beyond what its readers keep: ir's path -// for each declaration and each reference met before its target, the set +// with nothing wrong costs it no allocation but a step a map it keeps may grow +// by, beyond what its readers keep: ir's path for each declaration and each +// reference met before its target, 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 { diff --git a/ir/irverify/irverify_corpus_test.go b/ir/irverify/irverify_corpus_test.go index dfe0e7159..b53a51926 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" @@ -93,6 +96,78 @@ func TestWalkChecks_ACleanValueCostsNothing(t *testing.T) { assert.Empty(t, irtest.AllocatingVisits(doc, slices.Collect(maps.Values(checks))...)) } +// TestWalkChecks_AFirstMeetingCostsNothing holds the walking checks to costing +// nothing for a value when the walk first meets it, which +// irtest.AllocatingVisits never measures, so a check keeping a path for each +// PropID it meets passes the test above. A document compiled from twice the +// components must cost the checks less than one allocation more per component +// added, the walk's own growth taken out; a map a check keeps grows in steps, +// far fewer. checkReferentialIntegrity is left out, as above. It is not run in +// parallel: AllocsPerRun measures the process. +func TestWalkChecks_AFirstMeetingCostsNothing(t *testing.T) { + const components = 100 + few, many := scaledDocument(t, components), scaledDocument(t, 2*components) + + added := checkingAllocs(many) - checkingAllocs(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 +} + +// checkingAllocs returns what one walk of doc with every walking check but +// checkReferentialIntegrity allocates, the checks built afresh, less what the +// walk allocates visiting nothing. +func checkingAllocs(doc *ir.Document) float64 { + checking := testing.AllocsPerRun(5, func() { + checks := irverify.WalkChecks(doc) + delete(checks, "checkReferentialIntegrity") + ir.WalkValues(doc, ir.DocumentPath, slices.Collect(maps.Values(checks))...) + }) + walking := testing.AllocsPerRun(5, func() { + ir.WalkValues(doc, ir.DocumentPath, ir.VisitFunc(func(reflect.Value, ir.WalkPath) bool { return true })) + }) + return checking - 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 From 6d61f0946b1b7e9a0a3375ecb9feeb8b69961f85 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 27 Oct 2026 20:47:43 +0300 Subject: [PATCH 15/31] test(irverify): name the exemption test for the positions it pins TestCheckNaming_ExemptsAPrimitivesOwnNameWhereverItSits pins that a primitive embedded in a struct beside a Name of its own keeps no exemption, since the walk spells the primitive's promoted Name on the embedding struct, so its name claimed an exemption its second case shows does not hold. It is renamed for the two positions it holds, and its doc now says both Names at the shared path are reported. --- ir/irverify/naming_internal_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/ir/irverify/naming_internal_test.go b/ir/irverify/naming_internal_test.go index f99ef0d11..72c08d6a9 100644 --- a/ir/irverify/naming_internal_test.go +++ b/ir/irverify/naming_internal_test.go @@ -75,12 +75,13 @@ func TestNameExempt_IsTheOptionalNodesOwnNameAlone(t *testing.T) { }, got) } -// TestCheckNaming_ExemptsAPrimitivesOwnNameWhereverItSits holds the exemption +// 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, and a struct embedding -// one does not inherit the exemption, though its own Name and the primitive's -// promoted one share a path. An exemption recorded by path silenced both there. -func TestCheckNaming_ExemptsAPrimitivesOwnNameWhereverItSits(t *testing.T) { +// held in a field of any name keeps its own Name exempt, while a struct +// embedding one takes the primitive's promoted Name as its own, so neither that +// Name nor the one beside it 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 { From 495738702b7d258ee52fe84c53c232288da13e1e Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 15:24:00 +0300 Subject: [PATCH 16/31] docs(ir): spell out shadowed fields and why a visit needs the walk path WalkPath.Field said a field Go promotes through an embedded struct is spelled on the embedding struct. The walk also reaches a field Go shadows, and Field reports that one on the outermost struct too, as TestWalkPath_FieldIsTheFieldThePathEndsIn pins. Say so there, and in the naming test that measures the case, which called the shadowed Name promoted. AllocatingVisits said the zero WalkPath renders empty without saying why that matters: it renders without allocating, so a visit measured with it would hide every render. --- ir/irtest/visits.go | 8 ++++---- ir/irverify/naming_internal_test.go | 6 +++--- ir/walk.go | 9 +++++---- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/ir/irtest/visits.go b/ir/irtest/visits.go index 294d5ecbe..2bdbe00cb 100644 --- a/ir/irtest/visits.go +++ b/ir/irtest/visits.go @@ -12,12 +12,12 @@ import ( // 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. +// empty without allocating. // // Only a repeat of a visit is measured (fewestAllocs), so what a visitor builds -// on first meeting a value goes uncounted: a memo, but also anything it keeps -// per value, which a test growing the document finds. AllocsPerRun -// measures the process, so a caller is not run in parallel. +// 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 measured := make([]ir.Visitor, len(visitors)) diff --git a/ir/irverify/naming_internal_test.go b/ir/irverify/naming_internal_test.go index 72c08d6a9..cbe7f772c 100644 --- a/ir/irverify/naming_internal_test.go +++ b/ir/irverify/naming_internal_test.go @@ -78,9 +78,9 @@ func TestNameExempt_IsTheOptionalNodesOwnNameAlone(t *testing.T) { // 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 takes the primitive's promoted Name as its own, so neither that -// Name nor the one beside it is exempt, though both share a path. An exemption -// recorded by path silenced both there. +// 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 } diff --git a/ir/walk.go b/ir/walk.go index a8805fc2a..166ab6705 100644 --- a/ir/walk.go +++ b/ir/walk.go @@ -162,10 +162,11 @@ func (p WalkPath) AppendTo(b []byte) []byte { // 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 Go promotes through -// an embedded struct is spelled on the embedding struct, 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. +// 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 From eea24b796fa26ca34f0296706b1fbeb0332b53aa Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 15:24:02 +0300 Subject: [PATCH 17/31] test(irverify): hold the payload reader either side of the nesting limit TestPayloadReader_ReadsAsIsValidDoes reached the decoder's nesting limit only from far past it, at 20,000 levels. The reader skips a value token by token, where jsontext.Value.IsValid reads it whole, and the library bounds the two paths separately, so the rows that would show them parting are the ones either side of the limit: an array and an object nested 10,000 deep, which both accept, and 10,001, which both refuse. --- ir/irverify/rawpayloads_internal_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/ir/irverify/rawpayloads_internal_test.go b/ir/irverify/rawpayloads_internal_test.go index 7d0e425a3..7082a219e 100644 --- a/ir/irverify/rawpayloads_internal_test.go +++ b/ir/irverify/rawpayloads_internal_test.go @@ -52,8 +52,9 @@ func TestCheckRawPayloads_VisitingCostsNoAllocation(t *testing.T) { // 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. One reader reads them -// all, so what one payload leaves in it reaching the next would show too. +// 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 @@ -73,7 +74,11 @@ func TestPayloadReader_ReadsAsIsValidDoes(t *testing.T) { 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", strings.Repeat("[", 20000)+strings.Repeat("]", 20000)) + `{"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 { From 802c0773b4c9dc56ba2384af0328e134f8bf16b4 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 15:24:30 +0300 Subject: [PATCH 18/31] perf(irverify): bound what a reused map keeps between collections checkRequires, checkOAuthScopes, checkStringLists and the alias check each read every collection through one map they emptied with clear, so a collection cost no map of its own. Go never shrinks a map: clear returns at once on an empty one, and otherwise walks every table it grew to. One huge collection therefore taxed every later one. A model listing 1,000,000 Requires names beside 200,000 models listing two each took checkRequires 490 ms, against 166 ms making a map per model as before. Each site now reads through a reusedMap, which empties its map in place while the last collection left it at most 1,024 entries and replaces it past that. The same input takes 127 ms, and a collection within the bound still costs no allocation. A test refuses a bare clear anywhere else in the package, so a later site goes through the bound too. --- ir/irverify/naming.go | 18 +++++------ ir/irverify/requires.go | 11 ++++--- ir/irverify/reuse.go | 26 +++++++++++++++ ir/irverify/reuse_internal_test.go | 51 ++++++++++++++++++++++++++++++ ir/irverify/scopes.go | 8 ++--- ir/irverify/stringlists.go | 14 ++++---- 6 files changed, 103 insertions(+), 25 deletions(-) create mode 100644 ir/irverify/reuse.go create mode 100644 ir/irverify/reuse_internal_test.go diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index ff5726c58..e151ff2bf 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -50,15 +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 - // Every alias list is held through this one map, emptied for each, so a - // list costs no map of its own however long it is. - seen := map[string]int{} + // 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 || v.Type() != namingType { return true } - vs = appendNamingChecks(vs, v, at, seen) + vs = appendNamingChecks(vs, v, at, &aliases) return false // a Naming holds no Naming nested in it }, report: reporting(&vs), @@ -66,12 +66,12 @@ func checkNaming(*ir.Document) walkCheck { } // appendNamingChecks reports what is wrong with the one Naming naming, at at. -// seen is the map appendAliasViolations holds the aliases through. -func appendNamingChecks(vs []Violation, naming reflect.Value, at ir.WalkPath, seen map[string]int) []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) vs = appendNamingViolations(vs, source, canon, hint, at) - return appendAliasViolations(vs, source, aliases, at, seen) + return appendAliasViolations(vs, source, aliases, at, read) } // namingChannels reads every name channel off one Naming. It reads fields @@ -101,9 +101,9 @@ func namingChannels(naming reflect.Value) (source, canon, hint string, aliases r // Collisions with other entities' names are no rule of the list (ir-design // §3.2). func appendAliasViolations(vs []Violation, source string, aliases reflect.Value, at ir.WalkPath, - seen map[string]int, + read *reusedMap[string, int], ) []Violation { - clear(seen) // what another entity's list held collides with nothing here + 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]; { diff --git a/ir/irverify/requires.go b/ir/irverify/requires.go index 031f4a40e..1c54a2545 100644 --- a/ir/irverify/requires.go +++ b/ir/irverify/requires.go @@ -12,19 +12,20 @@ import ( // a repeat admits nothing more. A repeat is reported at its later entry. func checkRequires(doc *ir.Document) []Violation { var vs []Violation - // Emptied for each model rather than made afresh, so a model costs no map - // of its own however many properties and entries it has. - own, first := map[string]bool{}, map[string]int{} + // 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 } - clear(own) + own := owned.emptied() for _, p := range m.Properties { own[p.EffectiveWireName()] = true } - clear(first) + first := listed.emptied() for i, name := range m.Requires { if own[name] { vs = append(vs, Violation{ 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 eeabfc3c9..bc9c0b70a 100644 --- a/ir/irverify/scopes.go +++ b/ir/irverify/scopes.go @@ -15,12 +15,12 @@ import ( // is the compiler's. It is reported at the later entry. func checkOAuthScopes(doc *ir.Document) []Violation { var vs []Violation - // Emptied for each flow rather than made afresh, so a flow costs no map of - // its own however many scopes it offers. - seen := map[string]bool{} + // 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 { - clear(seen) + 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 3cd895f2e..037e7d300 100644 --- a/ir/irverify/stringlists.go +++ b/ir/irverify/stringlists.go @@ -98,9 +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 map, emptied for each, so a - // list costs no map of its own however long it is. - first := map[string]int{} + // 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 { @@ -112,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, first) + vs = appendListViolations(vs, v.Field(f.index), at, f, &entries) } return true }, @@ -138,11 +138,11 @@ 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, and -// first is the map the entries are read into, emptied before them. +// entries is what the list is read through. func appendListViolations(vs []Violation, list reflect.Value, owner ir.WalkPath, f heldList, - first map[string]int, + entries *reusedMap[string, int], ) []Violation { - clear(first) + first := entries.emptied() for i := range list.Len() { entry := list.Index(i).String() switch earlier, repeated := first[entry]; { From 40071a3925cb3893d1e5744365a1b4e008973443 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 15:32:56 +0300 Subject: [PATCH 19/31] perf(ir)!: keep declaration and reference paths unspelled until reported ir.IDDeclarations spelled the path of every declaration, and ir.IDReferences that of every reference met before its target, though a check reports few of them, and none on a sound document. On the golden spec's IR they were 140,062 of the 143,400 allocations irverify.Verify made once its checks built nothing else. PathLog keeps a walk's paths without spelling any. Each is stored as the steps it shares with the path recorded before it and the steps after those, so recording one copies a step or two and allocates nothing but the log's growth, by five halves at a time. A path is spelled only when a replay of the log is asked for its String. IDDeclarations keeps its paths there and yields them through Paths, and IDReferences keeps its references' there and spells, in Unresolved, only those still unresolved. irverify's duplicate and namespace checks spell only the declarations they report. WalkPath.IsMapKey tells a map key from its value without spelling the path, for a caller that keeps one and not the other. BREAKING CHANGE: ir.IDDeclaration has no Path. A caller spells a declaration's path from IDDeclarations.Paths, which yields each with its position in List. --- ir/declarations.go | 30 ++-- ir/declarations_test.go | 41 +++-- ir/export_test.go | 20 ++- ir/helpers_test.go | 17 ++- ir/irverify/declarations_internal_test.go | 19 +++ ir/irverify/declared_internal_test.go | 7 +- ir/irverify/duplicates.go | 97 +++++++++--- ir/irverify/export_test.go | 14 +- ir/irverify/idspaces.go | 42 ++++-- ir/irverify/indices_internal_test.go | 12 +- ir/irverify/irverify.go | 66 +++++--- ir/irverify/irverify_corpus_test.go | 31 ++-- ir/pathlog.go | 93 ++++++++++++ ir/pathlog_test.go | 176 ++++++++++++++++++++++ ir/references.go | 30 +++- ir/references_test.go | 18 +++ ir/walk.go | 7 + ir/walk_path_test.go | 26 ++++ 18 files changed, 611 insertions(+), 135 deletions(-) create mode 100644 ir/irverify/declarations_internal_test.go create mode 100644 ir/pathlog.go create mode 100644 ir/pathlog_test.go 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..24426c1f0 100644 --- a/ir/declarations_test.go +++ b/ir/declarations_test.go @@ -32,8 +32,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 +53,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 +252,16 @@ 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)) } diff --git a/ir/export_test.go b/ir/export_test.go index f1be1ce3d..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. @@ -46,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/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/duplicates.go b/ir/irverify/duplicates.go index a3410eb70..66b1cb0ed 100644 --- a/ir/irverify/duplicates.go +++ b/ir/irverify/duplicates.go @@ -43,53 +43,100 @@ type fingerprint 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 { - // The fingerprint of the first property met on each PropID, and the paths - // of the properties met later whose fingerprint differs from it. The walk - // meets properties in the order the declarations list them, so the first - // met is the PropID's first declaration, and only a property that differs, - // which is a defect, has its path rendered. - first := map[string]fingerprint{} - differs := map[string]bool{} + // 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 { + 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) - switch met, seen := first[id]; { - case !seen: - first[id] = got - case met != got: - differs[at.String()] = true + 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, differs) + 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 it is a property that is the first one's copy: of the properties -// declared again, differs holds the paths of those that are not. -func duplicateIDs(decls []ir.IDDeclaration, differs map[string]bool) []Violation { - first := make(map[identity]string, 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] = d.Path + seen[key] = declared{first: i, met: 1} continue } - if d.Class == propIDType && !differs[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: d.Path, + Message: "id " + strconv.Quote(d.ID) + " is declared here and at " + strconv.Quote(paths[r.first]), + Path: paths[r.at], }) } return vs diff --git a/ir/irverify/export_test.go b/ir/irverify/export_test.go index 7ae5c4ae5..0f53d36c5 100644 --- a/ir/irverify/export_test.go +++ b/ir/irverify/export_test.go @@ -51,12 +51,10 @@ func HeldLists() map[string]bool { return held } -// WalkChecks builds every walking check for doc, keyed by its function's name, -// as Verify's walk is shown them, for this package's external tests. -func WalkChecks(doc *ir.Document) map[string]ir.Visitor { - checks := map[string]ir.Visitor{} - for _, newCheck := range walkChecks() { - checks[checkName(newCheck)] = newCheck(doc) - } - return checks +// 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/idspaces.go b/ir/irverify/idspaces.go index 9b876c512..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)) 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 56bfd7c7d..8d4a6eb37 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -28,11 +28,10 @@ type Violation struct { // 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 it keeps may grow -// by, beyond what its readers keep: ir's path for each declaration and each -// reference met before its target, 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. +// 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) } @@ -71,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 @@ -82,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 len(at) == 0 || 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 @@ -163,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 { @@ -181,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 diff --git a/ir/irverify/irverify_corpus_test.go b/ir/irverify/irverify_corpus_test.go index b53a51926..9ce69a6bd 100644 --- a/ir/irverify/irverify_corpus_test.go +++ b/ir/irverify/irverify_corpus_test.go @@ -75,14 +75,12 @@ func TestVerify_Corpus(t *testing.T) { } } -// TestWalkChecks_ACleanValueCostsNothing holds the walking checks, 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 but the one left out here: -// checkReferentialIntegrity keeps a reference to what the walk has yet to -// declare with its path, ir.IDReferences' design, which ir pins. It is not run -// in parallel: AllocsPerRun measures the process. +// 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) @@ -90,10 +88,7 @@ func TestWalkChecks_ACleanValueCostsNothing(t *testing.T) { require.NotNil(t, doc) require.Empty(t, irverify.Verify(doc), "the document is clean, or what is measured is a report") - checks := irverify.WalkChecks(doc) - require.Contains(t, checks, "checkReferentialIntegrity") - delete(checks, "checkReferentialIntegrity") - assert.Empty(t, irtest.AllocatingVisits(doc, slices.Collect(maps.Values(checks))...)) + assert.Empty(t, irtest.AllocatingVisits(doc, irverify.WalkVisitors(doc)...)) } // TestWalkChecks_AFirstMeetingCostsNothing holds the walking checks to costing @@ -102,8 +97,7 @@ func TestWalkChecks_ACleanValueCostsNothing(t *testing.T) { // PropID it meets passes the test above. A document compiled from twice the // components must cost the checks less than one allocation more per component // added, the walk's own growth taken out; a map a check keeps grows in steps, -// far fewer. checkReferentialIntegrity is left out, as above. It is not run in -// parallel: AllocsPerRun measures the process. +// far fewer. It is not run in parallel: AllocsPerRun measures the process. func TestWalkChecks_AFirstMeetingCostsNothing(t *testing.T) { const components = 100 few, many := scaledDocument(t, components), scaledDocument(t, 2*components) @@ -153,14 +147,11 @@ func scaledDocument(t *testing.T, n int) *ir.Document { return doc } -// checkingAllocs returns what one walk of doc with every walking check but -// checkReferentialIntegrity allocates, the checks built afresh, less what the -// walk allocates visiting nothing. +// checkingAllocs returns what one walk of doc carrying what Verify's walk does +// allocates, built afresh, less what the walk allocates visiting nothing. func checkingAllocs(doc *ir.Document) float64 { checking := testing.AllocsPerRun(5, func() { - checks := irverify.WalkChecks(doc) - delete(checks, "checkReferentialIntegrity") - ir.WalkValues(doc, ir.DocumentPath, slices.Collect(maps.Values(checks))...) + ir.WalkValues(doc, ir.DocumentPath, irverify.WalkVisitors(doc)...) }) walking := testing.AllocsPerRun(5, func() { ir.WalkValues(doc, ir.DocumentPath, ir.VisitFunc(func(reflect.Value, ir.WalkPath) bool { return true })) 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..f9592e9c9 --- /dev/null +++ b/ir/pathlog_test.go @@ -0,0 +1,176 @@ +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_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 166ab6705..4deed87ea 100644 --- a/ir/walk.go +++ b/ir/walk.go @@ -186,6 +186,13 @@ func (p WalkPath) Field() (owner reflect.Type, name string, ok bool) { 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 diff --git a/ir/walk_path_test.go b/ir/walk_path_test.go index d34cfbaeb..2379dfa36 100644 --- a/ir/walk_path_test.go +++ b/ir/walk_path_test.go @@ -273,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") +} From a1154779046e40a1c718ed1e0cad90bef41bd399 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 15:32:56 +0300 Subject: [PATCH 20/31] perf(pass): spell, sort and collect only for a diagnostic pass.Validate built for nearly every value it held to a rule: a location formatted for every payload, error case, HTTP binding and pagination input, a map per model, enum, union, binding and channel, the sorted keys of every mapping, encoding map and SuccessStatus, and the path of each PropID the walk kept until its property was read. On the golden spec's IR, which it finds clean, that was most of its 85,700 allocations. Each rule now keeps a location as its parts and spells it in the branch that reports, reads its collections through a reusedMap or a key list it keeps for the run, and keeps a PropID's path in an ir.PathLog, spelled only if the PropID dangles. A mapping's and an encoding map's keys are still asked in sorted order, so the questions the shared facts index is asked do not follow map iteration. Every output is as it was. TestValidate_ASoundValueCostsNothing holds the walk and every rule family, the index built, to costing less than one allocation more per entity when the document doubles. The index itself still allocates per type it indexes and per question it walks (#1150), as the doc on the rule families says. --- pass/export_test.go | 54 ++++++++++- pass/reuse.go | 26 ++++++ pass/reuse_test.go | 55 +++++++++++ pass/validate.go | 6 ++ pass/validate_cost_test.go | 159 ++++++++++++++++++++++++++++++++ pass/validate_discriminators.go | 62 +++++++++---- pass/validate_enummembers.go | 6 +- pass/validate_indices.go | 26 ++++-- pass/validate_messages.go | 34 ++++--- pass/validate_params.go | 98 ++++++++++++-------- pass/validate_payloads.go | 107 +++++++++++++-------- pass/validate_refs.go | 40 +++++--- pass/validate_refs_test.go | 27 ++++-- pass/validate_wirenames.go | 16 ++-- 14 files changed, 570 insertions(+), 146 deletions(-) create mode 100644 pass/reuse.go create mode 100644 pass/reuse_test.go create mode 100644 pass/validate_cost_test.go 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..bd90a108d 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 spells a location, sorts a collection or makes a map only for a +// diagnostic, so a value with nothing wrong costs it no allocation but a step a +// map or a log it keeps 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..9073d5ef9 --- /dev/null +++ b/pass/validate_cost_test.go @@ -0,0 +1,159 @@ +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 +// and its messages, and an operation binding parameters, paginating, 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{}, + } + 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", 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))} + 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}} + 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 +// is spelled, a path kept spelled, a collection's keys sorted 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 + 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_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_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_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_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_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 { From 2d69be5d3bf3fddf3eda44cbf82ef67f21086a74 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 15:32:56 +0300 Subject: [PATCH 21/31] chore(skills): hold a first meeting to a doubling test The rule on measuring a render through a real walk told a reader to give each visitor a call of its own to irtest.AllocatingVisits, so that no other visitor's pruning hid a visit. AllocatingVisits now walks each visitor as it would walk alone, so that reason no longer holds, and the rule said nothing of what the helper cannot see: it measures only a repeat of each visit, so a path kept on every first meeting passed it. The rule now says both, and names the doubling test that holds a first meeting. --- .../references/cost-tests.md | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) 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` From b55681185f44dc53cbff2f6d0e031beede2b40bb Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 18:28:24 +0300 Subject: [PATCH 22/31] docs(ir): say how long a replayed path lives and how to keep one WalkPath's doc said a path is valid until the call it was passed to returns and that a visitor keeping one keeps its String. A PathLog now replays paths too, each valid until the next is yielded, and is the way to keep one without spelling it. --- ir/walk.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ir/walk.go b/ir/walk.go index 4deed87ea..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. From 1ae3192f350477fe48c47731a0d96ab374fa72e3 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 18:29:22 +0300 Subject: [PATCH 23/31] test(pass): hold a discriminator mapping's reports to sorted order checkMapping asks a mapping's keys in sorted order, so its reports within one discriminator do not follow Go's map iteration, and this change rewrote that sort to run in a list the check keeps. No test held it: iterating unsorted left the package green, as #821 records for this site. TestValidate_MappingDiagnosticsAreDeterministic maps five keys out of order onto no variant and requires their reports in key order across nine runs, which the messages show since every one is placed at the union. --- pass/validate_discriminators_test.go | 31 ++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pass/validate_discriminators_test.go b/pass/validate_discriminators_test.go index 840a966f3..2202c55c2 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,33 @@ 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()) + } +} From c04bbb067dadd8cd23ca8720e7a2859d09a062e2 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 18:29:22 +0300 Subject: [PATCH 24/31] test(pass): reach servers and a kept PropID in the sound-value test TestValidate_ASoundValueCostsNothing said each entity reaches every rule family, but its document named no server, so the server-index check ran over nothing, and kept no PropID for checkPropIDRefs to settle. A location spelled for every channel's servers passed it. Each channel and the service now name a server, and each operation paginates to an item property the walk meets before its model, which the test requires is kept. Both plants now redden it. --- pass/validate_cost_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pass/validate_cost_test.go b/pass/validate_cost_test.go index 9073d5ef9..29fa2c2b5 100644 --- a/pass/validate_cost_test.go +++ b/pass/validate_cost_test.go @@ -20,7 +20,8 @@ const wide = 12 // 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 -// and its messages, and an operation binding parameters, paginating, keyed by a +// 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 { @@ -29,6 +30,7 @@ func soundDocument(n int) *ir.Document { 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) @@ -38,7 +40,7 @@ func soundDocument(n int) *ir.Document { 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", CommonErrors: commonErrors, Groups: []ir.OperationGroup{group}}} + doc.Services = []ir.Service{{ID: "s/x", Servers: []int{0}, CommonErrors: commonErrors, Groups: []ir.OperationGroup{group}}} return doc } @@ -73,7 +75,7 @@ func addEntityTypes(doc *ir.Document, i int) *ir.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))} + 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")} @@ -107,7 +109,10 @@ func entityOperation(i int, model *ir.Model, messages entityMessages) ir.Operati 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}} + 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{ @@ -147,6 +152,7 @@ func TestValidate_ASoundValueCostsNothing(t *testing.T) { 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) From 5c08f25c3a0388c89c358e23410b4839bf291872 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 23:42:27 +0300 Subject: [PATCH 25/31] test(irverify): hold all of Verify to a clean component costing nothing The cost test for a first meeting measured the walk alone. What a check does once the walk is over went unmeasured: checkDuplicateIDs and checkIDSpaces spell a declaration's path only for one they report, and spelling every path again took irverify.Verify on the golden spec's IR from 3.3k allocations to 144k with every test green. The test now measures Verify as a whole, and is renamed TestVerify_ACleanComponentCostsNothing for what it holds, so the reports and the registry checks answer to it beside the walk. Restoring either report's eager paths reddens it, as a path kept on each first meeting still does. --- ir/irverify/irverify_corpus_test.go | 31 ++++++++++++++--------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/ir/irverify/irverify_corpus_test.go b/ir/irverify/irverify_corpus_test.go index 9ce69a6bd..e8c090e75 100644 --- a/ir/irverify/irverify_corpus_test.go +++ b/ir/irverify/irverify_corpus_test.go @@ -91,18 +91,19 @@ func TestWalkChecks_ACleanValueCostsNothing(t *testing.T) { assert.Empty(t, irtest.AllocatingVisits(doc, irverify.WalkVisitors(doc)...)) } -// TestWalkChecks_AFirstMeetingCostsNothing holds the walking checks to costing -// nothing for a value when the walk first meets it, which -// irtest.AllocatingVisits never measures, so a check keeping a path for each -// PropID it meets passes the test above. A document compiled from twice the -// components must cost the checks less than one allocation more per component -// added, the walk's own growth taken out; a map a check keeps grows in steps, -// far fewer. It is not run in parallel: AllocsPerRun measures the process. -func TestWalkChecks_AFirstMeetingCostsNothing(t *testing.T) { +// 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 := checkingAllocs(many) - checkingAllocs(few) + added := verifyingAllocs(many) - verifyingAllocs(few) assert.Less(t, added, float64(components), "%v allocations more for %d components more", added, components) } @@ -147,16 +148,14 @@ func scaledDocument(t *testing.T, n int) *ir.Document { return doc } -// checkingAllocs returns what one walk of doc carrying what Verify's walk does -// allocates, built afresh, less what the walk allocates visiting nothing. -func checkingAllocs(doc *ir.Document) float64 { - checking := testing.AllocsPerRun(5, func() { - ir.WalkValues(doc, ir.DocumentPath, irverify.WalkVisitors(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 checking - walking + return verifying - walking } // uncorpusedUnmodeled exercises the Unmodeled writes no committed fixture From 8fdefc35cd0a309de117c00cd1f52a9146ccc574 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 23:51:39 +0300 Subject: [PATCH 26/31] test(irverify): pin where namespace and member-scope reports land checkIDSpaces places each ID outside the declaration by its position among the declarations, and no case checked where two such IDs were reported, so placing all of them at the first one's node passed. A case with two, a declared ID between them, now pins each to its own node. beneath compares a member's path with its enum's in two steps, and dropping the first, that the path opens with the enum's, passed too: no case's member path opened with an empty segment. A member whose path opens with one before the enum's now pins it. --- ir/irverify/idspaces_test.go | 21 +++++++++++++++++++++ ir/irverify/memberids_test.go | 1 + 2 files changed, 22 insertions(+) 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/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() From da1e7023e5542ca0ecc1f2717d1ff21dd2a9622c Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 23:51:39 +0300 Subject: [PATCH 27/31] refactor(irverify): drop a check pathsAt's callers already make Both callers of declarations.pathsAt return before asking for no position, so its own test for an empty set decided nothing: removing it changed no result and reddened no test. The zero-value guard beside it stays. --- ir/irverify/irverify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ir/irverify/irverify.go b/ir/irverify/irverify.go index 8d4a6eb37..8246d791a 100644 --- a/ir/irverify/irverify.go +++ b/ir/irverify/irverify.go @@ -97,7 +97,7 @@ func (d declarations) list() []ir.IDDeclaration { // 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 len(at) == 0 || d.collected == nil { + if d.collected == nil { return nil } paths := make(map[int]string, len(at)) From de65106d5a8ca0620b7dd6a8002e629c5412fd16 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 23:51:39 +0300 Subject: [PATCH 28/31] test(pass): judge each collection a reused map reads on its own Validate reads each union's variants, each binding's bound parameters and locations, and each model's claimed names through one map per check, emptied for each collection. A site that stopped emptying it would carry one collection into the next, and nothing in pass noticed for four of them: a mapping onto the previous union's variant passed, a parameter the first binding placed was never unbound in a second, one placed once by each binding was bound twice, and a name another model claimed made an alias ambiguous. Each now has a case whose first collection would change the verdict on the second. The bindings case also pins that each report names its own binding, the second, which no case with one binding could. --- pass/validate_discriminators_test.go | 19 ++++++++++++ pass/validate_params_test.go | 44 ++++++++++++++++++++++++++++ pass/validate_wirenames_test.go | 19 +++++++----- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/pass/validate_discriminators_test.go b/pass/validate_discriminators_test.go index 2202c55c2..a63711e73 100644 --- a/pass/validate_discriminators_test.go +++ b/pass/validate_discriminators_test.go @@ -342,3 +342,22 @@ func TestValidate_MappingDiagnosticsAreDeterministic(t *testing.T) { 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_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_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() From 2b9fed5271e522a2fc52e4df519e931b13adcd2d Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 23:51:39 +0300 Subject: [PATCH 29/31] test(pass): pin each report's position past the first Validate spells a report's location from its parts only once there is a report, and no response-index, encoding-key or message-binding case reported anything past the first binding, content, response, error case or listed message, so a location spelling index 0 for a later one passed. A second binding with a response index out of range, a second content, response and error case each carrying an encoding key no property answers to, and a second listed message its channel does not carry now pin each report to its own position. --- pass/validate_indices_test.go | 14 ++++++++++++++ pass/validate_messages_test.go | 11 +++++++++++ pass/validate_payloads_test.go | 28 ++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+) 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_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_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 From 734955e8836cd0786e9e4cee9dc67a611899464f Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 23:51:39 +0300 Subject: [PATCH 30/31] test(ir): pin a logged step's owner and the declaration list's growth PathLog shares a step with the path recorded before it when the two are equal, owner included. A walk shows one value at one path, so within a walk the owner never decides; it does once a log is fed walks of two types, and comparing steps without it passed every test. A log fed a Document's IRVersion and then a Channel's ID, each its struct's first field, now pins it. IDDeclarations grows its list five halves at a time, as the log grows its own, and restoring append there passed too. A test beside the log's now holds the list to fewer growths than doubling would take. --- ir/declarations_test.go | 32 ++++++++++++++++++++++++++++++++ ir/pathlog_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/ir/declarations_test.go b/ir/declarations_test.go index 24426c1f0..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" @@ -265,3 +267,33 @@ func TestIDDeclarations_AllocatesNothingPerDeclaration(t *testing.T) { var visitor ir.IDDeclarations 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/pathlog_test.go b/ir/pathlog_test.go index f9592e9c9..3096e56c8 100644 --- a/ir/pathlog_test.go +++ b/ir/pathlog_test.go @@ -87,6 +87,35 @@ func TestPathLog_KeepsEachWalksRoot(t *testing.T) { 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) { From 340213867ecba6b5d76f130f7f0c5f8a1676fa68 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 29 Oct 2026 23:51:39 +0300 Subject: [PATCH 31/31] docs(pass): say a family sorts in a kept list, allocating to report The doc on Validate's rule families, and the sound-value cost test's, said a family sorts a collection only for a diagnostic. checkMapping and appendUnknownPartDiags sort every mapping's and encoding map's keys, so the facts index is asked in a fixed order; they sort into a list kept for the run, which costs nothing per value. Both now say what holds: a family allocates only for a diagnostic. --- pass/validate.go | 10 +++++----- pass/validate_cost_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pass/validate.go b/pass/validate.go index bd90a108d..756172cad 100644 --- a/pass/validate.go +++ b/pass/validate.go @@ -79,11 +79,11 @@ func validate(ctx context.Context, doc *ir.Document, walk walkFunc) ([]ir.Diagno // in the order they report. Validate asks its context between two, so a cancel // waits out one family at most. // -// A family spells a location, sorts a collection or makes a map only for a -// diagnostic, so a value with nothing wrong costs it no allocation but a step a -// map or a log it keeps 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). +// 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 index 29fa2c2b5..25829d148 100644 --- a/pass/validate_cost_test.go +++ b/pass/validate_cost_test.go @@ -140,12 +140,12 @@ func jsonPayload(target ir.TypeID) *ir.Payload { // 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 -// is spelled, a path kept spelled, a collection's keys sorted 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. +// 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)