Uh oh!
There was an error while loading. Please reload this page.
feat(engine)!: let compilers own format detection and options - #341
Merged
Conversation
This was referenced Aug 9, 2026
The branch was cut before the engine's format step became diagnostic-reporting, so the two sides disagree about what happens when no compiler will take a source. Resolved in favour of the merged behaviour, and the detection contract widened so it can be honoured without the engine parsing anything. main made every spec problem an ir.Diagnostic with exit 1, reserving the Go error return — and the CLI's exit 2 — for a misuse of the tool or an I/O failure. This branch's Run returned a Go error for a source no compiler recognizes, which would have put "your spec is unreadable" back on the exit code that means "you invoked morphic wrong". Keeping both required a third answer. main told a source that does not parse from one that parses and declares nothing, under engine/undecodable-source; this branch's Detect could not, because a compiler may only say "not mine" and the engine no longer parses. So Detect now reports diagnostics alongside its verdict, matching Compile's channel: Detect(src Source) (format SourceFormat, diags []ir.Diagnostic, ok bool) A compiler declines another format's bytes silently, and reports only when the source is recognizably its own and cannot be read. The registry carries what the decliners said out to the engine, which prefers it over its own account — it parses nothing and can say no more than that nobody claimed the source. Consequences: - engine/undecodable-source becomes openapi/undecodable-source, and now carries the parse position, which the engine-level code never had. A malformed .proto no longer reports "not YAML", which the engine-side probe would have said of every format that is not YAML. - engine/unsupported-format retires. Swagger is recognized by the OpenAPI compiler and served by nobody, which is engine/no-compiler-for-format; the engine cannot tell that from an unregistered format and should not pretend to. - NewWith() with no compilers now reports unrecognized-format rather than no-compiler-for-format: with nobody to read the source, no format is named. Registry.Lookup keeps only test callers now that Run detects rather than looks up. It is left in place as the read side of Register.
Registry.Detect ended its search on any compiler answering ok, including one that named no format at all. That answer says nothing — a compiler recognizing a source names what it recognized — and acting on it hides every compiler registered after: a source the next one would have taken comes back unrecognized, with nothing in the output naming the compiler that swallowed it. Skip such a compiler and keep asking. Its diagnostics are not collected, since the contract reads those only from a compiler that declined, and this one did not say it declined. Also pin the bound the key search shares with the decode. declaresProbeKey reads the same bounded prefix sniff does, so a source declaring an OpenAPI key only past the cap is declined silently rather than reported as this compiler's own and broken — claiming it would assert something about bytes detection never read. The truncation ran under the existing tests without any of them observing its effect; a source whose prefix fails to parse and whose key falls past the cap is the case that separates the two.
validate is compile's pipeline with the document dropped, so the two have to configure that pipeline the same way. --opt was bound only by compile, which left a spec that needs an option to compile impossible to check the way it would be built: `validate spec.yaml` and `compile spec.yaml --opt overlay=o.yaml` read different documents, and a gate could pass a spec the build then rejects. Bind it in bindSpecFlags with the rest of the shared flags, which also puts its spelling, default and help text under TestSpecFlags_SharedFlagsAgree rather than leaving two definitions to drift. bindSpecFlags now makes the settings map, so neither constructor has to remember to. Registry.Detect resolves its owner through Lookup rather than reaching into byFormat, which is the same read and leaves the registry's format-keyed lookup with a caller in the pipeline again now that Run detects instead of looking up.
… engine Two answers the detection rework left less useful than it found them. A refusal named what failed and not what would have worked. "no compiler registered for format swagger@2.0" reads as a configuration slip — as though a compiler had been left unregistered — when it is a fact about the build, and it covered a format morphic does not serve and a version outside the supported range with one wording that helps neither. Registry.Formats reports what the registry holds, sorted so the same set never reads as two, and both refusals now end with "this build compiles openapi@3.0, openapi@3.1, openapi@3.2". The engine still knows nothing about any format; it is reading its own registry. The compiler's own report, where there is one, is left alone: a parse error with a line number does not need a list appended to it. NewWith accepted an empty compiler set. Nothing can be added to a built engine, so that engine could never compile anything, and every source handed to it came back "unrecognized spec format" — blaming the document for a misconfiguration of the caller, which is the same category error the exit codes were untangled to avoid. It is refused at construction, where the mistake is. That reverses a note asking for exactly this precondition not to be added, because an empty engine was then the only way to reach Run's nothing-recognized branch. It was true while the engine sniffed formats itself and named one for every parseable spec. Detection belongs to the compilers now, so a source none of them claims reaches that branch with a full registry; removing the test that note protected leaves the coverage gate at 100%. The reasoning is recorded in both places rather than deleted.
Formats sorts versions as strings, and the comment said that was "enough while a version is major.minor". It is not: 3.10 is major.minor and sorts ahead of 3.2, so the stated condition is satisfied by exactly the case it fails on. The real condition is single-digit minors. State it that way, and say why the deviation is left standing rather than fixed — the order is read and never compared against, and a version comparator guessing at every format's scheme is a larger thing to get wrong than a list one line out of order. Also narrow served's claim to what makes it true: NewWith builds every Engine, so the list it renders is never empty.
An Engine that never went through New or NewWith carries a nil registry, and Run dereferenced it during detection: `var e engine.Engine` followed by e.Run panicked with a nil pointer dereference rather than returning the Go error a misuse of the API should produce. A panic out of a library is not a report — the caller cannot tell it from a bug in the compiler they were calling — and Run already reserves its error return for exactly this class. Guard the nil receiver and the nil registry alike, ahead of the file read: an engine that was never built is the caller's mistake whatever the path turns out to say, and reporting the path first would send them to look at the file. Closes#386
…ing it `--opt overlay=` decoded to no overlay at all and exited 0: the caller asked for one, got none, and was told nothing. That is the case DecodeOptions exists to prevent — its contract calls an unusable value an error precisely so a setting is never silently dropped — and an empty path is unusable, not a way to spell "no overlay". It also mislaid the blame when laxness came with it. `--opt overlay= --opt overlay-lax=true` reported that overlay-lax applies only with overlay, naming a flag the caller had in fact passed and sending them to look at the wrong one. Refuse it where the value is read, alongside the other per-setting checks, so finish's empty overlayPath keeps its one meaning: no overlay was named.
…red on A diagnostic prints as one line — severity, code, location, message — so a message carrying a newline splits one report into several, and every line after the first has no severity, code or location. A reader takes it for another finding; anything parsing stderr takes it for a malformed one. Two messages embed an error raised by a library that writes several lines. `openapi: []` reported "cannot unmarshal !!seq into string" on a second line of its own, and an overlay failing validation on two counts printed the second count as a line with nothing in front of it. The overlay path is not new, but --opt overlay is the first way to reach it without writing Go, so this is where it becomes something a user sees. diag.OneLine collapses the text: parts join with "; " so a flat list of findings reads as a list, except after a part ending in a colon, where the next line is that header's content and a semicolon would read as a break in it. The undecodable-source message also said a source "does not parse" when what failed was the shape of its version key — `openapi: []` parses perfectly well. It now says the source cannot be read, which covers both. TestEngine_RunDiagnosticsAreOneLineEach holds the rendering contract across the detection, parse, overlay and validation paths. The invariant was already asserted, but only inside the one test where a multi-line message had been found before, so it could not reach either of these.
The refusal of `--opt overlay=` was pinned in the compiler's own decode table but not through the CLI, which is where a user meets it. It is a fourth class the existing rows do not reach: the pair is well-formed and the name is known, and the value is still unusable. Reverting the guard leaves every other row green and reddens only this one.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two gaps at the boundary between
engine,compilersand the CLI, fixed with one seam.Format detection lived in the engine.
engine/sniff.goYAML-decoded the whole source and knewexactly two keys. Three of the five planned compilers take input that is not YAML, so a
.proto,.tspor.graphqlfile died with the decoder's complaint — quoting the engine's own private typename at the user — rather than being told no compiler takes it. Worse, which of the two outcomes a
file got was an accident of whether its bytes happened to parse: a one-line GraphQL document is
valid YAML and reached "unrecognized spec format", a two-line one was a YAML error.
compilers.Compilernow carriesDetect(Source) (SourceFormat, []ir.Diagnostic, bool), andRegistry.Detectaskseach registered compiler in registration order. The OpenAPI compiler owns the two discriminating
keys and the
major.minorversion grammar that used to sit inengine/sniff.go;enginenames nosource format anywhere, and its archtest allowlist no longer includes a YAML parser. Recognition is
deliberately not support — the OpenAPI compiler reports
swagger@2.0while serving no such format,so a Swagger document is reported as a format nothing is registered for rather than an unreadable
file.
A compiler also reports why it declined, through the same channel
Compilealready reports on. Itstays silent for another format's bytes — a compiler that complained about every source it did not
take would bury the one report that matters — and speaks only where the source is recognizably its
own and cannot be read. That is the case nothing else can cover:
openapi: [unterminatedis anOpenAPI document with a syntax error, and calling it unrecognized is wrong twice, since the compiler
did recognize it and holds the parse error explaining it. The engine prefers what a compiler said
over its own account, because it parses nothing and can say no more than that nobody claimed the
source. Reporting it engine-side instead would mean labelling every unparsed source "not YAML",
which is false for the first compiler whose format is not.
Detection is bounded. A source within 64 KiB is decoded whole; a larger one is read from a bounded
prefix — flow style (JSON) has its top-level entries streamed, block style is cut at its last
complete line — so cost is flat in document size instead of a full parse to read two keys. Measured
on a 10 MB block-style spec: 3.0 ms against 449 ms for the whole-document decode it replaces. The
cost is stated where it lands: a declaration past the cap is not seen, which
TestSniff_BeyondTheCappins in both directions.
No compiler option was reachable without writing Go.
cmd/morphic/compile.gobuilt itsengine.RunOptionswithSkipValidatealone;FormatOptionsappeared nowhere undercmd/. Theoptions themselves worked fine from a Go embedder, so this was a feature that existed and could not
be reached.
compilers.Compileralso gainsDecodeOptions(OptionSet) (any, error), and both spec-takingcommands gain a repeatable
--opt key=value. It is bound with the other shared flags rather than bycompilealone:validateis compile's pipeline with the document dropped, so a spec needing anoption to compile has to be checkable under that same option, or a gate passes a spec the build then
rejects. The CLI carries the pairs verbatim: it does not know which compiler
will read them, so the names, the accepted values and what counts as a file path are the compiler's,
and an unknown name is refused rather than ignored.
OptionSetalso carries the caller's filereader, which is what lets a path-valued option work while a compiler still does no file I/O of its
own —
enginesuppliesos.ReadFile, so the read stays on the caller's side of the contract.Both additions are required interface methods rather than optional interfaces checked by type
assertion. A compiler that answered neither would register successfully and be unreachable, which is
a hole no caller can see; and since both issues wanted to widen the same one-implementation
interface, widening it once beats two seams that can silently not participate.
On invariant 1
The IR is the ABI and the engine must not learn formats. Nothing above
compilersgained formatknowledge here — it lost some.
engineno longer holds a spec's discriminating keys, its versiongrammar, or a parser; the CLI holds no option vocabulary. What remains in
engine.Newis theone-line composition root that names the built-in compilers, which is what a composition root is
for: the registry documents that there is no
init()-time self-registration and that the enginecomposes explicitly. Registering a compiler needs no engine edit, which
TestEngine_RunNewFormatNeedsNoEngineEditproves with a compiler for a format no file underengine/mentions.internal/harnessstill bypasses detection by calling the compiler directly; that is deliberate andunchanged. The clearer failure this gives an unrecognized source may make the exit-code and
diagnostic work easier, but nothing about how diagnostics leave
engine.Runis touched here.Two corrections to #69's text
AllowExternalRefs, notDisableExternalRefs; the polarity was inverted after theissue was filed and it is now off by default.
Overlayis the third, and its own doc commentadmitted only a programmatic caller could set it. That comment is now wrong in the other
direction and has been rewritten. Overlay is the strongest case of the three, since it takes a
file — hence the reader on
OptionSet.Test plan
Written first, and each confirmed to go red with only the production change reverted:
engine:TestEngine_RunUnrecognizedFormatcompiles.proto,.tspand two GraphQL documentsand asserts the error names no compiler and quotes no parser. Red on all five subcases with the
old sniff restored.
cmd/morphic:TestRun_CompilerOptionReachesTheCompilercompiles a spec taggedzoounder apath starting
aand asserts--opt grouping=path-prefixmoves the group name, with a controlrun pinning that the default is what changed. Red (
"a"vs"zoo") with theRunOptionslinereverted.
compilers: registration-order, recognized-but-unregistered, declining and empty-source casesover
Registry.Detect.compilers/openapi: aDetecttable covering every dialect this compiler names, the foreignformats the other planned compilers take, and the sources it recognizes as its own and cannot
read — including one whose key falls past the sniff cap, which the key search must not see any
more than the decode did;
sniffpast the cap in block and flow style;decodeFlowPrefixagainstprefixes cut inside a key and inside a value; the entry cap;
DecodeOptionsreaching everysetting it defines and refusing each way one can be unusable;
TestDecodeOptions_FeedsCompileclosing the loop from decode to lowering.cmd/morphic:--opt overlay=<file>end to end, asserting the overlay applied and a secondsource recorded; a malformed pair, an unknown name, an unusable value and an empty one each
refused with exit 2 and no document.
cmd/morphic/testdata/compile-help.txtregenerated for the new flag.gofmt,go vet,golangci-lint(0 issues),go build, coverage at 100%.Breaking
compilers.CompilergainsDetectandDecodeOptions. Every implementation must add both; thein-tree one is updated.
engine.Sniffand its probe type are removed. Detection is reached through a registry ofcompilers, which is the only place that can answer for more than one format.
engine/undecodable-sourcebecomesopenapi/undecodable-sourceand gains the parse position theengine-level code never carried.
engine/unsupported-formatretires: Swagger is recognized andserved by nobody, which is
engine/no-compiler-for-format, and the engine cannot distinguish thatfrom an unregistered format.
NewWith()with no compilers reportsengine/unrecognized-formatrather thanengine/no-compiler-for-format. With nobody to read the source, no format is named.validateaccepts--opt, which it did not before. Its help text and golden change with it.engine.NewWith()refuses an empty compiler set. Nothing can be added to a built engine, soone with no compilers could only ever report every source as unrecognized — blaming the
document for a misconfiguration of the caller. A note here previously asked for this
precondition not to be added, because an empty engine was the only way to reach Run's
nothing-recognized branch; compiler-owned detection makes that branch reachable from an
ordinary unclaimed source, so the coverage that note protected no longer depends on it.
this build compiles <formats>, from the newRegistry.Formats. Naming what failed without naming what would have worked left the nextstep to guesswork, and it is the engine reading its own registry rather than learning a
format. A compiler's own report — a parse error with a line number — is left unadorned.
engine.RunOptionsgainsCompilerOptions. Setting it together withFormatOptionsis an errorrather than a precedence rule, since a run configured two ways is a mistake in the caller.
Closes#68
Closes#69
Closes#386