From 00becff0d74989cb54e52827ef809bef897a758e Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 30 Jul 2026 16:21:01 +0300 Subject: [PATCH 1/6] fix: pin tracked files to LF so goldens survive a Windows checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CompareGolden reads the golden file and compares it byte-for-byte against JSON that encodeGolden always writes with "\n". The repository carried no .gitattributes, so a checkout with core.autocrlf=true — the default on Windows — rewrote every fixture to CRLF and failed TestGolden, TestConformance and TestConformance_UnwitnessedIRFields with whole-file diffs unrelated to the IR. Cloning with that setting converts 346 tracked files, so the effect is the whole corpus, not a fixture or two. Pin the repository to LF instead of leaving it to the platform. One rule covers everything because every tracked file is text. The accompanying test is the only thing that would report the pin's removal: CI runs on Linux and checks out LF whether the rule is there or not, so the corpus itself cannot notice. It checks the file pins LF and then asks git what it would check the fixtures out as, which is the half that survives a reformulation of the patterns. --- .gitattributes | 12 +++++ ir/irtest/lineendings_test.go | 93 +++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 .gitattributes create mode 100644 ir/irtest/lineendings_test.go diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..2e00c69 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,12 @@ +# Line endings are pinned rather than left to the platform. +# +# The golden corpus is compared byte-for-byte against JSON that ir/irtest always +# encodes with "\n", so a checkout that rewrote the fixtures to CRLF would fail +# every golden and conformance test for a reason unrelated to the IR. That is the +# default on Windows (core.autocrlf=true), and CI is Linux-only, so nothing here +# would report it. +# +# One rule covers the repository because every tracked file is text: Go, YAML, +# JSON, Markdown, SVG, shell. text=auto still leaves genuinely binary content +# alone if any is ever added. +* text=auto eol=lf diff --git a/ir/irtest/lineendings_test.go b/ir/irtest/lineendings_test.go new file mode 100644 index 0000000..6484eb6 --- /dev/null +++ b/ir/irtest/lineendings_test.go @@ -0,0 +1,93 @@ +package irtest_test + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// eolProbes are paths whose line endings the golden comparison depends on: an IR +// snapshot, the spec it was compiled from, and a Go source file standing for the +// rest of the tree. Each is required to exist, so a rename cannot quietly turn +// the checks below into assertions about nothing. +var eolProbes = []string{ + "testdata/golden/openapi/petstore.golden.json", + "testdata/conformance/openapi/named-types.yaml", + "ir/irtest/golden.go", +} + +// TestGitAttributes_PinsLineEndings asserts the repository carries the +// .gitattributes that keeps CompareGolden's byte comparison meaningful. +// +// CompareGolden compares the file on disk against JSON encodeGolden always +// writes with "\n", so a checkout that rewrote the fixtures to CRLF fails every +// golden and conformance test with a whole-file diff unrelated to the IR — the +// default on Windows, where core.autocrlf=true. Nothing else in the suite would +// notice the pin's removal: CI is Linux-only and checks out LF regardless +// (GitHub #48). +func TestGitAttributes_PinsLineEndings(t *testing.T) { + t.Parallel() + root := repoRoot(t) + raw, err := os.ReadFile(filepath.Join(root, ".gitattributes")) + require.NoError(t, err, ".gitattributes must exist at the repository root") + + var pins []string + for _, line := range strings.Split(string(raw), "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" && !strings.HasPrefix(trimmed, "#") { + pins = append(pins, trimmed) + } + } + require.NotEmpty(t, pins, ".gitattributes carries only comments") + assert.True(t, strings.Contains(strings.Join(pins, "\n"), "eol=lf"), + ".gitattributes must pin line endings to LF, got rules %q", pins) +} + +// TestGitAttributes_ResolveToLF asks git what it would check the fixtures out +// as, rather than reading the patterns and reasoning about them. It is the half +// that survives a reformulation of the rules: any set of patterns resolving +// eol=lf for these paths passes, and any that leaves one unpinned fails however +// it is written. +func TestGitAttributes_ResolveToLF(t *testing.T) { + t.Parallel() + root := repoRoot(t) + for _, rel := range eolProbes { + require.FileExists(t, filepath.Join(root, rel)) + } + + args := append([]string{"check-attr", "eol", "--"}, eolProbes...) + cmd := exec.Command("git", args...) + cmd.Dir = root + out, err := cmd.Output() + if err != nil { + t.Skipf("git check-attr unavailable (%v); the pin itself is checked by TestGitAttributes_PinsLineEndings", err) + } + + lines := strings.Split(strings.TrimSpace(string(out)), "\n") + require.Len(t, lines, len(eolProbes), "git reports one line per queried path") + for i, line := range lines { + assert.Equal(t, eolProbes[i]+": eol: lf", line, + "git must check %s out with LF endings", eolProbes[i]) + } +} + +// repoRoot walks up from this file to the directory holding go.mod. +func repoRoot(t *testing.T) string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + require.True(t, ok, "runtime.Caller failed") + dir := filepath.Dir(file) + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + require.NotEqual(t, parent, dir, "reached filesystem root without finding go.mod") + dir = parent + } +} From 7cfc27f8dd98b04d962fee62825d710d92624cff Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 30 Jul 2026 16:32:15 +0300 Subject: [PATCH 2/6] refactor(compilers/compile): own the canonical naming grammar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ir.Naming.Canonical is ABI, and canonicalWords was written three times: once here, once in the GraphQL draft, once in the Protobuf draft. Run over the same inputs, 8 of 13 spellings canonicalized differently — the drafts leave "/", "[", "]", "+", ":", "{" and "}" inside a word sequence, and disagree with each other about ".". An emitter reading Canonical cannot tell which grammar produced it, which is what invariant 4 exists to rule out. Move the grammar to the framework, where one definition serves every compiler, and add NamingFor beside it: eleven of the twelve call sites were the same Source-plus-Canonical pairing, and the pairing is the invariant — a Source with no Canonical leaves the emitter to segment the spelling itself. Goldens do not move. The segmentation decision landed separately (#161), so this promotes the grammar that already ships; the drafts move when they rebase onto it, each carrying its own golden update. Two copies deleted is a state, not a rule, so the architecture test now asserts that only the framework and ir may fill Canonical. Its planted counter-test is what makes the sweep evidence: a matcher that recognized nothing would otherwise pass it and read as proof. The registry sweep beside it moves onto the same walk rather than keeping a second one. The conformance suite pins the boundaries irverify cannot see: "foo2bar" and "foo_2_bar" are both word sequences, so only a shared implementation settles which one a compiler owes. --- compilers/compile/doc.go | 16 ++- compilers/compile/naming.go | 88 ++++++++++++ compilers/compile/naming_test.go | 89 ++++++++++++ compilers/openapi/auth.go | 3 +- compilers/openapi/compose.go | 3 +- compilers/openapi/content.go | 3 +- compilers/openapi/hoist.go | 2 +- compilers/openapi/meta.go | 3 +- compilers/openapi/operations.go | 11 +- compilers/openapi/operations_test.go | 2 +- compilers/openapi/params.go | 3 +- compilers/openapi/schema.go | 65 +-------- compilers/openapi/schema_test.go | 37 ----- internal/archtest/grammar_test.go | 207 +++++++++++++++++++++++++++ internal/archtest/registry_test.go | 40 ++---- 15 files changed, 422 insertions(+), 150 deletions(-) create mode 100644 compilers/compile/naming.go create mode 100644 compilers/compile/naming_test.go create mode 100644 internal/archtest/grammar_test.go diff --git a/compilers/compile/doc.go b/compilers/compile/doc.go index bb189ef..66b33b1 100644 --- a/compilers/compile/doc.go +++ b/compilers/compile/doc.go @@ -1,14 +1,16 @@ // Package compile holds the state every spec compiler needs and the invariants // that state carries, so each compiler does not reimplement them. // -// It owns two things: the type registry together with the source-coordinate map -// that keeps invariant 3 true (stable IDs; one node per source coordinate), and -// diagnostic accumulation with identity dedup. Nothing else. It imports only ir. +// It owns three things: the type registry together with the source-coordinate +// map that keeps invariant 3 true (stable IDs; one node per source coordinate), +// diagnostic accumulation with identity dedup, and the canonical naming grammar +// invariant 4 makes a property of the IR rather than of a compiler. Nothing else. +// It imports only ir. // -// The package boundary is the point. An architecture test asserts that no -// package outside this one writes to an ir.TypeRegistry directly, and that rule -// is inexpressible without an outside — which is why a package this small is -// worth its own directory. +// The package boundary is the point. Architecture tests assert that no package +// outside this one writes to an ir.TypeRegistry or derives a canonical name of +// its own, and rules like those are inexpressible without an outside — which is +// why a package this small is worth its own directory. // // What deliberately stays with the compiler: ir.Document assembly (seventeen of // its eighteen fields carry no framework invariant), recursion depth bounds (the diff --git a/compilers/compile/naming.go b/compilers/compile/naming.go new file mode 100644 index 0000000..f4c8739 --- /dev/null +++ b/compilers/compile/naming.go @@ -0,0 +1,88 @@ +package compile + +import ( + "strings" + "unicode" + + "github.com/dexpace/morphic/ir" +) + +// NamingFor builds the neutral Naming of a name the source declares: the +// spelling it used, plus the canonical word sequence derived from it. +// +// It is the constructor to reach for wherever a source name becomes an IR name, +// because the pairing is the invariant — a Source with no Canonical leaves an +// emitter to segment the spelling itself, which is the casing decision invariant +// 4 moves out of the compilers. A Naming that carries only a Hint is a different +// case and does not come through here: nothing declared it. +func NamingFor(source string) ir.Naming { + return ir.Naming{Source: source, Canonical: CanonicalWords(source)} +} + +// CanonicalWords renders name as the neutral lower_snake word sequence +// ir.Naming.Canonical promises: it splits on every non-word rune and on +// camel-case and letter/digit boundaries, lowercases, and joins with "_". It +// holds no acronym opinion beyond boundary detection; casing policy is an +// emitter concern. +// +// The framework owns this rather than each compiler because Canonical is ABI. +// Invariant 4 makes neutral naming a property of the IR, so "example.v1" cannot +// canonicalize to example_v1 from one compiler and example.v1 from another: an +// emitter reading Canonical has no way to tell which grammar produced it. Three +// copies of this function disagreed on exactly that (GitHub #163). +// +// A name written with no word rune in it at all ("***") canonicalizes to the +// empty string. Naming.Source keeps the spelling either way, so nothing is lost +// — there is simply no word sequence to report, and inventing one from the +// punctuation would be a naming opinion the IR does not hold. +func CanonicalWords(name string) string { + var words []string + var cur []rune + flush := func() { + if len(cur) > 0 { + words = append(words, strings.ToLower(string(cur))) + cur = cur[:0] + } + } + runes := []rune(name) + for i, r := range runes { + if !isWordRune(r) { + flush() + continue + } + if len(cur) > 0 && wordBoundary(cur[len(cur)-1], r, runes, i) { + flush() + } + cur = append(cur, r) + } + flush() + return strings.Join(words, "_") +} + +// isWordRune reports whether r belongs to a word rather than separating two. +// Letters and digits are the word characters, and a combining mark is part of +// the letter it follows — a decomposed "é" is one letter written as two runes, +// so reading the mark as a separator would split a word in half. +// +// Everything else separates, which is what makes the result a word sequence +// whatever the source spelled the boundary as: a dot in a namespaced component +// name or a proto package, a slash in a media type or a path template, brackets +// around a query parameter, and the _/-/space a name may already use. +func isWordRune(r rune) bool { + return unicode.IsLetter(r) || unicode.IsDigit(r) || unicode.IsMark(r) +} + +// wordBoundary reports whether a new word starts at runes[i] given the previous +// accumulated rune prev. +func wordBoundary(prev, r rune, runes []rune, i int) bool { + switch { + case unicode.IsUpper(r) && (unicode.IsLower(prev) || unicode.IsDigit(prev)): + return true // lower/digit -> Upper: "userID" -> user|ID + case unicode.IsUpper(prev) && unicode.IsUpper(r) && i+1 < len(runes) && unicode.IsLower(runes[i+1]): + return true // acronym tail: "HTTPServer" -> HTTP|Server + case unicode.IsLetter(prev) && unicode.IsDigit(r), unicode.IsDigit(prev) && unicode.IsLetter(r): + return true // letter<->digit: "APIKey2" -> ...Key|2 + default: + return false + } +} diff --git a/compilers/compile/naming_test.go b/compilers/compile/naming_test.go new file mode 100644 index 0000000..0fd8218 --- /dev/null +++ b/compilers/compile/naming_test.go @@ -0,0 +1,89 @@ +package compile_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/dexpace/morphic/compilers/compile" + "github.com/dexpace/morphic/ir" +) + +// canonicalCases is the conformance suite for the one canonical-naming grammar. +// It is a suite rather than a handful of examples because Canonical is ABI: an +// emitter cannot tell which compiler produced a name, so every compiler owes the +// same answer for the same spelling, and the three copies this replaced gave +// different ones (GitHub #163). +// +// Each row names the rule it pins. The rows drawn from a format other than +// OpenAPI are the point of the suite: irverify rejects a canonical that is not a +// word sequence, so a compiler leaving "." in place is already caught, but +// nothing outside this table settles where the boundaries fall *inside* a word — +// "foo2bar" and "foo_2_bar" are both word sequences. +var canonicalCases = []struct { + rule string + in string + want string +}{ + {"a single lowercase word is itself", "user", "user"}, + {"casing is dropped", "User", "user"}, + {"camelCase splits", "firstName", "first_name"}, + {"a trailing acronym splits", "userID", "user_id"}, + {"a leading acronym keeps its tail", "HTTPServer", "http_server"}, + {"letter/digit boundaries split", "APIKey2", "api_key_2"}, + {"digit/letter boundaries split too", "v2Beta", "v_2_beta"}, + {"an inner digit run splits both sides", "foo2bar", "foo_2_bar"}, + {"an existing snake name is left alone", "user_name", "user_name"}, + {"a hyphen separates", "list-users", "list_users"}, + {"a space separates", "list users", "list_users"}, + {"a dot separates: a namespaced component name", "com.example.User", "com_example_user"}, + {"a dot separates: a proto package", "example.v1", "example_v_1"}, + {"a slash separates: a media type", "application/json", "application_json"}, + {"brackets separate: a deep-object parameter", "filter[name]", "filter_name"}, + {"braces and slashes separate: an operation hint", "get /pets/{petId}", "get_pets_pet_id"}, + {"a header mixing separators", "X-Trace.Id", "x_trace_id"}, + {"leading and trailing separators produce no empty words", "..padded.name..", "padded_name"}, + {"punctuation is not a word character", "$weird@name!", "weird_name"}, + // A decomposed é (e + combining acute) is one letter written as two runes: + // the mark belongs to the letter before it, so reading it as a separator + // would split a word in half. + {"a combining mark belongs to its letter", "cafe\u0301_v2", "cafe\u0301_v_2"}, + {"a name with no word rune has no words", "***", ""}, + {"the empty name is empty", "", ""}, +} + +func TestCanonicalWords_Conformance(t *testing.T) { + t.Parallel() + for _, tc := range canonicalCases { + t.Run(tc.rule, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, compile.CanonicalWords(tc.in), "input %q", tc.in) + }) + } +} + +// TestCanonicalWords_IsIdempotent asserts the grammar is a fixed point: feeding +// a canonical back in returns it unchanged. Without that, a name that crossed +// two stages would drift, and it is the property irverify's segmentation check +// relies on to be decidable from the value alone. +func TestCanonicalWords_IsIdempotent(t *testing.T) { + t.Parallel() + for _, tc := range canonicalCases { + t.Run(tc.rule, func(t *testing.T) { + t.Parallel() + once := compile.CanonicalWords(tc.in) + assert.Equal(t, once, compile.CanonicalWords(once), "re-canonicalizing %q", once) + }) + } +} + +// TestNamingFor_KeepsTheSpellingAndTheWords pins the pairing that makes a Naming +// neutral: the source spelling is preserved verbatim for anyone who needs the +// original, and the words beside it carry no casing an emitter should own. +func TestNamingFor_KeepsTheSpellingAndTheWords(t *testing.T) { + t.Parallel() + assert.Equal(t, ir.Naming{Source: "com.example.User", Canonical: "com_example_user"}, + compile.NamingFor("com.example.User")) + assert.Equal(t, ir.Naming{Source: "***", Canonical: ""}, compile.NamingFor("***"), + "a spelling with no words still keeps its spelling") +} diff --git a/compilers/openapi/auth.go b/compilers/openapi/auth.go index b9d2a61..c2829e9 100644 --- a/compilers/openapi/auth.go +++ b/compilers/openapi/auth.go @@ -6,6 +6,7 @@ import ( soa "github.com/speakeasy-api/openapi/openapi" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) @@ -39,7 +40,7 @@ func (l *lowerer) lowerSecuritySchemes() { func (l *lowerer) lowerSecurityScheme(name string, ss *soa.SecurityScheme) ir.AuthScheme { scheme := ir.AuthScheme{ ID: authIDFor(name), - Name: ir.Naming{Source: name, Canonical: canonicalWords(name)}, + Name: compile.NamingFor(name), Docs: ir.Docs{Description: ss.GetDescription()}, Provenance: ir.Provenance{Source: l.srcIndex, Pointer: ptr("components", "securitySchemes", name)}, } diff --git a/compilers/openapi/compose.go b/compilers/openapi/compose.go index 3a88ad1..d41ee37 100644 --- a/compilers/openapi/compose.go +++ b/compilers/openapi/compose.go @@ -10,6 +10,7 @@ import ( "github.com/speakeasy-api/openapi/values" yaml "gopkg.in/yaml.v3" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) @@ -917,7 +918,7 @@ func (l *lowerer) enumMembers(nodes []values.Value) ([]ir.EnumMember, ir.PrimKin return nil, "", false } members = append(members, ir.EnumMember{ - Name: ir.Naming{Source: text, Canonical: canonicalWords(text)}, + Name: compile.NamingFor(text), Value: val, }) } diff --git a/compilers/openapi/content.go b/compilers/openapi/content.go index e704dc5..d0812b2 100644 --- a/compilers/openapi/content.go +++ b/compilers/openapi/content.go @@ -9,6 +9,7 @@ import ( "github.com/speakeasy-api/openapi/sequencedmap" yaml "gopkg.in/yaml.v3" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) @@ -203,7 +204,7 @@ func (l *lowerer) lowerHeader(h *soa.Header, name, hptr, hdecl string) ir.Proper schemaPtr := hdecl + ptr("schema") p := ir.Property{ ID: propID(hptr), - Name: ir.Naming{Source: name, Canonical: canonicalWords(name)}, + Name: compile.NamingFor(name), WireName: name, Type: l.carriedSchemaRef(h.GetSchema(), schemaPtr, declarationHint(hdecl, name)), Required: h.GetRequired(), diff --git a/compilers/openapi/hoist.go b/compilers/openapi/hoist.go index 8e338cf..9a49d52 100644 --- a/compilers/openapi/hoist.go +++ b/compilers/openapi/hoist.go @@ -122,7 +122,7 @@ func (l *lowerer) commonFor(id ir.TypeID, pointer, hint string) ir.TypeCommon { Provenance: ir.Provenance{Source: l.srcIndex, Pointer: pointer}, } if name, ok := componentSchemaName(pointer); ok { - common.Name = ir.Naming{Source: name, Canonical: canonicalWords(name)} + common.Name = compile.NamingFor(name) } else { common.Anonymous = true common.Name = ir.Naming{Hint: hint} diff --git a/compilers/openapi/meta.go b/compilers/openapi/meta.go index c7eafa4..0890098 100644 --- a/compilers/openapi/meta.go +++ b/compilers/openapi/meta.go @@ -3,6 +3,7 @@ package openapi import ( soa "github.com/speakeasy-api/openapi/openapi" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) @@ -70,7 +71,7 @@ func lowerServer(s *soa.Server) ir.Server { Variables: serverVariables(s), } if name := s.GetName(); name != "" { - srv.Name = ir.Naming{Source: name, Canonical: canonicalWords(name)} + srv.Name = compile.NamingFor(name) } return srv } diff --git a/compilers/openapi/operations.go b/compilers/openapi/operations.go index 0d80448..2530058 100644 --- a/compilers/openapi/operations.go +++ b/compilers/openapi/operations.go @@ -8,6 +8,7 @@ import ( "github.com/speakeasy-api/openapi/references" yaml "gopkg.in/yaml.v3" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) @@ -38,7 +39,7 @@ func (l *lowerer) lowerService() ir.Service { } if info := l.doc.GetInfo(); info != nil { title := info.GetTitle() - svc.Name = ir.Naming{Source: title, Canonical: canonicalWords(title)} + svc.Name = compile.NamingFor(title) svc.Docs.Description = info.GetDescription() } svc.Auth = l.lowerSecurityRequirements(l.doc.GetSecurity()) @@ -165,14 +166,14 @@ func (l *lowerer) lowerWebhooks(groups *serviceGroups) { func (l *lowerer) groupFor(src *soa.Operation, path string) (key string, name ir.Naming, docs ir.Docs, inferred string) { if l.opts.Grouping == GroupByPathPrefix { seg := firstPathSegment(path) - return "seg:" + seg, ir.Naming{Source: seg, Canonical: canonicalWords(seg)}, ir.Docs{}, "group-path-prefix" + return "seg:" + seg, compile.NamingFor(seg), ir.Docs{}, "group-path-prefix" } tags := src.GetTags() if len(tags) == 0 { return "default", ir.Naming{Hint: "default"}, ir.Docs{}, "" } first := tags[0] - return "tag:" + first, ir.Naming{Source: first, Canonical: canonicalWords(first)}, l.tagDocs(first), "" + return "tag:" + first, compile.NamingFor(first), l.tagDocs(first), "" } // tagDocs returns the declared docs for a tag name, or empty when undeclared. @@ -283,9 +284,9 @@ func (l *lowerer) checkOperationIDUnique(op ir.Operation, mount string) { // hint so emitters can synthesize a name. func operationName(src *soa.Operation, method, uriTemplate string) ir.Naming { if id := src.GetOperationID(); id != "" { - return ir.Naming{Source: id, Canonical: canonicalWords(id)} + return compile.NamingFor(id) } - return ir.Naming{Hint: canonicalWords(method + " " + uriTemplate)} + return ir.Naming{Hint: compile.CanonicalWords(method + " " + uriTemplate)} } // fillOperationDocs maps an operation's summary, description, and externalDocs diff --git a/compilers/openapi/operations_test.go b/compilers/openapi/operations_test.go index 35aa9fc..39c8ddb 100644 --- a/compilers/openapi/operations_test.go +++ b/compilers/openapi/operations_test.go @@ -507,7 +507,7 @@ func TestOperation_NoOperationIdHint(t *testing.T) { requireNoErrorDiags(t, diags) op := firstOp(t, svc) assert.Empty(t, op.Name.Source, "no operationId leaves an empty source name") - assert.Equal(t, canonicalWords("get /ping"), op.Name.Hint) + assert.Equal(t, "get_ping", op.Name.Hint, "the hint is canonicalized, not the raw method and template") } const opsSpec = `openapi: 3.1.0 diff --git a/compilers/openapi/params.go b/compilers/openapi/params.go index a7210e5..3d242d0 100644 --- a/compilers/openapi/params.go +++ b/compilers/openapi/params.go @@ -4,6 +4,7 @@ import ( oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" soa "github.com/speakeasy-api/openapi/openapi" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) @@ -37,7 +38,7 @@ func (l *lowerer) lowerParameters(params []sourcedParam) ([]ir.Parameter, []ir.H func (l *lowerer) lowerParameter(p *soa.Parameter, pptr string) (ir.Parameter, ir.HTTPParamBinding) { name, in := p.GetName(), p.GetIn() param := ir.Parameter{ - Name: ir.Naming{Source: name, Canonical: canonicalWords(name)}, + Name: compile.NamingFor(name), Required: p.GetRequired() || in == soa.ParameterInPath, } style, explode := resolveStyleExplode(p, in) diff --git a/compilers/openapi/schema.go b/compilers/openapi/schema.go index f7e5db5..f583e91 100644 --- a/compilers/openapi/schema.go +++ b/compilers/openapi/schema.go @@ -7,12 +7,12 @@ import ( "slices" "strconv" "strings" - "unicode" "github.com/speakeasy-api/openapi/extensions" oas3 "github.com/speakeasy-api/openapi/jsonschema/oas3" yaml "gopkg.in/yaml.v3" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) @@ -630,7 +630,7 @@ func (l *lowerer) fillModelProperties(m *ir.Model, s *oas3.Schema, pointer strin ppointer := pointer + ptr("properties", name) p := ir.Property{ ID: propID(ppointer), - Name: ir.Naming{Source: name, Canonical: canonicalWords(name)}, + Name: compile.NamingFor(name), WireName: name, Type: l.carriedSchemaRef(js, ppointer, name), Required: required[name], @@ -1843,64 +1843,3 @@ func jsonObject(members []rawMember) ir.RawValue { b.WriteByte('}') return ir.RawValue(b.String()) } - -// canonicalWords renders name as a neutral lower_snake word sequence: it splits -// on every non-word rune and on camel-case and letter/digit boundaries, -// lowercases, and joins with "_". It holds no acronym opinion beyond boundary -// detection; casing policy is a emitter concern. -// -// A name written with no word rune in it at all ("***") canonicalizes to the -// empty string. Naming.Source keeps the spelling either way, so nothing is lost -// — there is simply no word sequence to report, and inventing one from the -// punctuation would be a naming opinion the IR does not hold. -func canonicalWords(name string) string { - var words []string - var cur []rune - flush := func() { - if len(cur) > 0 { - words = append(words, strings.ToLower(string(cur))) - cur = cur[:0] - } - } - runes := []rune(name) - for i, r := range runes { - if !isWordRune(r) { - flush() - continue - } - if len(cur) > 0 && wordBoundary(cur[len(cur)-1], r, runes, i) { - flush() - } - cur = append(cur, r) - } - flush() - return strings.Join(words, "_") -} - -// isWordRune reports whether r belongs to a word rather than separating two. -// Letters and digits are the word characters, and a combining mark is part of -// the letter it follows — a decomposed "é" is one letter written as two runes, -// so reading the mark as a separator would split a word in half. -// -// Everything else separates, which is what makes the result a word sequence -// whatever the source spelled the boundary as: a dot in a namespaced component -// name, a slash in a media type or a path template, brackets around a query -// parameter, and the _/-/space a name may already use. -func isWordRune(r rune) bool { - return unicode.IsLetter(r) || unicode.IsDigit(r) || unicode.IsMark(r) -} - -// wordBoundary reports whether a new word starts at runes[i] given the previous -// accumulated rune prev. -func wordBoundary(prev, r rune, runes []rune, i int) bool { - switch { - case unicode.IsUpper(r) && (unicode.IsLower(prev) || unicode.IsDigit(prev)): - return true // lower/digit -> Upper: "userID" -> user|ID - case unicode.IsUpper(prev) && unicode.IsUpper(r) && i+1 < len(runes) && unicode.IsLower(runes[i+1]): - return true // acronym tail: "HTTPServer" -> HTTP|Server - case unicode.IsLetter(prev) && unicode.IsDigit(r), unicode.IsDigit(prev) && unicode.IsLetter(r): - return true // letter<->digit: "APIKey2" -> ...Key|2 - default: - return false - } -} diff --git a/compilers/openapi/schema_test.go b/compilers/openapi/schema_test.go index 8000ff4..717bd91 100644 --- a/compilers/openapi/schema_test.go +++ b/compilers/openapi/schema_test.go @@ -208,43 +208,6 @@ func TestLower_InlineSchemaHoistedOnce(t *testing.T) { assert.Empty(t, item.Name.Source, "hoisted inline types carry a hint, not a source name") } -func TestCanonicalWords(t *testing.T) { - t.Parallel() - for in, want := range map[string]string{ - "userID": "user_id", "HTTPServer": "http_server", "list-users": "list_users", - "User": "user", "APIKey2": "api_key_2", - } { - assert.Equal(t, want, canonicalWords(in), "input %q", in) - } -} - -// TestCanonicalWords_NonWordRunesSeparate pins the segmentation rule: a word is -// letters, digits and the combining marks that belong to them, and every other -// rune separates two words. Names written with a dot, a slash, a bracket or a -// space reach the compiler from real specs — a namespaced component name, a -// bracketed query parameter, a path template — and each used to land in -// Canonical verbatim, which is not the word sequence ir.Naming promises. -func TestCanonicalWords_NonWordRunesSeparate(t *testing.T) { - t.Parallel() - for in, want := range map[string]string{ - "com.example.User": "com_example_user", - "filter[name]": "filter_name", - "application/json": "application_json", - "X-Trace.Id": "x_trace_id", - "get /pets/{petId}": "get_pets_pet_id", - "..padded.name..": "padded_name", - "$weird@name!": "weird_name", - // A decomposed é (e + combining acute) is one letter written as two - // runes: the mark belongs to the letter before it, so reading it as a - // separator would split a word in half. - "cafe\u0301_v2": "cafe\u0301_v_2", - "***": "", - "": "", - } { - assert.Equal(t, want, canonicalWords(in), "input %q", in) - } -} - func TestSchemaRef_BooleanAndUntypedShapes(t *testing.T) { t.Parallel() spec := componentSpec(` S: diff --git a/internal/archtest/grammar_test.go b/internal/archtest/grammar_test.go new file mode 100644 index 0000000..4e0dc9a --- /dev/null +++ b/internal/archtest/grammar_test.go @@ -0,0 +1,207 @@ +package archtest_test + +import ( + "fmt" + "go/ast" + "go/parser" + "go/printer" + "go/token" + "io/fs" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// grammarOwners are the packages permitted to spell the naming grammar +// themselves: the framework that implements it, and ir, which declares the field +// it fills. +var grammarOwners = []string{"compilers/compile", "ir"} + +// TestNamingGrammar_CanonicalIsFilledByTheFrameworkOnly asserts that no +// production package outside grammarOwners fills ir.Naming.Canonical itself. +// +// Canonical is ABI: an emitter reading it cannot tell which compiler produced the +// name, so a compiler holding its own segmentation opinion makes the field mean +// two things at once. That is not hypothetical — three copies of the grammar +// disagreed about "." and about every other non-word character (GitHub #163) — +// and deleting two copies does not stop a fourth being written. Only a rule +// outside the compilers does. +// +// Deliberately not checked: a Canonical filled from a local variable reads as a +// violation even when the variable came from the framework, so the call belongs +// at the site; Naming.Hint is out of scope and is cased today (GitHub #54); and a +// literal inside package ir is spelled Naming rather than ir.Naming, which is one +// reason ir is an owner rather than a swept package. +func TestNamingGrammar_CanonicalIsFilledByTheFrameworkOnly(t *testing.T) { + t.Parallel() + offenders := sweepProduction(t, repoRoot(t), grammarOwners, canonicalViolations) + assert.Empty(t, offenders, + "only %v may derive a canonical name; everything else goes through compile.NamingFor or compile.CanonicalWords", + grammarOwners) +} + +// TestCanonicalViolations_LocalGrammarIsCaught plants what the sweep exists to +// find — a compiler deriving its own canonical, in both the shapes it can be +// written — and pins that the framework call beside it stays clean. Without the +// planted half, a matcher recognizing nothing at all would pass the sweep above +// and read as proof. +func TestCanonicalViolations_LocalGrammarIsCaught(t *testing.T) { + t.Parallel() + const src = `package graphql + +func lower(name string) []ir.Naming { + declared := ir.Naming{Source: name, Canonical: canonicalWords(name)} + framework := ir.Naming{Source: name, Canonical: compile.CanonicalWords(name)} + hint := ir.Naming{Hint: localWords(name)} + var late ir.Naming + late.Canonical = strings.ToLower(name) + return []ir.Naming{declared, framework, hint, late} +} +` + offenders, err := canonicalViolations("planted.go", "compilers/graphql/naming.go", src) + require.NoError(t, err) + require.Len(t, offenders, 2, + "the literal and the later assignment, not the framework call or the hint: %v", offenders) + assert.Contains(t, offenders[0], "canonicalWords(name)") + assert.Contains(t, offenders[1], "strings.ToLower(name)") +} + +// canonicalViolations reports every place in one file that fills +// ir.Naming.Canonical from something other than a call into the framework +// package, whether as a composite-literal field or an assignment afterwards. +// +// src is nil to read the file at path, or the source itself for a planted test; +// rel names the file in the messages. +func canonicalViolations(path, rel string, src any) ([]string, error) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, src, 0) + if err != nil { + return nil, fmt.Errorf("archtest: parse %s: %w", path, err) + } + + var found []string + report := func(expr ast.Expr) { + found = append(found, fmt.Sprintf("%s:%d: Canonical is filled by %s rather than by the framework", + rel, fset.Position(expr.Pos()).Line, exprText(fset, expr))) + } + ast.Inspect(file, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.CompositeLit: + reportCanonicalField(node, report) + case *ast.AssignStmt: + reportCanonicalAssign(node, report) + default: + } + return true + }) + return found, nil +} + +// reportCanonicalField reports the Canonical field of an ir.Naming composite +// literal when it is not filled by a framework call. +func reportCanonicalField(lit *ast.CompositeLit, report func(ast.Expr)) { + if !isSelector(lit.Type, "ir", "Naming") { + return + } + for _, elt := range lit.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + key, ok := kv.Key.(*ast.Ident) + if ok && key.Name == "Canonical" && !isFrameworkCall(kv.Value) { + report(kv.Value) + } + } +} + +// reportCanonicalAssign reports an assignment to a .Canonical field whose value +// is not a framework call. It closes the way round the literal check: build an +// empty Naming, then fill the field. +func reportCanonicalAssign(stmt *ast.AssignStmt, report func(ast.Expr)) { + for i, lhs := range stmt.Lhs { + sel, ok := lhs.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Canonical" { + continue + } + // A multi-value right-hand side has no single expression to attribute to + // the field, and no legitimate shape assigns Canonical that way. + if len(stmt.Rhs) != len(stmt.Lhs) { + report(lhs) + continue + } + if !isFrameworkCall(stmt.Rhs[i]) { + report(stmt.Rhs[i]) + } + } +} + +// isFrameworkCall reports whether expr is a call on the framework package, such +// as compile.CanonicalWords(name). Any of its functions counts: the rule is where +// the grammar lives, not which entry point reaches it. +func isFrameworkCall(expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + pkg, ok := sel.X.(*ast.Ident) + return ok && pkg.Name == "compile" +} + +// exprText renders expr as source, so a failure names the expression a reader has +// to go and change rather than only where it is. +func exprText(fset *token.FileSet, expr ast.Expr) string { + var b strings.Builder + if err := printer.Fprint(&b, fset, expr); err != nil { + return "" + } + return b.String() +} + +// sweepProduction runs scan over every production Go file under root except +// those inside owners, and returns everything the scans found. +// +// It is shared by the rules that are about what a package may write rather than +// what it may import, so each new rule is a scan function rather than another +// tree walk. +func sweepProduction(t *testing.T, root string, owners []string, + scan func(path, rel string, src any) ([]string, error), +) []string { + t.Helper() + var found []string + var scanned int + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return skipUninterestingDir(root, p, d) + } + if !isProductionGoFile(d.Name()) { + return nil + } + rel, relErr := filepath.Rel(root, p) + require.NoError(t, relErr) + slashed := filepath.ToSlash(rel) + if hasPrefixDir(slashed, owners) { + return nil + } + hits, scanErr := scan(p, slashed, nil) + if scanErr != nil { + return scanErr + } + scanned++ + found = append(found, hits...) + return nil + }) + require.NoError(t, err) + require.NotZero(t, scanned, "the sweep reached no production Go file, so an empty result proves nothing") + return found +} diff --git a/internal/archtest/registry_test.go b/internal/archtest/registry_test.go index 8326d35..00a5a94 100644 --- a/internal/archtest/registry_test.go +++ b/internal/archtest/registry_test.go @@ -1,16 +1,15 @@ package archtest_test import ( + "fmt" "go/ast" "go/parser" "go/token" "io/fs" - "path/filepath" "strings" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) // registryOwners are the packages permitted to write an ir.TypeRegistry @@ -38,29 +37,7 @@ var registryOwners = []string{"compilers/compile", "ir"} // package would obtain a registry to write in the first place. func TestRegistryWrites_StayInsideTheFramework(t *testing.T) { t.Parallel() - root := repoRoot(t) - - var offenders []string - err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() { - return skipUninterestingDir(root, path, d) - } - if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { - return nil - } - rel, relErr := filepath.Rel(root, path) - require.NoError(t, relErr) - if hasPrefixDir(filepath.ToSlash(rel), registryOwners) { - return nil - } - offenders = append(offenders, registryWrites(t, path, filepath.ToSlash(rel))...) - return nil - }) - require.NoError(t, err) - + offenders := sweepProduction(t, repoRoot(t), registryOwners, registryWrites) assert.Empty(t, offenders, "only %v may write an ir.TypeRegistry; everything else goes through compile.Types", registryOwners) @@ -90,12 +67,13 @@ func hasPrefixDir(rel string, dirs []string) bool { } // registryWrites returns a description of every registry construction or write -// in the file at path. -func registryWrites(t *testing.T, path, rel string) []string { - t.Helper() +// in the file at path. src is nil to read that file, or the source itself. +func registryWrites(path, rel string, src any) ([]string, error) { fset := token.NewFileSet() - file, err := parser.ParseFile(fset, path, nil, 0) - require.NoError(t, err) + file, err := parser.ParseFile(fset, path, src, 0) + if err != nil { + return nil, fmt.Errorf("archtest: parse %s: %w", path, err) + } var found []string ast.Inspect(file, func(n ast.Node) bool { @@ -114,7 +92,7 @@ func registryWrites(t *testing.T, path, rel string) []string { } return true }) - return found + return found, nil } // isSelector reports whether e is the qualified identifier pkg.name. From c4cf6605e2977c62cbafd3e0eef53dcd26808818 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 30 Jul 2026 16:46:40 +0300 Subject: [PATCH 3/6] refactor(compilers/compile): own the ID grammar and namespace rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every compiler derives IR identifiers and all three spelled the grammar themselves: the t/, op/, p/, s/ and auth/ prefixes, and the namespace that follows them. The prefixes are read across compilers — a diagnostic renderer or an IR diff sees IDs from all of them — and the drafts had already drifted, with two of the three leaving the anonymous namespace unqualified by format, so nothing but coincidence kept their IDs apart. Move the grammar and leave the derivation: the framework supplies the prefix, the namespace and the separators, while the path stays with the compiler that can compute it. A Space is a named type so a call cannot transpose the namespace and the path. The namespace rule stops being a comment. Invariant 3's corollary — a minted node takes a namespace no source coordinate addresses — was enforced by prose in composedTypeID and by nothing else: minting into the source namespace instead moved one golden line-for-line, and after -update the whole suite was green, irverify and pass.Validate included. compile.Types now records how each namespace is addressed and refuses the second use, whichever order they arrive in, so the same mutation raises an internal-invariant diagnostic that survives a golden regeneration. Goldens are byte-identical; this changes no IR. The architecture test gains the rule that keeps it that way: inside compilers, only the framework builds an ID from a string. Elsewhere the conversion is legitimate — pass and irverify re-type an ID they already hold — so the sweep stops at the compilers. --- compilers/compile/doc.go | 31 +++++--- compilers/compile/ids.go | 88 +++++++++++++++++++++++ compilers/compile/ids_test.go | 111 +++++++++++++++++++++++++++++ compilers/compile/types.go | 50 ++++++++++--- compilers/openapi/ids.go | 45 ++++++++---- internal/archtest/grammar_test.go | 94 +++++++++++++++++++++--- internal/archtest/registry_test.go | 2 +- 7 files changed, 377 insertions(+), 44 deletions(-) create mode 100644 compilers/compile/ids.go create mode 100644 compilers/compile/ids_test.go diff --git a/compilers/compile/doc.go b/compilers/compile/doc.go index 66b33b1..8dd6a3d 100644 --- a/compilers/compile/doc.go +++ b/compilers/compile/doc.go @@ -1,21 +1,30 @@ // Package compile holds the state every spec compiler needs and the invariants // that state carries, so each compiler does not reimplement them. // -// It owns three things: the type registry together with the source-coordinate -// map that keeps invariant 3 true (stable IDs; one node per source coordinate), -// diagnostic accumulation with identity dedup, and the canonical naming grammar -// invariant 4 makes a property of the IR rather than of a compiler. Nothing else. -// It imports only ir. +// It owns what every compiler must agree on, and nothing else. It imports only +// ir. +// +// - The type registry with the source-coordinate map that keeps invariant 3 +// true (stable IDs; one node per source coordinate), and the namespace rule +// that comes with it: a node a lowering mints takes a namespace no source +// coordinate addresses. +// - Diagnostic accumulation with identity dedup. +// - The canonical naming grammar, which invariant 4 makes a property of the IR +// rather than of a compiler. +// - The identifier grammar: the kind prefix that opens an ID and the namespace +// that follows it. // // The package boundary is the point. Architecture tests assert that no package -// outside this one writes to an ir.TypeRegistry or derives a canonical name of -// its own, and rules like those are inexpressible without an outside — which is -// why a package this small is worth its own directory. +// outside this one writes to an ir.TypeRegistry, derives a canonical name, or +// builds an ID from a string, and rules like those are inexpressible without an +// outside — which is why a package this small is worth its own directory. // // What deliberately stays with the compiler: ir.Document assembly (seventeen of // its eighteen fields carry no framework invariant), recursion depth bounds (the // right cap for a JSON Schema walk is not the right one for an SDL walk), and -// the ID-derivation scheme (Intern takes the ID as a parameter). Promoting a -// borderline item here later is additive; demoting one is a breaking change -// across every compiler, so borderline items start outside. +// the derivation an ID's path comes from — a JSON Pointer, a GraphQL structural +// path and a protobuf fully-qualified name are different things, and the format +// that computes one is the only place that can. Promoting a borderline item here +// later is additive; demoting one is a breaking change across every compiler, so +// borderline items start outside. package compile diff --git a/compilers/compile/ids.go b/compilers/compile/ids.go new file mode 100644 index 0000000..7138d36 --- /dev/null +++ b/compilers/compile/ids.go @@ -0,0 +1,88 @@ +package compile + +import ( + "strings" + + "github.com/dexpace/morphic/ir" +) + +// A Space is the namespace an ID's path is addressed in: the format's own name +// for a node some source coordinate denotes ("openapi"), or a space of its own +// for a node a lowering mints ("composed"). +// +// It is a named type rather than a string so a call cannot transpose the space +// and the path — two same-typed string parameters being exactly the argument +// order this package cannot check for a caller. +type Space string + +// PrimSpace holds the primitive leaves. +// +// It is the one space deliberately shared across formats: every compiler must +// reach t/prim/string for the same leaf, or two documents lowered from different +// formats disagree about the identity of the same type. Every other space is one +// format's, and a space that is shared by accident rather than on purpose is what +// this distinction exists to make visible. +const PrimSpace Space = "prim" + +// The kind prefix that opens an ID. They are spelled once here because a +// consumer switching on a prefix — a diagnostic renderer, an IR diff — reads +// every compiler's IDs, so a compiler with its own vocabulary breaks it +// (GitHub #162). +const ( + typeKind = "t" + opKind = "op" + propKind = "p" + authKind = "auth" + serviceKind = "s" +) + +// TypeID returns the ID of the type at path within space. +// +// The path is the compiler's own derivation and stays there: a JSON Pointer, a +// GraphQL structural path and a protobuf fully-qualified name are different +// things, and nothing here can compute them. What the framework fixes is the +// grammar around the path — the kind prefix, the space, and the separator +// between them. +func TypeID(space Space, path string) ir.TypeID { return ir.TypeID(idFor(typeKind, space, path)) } + +// OpID returns the ID of the operation at path within space. +func OpID(space Space, path string) ir.OpID { return ir.OpID(idFor(opKind, space, path)) } + +// PropID returns the ID of the property at path within space. +func PropID(space Space, path string) ir.PropID { return ir.PropID(idFor(propKind, space, path)) } + +// AuthID returns the ID of the security scheme at path within space. +func AuthID(space Space, path string) ir.AuthID { return ir.AuthID(idFor(authKind, space, path)) } + +// ServiceID returns the ID of the service at path within space. +func ServiceID(space Space, path string) ir.ServiceID { + return ir.ServiceID(idFor(serviceKind, space, path)) +} + +// PrimTypeID returns the shared ID of the primitive of kind k. +func PrimTypeID(k ir.PrimKind) ir.TypeID { return TypeID(PrimSpace, string(k)) } + +// idFor joins a kind prefix, a space and a path with single separators. +// +// The path's leading separator is supplied here rather than assumed, so a +// compiler whose paths carry one and a compiler whose paths do not derive the +// same ID, and neither can produce "t/openapicomponents/schemas/User". A space +// with no path is an ID in its own right — the space names one node — and gets no +// trailing separator. +func idFor(kind string, space Space, path string) string { + trimmed := strings.TrimPrefix(path, "/") + if trimmed == "" { + return kind + "/" + string(space) + } + return kind + "/" + string(space) + "/" + trimmed +} + +// spaceOf returns the space id is addressed in, or "" when id carries no space +// segment — which no ID built through this package does. +func spaceOf(id ir.TypeID) Space { + parts := strings.SplitN(string(id), "/", 3) + if len(parts) < 2 { + return "" + } + return Space(parts[1]) +} diff --git a/compilers/compile/ids_test.go b/compilers/compile/ids_test.go new file mode 100644 index 0000000..35fc025 --- /dev/null +++ b/compilers/compile/ids_test.go @@ -0,0 +1,111 @@ +package compile_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/compilers/compile" + "github.com/dexpace/morphic/ir" +) + +// TestIDGrammar_KindPrefixes pins the spelling of every ID kind, because the +// prefix is what a consumer reading IDs from more than one compiler switches on. +// The expected strings are written out rather than derived: a test that built +// them the way the code does would agree with any change to it. +func TestIDGrammar_KindPrefixes(t *testing.T) { + t.Parallel() + const space compile.Space = "openapi" + const pointer = "/components/schemas/User" + + assert.Equal(t, ir.TypeID("t/openapi/components/schemas/User"), compile.TypeID(space, pointer)) + assert.Equal(t, ir.OpID("op/openapi/paths/~1pets/get"), compile.OpID(space, "/paths/~1pets/get")) + assert.Equal(t, ir.PropID("p/openapi/components/schemas/User/properties/id"), + compile.PropID(space, "/components/schemas/User/properties/id")) + assert.Equal(t, ir.AuthID("auth/openapi/components/securitySchemes/apiKey"), + compile.AuthID(space, "/components/securitySchemes/apiKey")) + assert.Equal(t, ir.ServiceID("s/openapi/0"), compile.ServiceID(space, "0")) + assert.Equal(t, ir.TypeID("t/prim/string"), compile.PrimTypeID(ir.PrimString)) +} + +// TestIDGrammar_PathSeparatorIsSuppliedOnce pins that the framework owns the +// boundary between the space and the path. A format whose paths carry a leading +// separator (an RFC 6901 pointer) and one whose paths do not (a protobuf +// fully-qualified name) must reach the same shape, and neither may run the space +// into the path. +func TestIDGrammar_PathSeparatorIsSuppliedOnce(t *testing.T) { + t.Parallel() + const space compile.Space = "protobuf" + + assert.Equal(t, compile.TypeID(space, "/example.v1.User"), compile.TypeID(space, "example.v1.User")) + assert.Equal(t, ir.TypeID("t/protobuf/example.v1.User"), compile.TypeID(space, "example.v1.User")) + assert.Equal(t, ir.TypeID("t/protobuf"), compile.TypeID(space, ""), + "a space naming one node gets no trailing separator") +} + +// TestTypes_MintingIntoASourceSpaceIsRefused plants invariant 3's corollary +// broken — a minted node taking a namespace that addresses source coordinates — +// and asserts it is refused whichever order the two arrive in. +// +// Both orders are asserted because that is the failure's whole character: the +// document is well-formed either way and only the winner of the collision +// changes, so a single-order test passes on a compiler that is already wrong. +func TestTypes_MintingIntoASourceSpaceIsRefused(t *testing.T) { + t.Parallel() + tests := []struct { + name string + run func(types *compile.Types) + }{ + {"source coordinate first", func(types *compile.Types) { + types.Intern("/components/schemas/User", "t/openapi/components/schemas/User", model) + types.Register("t/openapi/components/schemas/User/oneOf/0", model()) + }}, + {"minted node first", func(types *compile.Types) { + types.Register("t/openapi/components/schemas/User/oneOf/0", model()) + types.Intern("/components/schemas/User", "t/openapi/components/schemas/User", model) + }}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + types := compile.NewTypes(0) + tc.run(types) + require.Len(t, types.Violations(), 1, "the collision is reported exactly once") + assert.Contains(t, types.Violations()[0], `namespace "openapi"`) + assert.Contains(t, types.Violations()[0], "needs a namespace of its own") + }) + } +} + +// TestTypes_SeparateSpacesAreNotRefused is the negative control: the arrangement +// the OpenAPI compiler actually uses — coordinates in openapi and anon, minted +// variants in composed — must stay silent, or the check above would be reporting +// on every compile. +func TestTypes_SeparateSpacesAreNotRefused(t *testing.T) { + t.Parallel() + types := compile.NewTypes(0) + types.Intern("/components/schemas/User", "t/openapi/components/schemas/User", model) + types.Intern("/components/schemas/User/properties/tags", "t/anon/components/schemas/User/properties/tags", model) + types.Register("t/composed/components/schemas/User/oneOf/0", model()) + types.Register("t/composed/components/schemas/User/oneOf/1", model()) + + assert.Empty(t, types.Violations()) + assert.Equal(t, 4, types.Len()) +} + +// TestTypes_IDWithNoSpaceSegmentClaimsNothing covers the one shape the namespace +// check cannot judge. Nothing built through this package produces it, so it is +// left alone rather than refused: an ID with no namespace cannot be a namespace +// used two ways. +func TestTypes_IDWithNoSpaceSegmentClaimsNothing(t *testing.T) { + t.Parallel() + types := compile.NewTypes(0) + types.Register("bare", model()) + types.Intern("/p", "bare", model) + + assert.Empty(t, types.Violations()) +} + +// model returns a distinct empty Model, the simplest node the registry accepts. +func model() ir.TypeDef { return &ir.Model{} } diff --git a/compilers/compile/types.go b/compilers/compile/types.go index c2595da..ffaa440 100644 --- a/compilers/compile/types.go +++ b/compilers/compile/types.go @@ -7,15 +7,6 @@ import ( "github.com/dexpace/morphic/ir" ) -// PrimTypeID returns the shared ID of the primitive of kind k. -// -// Primitives are IR-universal: every compiler must intern t/prim/string under -// the same ID, or two documents lowered from different formats disagree about -// the identity of the same leaf type. The scheme lives here rather than in a -// compiler for that reason — unlike named and anonymous IDs, which stay -// per-compiler and reach Intern as a parameter. -func PrimTypeID(k ir.PrimKind) ir.TypeID { return ir.TypeID("t/prim/" + string(k)) } - // Types owns the type registry and the source-coordinate to ir.TypeID map that // together keep invariant 3 true: one node per source coordinate, addressed by a // stable synthetic ID. @@ -28,6 +19,10 @@ type Types struct { byPointer map[string]ir.TypeID src int refused []string + // spaces records how each namespace is addressed — true for minted, false + // for source-addressed — so claimSpace can catch one namespace used both + // ways whichever way round it happens. + spaces map[Space]bool } // isNilTypeDef reports whether td is a nil TypeDef — an untyped nil interface or @@ -66,7 +61,36 @@ func NewTypes(src int) *Types { reg: ir.TypeRegistry{}, byPointer: make(map[string]ir.TypeID), src: src, + spaces: make(map[Space]bool), + } +} + +// claimSpace records whether id's namespace holds minted or source-addressed +// nodes, and refuses the second one to arrive when they disagree. +// +// This is invariant 3's corollary made mechanical: a node a lowering mints must +// occupy a namespace no source coordinate can produce. Sharing one leaves the two +// racing for a single ID, and the winner is whichever declaration lowered first — +// silently, because the document is well-formed either way. Reversing the branch +// order of a colliding spec produces no diagnostic, irverify is clean, and +// pass.Validate passes; only a golden diff shows it, and regenerating the golden +// makes it green again. +// +// The check is at the namespace rather than the ID because a minted ID that +// happens not to collide today collides as soon as the source names one more +// position — safety that rests on which paths a format's pointers cannot spell is +// the reasoning the corollary exists to replace. +func (t *Types) claimSpace(id ir.TypeID, minted bool) { + space := spaceOf(id) + if space == "" { + return // no space segment to share; the ID grammar is checked elsewhere } + if was, seen := t.spaces[space]; seen && was != minted { + t.refuse("namespace %q holds both minted and source-addressed nodes (at id=%q); "+ + "a minted node needs a namespace of its own", space, id) + return + } + t.spaces[space] = minted } // Intern returns the ID for pointer, calling build on first visit only. @@ -87,6 +111,7 @@ func (t *Types) Intern(pointer string, id ir.TypeID, build func() ir.TypeDef) ir return id } + t.claimSpace(id, false) t.byPointer[pointer] = id td := build() // may recurse; a self-reference hits byPointer above if isNilTypeDef(td) { @@ -114,6 +139,9 @@ func (t *Types) Intern(pointer string, id ir.TypeID, build func() ir.TypeDef) ir // recursion. Register overwrites a colliding ID rather than deduplicating, // because a synthetic ID that collides is a bug in the minting scheme, not a // revisit. +// +// The namespace a minted node lands in is checked rather than assumed: see +// claimSpace. func (t *Types) Register(id ir.TypeID, td ir.TypeDef) { if id == "" { t.refuse("register rejected: empty type id") @@ -123,6 +151,10 @@ func (t *Types) Register(id ir.TypeID, td ir.TypeDef) { t.refuse("register rejected: nil type definition for id=%q", id) return } + // A namespace refusal does not withhold the node: the diagnostic names the + // cause, and dropping the node would add dangling references on top of it — + // a second symptom of the same bug, further from its source. + t.claimSpace(id, true) t.reg[id] = td } diff --git a/compilers/openapi/ids.go b/compilers/openapi/ids.go index cc2c62b..34bbf4c 100644 --- a/compilers/openapi/ids.go +++ b/compilers/openapi/ids.go @@ -4,11 +4,13 @@ import ( "strconv" "strings" + "github.com/dexpace/morphic/compilers/compile" "github.com/dexpace/morphic/ir" ) // ptr joins segments into an RFC 6901 JSON pointer. IDs are derived from these -// pointers (ir-design §3.1); no other code may construct IDs or pointers. +// pointers (ir-design §3.1) and no other code in this package constructs one; +// the grammar wrapped around a pointer to make an ID belongs to the framework. func ptr(segments ...string) string { if len(segments) == 0 { return "" @@ -35,37 +37,52 @@ func unescapeSegment(s string) string { return strings.ReplaceAll(s, "~0", "~") } +// The namespaces this compiler addresses. The framework spells the grammar +// around them — the kind prefix and the separators (compile.TypeID and friends); +// what is chosen here is which namespace a node belongs in, and the pointer +// arithmetic that produces its path. +const ( + // openapiSpace and anonSpace both address source coordinates: a component + // schema is named at its own pointer, an inline schema is anonymous at its + // own pointer, and no pointer is both. + openapiSpace compile.Space = "openapi" + anonSpace compile.Space = "anon" + // composedSpace holds the Models synthesized for distributed union variants + // (§4.3) — nodes no schema in the source occupies. The branch pointer denotes + // the branch schema, so a $ref naming it must keep resolving to the branch, + // and the variant must not be reachable by any pointer a $ref can spell. + // typeIDForPointer yields only the openapi and anon spaces, so + // resolveSchemaRef can never hand a composed ID to a reference; compile.Types + // rejects the mistake of minting into a space that addresses coordinates. + composedSpace compile.Space = "composed" +) + // namedTypeID returns the stable ID of a components-named schema at pointer. -func namedTypeID(pointer string) ir.TypeID { return ir.TypeID("t/openapi" + pointer) } +func namedTypeID(pointer string) ir.TypeID { return compile.TypeID(openapiSpace, pointer) } // anonTypeID returns the stable ID of a hoisted inline type at pointer. -func anonTypeID(pointer string) ir.TypeID { return ir.TypeID("t/anon" + pointer) } +func anonTypeID(pointer string) ir.TypeID { return compile.TypeID(anonSpace, pointer) } // composedTypeID returns the stable ID of the Model synthesized for the -// distributed union variant at a branch pointer (§4.3). It is a namespace of -// its own because no schema in the source occupies that node: the branch -// pointer denotes the branch schema, so a $ref naming it must keep resolving to -// the branch, and the variant must not be reachable by any pointer a $ref can -// spell. typeIDForPointer yields only the t/openapi and t/anon namespaces, so -// resolveSchemaRef can never hand a t/composed ID to a reference. +// distributed union variant at a branch pointer (§4.3). func composedTypeID(branchPointer string) ir.TypeID { - return ir.TypeID("t/composed" + branchPointer) + return compile.TypeID(composedSpace, branchPointer) } // opID returns the stable ID of the operation at pointer. -func opID(pointer string) ir.OpID { return ir.OpID("op/openapi" + pointer) } +func opID(pointer string) ir.OpID { return compile.OpID(openapiSpace, pointer) } // propID returns the stable ID of the property at pointer. -func propID(pointer string) ir.PropID { return ir.PropID("p/openapi" + pointer) } +func propID(pointer string) ir.PropID { return compile.PropID(openapiSpace, pointer) } // authIDFor returns the stable ID of the named security scheme. func authIDFor(name string) ir.AuthID { - return ir.AuthID("auth/openapi" + ptr("components", "securitySchemes", name)) + return compile.AuthID(openapiSpace, ptr("components", "securitySchemes", name)) } // serviceID returns the stable ID of the service for the given source index. func serviceID(sourceIndex int) ir.ServiceID { - return ir.ServiceID("s/openapi/" + strconv.Itoa(sourceIndex)) + return compile.ServiceID(openapiSpace, strconv.Itoa(sourceIndex)) } // declarationHint returns the name hint a node hoisted under pointer should diff --git a/internal/archtest/grammar_test.go b/internal/archtest/grammar_test.go index 4e0dc9a..b427fa3 100644 --- a/internal/archtest/grammar_test.go +++ b/internal/archtest/grammar_test.go @@ -37,7 +37,7 @@ var grammarOwners = []string{"compilers/compile", "ir"} // reason ir is an owner rather than a swept package. func TestNamingGrammar_CanonicalIsFilledByTheFrameworkOnly(t *testing.T) { t.Parallel() - offenders := sweepProduction(t, repoRoot(t), grammarOwners, canonicalViolations) + offenders := sweepProduction(t, repoRoot(t), "", grammarOwners, canonicalViolations) assert.Empty(t, offenders, "only %v may derive a canonical name; everything else goes through compile.NamingFor or compile.CanonicalWords", grammarOwners) @@ -69,6 +69,80 @@ func lower(name string) []ir.Naming { assert.Contains(t, offenders[1], "strings.ToLower(name)") } +// idOwners is the package permitted to construct an ir ID type from a string. +var idOwners = []string{"compilers/compile"} + +// idTypes are ir's ID types. A compiler converting a string into one of them is +// deriving an identifier. +var idTypes = []string{"TypeID", "OpID", "PropID", "AuthID", "ServiceID", "ChannelID", "MessageID"} + +// TestIDGrammar_CompilersDeriveIDsThroughTheFramework asserts that no compiler +// but the framework builds an ir ID out of a string. +// +// The derivation stays with the compiler — a JSON Pointer, a GraphQL structural +// path and a protobuf fully-qualified name are different things — but the grammar +// around it does not: the kind prefix, the namespace, and the rule that a minted +// node takes a namespace of its own (GitHub #162). Three compilers each spelled +// that themselves, and two of the three left the anonymous namespace unqualified +// by format, so nothing but coincidence kept their IDs apart. +// +// The sweep is the compilers rather than the repository because converting a +// string that is already an ID back into its type is legitimate elsewhere: +// pass and irverify do it to look a node up, which derives nothing. +func TestIDGrammar_CompilersDeriveIDsThroughTheFramework(t *testing.T) { + t.Parallel() + offenders := sweepProduction(t, repoRoot(t), "compilers", idOwners, idDerivations) + assert.Empty(t, offenders, + "only %v may build an ID from a string; a compiler supplies the path and the namespace", idOwners) +} + +// TestIDDerivations_LocalGrammarIsCaught plants a compiler spelling an ID prefix +// itself — the shape every compiler used before the promotion — and pins that +// deriving through the framework beside it stays clean. +func TestIDDerivations_LocalGrammarIsCaught(t *testing.T) { + t.Parallel() + const src = `package graphql + +func ids(pointer string) (ir.TypeID, ir.OpID, ir.PropID) { + named := ir.TypeID("t/graphql" + pointer) + op := compile.OpID(graphqlSpace, pointer) + prop := ir.PropID("p/graphql" + pointer) + return named, op, prop +} +` + offenders, err := idDerivations("planted.go", "compilers/graphql/ids.go", src) + require.NoError(t, err) + require.Len(t, offenders, 2, "the two local derivations, not the framework call: %v", offenders) + assert.Contains(t, offenders[0], "ir.TypeID") + assert.Contains(t, offenders[1], "ir.PropID") +} + +// idDerivations reports every conversion of a string into one of ir's ID types +// in one file. src is nil to read the file at path, or the source itself. +func idDerivations(path, rel string, src any) ([]string, error) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, src, 0) + if err != nil { + return nil, fmt.Errorf("archtest: parse %s: %w", path, err) + } + + var found []string + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + for _, idType := range idTypes { + if isSelector(call.Fun, "ir", idType) { + found = append(found, fmt.Sprintf("%s:%d: builds an ir.%s from a string rather than through the framework", + rel, fset.Position(call.Pos()).Line, idType)) + } + } + return true + }) + return found, nil +} + // canonicalViolations reports every place in one file that fills // ir.Naming.Canonical from something other than a call into the framework // package, whether as a composite-literal field or an assignment afterwards. @@ -165,24 +239,26 @@ func exprText(fset *token.FileSet, expr ast.Expr) string { return b.String() } -// sweepProduction runs scan over every production Go file under root except -// those inside owners, and returns everything the scans found. +// sweepProduction runs scan over every production Go file under the repo-relative +// subtree (empty for the whole repository), except those inside owners, and +// returns everything the scans found. Paths reported to scan stay repo-relative +// whatever the subtree, so a failure names a file the reader can open. // -// It is shared by the rules that are about what a package may write rather than -// what it may import, so each new rule is a scan function rather than another -// tree walk. -func sweepProduction(t *testing.T, root string, owners []string, +// It is shared by the rules about what a package may write rather than what it +// may import, so each new rule is a scan function rather than another tree walk. +func sweepProduction(t *testing.T, root, subtree string, owners []string, scan func(path, rel string, src any) ([]string, error), ) []string { t.Helper() + base := filepath.Join(root, subtree) var found []string var scanned int - err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + err := filepath.WalkDir(base, func(p string, d fs.DirEntry, err error) error { if err != nil { return err } if d.IsDir() { - return skipUninterestingDir(root, p, d) + return skipUninterestingDir(base, p, d) } if !isProductionGoFile(d.Name()) { return nil diff --git a/internal/archtest/registry_test.go b/internal/archtest/registry_test.go index 00a5a94..37beef3 100644 --- a/internal/archtest/registry_test.go +++ b/internal/archtest/registry_test.go @@ -37,7 +37,7 @@ var registryOwners = []string{"compilers/compile", "ir"} // package would obtain a registry to write in the first place. func TestRegistryWrites_StayInsideTheFramework(t *testing.T) { t.Parallel() - offenders := sweepProduction(t, repoRoot(t), registryOwners, registryWrites) + offenders := sweepProduction(t, repoRoot(t), "", registryOwners, registryWrites) assert.Empty(t, offenders, "only %v may write an ir.TypeRegistry; everything else goes through compile.Types", registryOwners) From 882a64dd2dff69d5278dddd0b24b6b0db0f929c0 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 30 Jul 2026 16:46:40 +0300 Subject: [PATCH 4/6] docs: record the framework promotion and the LF pin as landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design's §3 table, §3.1 and §12 rows 1.1/1.2 described the two promotions as pending, and §8.4 described the line-ending hazard as a standing one. Update them to what ships, and add the enforcement clause the two promotions taught: deleting a duplicate leaves a state rather than a rule, so each promotion arrives with a sweep over the syntax that would rebuild it, and each sweep with a planted counter-test. The plan's prerequisites and framework-promotion tables follow, along with the critical-path prose that still named #48 as outstanding. --- docs/micro-compiler-design.md | 44 +++++++++++++++++++++++------------ docs/micro-compiler-plan.md | 33 ++++++++++++++------------ 2 files changed, 47 insertions(+), 30 deletions(-) diff --git a/docs/micro-compiler-design.md b/docs/micro-compiler-design.md index 8c30122..31e2730 100644 --- a/docs/micro-compiler-design.md +++ b/docs/micro-compiler-design.md @@ -82,8 +82,8 @@ promotion requires evidence from all three, not two and an expectation. |---|---|---|---|---| | Interning + type registry | `compile.Types` | own `types.go` | own `types.go` | already framework | | Diagnostic accumulation | `compile.Diags` | own `diag.go` | own `diag.go` | already framework | -| Canonical naming grammar | `schema.go` | `naming.go` | `naming.go` | **promote** | -| ID grammar | `ids.go` | `ids.go` | `ids.go` | **promote grammar, not derivation** | +| Canonical naming grammar | `schema.go` | `naming.go` | `naming.go` | **promoted** — `compile.CanonicalWords` | +| ID grammar | `ids.go` | `ids.go` | `ids.go` | **promoted, derivation left behind** — `compile.TypeID` and friends over a `compile.Space` | | Bounded-recursion guard | `depth`, cap 256 | — | — | promote only if the drafts show need | | Reference resolution | `resolve.go` | `resolve.go` | — | do not promote | | Loading, options | yes | yes | yes | do not promote — format-specific | @@ -110,10 +110,16 @@ disagreeing about `.` is caught wherever the corpus reaches it. What that check a compiler puts the boundaries *inside* a word (`foo2bar` against `foo_2_bar`), which is what the promotion still buys. -So 1.2 is now the move rather than the decision: `compilers/compile` gets the single -implementation, and the graphql and protobuf copies are deleted against it as those drafts rebase. -Their outputs move at that point — the protobuf copy separates on `.` alone and the graphql copy on -nothing but `_`/`-`/space — so each rebase carries its own golden update, argued there. +So 1.2 was the move rather than the decision, and it has **landed**: `compilers/compile` holds the +single implementation, `compilers/openapi` derives no name of its own, and the graphql and protobuf +copies are deleted against it as those drafts rebase. Their outputs move at that point — run over +the same inputs, 8 of 13 spellings differ between the three, because the protobuf copy separates on +`.` alone and the graphql copy on nothing but `_`/`-`/space — so each rebase carries its own golden +update, argued there. The OpenAPI goldens did not move, since #161 had already made its copy the +grammar that ships. + +Deleting two copies is a state rather than a rule, so the architecture test now asserts that only +the framework and `ir` may fill `Naming.Canonical`. What it still cannot see is a `Hint` (#54). The divergence was filed separately so it would not be lost if this work were deferred; that is what it is now closed by. @@ -326,7 +332,13 @@ some reordering is expected and each instance must be understood rather than abs 2. **Every new package gets a `rules` entry**, or `TestImportGraph_EveryPackageIsRuledOrExempt` fails. An unkeyed subdirectory is audited under its nearest keyed ancestor's allowlist, so nesting must not become a way to widen one. -3. **Anti-regrowth needs a type-surface cap.** Function-size and complexity caps (#83) are worth +3. **What a package may *write* is enforced beside what it may import.** Deleting a duplicate + leaves a state, not a rule, so each promotion into the framework arrives with a sweep over the + syntax that would rebuild it: an `ir.TypeRegistry` written outside the framework, a + `Naming.Canonical` derived locally, an ID built from a string inside a compiler. Each sweep + carries a planted counter-test, because a matcher that recognizes nothing passes a clean tree and + reads as proof. +4. **Anti-regrowth needs a type-surface cap.** Function-size and complexity caps (#83) are worth having but were already satisfied while the god object grew; they measure the wrong dimension. `internal/archtest` already carries `go/parser`, so a cap on methods per type in `compilers/*` is cheap, and it is the only guard that would have caught this happening. @@ -423,11 +435,13 @@ Both oracles land before Tier 1, proven against the current code first. the property, but it lives in the harness rather than the verifier, so it holds under test rather than for every consumer of a `Document`. Closing that gap means deciding whether the ID shape belongs in `ir`. -- **Byte-identical goldens are the load-bearing neutrality guard, and the comparison has a known - hazard.** `compareGolden` has no line-ending guard and the repository has no `.gitattributes` - (#48), so a checkout with `core.autocrlf=true` fails every golden test for reasons unrelated to - the IR. Harmless on the Linux runner; corrosive if it trains a reader to treat golden diffs as - environmental noise. Fixed before the programme starts rather than during it. +- **Byte-identical goldens are the load-bearing neutrality guard, and the comparison had a known + hazard.** `compareGolden` has no line-ending guard, and the repository carried no + `.gitattributes` (#48), so a checkout with `core.autocrlf=true` converted 346 files and failed + three test functions for reasons unrelated to the IR. Harmless on the Linux runner; corrosive if + it trains a reader to treat golden diffs as environmental noise. **Landed** before the programme + started rather than during it: the pin, and the test that reports its removal, since CI checks out + LF whether the rule is there or not. ### 8.5 Held at every step @@ -508,8 +522,8 @@ Each row is one PR unless noted. "Done when" is the acceptance test, not a summa | ~~0.1~~ | ~~Fix archtest prefix matching so one compiler cannot import another (#57)~~ | **Landed.** Allowlist entries are exact unless suffixed `/...`, and a planted sibling import now fails | — | | 0.2 | General two-order oracle in `internal/harness` | Reverses declaration order across the conformance corpus and diffs; proven by reverting #108's fix and watching it redden | — | | 0.3 | ID-collision oracle | `TypeID` → source pointer is injective across the corpus, and minted IDs occupy a namespace no source pointer produces; proven by planting a colliding derivation | — | -| 1.1 | Promote the ID grammar into `compilers/compile` | `compilers/openapi` derives no ID except through the framework; goldens byte-identical | 0.1 | -| 1.2 | Promote the canonical naming grammar (the segmentation is decided — §3.1) | `compilers/compile` holds the one implementation and no compiler keeps a copy; each rebasing draft carries its own golden update | 0.1 | +| ~~1.1~~ | ~~Promote the ID grammar into `compilers/compile`~~ | **Landed.** `compilers/openapi` derives no ID except through the framework, goldens byte-identical, and the minted-namespace rule is refused by `compile.Types` rather than asserted in a comment | — | +| ~~1.2~~ | ~~Promote the canonical naming grammar (the segmentation is decided — §3.1)~~ | **Landed.** `compilers/compile` holds the one implementation, an architecture test keeps a second from being written, and each rebasing draft carries its own golden update | — | | ~~1.3~~ | ~~Extend `irverify` to check segmentation, not only casing (#73, #54)~~ | **Landed with #161**, ahead of 1.2: `ir/naming-not-words` rejects a lowercase but unsegmented canonical, proven by planting the old grammar and watching the corpus sweep redden. #54 (`Hint`) stays open | — | | 2.1–2.7 | Tier-0 extractions, one PR each: `diag`, `load`, `scan`, `ids`, `value`, `annotation` (+ site), `merge` | Per §8.1: goldens byte-identical, rules entry added, and each package carries table-driven unit tests needing no document | 0.1 | | 3.1 | Introduce `Ctx` with accessors; derive indexes at entry | No exported `Ctx` field is a map; goldens byte-identical | 2.x | @@ -529,7 +543,7 @@ landing them first would only encode the current one. | Issue | Disposition | |---|---| | #57 archtest cannot enforce compiler isolation | **Closed.** Landed with #161/#143; it was a prerequisite for every package boundary here | -| #73 naming grammar and primitive IDs are cross-compiler ABI in one compiler | **Half closed.** The naming half landed with #161 (grammar + `irverify` check); the ID half is 1.1 | +| #73 naming grammar and primitive IDs are cross-compiler ABI in one compiler | **Answered.** The naming half landed with #161 (grammar + `irverify` check) and 1.2; the ID half with 1.1. Its own text proposed `ir` as the destination, and the framework package is where they went, so it is closed with that reasoning rather than silently | | #54 cased `Naming.Hint` passes the neutrality check | **Adjacent to 1.3**; fixed there if the segmentation work reaches `Hint`, otherwise left open | | #83 enforce size and complexity caps in lint | **Closed by 4.2**, deliberately last | | #66 extract a shared JSON-Schema→IR lowering core before the next compilers land | **Superseded.** Its premise expired — the next compilers landed without it (#20, #21). §3 replaces it with evidence-based promotion. To be closed with that reasoning, not silently | diff --git a/docs/micro-compiler-plan.md b/docs/micro-compiler-plan.md index bf547e7..09e9738 100644 --- a/docs/micro-compiler-plan.md +++ b/docs/micro-compiler-plan.md @@ -42,28 +42,30 @@ Every task inherits these. They are not restated per issue. | Issue | Work | |---|---| | ~~#57~~ | **Landed.** Allowlist entries are exact unless suffixed `/...`, so a `compilers` entry no longer licenses `compilers/graphql`, and a nested directory sharing a ruled sibling's basename is audited rather than skipped | -| #48 | Pin golden files to LF. The repository has no `.gitattributes`, so the comparison that proves twenty pull requests neutral can fail for reasons unrelated to the IR | +| ~~#48~~ | **Landed.** `* text=auto eol=lf`, so the comparison that proves twenty pull requests neutral cannot fail for reasons unrelated to the IR. A clone with `core.autocrlf=true` converted 346 files and failed three test functions before it | | #159 | The general two-order oracle. Exists today only as three hand-written cases at the site where the pointer collision was found | | #160 | Type-ID integrity: no collisions, **and** every ID agrees with its own provenance pointer. The second assertion is the only mechanical guard on provenance correctness — `irverify` validates the source index range and never the pointer | -#48 looks like hygiene and is not. Byte-identical goldens are the sole proof of neutrality across all -59 conformance snapshots, and a comparison that fails environmentally teaches a reader to dismiss -golden diffs at precisely the point where each one has to be treated as a finding. It blocks #162 and -#165, the two entry points, so the ordering is inherited rather than remembered. +#48 looked like hygiene and was not. Byte-identical goldens are the sole proof of neutrality across +every conformance snapshot, and a comparison that fails environmentally teaches a reader to dismiss +golden diffs at precisely the point where each one has to be treated as a finding. It blocked #162 +and #165, the two entry points, so the ordering was inherited rather than remembered — and #162 +landed with it, in that order, in one pull request. ### Framework promotion | Issue | Work | Blocked by | |---|---|---| -| #162 | Identifier grammar into `compilers/compile` | #57, #48 | -| #163 | Canonical naming grammar into `compilers/compile`. The segmentation is settled (#161); what is left is the move, and each rebasing draft's goldens with it | #57 | +| ~~#162~~ | **Landed.** Identifier grammar into `compilers/compile`: `compile.TypeID` and friends over a `compile.Space`, with the minted-namespace rule refused by `compile.Types` | — | +| ~~#163~~ | **Landed.** Canonical naming grammar into `compilers/compile`, with `compile.NamingFor` beside it and a conformance suite pinning the boundaries `irverify` cannot see | — | | ~~#164~~ | **Landed with #161**, ahead of #163: `ir/naming-not-words` rejects a lowercase but unsegmented canonical | — | -| #73 | Closed when the three above land | #162, #163, #164 | +| #73 | Answered by the three above; to be closed with the reasoning that they landed in `compilers/compile` rather than in `ir` as its text proposed | — | -#163 still changes output, but no longer decides anything: #161 fixed the segmentation in -`compilers/openapi` and wrote it into `ir-design.md` §3.2, so the move is measured against a rule -already in the contract. The graphql and protobuf copies disagree with it, so their goldens move as -those drafts rebase, each with a reddening test and a deliberate golden update. +#163 changed no output here: #161 had already fixed the segmentation in `compilers/openapi` and +written it into `ir-design.md` §3.2, so the move was measured against a rule already in the +contract. The graphql and protobuf copies disagree with it — 8 of 13 spellings differ across the +three — so their goldens move as those drafts rebase, each with a reddening test and a deliberate +golden update. ### Tier 0 — extractions that are pure moves @@ -154,9 +156,10 @@ Work already filed that lands inside this restructuring rather than alongside it Twelve steps deep at its longest, with Tier 0 wide enough that six of its seven extractions can proceed in parallel once `diag` lands. -Of the five that were unblocked at the start, #57 and #161 have landed. **#48**, **#159** and -**#160** remain, and are the prerequisites everything else waits on. The graph is acyclic, and the dependency columns above are derived from the API rather than -maintained by hand — check both rather than trusting either: +Of the five that were unblocked at the start, #57, #161 and #48 have landed, and the framework +promotion (#162, #163) with them. **#159** and **#160** remain, and are the prerequisites the two +new oracles wait on. The graph is acyclic, and the dependency columns above are derived from the API +rather than maintained by hand — check both rather than trusting either: ```bash gh api repos/dexpace/morphic/issues/N/dependencies/blocked_by --jq '[.[].number]' From afb1800f99ef72837dc16d8cee97594a7f08aae3 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 30 Jul 2026 16:53:31 +0300 Subject: [PATCH 5/6] docs: record the shared compiler framework in the layout docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The layout docs describe compilers/* as "one compiler per format", which has not been the whole picture since compilers/compile landed, and none of the three named that package at all. A reader following them would put shared compiler machinery in a compiler, which is the arrangement the framework exists to end. Give it a line in each layout — the README table, the architecture tree, and the CLAUDE.md diagram — and say in architecture §2.1 what the split actually is: the framework holds what every compiler must agree on, the compiler keeps what only it can compute, and architecture tests rather than prose decide which is which. ir-design gains the general form of two rules it stated only in the particular: an ID's shape around its path is shared while the path is the format's, and a minted node takes a namespace of its own — which §4.3 applied to distributed unions and §3.1 now states for every minting. --- CLAUDE.md | 7 +++++-- README.md | 3 ++- docs/architecture.md | 19 ++++++++++++++++++- docs/ir-design.md | 11 ++++++++++- 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index df0e999..adc8b5a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,8 +88,11 @@ in the docs. ir/ Layer 0 — IR nodes, IDs, traversal, JSON round-trip. Imports ONLY the stdlib. irverify/ Layer 0 — structural-invariant checks over a compiled Document. Imports ir only. irtest/ Layer 0 — golden-file helper (`-update` rewrites). Imports ir + go-cmp. -compilers/* Layer 1 — one compiler per format. Imports ir (+ own format libs); never each - other, never emitters/engine. +compilers/ Layer 1 — the Compiler contract and the format-keyed registry. Imports ir only. + compile/ Layer 1 — what every compiler shares: the type registry with its coordinate map, + diagnostics, and the naming and identifier grammars. Imports ir only. + / Layer 1 — one compiler per format. Imports ir, compilers, compilers/compile + (+ own format libs); never each other, never emitters/engine. pass/ Layer 1 — IR → IR passes. Imports ir only. emitters/* Layer 2 — imports ir + emitter contract; never compiler. (Not built yet.) engine/ Layer 3 — orchestration; imports everything below. diff --git a/README.md b/README.md index 1b242d8..e8c63cb 100644 --- a/README.md +++ b/README.md @@ -145,7 +145,8 @@ package may import only the packages one layer below it. | Package | Layer | Imports | |---|---|---| | `ir/` | 0 — IR nodes, IDs, traversal, JSON round-trip | stdlib only | -| `compilers/*` | 1 — one compiler per format (`compilers/openapi`) | `ir` + own format libs | +| `compilers/compile/` | 1 — what every compiler shares: type registry, diagnostics, naming and identifier grammars | `ir` only | +| `compilers/*` | 1 — one compiler per format (`compilers/openapi`) | `ir` + `compilers` + `compilers/compile` + own format libs | | `pass/` | 1 — IR → IR passes | `ir` only | | `emitters/*` | 2 — IR → artifacts (future) | `ir` + emitter contract | | `engine/` | 3 — orchestration | everything below | diff --git a/docs/architecture.md b/docs/architecture.md index 6911e76..47337e4 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -85,6 +85,18 @@ Internal phases every compiler follows (each format implements them its own way) detection, envelope unwrapping) run only if the corresponding policy is enabled and mark their output as inferred. +What a compiler does *not* own is in `compilers/compile`: the state and the grammars every compiler +must agree on. That is the type registry with the source-coordinate map behind stable IDs (and the +rule that a minted node takes a namespace no source coordinate addresses), diagnostic accumulation, +the canonical naming grammar, and the identifier grammar — the kind prefix and the namespace after +it. What stays with the compiler is what only it can compute: the path an ID derives from, since a +JSON Pointer, a GraphQL structural path and a protobuf fully-qualified name are different things. + +The split is enforced rather than documented: architecture tests fail a package outside the +framework that writes the type registry, derives a canonical name, or builds an ID out of a string. +Promoting something into the framework later is additive, while demoting it breaks every compiler, +so borderline machinery starts outside and moves in on evidence from more than one format. + Compilers are registered in a registry keyed by detected format; the engine sniffs the source format and dispatches. Milestone 1 ships the OpenAPI 3.x compiler only; the compiler registry, provenance model, and IR are built for all eight from day one. @@ -188,6 +200,8 @@ morphic/ │ ├── irtest/ # Golden-snapshot helpers for IR documents. │ └── irverify/ # Structural-invariant oracle (dangling refs, IDs, naming). ├── compilers/ # Layer 1 — compiler contract + registry. +│ ├── compile/ # What every compiler shares: type registry + coordinates, +│ │ # diagnostics, naming and identifier grammars. Imports ir only. │ ├── openapi/ # OpenAPI 3.x → IR (milestone 1). │ ├── swagger/ # 2.0 lift → openapi compiler (future). │ ├── typespec/ smithy/ graphql/ asyncapi/ protobuf/ otp/ (future) @@ -202,8 +216,11 @@ morphic/ Dependency rules, enforced by an architecture test as in oagen: - `ir` imports only the standard library. It contains no parsing, no generation, no I/O. +- `compilers/compile` imports only `ir`: it is below every compiler, not beside them. - `compilers/*` and `pass` import `ir` (and their own format libraries) — never each other, - never `emitter` or `engine`. + never `emitter` or `engine`. A compiler also names the contract package and + `compilers/compile`; each is allowed in its own right, so no sibling compiler rides in beside + them. - `emitters/*` imports `ir` and `emitter` (contract) — never `compiler`. - `engine` imports everything below it; `cmd` imports `engine`. diff --git a/docs/ir-design.md b/docs/ir-design.md index 1c724cd..9378c19 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -101,6 +101,13 @@ snapshots, cachable, diffable across spec revisions). IDs are never derived from and never rewritten by renames. The `dedup` pass may alias two structurally identical anonymous types; aliases are recorded so both IDs stay resolvable. +The shape around the pointer is one grammar every compiler shares: a kind prefix (`t`, `op`, `p`, +`s`, `auth`), then the namespace, then the path. Only the path is the format's, because a JSON +Pointer, a GraphQL structural path and a protobuf fully-qualified name are different things and +nothing outside the format can compute one. A node a lowering *mints* rather than finds takes a +namespace of its own, so no pointer a reference can spell ever reaches it — the general form of the +rule §4.3 applies to distributed unions. + Every named entity has an ID — including services (Thrift `service B extends A`, WSDL 2.0 interface extension, and Cap'n Proto interface inheritance all reference services by identity) and messages (AsyncAPI reuses one named message across channels, operations, and replies). @@ -131,7 +138,9 @@ starts a new one, and **every other character separates** — `.`, `/`, `[`, `-` `com.example.User` and `com-example-user` canonicalize the same, and an emitter reading `Canonical` never has to ask which compiler produced it. A source name with no word character in it canonicalizes to the empty string; `Source` keeps the spelling. `irverify` checks the shape, so an -unsegmented canonical is a compiler bug rather than a variant reading. +unsegmented canonical is a compiler bug rather than a variant reading — and one implementation +serves every compiler, since what the check cannot see is where a compiler puts the boundaries +*inside* a word (`foo2bar` against `foo_2_bar`). ### 3.3 Type references From 107ed991555c74691131e9d36bf35b76adbb6091 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 30 Jul 2026 16:57:54 +0300 Subject: [PATCH 6/6] test: widen the new guards to the shapes they missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A self-review pass over the branch as it will merge, closing three gaps in the guards it adds. Each shape below is planted, and the two that can be reached from the tree were planted there as well as in a fixture. The ID sweep matched conversions only, so a typed constant — the Protobuf draft's `const anyTypeID ir.TypeID = "t/protobuf/any"` — spelled a whole identifier past it. It now also reports a declaration of an ID type holding a literal, while a declaration copying an existing ID stays clean. The naming sweep required the literal to be spelled ir.Naming, which an element of a []ir.Naming elides. The type requirement is dropped: Naming is the only type in the repository with a Canonical field, so the field name identifies it, and a second one would want a look of its own. The line-ending guards checked the rule and what git would do with it, but not the working tree the tests actually read. A clone taken before the pin keeps its CRLF files until they are renormalized, and that checkout fails the golden suites with diffs that say the IR changed when the line endings did. The third test names those files and the command that fixes them. The two design docs lose a ratio that read as a fact about the compilers and was a property of the inputs I happened to probe with. What separates the three grammars is exact and stays. Three doc comments that overstated the reach of a rule are narrowed to what it covers: ir is an owner of the registry and naming rules too, the ID rule is asked of the compilers alone, and an ID built with an empty Space has no namespace segment either. --- compilers/compile/doc.go | 10 ++-- compilers/compile/ids.go | 5 +- compilers/compile/ids_test.go | 6 +-- docs/ir-design.md | 2 +- docs/micro-compiler-design.md | 10 ++-- docs/micro-compiler-plan.md | 6 +-- internal/archtest/grammar_test.go | 90 +++++++++++++++++++++++-------- ir/irtest/lineendings_test.go | 36 +++++++++++++ 8 files changed, 125 insertions(+), 40 deletions(-) diff --git a/compilers/compile/doc.go b/compilers/compile/doc.go index 8dd6a3d..54312c8 100644 --- a/compilers/compile/doc.go +++ b/compilers/compile/doc.go @@ -14,10 +14,12 @@ // - The identifier grammar: the kind prefix that opens an ID and the namespace // that follows it. // -// The package boundary is the point. Architecture tests assert that no package -// outside this one writes to an ir.TypeRegistry, derives a canonical name, or -// builds an ID from a string, and rules like those are inexpressible without an -// outside — which is why a package this small is worth its own directory. +// The package boundary is the point. Architecture tests assert that nothing but +// this package and ir writes an ir.TypeRegistry or derives a canonical name, and +// that no compiler builds an ID out of a string — that last one is asked of the +// compilers alone, because re-typing an ID that already exists is legitimate +// above them. Rules like those are inexpressible without an outside, which is +// why a package this small is worth its own directory. // // What deliberately stays with the compiler: ir.Document assembly (seventeen of // its eighteen fields carry no framework invariant), recursion depth bounds (the diff --git a/compilers/compile/ids.go b/compilers/compile/ids.go index 7138d36..3616a07 100644 --- a/compilers/compile/ids.go +++ b/compilers/compile/ids.go @@ -77,8 +77,9 @@ func idFor(kind string, space Space, path string) string { return kind + "/" + string(space) + "/" + trimmed } -// spaceOf returns the space id is addressed in, or "" when id carries no space -// segment — which no ID built through this package does. +// spaceOf returns the space id is addressed in — the segment after the kind +// prefix — or "" when there is none. Only an ID that was not built here, or one +// built with an empty Space, has none. func spaceOf(id ir.TypeID) Space { parts := strings.SplitN(string(id), "/", 3) if len(parts) < 2 { diff --git a/compilers/compile/ids_test.go b/compilers/compile/ids_test.go index 35fc025..8313683 100644 --- a/compilers/compile/ids_test.go +++ b/compilers/compile/ids_test.go @@ -95,9 +95,9 @@ func TestTypes_SeparateSpacesAreNotRefused(t *testing.T) { } // TestTypes_IDWithNoSpaceSegmentClaimsNothing covers the one shape the namespace -// check cannot judge. Nothing built through this package produces it, so it is -// left alone rather than refused: an ID with no namespace cannot be a namespace -// used two ways. +// check cannot judge: an ID that was not built here, or was built with an empty +// Space. It is left alone rather than refused, because an ID with no namespace +// cannot be a namespace used two ways. func TestTypes_IDWithNoSpaceSegmentClaimsNothing(t *testing.T) { t.Parallel() types := compile.NewTypes(0) diff --git a/docs/ir-design.md b/docs/ir-design.md index 9378c19..34dbdd5 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -106,7 +106,7 @@ The shape around the pointer is one grammar every compiler shares: a kind prefix Pointer, a GraphQL structural path and a protobuf fully-qualified name are different things and nothing outside the format can compute one. A node a lowering *mints* rather than finds takes a namespace of its own, so no pointer a reference can spell ever reaches it — the general form of the -rule §4.3 applies to distributed unions. +rule §4.3 states for distributed unions. Every named entity has an ID — including services (Thrift `service B extends A`, WSDL 2.0 interface extension, and Cap'n Proto interface inheritance all reference services by identity) diff --git a/docs/micro-compiler-design.md b/docs/micro-compiler-design.md index 31e2730..b09c629 100644 --- a/docs/micro-compiler-design.md +++ b/docs/micro-compiler-design.md @@ -112,11 +112,11 @@ promotion still buys. So 1.2 was the move rather than the decision, and it has **landed**: `compilers/compile` holds the single implementation, `compilers/openapi` derives no name of its own, and the graphql and protobuf -copies are deleted against it as those drafts rebase. Their outputs move at that point — run over -the same inputs, 8 of 13 spellings differ between the three, because the protobuf copy separates on -`.` alone and the graphql copy on nothing but `_`/`-`/space — so each rebase carries its own golden -update, argued there. The OpenAPI goldens did not move, since #161 had already made its copy the -grammar that ships. +copies are deleted against it as those drafts rebase. Their outputs move at that point — the +protobuf copy separates on `_`, `-`, space and `.`, and the graphql copy on the first three alone, +so both leave `/`, `[`, `]`, `+`, `:` and braces inside a word — and each rebase carries its own +golden update, argued there. The OpenAPI goldens did not move, since #161 had already made its copy +the grammar that ships. Deleting two copies is a state rather than a rule, so the architecture test now asserts that only the framework and `ir` may fill `Naming.Canonical`. What it still cannot see is a `Hint` (#54). diff --git a/docs/micro-compiler-plan.md b/docs/micro-compiler-plan.md index 09e9738..0b6b844 100644 --- a/docs/micro-compiler-plan.md +++ b/docs/micro-compiler-plan.md @@ -63,9 +63,9 @@ landed with it, in that order, in one pull request. #163 changed no output here: #161 had already fixed the segmentation in `compilers/openapi` and written it into `ir-design.md` §3.2, so the move was measured against a rule already in the -contract. The graphql and protobuf copies disagree with it — 8 of 13 spellings differ across the -three — so their goldens move as those drafts rebase, each with a reddening test and a deliberate -golden update. +contract. The graphql and protobuf copies disagree with it — each splits on a short list of +separators rather than on every non-word character — so their goldens move as those drafts rebase, +each with a reddening test and a deliberate golden update. ### Tier 0 — extractions that are pure moves diff --git a/internal/archtest/grammar_test.go b/internal/archtest/grammar_test.go index b427fa3..5388bc6 100644 --- a/internal/archtest/grammar_test.go +++ b/internal/archtest/grammar_test.go @@ -58,15 +58,16 @@ func lower(name string) []ir.Naming { hint := ir.Naming{Hint: localWords(name)} var late ir.Naming late.Canonical = strings.ToLower(name) - return []ir.Naming{declared, framework, hint, late} + return []ir.Naming{declared, framework, hint, late, {Canonical: lower(name)}} } ` offenders, err := canonicalViolations("planted.go", "compilers/graphql/naming.go", src) require.NoError(t, err) - require.Len(t, offenders, 2, - "the literal and the later assignment, not the framework call or the hint: %v", offenders) + require.Len(t, offenders, 3, + "the literal, the assignment and the elided literal — not the framework call or the hint: %v", offenders) assert.Contains(t, offenders[0], "canonicalWords(name)") assert.Contains(t, offenders[1], "strings.ToLower(name)") + assert.Contains(t, offenders[2], "lower(name)", "an element of a []ir.Naming elides the type") } // idOwners is the package permitted to construct an ir ID type from a string. @@ -103,22 +104,29 @@ func TestIDDerivations_LocalGrammarIsCaught(t *testing.T) { t.Parallel() const src = `package graphql -func ids(pointer string) (ir.TypeID, ir.OpID, ir.PropID) { +const anyTypeID ir.TypeID = "t/graphql/any" + +func ids(pointer string, existing ir.OpID) (ir.TypeID, ir.OpID, ir.PropID) { named := ir.TypeID("t/graphql" + pointer) op := compile.OpID(graphqlSpace, pointer) + var copied ir.OpID = existing prop := ir.PropID("p/graphql" + pointer) return named, op, prop } ` offenders, err := idDerivations("planted.go", "compilers/graphql/ids.go", src) require.NoError(t, err) - require.Len(t, offenders, 2, "the two local derivations, not the framework call: %v", offenders) - assert.Contains(t, offenders[0], "ir.TypeID") - assert.Contains(t, offenders[1], "ir.PropID") + require.Len(t, offenders, 3, + "the constant and the two conversions, not the framework call or the copy: %v", offenders) + assert.Contains(t, offenders[0], "declares a literal as an ir.TypeID") + assert.Contains(t, offenders[1], "converts a string to an ir.TypeID") + assert.Contains(t, offenders[2], "converts a string to an ir.PropID") } -// idDerivations reports every conversion of a string into one of ir's ID types -// in one file. src is nil to read the file at path, or the source itself. +// idDerivations reports every place in one file that builds an ir ID out of a +// string: a conversion, and a typed declaration holding a literal — the shape +// that spells an ID without converting anything. src is nil to read the file at +// path, or the source itself. func idDerivations(path, rel string, src any) ([]string, error) { fset := token.NewFileSet() file, err := parser.ParseFile(fset, path, src, 0) @@ -127,22 +135,58 @@ func idDerivations(path, rel string, src any) ([]string, error) { } var found []string + report := func(pos token.Pos, how, idType string) { + found = append(found, fmt.Sprintf("%s:%d: %s an ir.%s rather than deriving it through the framework", + rel, fset.Position(pos).Line, how, idType)) + } ast.Inspect(file, func(n ast.Node) bool { - call, ok := n.(*ast.CallExpr) - if !ok { - return true - } - for _, idType := range idTypes { - if isSelector(call.Fun, "ir", idType) { - found = append(found, fmt.Sprintf("%s:%d: builds an ir.%s from a string rather than through the framework", - rel, fset.Position(call.Pos()).Line, idType)) + switch node := n.(type) { + case *ast.CallExpr: + if idType, ok := idTypeName(node.Fun); ok { + report(node.Pos(), "converts a string to", idType) + } + case *ast.ValueSpec: + // `const anyTypeID ir.TypeID = "t/protobuf/any"` — the Protobuf draft's + // shape, which spells a whole ID and converts nothing. A declaration + // with no literal in it is copying an ID, not deriving one. + if idType, ok := idTypeName(node.Type); ok && holdsStringLiteral(node.Values) { + report(node.Pos(), "declares a literal as", idType) } + default: } return true }) return found, nil } +// idTypeName returns the name of the ir ID type expr names, if it names one. +func idTypeName(expr ast.Expr) (string, bool) { + for _, idType := range idTypes { + if isSelector(expr, "ir", idType) { + return idType, true + } + } + return "", false +} + +// holdsStringLiteral reports whether any of exprs contains a string literal, so +// a declaration built from one is told apart from one assigned an existing ID. +func holdsStringLiteral(exprs []ast.Expr) bool { + for _, expr := range exprs { + var seen bool + ast.Inspect(expr, func(n ast.Node) bool { + if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { + seen = true + } + return !seen + }) + if seen { + return true + } + } + return false +} + // canonicalViolations reports every place in one file that fills // ir.Naming.Canonical from something other than a call into the framework // package, whether as a composite-literal field or an assignment afterwards. @@ -174,12 +218,14 @@ func canonicalViolations(path, rel string, src any) ([]string, error) { return found, nil } -// reportCanonicalField reports the Canonical field of an ir.Naming composite -// literal when it is not filled by a framework call. +// reportCanonicalField reports a Canonical field of a composite literal when it +// is not filled by a framework call. +// +// The literal's type is not required to be ir.Naming, and not only because an +// element of a []ir.Naming elides it: ir.Naming is the one type in the repository +// with a Canonical field, so the field name identifies it. A second type carrying +// that name would need this narrowed — and would be worth a look on its own. func reportCanonicalField(lit *ast.CompositeLit, report func(ast.Expr)) { - if !isSelector(lit.Type, "ir", "Naming") { - return - } for _, elt := range lit.Elts { kv, ok := elt.(*ast.KeyValueExpr) if !ok { diff --git a/ir/irtest/lineendings_test.go b/ir/irtest/lineendings_test.go index 6484eb6..2fe5dd9 100644 --- a/ir/irtest/lineendings_test.go +++ b/ir/irtest/lineendings_test.go @@ -1,6 +1,7 @@ package irtest_test import ( + "bytes" "os" "os/exec" "path/filepath" @@ -76,6 +77,41 @@ func TestGitAttributes_ResolveToLF(t *testing.T) { } } +// TestTrackedFiles_HoldNoCarriageReturn checks the working tree the tests +// actually read, which the two above do not: `.gitattributes` governs new +// checkouts, and a clone taken before it landed keeps its CRLF files until they +// are renormalized. That checkout fails the golden and conformance suites with +// whole-file diffs, and the diff says the IR changed when the line endings did. +// +// It reaches only where the problem exists, so it can never redden on the Linux +// runner — the same shape as the pin it accompanies. +func TestTrackedFiles_HoldNoCarriageReturn(t *testing.T) { + t.Parallel() + root := repoRoot(t) + cmd := exec.Command("git", "ls-files", "-z") + cmd.Dir = root + out, err := cmd.Output() + if err != nil { + t.Skipf("git ls-files unavailable (%v)", err) + } + + var offenders []string + tracked := strings.Split(strings.TrimSuffix(string(out), "\x00"), "\x00") + require.NotEmpty(t, tracked, "git tracks no files, so an empty result proves nothing") + for _, rel := range tracked { + raw, readErr := os.ReadFile(filepath.Join(root, rel)) + if readErr != nil { + continue // a file staged for deletion, or a submodule entry + } + if bytes.ContainsRune(raw, '\r') { + offenders = append(offenders, rel) + } + } + assert.Empty(t, offenders, + "these tracked files hold CR bytes; run `git add --renormalize .` to bring the checkout "+ + "back to LF, or mark a file that needs them `-text` in .gitattributes") +} + // repoRoot walks up from this file to the directory holding go.mod. func repoRoot(t *testing.T) string { t.Helper()