From 72d141dce741c648d0b8387979d3acb0849b7ae1 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 06:32:03 +0300 Subject: [PATCH 1/5] feat(irverify): hold Naming.Aliases to a rule Naming has four channels and irverify read three. Aliases was read by nothing anywhere in the pipeline -- it appeared in production code exactly once, at its own declaration -- so a model whose aliases were empty and duplicated verified clean and validated clean. Which rules apply had to be settled first, and the answer is not Canonical's. An alias is matched against a name some other schema wrote: an Avro alias is a full name such as com.example.User, and neutralizing it to words discards the separators and the casing the match is made of. That makes it a verbatim channel like Source rather than a neutral one like Canonical, and holding it to the neutrality rules would be the lossy direction invariant 2 forbids. The IR is not deciding this spelling, it is recording one. What is left is decidable without a grammar. An empty alias matches nothing and a repeated one matches twice, so neither can be what a producer intended: both say a list was built wrong rather than that a name was spelled wrong. checkNaming reports each as its own violation, at a path naming the offending entry, and the field's doc comment now states what a well-formed entry looks like instead of only what the field is for. --- ir/irverify/naming.go | 68 ++++++++++++++++++++--- ir/irverify/naming_alias_test.go | 93 ++++++++++++++++++++++++++++++++ ir/naming.go | 10 ++++ 3 files changed, 165 insertions(+), 6 deletions(-) create mode 100644 ir/irverify/naming_alias_test.go diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 9899a116..0e437eec 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -2,6 +2,7 @@ package irverify import ( "reflect" + "strconv" "strings" "unicode" @@ -52,12 +53,16 @@ var nameOptional = map[reflect.Type]bool{ // vacuously true of the empty string: an entirely empty Naming satisfied all // three while leaving an emitter nothing to name the entity by (GitHub #251). // -// Only Canonical is checked for content. Naming.Hint — the generated-name +// Only Canonical is checked for neutrality. Naming.Hint — the generated-name // channel — is held to none of the content rules, so casing // and punctuation still reach the IR through it. That is GitHub #54, left open // deliberately: closing it means changing how the compilers derive hints and // regenerating every golden, which is a different change from tightening this // checker. +// +// Naming.Aliases is held instead to the two rules that need no neutrality — +// non-empty and non-repeating — because an alias is a verbatim channel like +// Source rather than a neutral one like Canonical. See appendAliasViolations. func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { var vs []Violation optional := map[string]bool{} @@ -74,23 +79,74 @@ func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { if v.Type() != namingType { return true } - source, canon, hint := namingChannels(v) + source, canon, hint, aliases := namingChannels(v) if !optional[path] { vs = appendAbsentViolation(vs, source, canon, hint, path) } vs = appendNamingViolations(vs, source, canon, path) + vs = appendAliasViolations(vs, aliases, path) return false // Naming holds no references or nested Naming to descend into }) return vs, truncated } -// namingChannels reads the three name channels off one Naming. It reads fields +// namingChannels reads the four name channels off one Naming. It reads fields // rather than converting the value back to an ir.Naming because a value the walk -// reached through an unexported field cannot be (see ir.WalkValues). -func namingChannels(naming reflect.Value) (source, canon, hint string) { +// reached through an unexported field cannot be (see ir.WalkValues) — which is +// also why the aliases are copied out element by element rather than through +// Interface(). +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() + } return naming.FieldByName("Source").String(), naming.FieldByName("Canonical").String(), - naming.FieldByName("Hint").String() + naming.FieldByName("Hint").String(), + aliases +} + +// appendAliasViolations reports the ways an alias list can be one no producer +// meant to write. +// +// An alias is matched against a name some other schema wrote — an Avro alias is +// a full name such as "com.example.User" — so it is a verbatim channel like +// Source, not a neutral one like Canonical, and none of the neutrality rules +// above apply to it. Holding it to Canonical's grammar would be the lossy +// direction: neutralizing "com.example.User" to words discards the separators +// and the casing the match is made of, and invariant #2 forbids a lowering that +// throws that away. The IR is not deciding this spelling, it is recording one. +// +// What is left is decidable without a grammar. An empty alias matches nothing +// and a repeated one matches twice, so neither can be what a producer intended: +// both say a list was built wrong rather than that a name was spelled wrong. +// +// Paths name the offending entry the way the walk would have reached it, so a +// violation on a list of several says which one. +func appendAliasViolations(vs []Violation, aliases []string, path string) []Violation { + seen := make(map[string]bool, len(aliases)) + for i, alias := range aliases { + at := path + ".Aliases[" + strconv.Itoa(i) + "]" + if alias == "" { + vs = append(vs, Violation{ + Code: "ir/naming-alias-empty", + Message: "alias is empty, so it matches no name", + Path: at, + }) + continue + } + if seen[alias] { + vs = append(vs, Violation{ + Code: "ir/naming-alias-duplicate", + Message: "alias " + alias + " is listed more than once", + Path: at, + }) + continue + } + seen[alias] = true + } + return vs } // appendAbsentViolation reports an entity that no channel names. diff --git a/ir/irverify/naming_alias_test.go b/ir/irverify/naming_alias_test.go new file mode 100644 index 00000000..67f5d4e4 --- /dev/null +++ b/ir/irverify/naming_alias_test.go @@ -0,0 +1,93 @@ +package irverify_test + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/ir" + "github.com/dexpace/morphic/ir/irverify" +) + +// aliasViolations returns the alias violations in a model named with aliases, +// filtered by code prefix so a test asserting none is not satisfied by some +// unrelated violation being absent. +func aliasViolations(t *testing.T, aliases ...string) []irverify.Violation { + t.Helper() + doc := modelNamed(ir.Naming{Source: "m", Canonical: "m", Aliases: aliases}) + var out []irverify.Violation + for _, v := range irverify.Verify(doc) { + if strings.HasPrefix(v.Code, "ir/naming-alias-") { + out = append(out, v) + } + } + return out +} + +// TestVerify_VerbatimAliasesAreClean pins the settlement this check rests on: an +// alias is matched against a name another schema wrote, so it is a verbatim +// channel like Source and none of Canonical's neutrality rules apply to it. +// "com.example.User" is what an Avro alias looks like, and every one of +// ir/naming-cased, ir/naming-not-words and ir/naming-unsegmented would fire on +// it if the alias were held to Canonical's grammar. +func TestVerify_VerbatimAliasesAreClean(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t, "com.example.User", "UserID", "user_id")) +} + +func TestVerify_EmptyAliasIsAViolation(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "ok", "") + require.Len(t, got, 1, "one violation for the one empty entry") + assert.Equal(t, "ir/naming-alias-empty", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[1]", got[0].Path, + "the violation names the offending entry, not just the naming") +} + +func TestVerify_DuplicateAliasIsAViolation(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "dup", "other", "dup") + require.Len(t, got, 1, "the repeat is reported, not the first occurrence") + assert.Equal(t, "ir/naming-alias-duplicate", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[2]", got[0].Path) + assert.Contains(t, got[0].Message, "dup") +} + +// TestVerify_RepeatedEmptyAliasReportsEachAsEmpty holds the interaction between +// the two rules: a second empty entry is a repeat as well as an empty one, and +// reporting it as a duplicate would name the wrong repair. +func TestVerify_RepeatedEmptyAliasReportsEachAsEmpty(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "", "") + require.Len(t, got, 2) + for _, v := range got { + assert.Equal(t, "ir/naming-alias-empty", v.Code) + } +} + +// TestVerify_IssueReproducerIsReported drives the exact value from the issue — +// cased, punctuated, empty and duplicated together — and states which of the +// four the IR objects to and which it accepts by design. +func TestVerify_IssueReproducerIsReported(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "UserID", "com.example.User", "", "dup", "dup") + require.Len(t, got, 2, "the cased and dotted entries are legitimate aliases") + + // Keyed rather than indexed: Verify sorts by (Code, Path), so asserting + // positionally would pin the sort order rather than what was reported. + byCode := map[string]string{} + for _, v := range got { + byCode[v.Code] = v.Path + } + assert.Equal(t, map[string]string{ + "ir/naming-alias-empty": "doc.Types[t/x/M].Name.Aliases[2]", + "ir/naming-alias-duplicate": "doc.Types[t/x/M].Name.Aliases[4]", + }, byCode) +} + +func TestVerify_NoAliasesIsClean(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t)) +} diff --git a/ir/naming.go b/ir/naming.go index 94a9fea3..328eb06a 100644 --- a/ir/naming.go +++ b/ir/naming.go @@ -28,6 +28,16 @@ type Naming struct { // Aliases are alternate names for schema-resolution matching (Avro // aliases). Versionless — rename history tied to version labels lives in // Availability.RenamedFrom. + // + // An alias is a verbatim channel like Source, not a neutral one like + // Canonical: it is matched against a name another schema wrote, so the + // casing and punctuation are the value. An Avro alias is a full name + // ("com.example.User"), and neutralizing it to words would lose the + // separators and the case the match depends on. So no neutrality rule + // applies to an entry, and irverify holds only what is decidable without + // one: every entry is non-empty and no entry repeats, since an empty alias + // matches nothing and a repeated one matches twice. Both mean a producer + // wrote a list it did not mean to write. Aliases []string `json:"aliases,omitempty"` } From d1c19a8fe5fe7349361b4fa2ae72074c45d6432d Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 11 Aug 2026 21:39:34 +0300 Subject: [PATCH 2/5] fix(irverify): widen the blank-alias rule and pin what the tests claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blank rule was strings.TrimSpace, which reports false for the zero-width format characters: an alias of nothing but U+200B, U+FEFF, a soft hyphen or U+0000 verified clean while naming exactly as little as " " does. isBlankName replaces it with the widest test that still needs no format grammar — every rune a space, a control or a category Cf — and a case for a visible rune beside an invisible one holds the other side of that line, since judging that one would need the grammar. Three test claims did not hold. The blank/duplicate interaction case repeated two different blanks, so the duplicate rule could not fire on it under any implementation; it now repeats one string, and the natural rewrite it exists to forbid reddens it. A new case ties the hand-assembled violation path to what ir.WalkValues renders, which nothing did while checkNaming prunes at Naming. Two doc comments named the wrong witnesses: ir/naming-unsegmented fires on API2Key rather than the dotted name, and the base document is pinned by TestVerify_NoAliasesIsClean rather than TestVerify_NeutralCanonicalIsClean. Naming.Aliases justified the duplicate rule with "a repeated one matches twice" and then explained that it matches nothing extra at all. The second is right, and it is what makes the rule and the compiler-side deduplication one argument rather than two. --- docs/ir-design.md | 21 ++++-- ir/irverify/naming.go | 29 ++++++-- ir/irverify/naming_alias_test.go | 115 +++++++++++++++++++++++++++---- ir/naming.go | 14 ++-- 4 files changed, 148 insertions(+), 31 deletions(-) diff --git a/docs/ir-design.md b/docs/ir-design.md index 54b5dfec..56f320f8 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -244,12 +244,21 @@ what the match is made of rather than a spelling the IR gets to decide. Neutrali throw away precisely that, which is the lossy direction lossless-by-default rules out. `Source` is the internal precedent: it carries `UserID` today and no content rule touches it, because it records what the spec said rather than deciding a spelling. What is left is decidable without any -grammar, and `irverify` holds an alias to it: every entry names something — `""` and `" "` alike -match nothing (`ir/naming-alias-blank`) — and no entry repeats (`ir/naming-alias-duplicate`), -reported at the later entry so the path names the one to delete. Where a *source* repeats an alias -the compiler records it once with a diagnostic rather than carrying the repeat through: the second -entry admits no name the first does not, so the same set of names resolves to the entity either -way. +grammar, and `irverify` holds an alias to it: every entry names something, and no entry repeats. + +**Names something** is the widest emptiness test that needs no grammar: an entry whose every rune +is a space, a control character or a zero-width format character is invisible under all of them, so +it matches nothing anywhere (`ir/naming-alias-blank`). `""`, `" "`, `"\u200b"` and `"\ufeff"` are +alike here — trimming only what `unicode.IsSpace` reports would keep the last two, which name as +little as the first two do. An invisible rune sitting *beside* a visible one is a different +question and is not asked: whether `com.example.User` is a legal name is decidable only under +the grammar of the format it will be matched against, which the IR does not know. + +**No entry repeats** because a repeat admits no name the entry before it already did +(`ir/naming-alias-duplicate`, reported at the later entry so the path names the one to delete). A +producer that wrote one built the list wrong. That the repeat is inert is also why a *source* that +declares one is recorded once with a diagnostic rather than carried through: dropping it is not the +lossy direction, since the same set of names resolves to the entity either way. ### 3.3 Type references diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index b23d4942..3867fec8 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -116,10 +116,10 @@ func namingChannels(naming reflect.Value) (source, canon, hint string, aliases [ // neutrality rules above are — an alias is a spelling the IR records rather than // one it decides. // -// Blank rather than empty is the line, because it is the widest one decidable -// without a grammar: "" and " " name nothing under any format's rules, while -// deciding whether a space inside "com.example. User" is legal needs the grammar -// of the format the alias will be matched under, which the IR does not know. +// Blank rather than empty is the line, and isBlankName is how wide it goes. +// Deciding whether a space *inside* "com.example. User" is legal would need the +// grammar of the format the alias is matched under, which the IR does not know; +// deciding that an entry has nothing visible in it at all needs no grammar. // // A repeat is reported at its later entry, so the path names the one to delete // rather than the one to keep. A blank repeat is reported blank: the repair is @@ -136,7 +136,7 @@ func appendAliasViolations(vs []Violation, aliases []string, path string) []Viol for i, alias := range aliases { at := path + ".Aliases[" + strconv.Itoa(i) + "]" switch { - case strings.TrimSpace(alias) == "": + case isBlankName(alias): vs = append(vs, Violation{ Code: "ir/naming-alias-blank", Message: "alias is blank, so it matches no name", @@ -306,6 +306,25 @@ func isWordSequence(s string) bool { return true } +// isBlankName reports whether s holds no rune a name could be made of — the +// widest emptiness test there is that needs no format's grammar. A space, a +// control character and a zero-width format character are invisible under every +// grammar, so a string of nothing but those names nothing anywhere. +// +// strings.TrimSpace is not that test. unicode.IsSpace reports false for the +// zero-width joiners, the soft hyphen and the BOM — all category Cf — so an +// alias of nothing but U+200B or U+FEFF passes a check built on it while naming +// exactly as little as " " does, and so does one of nothing but U+0000, which is +// neither a space nor Cf. +func isBlankName(s string) bool { + for _, r := range s { + if !unicode.IsSpace(r) && !unicode.IsControl(r) && !unicode.Is(unicode.Cf, r) { + return false + } + } + return true +} + // isCased reports whether s still carries casing an emitter should own. The test // is lowercase-idempotence, not unicode.IsUpper: a compiler neutralizes names // with strings.ToLower, so a rune that has no lowercase form (double-struck ℤ, diff --git a/ir/irverify/naming_alias_test.go b/ir/irverify/naming_alias_test.go index b2ea31f9..c6a08a1a 100644 --- a/ir/irverify/naming_alias_test.go +++ b/ir/irverify/naming_alias_test.go @@ -1,7 +1,8 @@ package irverify_test import ( - "strconv" + "fmt" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -19,8 +20,8 @@ import ( // ir/naming-cased and ir/naming-not-words if some later change starts holding // aliases to Canonical's grammar; filtering to the alias codes would leave that // test unable to fail for the reason it exists. Nothing unrelated is in the way -// either — TestVerify_NeutralCanonicalIsClean asserts this same document, minus -// the aliases, verifies empty. +// either — TestVerify_NoAliasesIsClean asserts this exact document, with the +// alias list empty, verifies empty. func aliasViolations(t *testing.T, aliases ...string) []irverify.Violation { t.Helper() return irverify.Verify(modelNamed(ir.Naming{Source: "m", Canonical: "m", Aliases: aliases})) @@ -29,22 +30,40 @@ func aliasViolations(t *testing.T, aliases ...string) []irverify.Violation { // TestVerify_VerbatimAliasesAreClean pins the settlement this check rests on: an // alias is matched against a name another schema wrote, so it is a verbatim // channel like Source and none of Canonical's neutrality rules apply to it. -// "com.example.User" is what an Avro alias looks like, and every one of -// ir/naming-cased, ir/naming-not-words and ir/naming-unsegmented would fire on -// it if the alias were held to Canonical's grammar. +// "com.example.User" is what an Avro alias looks like, and between them these +// four aliases would draw all three of ir/naming-cased, ir/naming-not-words and +// ir/naming-unsegmented if an alias were held to Canonical's grammar — the +// dotted name the first two, "API2Key" the first and last. func TestVerify_VerbatimAliasesAreClean(t *testing.T) { t.Parallel() assert.Empty(t, aliasViolations(t, "com.example.User", "UserID", "user_id", "API2Key")) } // TestVerify_BlankAliasIsAViolation covers the whole of what "names nothing" -// means. Whitespace is as blank as "" — no format's grammar admits a name made -// of it — and testing emptiness alone would let " " through the one rule that +// means. An entry made only of runes no grammar can render a name from is as +// blank as "", and testing emptiness alone — or trimming only what +// unicode.IsSpace reports — would let most of these through the one rule that // exists to catch an entry matching nothing. func TestVerify_BlankAliasIsAViolation(t *testing.T) { t.Parallel() - for _, alias := range []string{"", " ", "\t", "\n", " \t "} { - t.Run(strconv.Quote(alias), func(t *testing.T) { + blanks := map[string]string{ + "empty": "", + "space": " ", + "tab": "\t", + "newline": "\n", + "mixed spaces": " \t ", + "no-break": "\u00a0", + "ideographic": "\u3000", + "zero width": "\u200b", + "byte order": "\ufeff", + "joiner": "\u200d", + "soft hyphen": "\u00ad", + "word joiner": "\u2060", + "nul": "\x00", + "invisible mix": "\u200b\t\ufeff", + } + for name, alias := range blanks { + t.Run(name, func(t *testing.T) { t.Parallel() got := aliasViolations(t, "ok", alias) require.Len(t, got, 1, "one violation for the one blank entry") @@ -55,6 +74,15 @@ func TestVerify_BlankAliasIsAViolation(t *testing.T) { } } +// TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank holds the other side of +// that line. Only an entry with nothing visible in it is blank; judging an +// invisible rune sitting beside a visible one needs the grammar of the format +// the alias is matched under, which the IR does not have. +func TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t, "com.example.\u200bUser", " padded ")) +} + func TestVerify_DuplicateAliasIsAViolation(t *testing.T) { t.Parallel() got := aliasViolations(t, "dup", "other", "dup") @@ -67,12 +95,23 @@ func TestVerify_DuplicateAliasIsAViolation(t *testing.T) { // TestVerify_RepeatedBlankAliasReportsEachAsBlank holds the interaction between // the two rules: a second blank entry is a repeat as well as a blank one, and // reporting it as a duplicate would name the wrong repair. +// +// Each case repeats *the same* string, which is what makes the claim testable. +// With two different blanks the duplicate rule cannot fire whatever the +// implementation does, so the natural rewrite — report the repeat, then the +// blank, setting seen unconditionally — passes a two-different-blanks fixture +// while emitting exactly the wrong repair this test forbids. func TestVerify_RepeatedBlankAliasReportsEachAsBlank(t *testing.T) { t.Parallel() - got := aliasViolations(t, "", " ") - require.Len(t, got, 2) - for _, v := range got { - assert.Equal(t, "ir/naming-alias-blank", v.Code) + for name, alias := range map[string]string{"empty": "", "space": " ", "zero width": "\u200b"} { + t.Run(name, func(t *testing.T) { + t.Parallel() + got := aliasViolations(t, alias, alias) + require.Len(t, got, 2) + for _, v := range got { + assert.Equal(t, "ir/naming-alias-blank", v.Code) + } + }) } } @@ -100,3 +139,51 @@ func TestVerify_NoAliasesIsClean(t *testing.T) { t.Parallel() assert.Empty(t, aliasViolations(t)) } + +// TestVerify_AliasPathIsSpelledAsTheWalkWould ties the hand-assembled violation +// path to ir.WalkValues' own grammar. +// +// checkNaming prunes at ir.Naming — it holds no reference and no nested Naming +// to descend into — so the walk never renders these paths itself and the check +// spells them by hand. That leaves two statements of one grammar with nothing +// between them: were ir's slice-index rendering to change, every walk-produced +// path in every other check would move while these two codes alone kept the old +// spelling, and no test would say so. This is that seam, so it reddens here. +// +// Past the single digits too, which is where a hand-built path and a formatted +// one last agree. +func TestVerify_AliasPathIsSpelledAsTheWalkWould(t *testing.T) { + t.Parallel() + const size = 12 + aliases := make([]string, size) + for i := range aliases { + aliases[i] = fmt.Sprintf("alias.%d", i) // distinct, so no entry is a repeat + } + doc := modelNamed(ir.Naming{Source: "m", Canonical: "m", Aliases: aliases}) + + walked := map[string]string{} // alias → the path the walk renders for it + ir.WalkValues(doc, ir.DocumentPath, func(v reflect.Value, path string) bool { + if v.Kind() == reflect.String && v.String() != "" { + walked[v.String()] = path + } + return true + }) + for _, alias := range aliases { + require.Contains(t, walked, alias, "the walk reaches every entry when nothing prunes it") + } + + // Blank every entry so the check reports one violation per index, then hold + // each reported path to the one the walk rendered at that same index. + blank := make([]string, size) + got := aliasViolations(t, blank...) + require.Len(t, got, size) + paths := make([]string, len(got)) + for i, v := range got { + paths[i] = v.Path + } + want := make([]string, 0, size) + for _, alias := range aliases { + want = append(want, walked[alias]) + } + assert.ElementsMatch(t, want, paths) +} diff --git a/ir/naming.go b/ir/naming.go index a406bda1..41e827c3 100644 --- a/ir/naming.go +++ b/ir/naming.go @@ -38,13 +38,15 @@ type Naming struct { // ("com.example.User"), and neutralizing it to words would lose the // separators and the case the match depends on. So no neutrality rule // applies to an entry, and irverify holds only what is decidable without - // one: every entry names something and no entry repeats, since a blank - // alias matches nothing and a repeated one matches twice. + // one: every entry names something, since an entry with nothing visible in + // it matches nothing; and no entry repeats, since a repeat admits no name + // the entry before it already did, so a producer that wrote one built the + // list wrong. // - // A source that repeats an alias is recorded once, with a Diagnostic naming - // the repeat — not carried through as a repeat. That is not the lossy - // flattening invariant #2 forbids: the second entry admits no name the first - // does not, so the same set of names resolves to this entity either way. + // That a repeat is inert is also why a source that declares one is recorded + // once, with a Diagnostic naming it, rather than carried through: dropping + // it is not the lossy flattening invariant #2 forbids, because the same set + // of names resolves to this entity either way. Aliases []string `json:"aliases,omitempty"` } From b75eec57c487ac935a08af2cf74862ac3edaab48 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 11 Aug 2026 22:16:50 +0300 Subject: [PATCH 3/5] feat(irverify): hold every name channel's bytes, and an alias to its Source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ill-formed UTF-8 reached the IR through any name channel unremarked. It does not survive the round-trip the Serializable invariant promises: a marshal writes the replacement rune, so the document decodes to one that re-marshals to different bytes. checkDiagnostics already makes this claim over Diagnostic.Message, the only other free-form spec text the IR carries; ir/naming-invalid-utf8 makes it over Source, Canonical, Hint and every alias. Canonical and Hint were incidentally covered — the replacement rune is not a word character — but by a violation naming the wrong repair. An alias equal to the entity's own Source is the redundancy the duplicate rule is about, one channel over: a reader matches the name before any alias, so listing it again admits nothing new. Canonical and Hint are excluded, being names the IR derived rather than names a writer schema could have spelled. isBlankName missed the default-ignorable code points, so an alias of nothing but U+3164 HANGUL FILLER — the character conventionally used to pass off a name as empty — passed the rule written to catch exactly that. Every rune it now accepts as invisible is one Unicode classifies that way. U+2800 BRAILLE PATTERN BLANK is graphic and stays out. The duplicate message names the entry it repeats, as checkDuplicateIDs does, rather than leaving a reader of a long list to find the twin. And TestVerify_AliasSharedByTwoNamings pins the scope boundary GitHub #398 defers, which nothing held: hoisting seen into checkNaming's closure made two Namings share an alias and the suite stayed green. --- docs/ir-design.md | 37 ++++---- ir/irverify/naming.go | 147 +++++++++++++++++++++++-------- ir/irverify/naming_alias_test.go | 84 ++++++++++++++++-- ir/irverify/naming_test.go | 42 +++++++++ ir/naming.go | 21 +++-- 5 files changed, 264 insertions(+), 67 deletions(-) diff --git a/docs/ir-design.md b/docs/ir-design.md index 56f320f8..b53d6953 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -244,21 +244,28 @@ what the match is made of rather than a spelling the IR gets to decide. Neutrali throw away precisely that, which is the lossy direction lossless-by-default rules out. `Source` is the internal precedent: it carries `UserID` today and no content rule touches it, because it records what the spec said rather than deciding a spelling. What is left is decidable without any -grammar, and `irverify` holds an alias to it: every entry names something, and no entry repeats. - -**Names something** is the widest emptiness test that needs no grammar: an entry whose every rune -is a space, a control character or a zero-width format character is invisible under all of them, so -it matches nothing anywhere (`ir/naming-alias-blank`). `""`, `" "`, `"\u200b"` and `"\ufeff"` are -alike here — trimming only what `unicode.IsSpace` reports would keep the last two, which name as -little as the first two do. An invisible rune sitting *beside* a visible one is a different -question and is not asked: whether `com.example.User` is a legal name is decidable only under -the grammar of the format it will be matched against, which the IR does not know. - -**No entry repeats** because a repeat admits no name the entry before it already did -(`ir/naming-alias-duplicate`, reported at the later entry so the path names the one to delete). A -producer that wrote one built the list wrong. That the repeat is inert is also why a *source* that -declares one is recorded once with a diagnostic rather than carried through: dropping it is not the -lossy direction, since the same set of names resolves to the entity either way. +grammar, and `irverify` holds an alias to exactly that much: + +- **Every entry names something** (`ir/naming-alias-blank`). An entry whose every rune is one + Unicode classifies as invisible — a space, a control, a format character, or a default-ignorable + one — matches nothing under any grammar. `""`, `" "`, `"\u200b"` and `"\u3164"` are alike here. + An invisible rune sitting *beside* a visible one is a different question and is not asked: + whether `com.example.User` is a legal name is decidable only under the grammar of the + format it will be matched against, which the IR does not know. +- **Every entry decodes** (`ir/naming-invalid-utf8`, the one rule every channel of a `Naming` + shares). Ill-formed UTF-8 survives a marshal as the replacement rune, so the document decodes to + one that re-marshals to different bytes and the "Serializable" invariant above stops holding — + broken by a name nothing else here objects to. +- **No entry repeats another, or the entity's own `Source`** (`ir/naming-alias-duplicate`, + `ir/naming-alias-redundant`), reported at the later entry and naming the earlier, so the message + says which to delete and which to keep. Either admits no name that was not already admitted, so a + producer that wrote one built the list wrong. Only `Source` is compared against: `Canonical` and + `Hint` are names the IR derived for an emitter to render, never names a writer schema could have + spelled. + +That such an entry is inert is also why a *source* declaring one is recorded once with a diagnostic +rather than carried through: dropping it is not the lossy direction, since the same set of names +resolves to the entity either way. ### 3.3 Type references diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 3867fec8..51ebbaa3 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -5,6 +5,7 @@ import ( "strconv" "strings" "unicode" + "unicode/utf8" "github.com/dexpace/morphic/ir" ) @@ -63,9 +64,10 @@ var nameOptional = map[reflect.Type]bool{ // Only the grammar rule stays canonical-only, because a hint has no source // spelling beside it to be recomputed from. // -// Naming.Aliases is held to none of those and to two rules of its own instead, +// Naming.Aliases is held to none of those and to rules of its own instead, // because it is a verbatim channel rather than a name the IR decides — see -// appendAliasViolations, and ir.Naming.Aliases for why. +// appendAliasViolations, and ir.Naming.Aliases for why. The one rule every +// channel shares, aliases included, is appendUTF8Violation's. func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { var vs []Violation optional := map[string]bool{} @@ -87,7 +89,7 @@ func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { vs = appendAbsentViolation(vs, source, canon, hint, path) } vs = appendNamingViolations(vs, source, canon, hint, path) - vs = appendAliasViolations(vs, aliases, path) + vs = appendAliasViolations(vs, source, aliases, path) return false // Naming holds no references or nested Naming to descend into }) return vs, truncated @@ -98,6 +100,13 @@ func checkNaming(doc *ir.Document, _ declarations) ([]Violation, bool) { // reached through an unexported field cannot be (see ir.WalkValues) — which is // also why the aliases are copied out element by element rather than through // Interface(). +// +// Nothing guards the field lookups. A rename of any of these fields is a +// compile-clean change that reddens the naming tests on the next run either way: +// the three String() reads degrade to "", and Len() on the +// invalid Value panics. Neither is reachable from a document — checkNaming only +// calls this for a value whose type is ir.Naming, so every field is present — +// and a guard for the unreachable one would be a statement no test can cover. func namingChannels(naming reflect.Value) (source, canon, hint string, aliases []string) { list := naming.FieldByName("Aliases") aliases = make([]string, list.Len()) @@ -110,51 +119,70 @@ func namingChannels(naming reflect.Value) (source, canon, hint string, aliases [ aliases } -// appendAliasViolations holds one alias list to the two rules ir.Naming.Aliases -// states: every entry names something, and no entry repeats. That comment is -// where the argument lives for why those are the rules and why none of the -// neutrality rules above are — an alias is a spelling the IR records rather than -// one it decides. +// appendAliasViolations holds one alias list to the rules ir.Naming.Aliases +// states. That comment is where the argument for them lives, and for why none of +// the neutrality rules above apply. // -// Blank rather than empty is the line, and isBlankName is how wide it goes. -// Deciding whether a space *inside* "com.example. User" is legal would need the -// grammar of the format the alias is matched under, which the IR does not know; -// deciding that an entry has nothing visible in it at all needs no grammar. +// Each is decidable from the list and the Naming carrying it, with no grammar +// and no second node: whether an entry has anything visible in it +// (isBlankName), whether its bytes decode at all, and whether it admits a name +// some earlier entry — or the entity's own Source — already did. A repeat is +// reported at its later occurrence, naming the earlier one, so the message says +// which to delete and which to keep. A blank repeat is reported blank: the +// repair is to fill it in or drop it, not to distinguish it from the other +// blank. // -// A repeat is reported at its later entry, so the path names the one to delete -// rather than the one to keep. A blank repeat is reported blank: the repair is -// to fill it in or drop it, not to distinguish it from the other blank. +// Only Source is compared against. Canonical and Hint are names the IR derived +// for an emitter to render, never names a writer schema could have spelled, so +// an alias equal to one of those is not the redundancy this rule is about. // // Two neighbouring defects are deliberately left out of scope here. Repeats // across two Namings — the ambiguity that actually changes what a reader // resolves — need the whole document rather than one list, and land in -// checkDuplicateIDs' shape (GitHub #398). And every other []string in the IR -// (Namespace, Tags, Scopes, ContentTypes …) admits the same blank and repeated -// entries this rule rejects, held by nothing (GitHub #399). -func appendAliasViolations(vs []Violation, aliases []string, path string) []Violation { - seen := make(map[string]bool, len(aliases)) +// checkDuplicateIDs' shape (GitHub #398); TestVerify_AliasSharedByTwoNamings +// pins that they go unreported today, so implementing that rule cannot move the +// boundary in silence. And every other []string in the IR (Namespace, Tags, +// Scopes, ContentTypes …) admits the same blank and repeated entries this rule +// rejects, held by nothing (GitHub #399). +func appendAliasViolations(vs []Violation, source string, aliases []string, path string) []Violation { + seen := make(map[string]int, len(aliases)) for i, alias := range aliases { - at := path + ".Aliases[" + strconv.Itoa(i) + "]" - switch { + switch first, repeated := seen[alias]; { case isBlankName(alias): vs = append(vs, Violation{ Code: "ir/naming-alias-blank", Message: "alias is blank, so it matches no name", - Path: at, + Path: aliasPath(path, i), }) - case seen[alias]: + case !utf8.ValidString(alias): + vs = appendUTF8Violation(vs, "alias", alias, aliasPath(path, i)) + case repeated: vs = append(vs, Violation{ Code: "ir/naming-alias-duplicate", - Message: "alias " + alias + " is listed more than once", - Path: at, + Message: "alias " + alias + " is listed here and at index " + strconv.Itoa(first), + Path: aliasPath(path, i), + }) + case alias == source: + vs = append(vs, Violation{ + Code: "ir/naming-alias-redundant", + Message: "alias " + alias + " is the entity's own source name, so it matches nothing more", + Path: aliasPath(path, i), }) default: - seen[alias] = true + seen[alias] = i } } return vs } +// aliasPath spells one alias entry the way ir.WalkValues would have reached it. +// checkNaming prunes at ir.Naming, so the walk never renders these itself — +// TestVerify_AliasPathIsSpelledAsTheWalkWould is what holds the two spellings +// together. +func aliasPath(path string, i int) string { + return path + ".Aliases[" + strconv.Itoa(i) + "]" +} + // appendAbsentViolation reports an entity that no channel names. // // Which channel is filled is not this rule's business — a declared name goes in @@ -174,13 +202,43 @@ func appendAbsentViolation(vs []Violation, source, canon, hint, path string) []V // 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. +// carries a name for an emitter to render — plus the byte rule below, which +// every channel is held to because none of them can be read back otherwise. func appendNamingViolations(vs []Violation, source, canon, hint, path string) []Violation { + vs = appendUTF8Violation(vs, "source name", source, path) + vs = appendUTF8Violation(vs, "canonical name", canon, path) + vs = appendUTF8Violation(vs, "name hint", hint, path) vs = appendGrammarViolation(vs, source, canon, path) vs = appendContentViolations(vs, "canonical name", canon, path) return appendContentViolations(vs, "name hint", hint, path) } +// appendUTF8Violation reports a name channel carrying bytes no decoder reads +// back as what was written. It is the one rule every channel shares, aliases +// included, because it is about the encoding rather than the spelling: an +// ill-formed sequence survives a marshal as the replacement rune, so the +// document decodes to something that re-marshals to different bytes and +// invariant #7 is broken by a name nothing else here objects to. +// +// checkDiagnostics makes the same claim over the only other free-form spec text +// the IR carries (ir/diagnostic-invalid-utf8), and like it this message quotes +// nothing: repeating the bytes would put them in the report too. +// +// Canonical and Hint are only incidentally covered without this — the +// replacement rune is not a word character, so isWordSequence rejects it — and +// incidentally is not covered: the violation would name the wrong repair, since +// splitting on non-word characters is not what fixes undecodable bytes. +func appendUTF8Violation(vs []Violation, channel, name, path string) []Violation { + if utf8.ValidString(name) { + return vs + } + return append(vs, Violation{ + Code: "ir/naming-invalid-utf8", + Message: channel + " is not valid UTF-8", + Path: path, + }) +} + // appendContentViolations reports the ways the name in one channel can break // neutrality. channel is how the message spells which channel was wrong: the // two share a Path and a defect class, so the message is what tells them @@ -306,25 +364,38 @@ func isWordSequence(s string) bool { return true } -// isBlankName reports whether s holds no rune a name could be made of — the -// widest emptiness test there is that needs no format's grammar. A space, a -// control character and a zero-width format character are invisible under every -// grammar, so a string of nothing but those names nothing anywhere. +// isBlankName reports whether s holds no rune a name could be made of. Every +// rune it accepts as invisible is one Unicode itself classifies that way — a +// space, a control, a format character, or a default-ignorable one — so the +// judgement needs no format's grammar and this function decides nothing on its +// own account. +// +// strings.TrimSpace is not that test, and neither is IsSpace-plus-Cf. IsSpace +// reports false for the zero-width joiners, the soft hyphen and the BOM (all +// Cf), and all three predicates report false for U+3164 HANGUL FILLER and its +// two jamo siblings, which are default-ignorable and are the characters +// conventionally used to pass off a name as empty. An alias of nothing but any +// of these names exactly as little as " " does. // -// strings.TrimSpace is not that test. unicode.IsSpace reports false for the -// zero-width joiners, the soft hyphen and the BOM — all category Cf — so an -// alias of nothing but U+200B or U+FEFF passes a check built on it while naming -// exactly as little as " " does, and so does one of nothing but U+0000, which is -// neither a space nor Cf. +// It does not reach every rune that renders as whitespace: U+2800 BRAILLE +// PATTERN BLANK is a graphic character Unicode does not call invisible, so it is +// left alone rather than judged here — that is the boundary this test declines +// to cross without knowing the grammar the name is read under. func isBlankName(s string) bool { for _, r := range s { - if !unicode.IsSpace(r) && !unicode.IsControl(r) && !unicode.Is(unicode.Cf, r) { + if !isInvisible(r) { return false } } return true } +// isInvisible reports whether Unicode classifies r as carrying no visible mark. +func isInvisible(r rune) bool { + return unicode.IsSpace(r) || unicode.IsControl(r) || unicode.Is(unicode.Cf, r) || + unicode.Is(unicode.Other_Default_Ignorable_Code_Point, r) +} + // isCased reports whether s still carries casing an emitter should own. The test // is lowercase-idempotence, not unicode.IsUpper: a compiler neutralizes names // with strings.ToLower, so a rune that has no lowercase form (double-struck ℤ, diff --git a/ir/irverify/naming_alias_test.go b/ir/irverify/naming_alias_test.go index c6a08a1a..f951a798 100644 --- a/ir/irverify/naming_alias_test.go +++ b/ir/irverify/naming_alias_test.go @@ -4,6 +4,7 @@ import ( "fmt" "reflect" "testing" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -60,7 +61,9 @@ func TestVerify_BlankAliasIsAViolation(t *testing.T) { "soft hyphen": "\u00ad", "word joiner": "\u2060", "nul": "\x00", - "invisible mix": "\u200b\t\ufeff", + "hangul filler": "\u3164", + "jamo filler": "\u115f", + "invisible mix": "\u200b\t\ufeff\u3164", } for name, alias := range blanks { t.Run(name, func(t *testing.T) { @@ -75,21 +78,90 @@ func TestVerify_BlankAliasIsAViolation(t *testing.T) { } // TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank holds the other side of -// that line. Only an entry with nothing visible in it is blank; judging an -// invisible rune sitting beside a visible one needs the grammar of the format -// the alias is matched under, which the IR does not have. +// the line isBlankName draws — see its comment for why the IR declines to judge +// this one. U+2800 renders as blank but is a graphic character, so it belongs +// here rather than in the table above. func TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank(t *testing.T) { t.Parallel() - assert.Empty(t, aliasViolations(t, "com.example.\u200bUser", " padded ")) + assert.Empty(t, aliasViolations(t, "com.example.\u200bUser", " padded ", "\u2800")) } +// TestVerify_IllFormedAliasIsAViolation covers the rule every channel shares. +// The bytes here are a lone continuation byte: json.Marshal writes it as the +// replacement rune, so a document carrying one decodes to a different document +// and stops round-tripping — which no other rule in this file would notice, +// since ill-formed bytes are neither blank nor a repeat. +func TestVerify_IllFormedAliasIsAViolation(t *testing.T) { + t.Parallel() + ill := string([]byte{'c', 'a', 'f', 0xe9}) + require.False(t, utf8.ValidString(ill), "the fixture has to be ill-formed to test anything") + + got := aliasViolations(t, "ok", ill) + require.Len(t, got, 1) + assert.Equal(t, "ir/naming-invalid-utf8", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[1]", got[0].Path) + assert.NotContains(t, got[0].Message, ill, "the report does not repeat the bad bytes") +} + +// TestVerify_AliasRepeatingItsOwnSourceIsAViolation covers the other way an +// entry can admit no name that was not already admitted. The entity's own +// Source is matched before any alias is, so listing it again adds nothing — +// the same argument the duplicate rule rests on, one channel over. +func TestVerify_AliasRepeatingItsOwnSourceIsAViolation(t *testing.T) { + t.Parallel() + doc := modelNamed(ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"User"}}) + got := irverify.Verify(doc) + require.Len(t, got, 1) + assert.Equal(t, "ir/naming-alias-redundant", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[0]", got[0].Path) + assert.Contains(t, got[0].Message, "User") +} + +// TestVerify_AliasMatchingDerivedChannelsIsClean holds the boundary that rule +// stops at. Canonical and Hint are names the IR derived for an emitter to +// render, not names a writer schema could have spelled, so an alias equal to +// one of them is not redundant with anything a reader would match. +func TestVerify_AliasMatchingDerivedChannelsIsClean(t *testing.T) { + t.Parallel() + assert.Empty(t, irverify.Verify(modelNamed( + ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"user"}}))) + assert.Empty(t, irverify.Verify(modelNamed( + ir.Naming{Hint: "user", Aliases: []string{"user"}}))) +} + +// TestVerify_AliasSharedByTwoNamings pins the scope boundary appendAliasViolations +// declares: a repeat across two Namings goes unreported today, and closing +// GitHub #398 is what should change it. +// +// Without this, nothing holds seen to being per-Naming. Hoisting it into +// checkNaming's closure — the one-line change anyone implementing #398 reaches +// for first — makes this document report ir/naming-alias-duplicate, and every +// other test in this file stays green because each drives a document with one +// Naming in it. +func TestVerify_AliasSharedByTwoNamings(t *testing.T) { + t.Parallel() + const shared = "com.example.User" + a := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/A", + Name: ir.Naming{Source: "a", Canonical: "a", Aliases: []string{shared}}}} + b := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/B", + Name: ir.Naming{Source: "b", Canonical: "b", Aliases: []string{shared}}}} + + got := irverify.Verify(&ir.Document{IRVersion: ir.IRVersion, + Types: ir.TypeRegistry{a.ID: a, b.ID: b}}) + assert.Empty(t, got, "out of scope until GitHub #398; this is the fixture that says so") +} + +// TestVerify_DuplicateAliasIsAViolation asserts both ends of the pair. The path +// carries the entry to delete; the message carries the one it repeats, so a +// reader of a long list is not left scanning for the twin — which is how +// checkDuplicateIDs words the same defect ("declared here and at …"). func TestVerify_DuplicateAliasIsAViolation(t *testing.T) { t.Parallel() got := aliasViolations(t, "dup", "other", "dup") require.Len(t, got, 1, "the repeat is reported, not the first occurrence") assert.Equal(t, "ir/naming-alias-duplicate", got[0].Code) assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[2]", got[0].Path) - assert.Contains(t, got[0].Message, "dup") + assert.Equal(t, "alias dup is listed here and at index 0", got[0].Message) } // TestVerify_RepeatedBlankAliasReportsEachAsBlank holds the interaction between diff --git a/ir/irverify/naming_test.go b/ir/irverify/naming_test.go index fb8af191..0fdfa9e9 100644 --- a/ir/irverify/naming_test.go +++ b/ir/irverify/naming_test.go @@ -2,6 +2,7 @@ package irverify_test import ( "testing" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -368,3 +369,44 @@ func TestVerify_PresenceReachesANamingNoNameFieldOwns(t *testing.T) { assert.Equal(t, "ir/naming-absent", got[0].Code) assert.Equal(t, "doc.Services[0].Renames[t/x/Model]", got[0].Path) } + +// TestVerify_IllFormedNameIsAViolation covers the one rule every channel of a +// Naming shares. It is about the encoding rather than the spelling: ill-formed +// bytes survive a marshal as the replacement rune, so a document carrying them +// decodes to one that re-marshals differently and stops round-tripping. +// +// Canonical and Hint would each draw a violation without this rule — the +// replacement rune is not a word character, so isWordSequence rejects it — but +// one naming the wrong repair, since splitting on non-word characters is not +// what fixes undecodable bytes. Source draws nothing at all without it. So the +// assertion is that ir/naming-invalid-utf8 is among what is reported, not that +// it is all of it. +// +// Only this rule's own message is held to quoting nothing. The content rules +// beside it deliberately carry the spelling they object to, which puts the +// ill-formed bytes in their message (GitHub #400) — a separate question from +// whether the encoding rule fires. +func TestVerify_IllFormedNameIsAViolation(t *testing.T) { + t.Parallel() + ill := string([]byte{'c', 'a', 'f', 0xe9}) + require.False(t, utf8.ValidString(ill), "the fixture has to be ill-formed to test anything") + + for channel, n := range map[string]ir.Naming{ + "source": {Source: ill, Canonical: ir.CanonicalWords(ill)}, + "canonical": {Canonical: ill}, + "hint": {Hint: ill}, + } { + t.Run(channel, func(t *testing.T) { + t.Parallel() + var reported *irverify.Violation + for _, v := range irverify.Verify(modelNamed(n)) { + if v.Code == "ir/naming-invalid-utf8" { + reported = &v + } + } + require.NotNil(t, reported, "the encoding rule fires on %s", channel) + assert.Equal(t, "doc.Types[t/x/M].Name", reported.Path) + assert.NotContains(t, reported.Message, ill, "the report does not repeat the bad bytes") + }) + } +} diff --git a/ir/naming.go b/ir/naming.go index 41e827c3..1fd93e7f 100644 --- a/ir/naming.go +++ b/ir/naming.go @@ -38,15 +38,20 @@ type Naming struct { // ("com.example.User"), and neutralizing it to words would lose the // separators and the case the match depends on. So no neutrality rule // applies to an entry, and irverify holds only what is decidable without - // one: every entry names something, since an entry with nothing visible in - // it matches nothing; and no entry repeats, since a repeat admits no name - // the entry before it already did, so a producer that wrote one built the - // list wrong. + // one: // - // That a repeat is inert is also why a source that declares one is recorded - // once, with a Diagnostic naming it, rather than carried through: dropping - // it is not the lossy flattening invariant #2 forbids, because the same set - // of names resolves to this entity either way. + // - every entry names something, since one with nothing visible in it + // matches nothing; + // - every entry decodes, since one holding ill-formed UTF-8 does not + // survive the round-trip invariant #7 promises; + // - no entry repeats another, or the entity's own Source, since either + // admits no name that was not already admitted — so a producer that + // wrote one built the list wrong. + // + // That such an entry is inert is also why a source declaring one is + // recorded once, with a Diagnostic naming it, rather than carried through: + // dropping it is not the lossy flattening invariant #2 forbids, because the + // same set of names resolves to this entity either way. Aliases []string `json:"aliases,omitempty"` } From a32bc89d57d3f773c237275142fee9230f796d1c Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 11 Aug 2026 22:23:45 +0300 Subject: [PATCH 4/5] test(irverify): guard the alias interactions the rule order decides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two entries both equal to the entity's own Source were reported twice as redundant and nothing said so. That is the right report — each is wrong on its own, so both go — but recording a redundant entry in seen would make the second a duplicate of the first and name a repair that deletes one of two bad entries. The same reasoning already forced the blank/duplicate case to repeat one string rather than two different blanks. U+2800 BRAILLE PATTERN BLANK was asserted under a test named for invisible runes sitting beside visible ones, which is a different claim: it is a graphic character that renders as whitespace, and the point is that the rule leaves it alone. It gets its own name, and treating it as invisible now reddens that test rather than the other one. The file also follows the order the switch decides the rules in, and the redundant-alias message is asserted whole rather than by substring. --- ir/irverify/naming.go | 14 +-- ir/irverify/naming_alias_test.go | 165 ++++++++++++++++++------------- 2 files changed, 106 insertions(+), 73 deletions(-) diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 51ebbaa3..7ff5670c 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -43,12 +43,14 @@ var nameOptional = map[reflect.Type]bool{ reflect.TypeFor[ir.Primitive](): true, } -// checkNaming asserts every named entity has a name at all, and that the names -// it carries are what invariant #4 promises: neutral lower_snake word -// sequences, carrying no casing an emitter should own and no character that is -// not part of a word. It reuses the shared bounded walk to reach every ir.Naming -// value in the document, and reports whether that walk was cut short so a name -// past the cap cannot go unchecked in silence. +// checkNaming asserts every named entity has a name at all; that the names it +// carries are what invariant #4 promises — neutral lower_snake word sequences, +// carrying no casing an emitter should own and no character that is not part of +// a word; and that every channel's bytes decode, which is a claim about the +// encoding rather than the spelling and so is the one rule they all share. It +// reuses the shared bounded walk to reach every ir.Naming value in the document, +// and reports whether that walk was cut short so a name past the cap cannot go +// unchecked in silence. // // Presence is separate from those content rules because each of them is // vacuously true of the empty string: an entirely empty Naming satisfied all diff --git a/ir/irverify/naming_alias_test.go b/ir/irverify/naming_alias_test.go index f951a798..d6b7cd5e 100644 --- a/ir/irverify/naming_alias_test.go +++ b/ir/irverify/naming_alias_test.go @@ -14,10 +14,11 @@ import ( ) // aliasViolations returns everything Verify reports on a model named with -// aliases. +// aliases. Its Source is "m", which no fixture below lists, so the +// redundant-with-Source rule is out of the way of the ones being measured. // -// Unfiltered on purpose. The test below asserting this is empty is the one -// pinning that no neutrality rule reaches an alias, so it has to be able to see +// Unfiltered on purpose. TestVerify_VerbatimAliasesAreClean is the one pinning +// that no neutrality rule reaches an alias, so it has to be able to see // ir/naming-cased and ir/naming-not-words if some later change starts holding // aliases to Canonical's grammar; filtering to the alias codes would leave that // test unable to fail for the reason it exists. Nothing unrelated is in the way @@ -40,11 +41,16 @@ func TestVerify_VerbatimAliasesAreClean(t *testing.T) { assert.Empty(t, aliasViolations(t, "com.example.User", "UserID", "user_id", "API2Key")) } +func TestVerify_NoAliasesIsClean(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t)) +} + // TestVerify_BlankAliasIsAViolation covers the whole of what "names nothing" -// means. An entry made only of runes no grammar can render a name from is as -// blank as "", and testing emptiness alone — or trimming only what -// unicode.IsSpace reports — would let most of these through the one rule that -// exists to catch an entry matching nothing. +// means. An entry made only of runes Unicode calls invisible is as blank as "", +// and testing emptiness alone — or trimming only what unicode.IsSpace reports — +// would let most of these through the one rule that exists to catch an entry +// matching nothing. func TestVerify_BlankAliasIsAViolation(t *testing.T) { t.Parallel() blanks := map[string]string{ @@ -77,20 +83,28 @@ func TestVerify_BlankAliasIsAViolation(t *testing.T) { } } -// TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank holds the other side of -// the line isBlankName draws — see its comment for why the IR declines to judge -// this one. U+2800 renders as blank but is a graphic character, so it belongs -// here rather than in the table above. +// TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank holds one side of the line +// isBlankName draws — see its comment for why the IR declines to judge an +// invisible rune that is not the whole entry. func TestVerify_InvisibleRuneBesideAVisibleOneIsNotBlank(t *testing.T) { t.Parallel() - assert.Empty(t, aliasViolations(t, "com.example.\u200bUser", " padded ", "\u2800")) + assert.Empty(t, aliasViolations(t, "com.example.\u200bUser", " padded ")) +} + +// TestVerify_BlankLookingGraphicAliasIsNotBlank holds the other side. U+2800 +// BRAILLE PATTERN BLANK renders as whitespace but is a graphic character +// Unicode does not classify as invisible, so the rule leaves it alone rather +// than deciding on its own account what looks empty. +func TestVerify_BlankLookingGraphicAliasIsNotBlank(t *testing.T) { + t.Parallel() + assert.Empty(t, aliasViolations(t, "\u2800")) } // TestVerify_IllFormedAliasIsAViolation covers the rule every channel shares. // The bytes here are a lone continuation byte: json.Marshal writes it as the // replacement rune, so a document carrying one decodes to a different document -// and stops round-tripping — which no other rule in this file would notice, -// since ill-formed bytes are neither blank nor a repeat. +// and stops round-tripping. No other rule here would notice — the replacement +// rune is visible, so an ill-formed entry is not blank. func TestVerify_IllFormedAliasIsAViolation(t *testing.T) { t.Parallel() ill := string([]byte{'c', 'a', 'f', 0xe9}) @@ -103,18 +117,32 @@ func TestVerify_IllFormedAliasIsAViolation(t *testing.T) { assert.NotContains(t, got[0].Message, ill, "the report does not repeat the bad bytes") } +// TestVerify_DuplicateAliasIsAViolation asserts both ends of the pair. The path +// carries the entry to delete; the message carries the one it repeats, so a +// reader of a long list is not left scanning for the twin — which is how +// checkDuplicateIDs words the same defect ("declared here and at …"). +func TestVerify_DuplicateAliasIsAViolation(t *testing.T) { + t.Parallel() + got := aliasViolations(t, "dup", "other", "dup") + require.Len(t, got, 1, "the repeat is reported, not the first occurrence") + assert.Equal(t, "ir/naming-alias-duplicate", got[0].Code) + assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[2]", got[0].Path) + assert.Equal(t, "alias dup is listed here and at index 0", got[0].Message) +} + // TestVerify_AliasRepeatingItsOwnSourceIsAViolation covers the other way an -// entry can admit no name that was not already admitted. The entity's own -// Source is matched before any alias is, so listing it again adds nothing — -// the same argument the duplicate rule rests on, one channel over. +// entry can admit no name that was not already admitted. A reader matches the +// entity's own Source before any alias, so listing it again adds nothing — the +// argument the duplicate rule rests on, one channel over. func TestVerify_AliasRepeatingItsOwnSourceIsAViolation(t *testing.T) { t.Parallel() - doc := modelNamed(ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"User"}}) - got := irverify.Verify(doc) + got := irverify.Verify(modelNamed( + ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"User"}})) require.Len(t, got, 1) assert.Equal(t, "ir/naming-alias-redundant", got[0].Code) assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[0]", got[0].Path) - assert.Contains(t, got[0].Message, "User") + assert.Equal(t, "alias User is the entity's own source name, so it matches nothing more", + got[0].Message) } // TestVerify_AliasMatchingDerivedChannelsIsClean holds the boundary that rule @@ -123,50 +151,21 @@ func TestVerify_AliasRepeatingItsOwnSourceIsAViolation(t *testing.T) { // one of them is not redundant with anything a reader would match. func TestVerify_AliasMatchingDerivedChannelsIsClean(t *testing.T) { t.Parallel() - assert.Empty(t, irverify.Verify(modelNamed( - ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"user"}}))) - assert.Empty(t, irverify.Verify(modelNamed( - ir.Naming{Hint: "user", Aliases: []string{"user"}}))) -} - -// TestVerify_AliasSharedByTwoNamings pins the scope boundary appendAliasViolations -// declares: a repeat across two Namings goes unreported today, and closing -// GitHub #398 is what should change it. -// -// Without this, nothing holds seen to being per-Naming. Hoisting it into -// checkNaming's closure — the one-line change anyone implementing #398 reaches -// for first — makes this document report ir/naming-alias-duplicate, and every -// other test in this file stays green because each drives a document with one -// Naming in it. -func TestVerify_AliasSharedByTwoNamings(t *testing.T) { - t.Parallel() - const shared = "com.example.User" - a := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/A", - Name: ir.Naming{Source: "a", Canonical: "a", Aliases: []string{shared}}}} - b := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/B", - Name: ir.Naming{Source: "b", Canonical: "b", Aliases: []string{shared}}}} - - got := irverify.Verify(&ir.Document{IRVersion: ir.IRVersion, - Types: ir.TypeRegistry{a.ID: a, b.ID: b}}) - assert.Empty(t, got, "out of scope until GitHub #398; this is the fixture that says so") -} - -// TestVerify_DuplicateAliasIsAViolation asserts both ends of the pair. The path -// carries the entry to delete; the message carries the one it repeats, so a -// reader of a long list is not left scanning for the twin — which is how -// checkDuplicateIDs words the same defect ("declared here and at …"). -func TestVerify_DuplicateAliasIsAViolation(t *testing.T) { - t.Parallel() - got := aliasViolations(t, "dup", "other", "dup") - require.Len(t, got, 1, "the repeat is reported, not the first occurrence") - assert.Equal(t, "ir/naming-alias-duplicate", got[0].Code) - assert.Equal(t, "doc.Types[t/x/M].Name.Aliases[2]", got[0].Path) - assert.Equal(t, "alias dup is listed here and at index 0", got[0].Message) + for channel, n := range map[string]ir.Naming{ + "canonical": {Source: "User", Canonical: "user", Aliases: []string{"user"}}, + "hint": {Hint: "user", Aliases: []string{"user"}}, + } { + t.Run(channel, func(t *testing.T) { + t.Parallel() + assert.Empty(t, irverify.Verify(modelNamed(n))) + }) + } } // TestVerify_RepeatedBlankAliasReportsEachAsBlank holds the interaction between -// the two rules: a second blank entry is a repeat as well as a blank one, and -// reporting it as a duplicate would name the wrong repair. +// the blank rule and the duplicate rule: a second blank entry is a repeat as +// well as a blank one, and reporting it as a duplicate would name the wrong +// repair. // // Each case repeats *the same* string, which is what makes the claim testable. // With two different blanks the duplicate rule cannot fire whatever the @@ -187,6 +186,22 @@ func TestVerify_RepeatedBlankAliasReportsEachAsBlank(t *testing.T) { } } +// TestVerify_RepeatedSourceAliasReportsEachAsRedundant holds the same +// interaction one rule over. Both entries name what Source already names, so +// both go; recording a redundant entry in seen would report the second as a +// duplicate of the first and leave a reader deleting one of two entries that +// are each wrong on their own. +func TestVerify_RepeatedSourceAliasReportsEachAsRedundant(t *testing.T) { + t.Parallel() + got := irverify.Verify(modelNamed( + ir.Naming{Source: "User", Canonical: "user", Aliases: []string{"User", "User"}})) + require.Len(t, got, 2) + for i, v := range got { + assert.Equal(t, "ir/naming-alias-redundant", v.Code) + assert.Equal(t, fmt.Sprintf("doc.Types[t/x/M].Name.Aliases[%d]", i), v.Path) + } +} + // TestVerify_IssueReproducerIsReported drives the exact value from the issue — // cased, punctuated, empty and duplicated together — and states which of the // four the IR objects to and which it accepts by design. @@ -207,19 +222,36 @@ func TestVerify_IssueReproducerIsReported(t *testing.T) { }, byCode) } -func TestVerify_NoAliasesIsClean(t *testing.T) { +// TestVerify_AliasSharedByTwoNamings pins the scope boundary +// appendAliasViolations declares: a repeat across two Namings goes unreported +// today, and closing GitHub #398 is what should change it. +// +// Without this, nothing holds seen to being per-Naming. Hoisting it into +// checkNaming's closure — the one-line change anyone implementing #398 reaches +// for first — makes this document report ir/naming-alias-duplicate, and every +// other test in this file stays green, because each drives a document with one +// Naming in it. +func TestVerify_AliasSharedByTwoNamings(t *testing.T) { t.Parallel() - assert.Empty(t, aliasViolations(t)) + const shared = "com.example.User" + a := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/A", + Name: ir.Naming{Source: "a", Canonical: "a", Aliases: []string{shared}}}} + b := &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/x/B", + Name: ir.Naming{Source: "b", Canonical: "b", Aliases: []string{shared}}}} + + got := irverify.Verify(&ir.Document{IRVersion: ir.IRVersion, + Types: ir.TypeRegistry{a.ID: a, b.ID: b}}) + assert.Empty(t, got, "out of scope until GitHub #398; this is the fixture that says so") } // TestVerify_AliasPathIsSpelledAsTheWalkWould ties the hand-assembled violation // path to ir.WalkValues' own grammar. // // checkNaming prunes at ir.Naming — it holds no reference and no nested Naming -// to descend into — so the walk never renders these paths itself and the check +// to descend into — so the walk never renders these paths itself and aliasPath // spells them by hand. That leaves two statements of one grammar with nothing // between them: were ir's slice-index rendering to change, every walk-produced -// path in every other check would move while these two codes alone kept the old +// path in every other check would move while these codes alone kept the old // spelling, and no test would say so. This is that seam, so it reddens here. // // Past the single digits too, which is where a hand-built path and a formatted @@ -246,8 +278,7 @@ func TestVerify_AliasPathIsSpelledAsTheWalkWould(t *testing.T) { // Blank every entry so the check reports one violation per index, then hold // each reported path to the one the walk rendered at that same index. - blank := make([]string, size) - got := aliasViolations(t, blank...) + got := aliasViolations(t, make([]string, size)...) require.Len(t, got, size) paths := make([]string, len(got)) for i, v := range got { From ec6cb6f04a7d236b1f077e224cbc8370d72ea9e2 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 11 Aug 2026 22:58:22 +0300 Subject: [PATCH 5/5] docs(ir-design): state the byte rule where each channel's rules are The rule reaching every name channel was written down only inside the Aliases paragraph, so a reader asking what Canonical or Hint is held to would not find it. It moves up beside the shape rules, and the alias bullet points at it instead of restating the argument. Two things went stale in that paragraph as the rules grew: it counted the alias rules, which is the kind of number CLAUDE.md says to derive or omit, and it said aliases are held to "none of that" immediately after describing a rule they are held to. utf8Violation splits the report from the test that decides it, so the alias switch appends it directly rather than calling a helper that re-validates what the branch condition already established. --- docs/ir-design.md | 38 +++++++++++++++++++++----------------- ir/irverify/naming.go | 34 ++++++++++++++++++++++------------ 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/docs/ir-design.md b/docs/ir-design.md index b53d6953..e7354e5b 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -232,19 +232,25 @@ category alone would lose the second. `irverify` holds every `Naming` to this: the shape rules run over `Canonical` and `Hint` alike, and it recomputes the canonical from the `Source` beside it, so a boundary in the wrong place is a -compiler bug rather than a variant reading. What no check can say -is that the grammar itself is right — a check that recomputes moves with what it recomputes through -— so the answers are pinned by a conformance table and the properties every answer must satisfy by -a fuzz target beside it (GitHub #186). - -**`Aliases` is held to none of that, and to two rules of its own.** An alias is matched against a -name *another* schema wrote — an Avro alias is a full name, `com.example.User`, and resolution -compares it verbatim against the writer schema's full name — so the separators and the casing are -what the match is made of rather than a spelling the IR gets to decide. Neutralizing one would -throw away precisely that, which is the lossy direction lossless-by-default rules out. `Source` is -the internal precedent: it carries `UserID` today and no content rule touches it, because it -records what the spec said rather than deciding a spelling. What is left is decidable without any -grammar, and `irverify` holds an alias to exactly that much: +compiler bug rather than a variant reading. What no check can say is that the grammar itself is +right — a check that recomputes moves with what it recomputes through — so the answers are pinned +by a conformance table and the properties every answer must satisfy by a fuzz target beside it +(GitHub #186). + +One rule sits under all of those and under `Aliases` too, because it is about the encoding rather +than the spelling: **every channel's bytes must decode** (`ir/naming-invalid-utf8`). Ill-formed +UTF-8 survives a marshal as the replacement rune, so a document carrying it decodes to one that +re-marshals to different bytes and the "Serializable" invariant above stops holding — broken by a +name nothing else here objects to. + +**`Aliases` is held to none of the shape rules, and to rules of its own instead.** An alias is +matched against a name *another* schema wrote — an Avro alias is a full name, `com.example.User`, +and resolution compares it verbatim against the writer schema's full name — so the separators and +the casing are what the match is made of rather than a spelling the IR gets to decide. +Neutralizing one would throw away precisely that, which is the lossy direction +lossless-by-default rules out. `Source` is the internal precedent: it carries `UserID` today and +no content rule touches it, because it records what the spec said rather than deciding a spelling. +What is left is decidable without any grammar, and `irverify` holds an alias to exactly that much: - **Every entry names something** (`ir/naming-alias-blank`). An entry whose every rune is one Unicode classifies as invisible — a space, a control, a format character, or a default-ignorable @@ -252,10 +258,8 @@ grammar, and `irverify` holds an alias to exactly that much: An invisible rune sitting *beside* a visible one is a different question and is not asked: whether `com.example.User` is a legal name is decidable only under the grammar of the format it will be matched against, which the IR does not know. -- **Every entry decodes** (`ir/naming-invalid-utf8`, the one rule every channel of a `Naming` - shares). Ill-formed UTF-8 survives a marshal as the replacement rune, so the document decodes to - one that re-marshals to different bytes and the "Serializable" invariant above stops holding — - broken by a name nothing else here objects to. +- **Every entry decodes** — the shared byte rule above, which reaches an alias the same way it + reaches the other three channels. - **No entry repeats another, or the entity's own `Source`** (`ir/naming-alias-duplicate`, `ir/naming-alias-redundant`), reported at the later entry and naming the earlier, so the message says which to delete and which to keep. Either admits no name that was not already admitted, so a diff --git a/ir/irverify/naming.go b/ir/irverify/naming.go index 7ff5670c..5966d6d9 100644 --- a/ir/irverify/naming.go +++ b/ir/irverify/naming.go @@ -157,7 +157,7 @@ func appendAliasViolations(vs []Violation, source string, aliases []string, path Path: aliasPath(path, i), }) case !utf8.ValidString(alias): - vs = appendUTF8Violation(vs, "alias", alias, aliasPath(path, i)) + vs = append(vs, utf8Violation("alias", aliasPath(path, i))) case repeated: vs = append(vs, Violation{ Code: "ir/naming-alias-duplicate", @@ -215,30 +215,40 @@ func appendNamingViolations(vs []Violation, source, canon, hint, path string) [] return appendContentViolations(vs, "name hint", hint, path) } -// appendUTF8Violation reports a name channel carrying bytes no decoder reads -// back as what was written. It is the one rule every channel shares, aliases -// included, because it is about the encoding rather than the spelling: an -// ill-formed sequence survives a marshal as the replacement rune, so the +// utf8Violation is the report for a name channel carrying bytes no decoder +// reads back as what was written. It is the one rule every channel shares, +// aliases included, because it is about the encoding rather than the spelling: +// an ill-formed sequence survives a marshal as the replacement rune, so the // document decodes to something that re-marshals to different bytes and // invariant #7 is broken by a name nothing else here objects to. // // checkDiagnostics makes the same claim over the only other free-form spec text // the IR carries (ir/diagnostic-invalid-utf8), and like it this message quotes -// nothing: repeating the bytes would put them in the report too. +// nothing: repeating the bytes would put them in the report too. That is also +// why the value is not a parameter — there is nothing here to say about it +// beyond which channel it arrived in. // -// Canonical and Hint are only incidentally covered without this — the +// Canonical and Hint are only incidentally covered without this rule — the // replacement rune is not a word character, so isWordSequence rejects it — and // incidentally is not covered: the violation would name the wrong repair, since // splitting on non-word characters is not what fixes undecodable bytes. +func utf8Violation(channel, path string) Violation { + return Violation{ + Code: "ir/naming-invalid-utf8", + Message: channel + " is not valid UTF-8", + Path: path, + } +} + +// appendUTF8Violation reports channel's name when its bytes are ill-formed. The +// alias rule decides the same thing in its own switch and appends +// utf8Violation directly, since a switch branch needs the test separate from +// the report. func appendUTF8Violation(vs []Violation, channel, name, path string) []Violation { if utf8.ValidString(name) { return vs } - return append(vs, Violation{ - Code: "ir/naming-invalid-utf8", - Message: channel + " is not valid UTF-8", - Path: path, - }) + return append(vs, utf8Violation(channel, path)) } // appendContentViolations reports the ways the name in one channel can break