Repository navigation
fix(compilers/openapi): treat a restated union branch as bare - #1278
Wahbeh-Mohammad wants to merge 5 commits into
Conversation
Follow-up: open items, by scopeAll in-scope items landed in
Borderline: deferred, same mechanism
Out of scope: file separately
Corrections to the PR descriptionThe description was written at
Verification state
|
OmarAlJarrah
left a comment
OmarAlJarrah
left a comment
There was a problem hiding this comment.
Review of afb4a4f0. The code holds up: I found no wrong lowering. One issue with the PR text and a few smaller items follow, each in its own comment.
CI: both jobs passed on afb4a4f0, but that head is behind main by #1238, #1191 and #1210, and #1191 edits union.go, docs/compilers/openapi.md and conformance_test.go. The merge has no conflicts, and go test ./compilers/openapi/... ./internal/... ./cmd/... ./ir/... ./pass/... passes on it. Please merge main so CI covers that tree. A local make gate there passed through build and was stopped during coverage, so it says nothing beyond that.
Probed clean:
- 34 planted mutants of the new code: 33 reddened a test, and the survivor has its own comment.
- Bare, restated and restated-with-
notforms lower alike at 19 schema positions, withmainas the control (it differs at 16). - 58 near-miss specs (aliases, merge keys, discriminators,
$refsites) gave no wrong verdict, and 7 hostile specs (cycles, depth) no crash. testdata: 1 of 216 specs differs, this PR's own case.- Golden spec,
openapi.json@319dc756: only 17reasonvalues change, on the head and on the merged tree, and inline-degraded unions go from 45 to 28.
| or `format`, does not repeat the body, so it is judged as written: one that writes a `type`, a | ||
| `properties` or another keyword declaring shape falls to the five shapes above, even beside a | ||
| `not`, and one that only narrows, as `{required: [a], minProperties: 2}` does, stays validation | ||
| logic. A `not` written inside a property is no repeat either. A property the schema only composes |
There was a problem hiding this comment.
nit: this sentence is false when the body writes the same not.
S:
type: object
properties: {a: {type: string, not: {enum: [x]}}}
oneOf:
- {properties: {a: {type: string, not: {enum: [x]}}}, required: [a]}
- required: [a]morphic compile --verify reports two info openapi/validation-only-keyword, for the property's not and for the oneOf, and no openapi/degraded-construct: the branch's property is written alike, so restatesProperty returns true at its first EqualNodes check. That verdict agrees with "written alike" earlier in this rule, so the sentence is what is wrong. The follow-up comment repeats it.
Rule: this document is normative (CLAUDE.md), and "Zero nits" covers imprecise text.
Fix: limit the sentence, for example "A not inside a property repeats the body only when the declaration is written alike."
There was a problem hiding this comment.
Fixed in 5d191b1. Your spec gives two info openapi/validation-only-keyword and no openapi/degraded-construct on the head, which agrees with "written alike" earlier in the rule, so the sentence was what was wrong. It now reads: "A not written inside a property repeats the body only where the property is written alike." With the not in the branch's property alone, the union still degrades. The follow-up comment's restatement of the old sentence is superseded by the updated description.
| } | ||
|
|
||
| // typeKeywords are the keywords a branch can repeat of a property with more | ||
| // written beside the body's copy. Each means the same whatever stands beside |
There was a problem hiding this comment.
nit: "Each means the same whatever stands beside it" does not hold for $ref or nullable, and the comment on restatesProperty just below says so: a $ref "in 3.0 replaces what stands beside it" and a nullable "widens type". OpenAPI 3.0.3 also applies nullable only if type is defined in the same Schema Object. No verdict changes, because restatesProperty holds both to agree.
Rule: CLAUDE.md, "Zero nits", names "a doc comment that is imprecise".
Fix: say what the list is for rather than what each keyword means, leaving the two exceptions to restatesProperty's comment.
There was a problem hiding this comment.
Fixed in 5d191b1. The comment no longer says what each keyword means. It says what the list is for: the keywords naming the property's type, and that the list is closed, so any other keyword keeps the branch from being a repeat even when written as the body writes it, rather than asking of each keyword whether it reads its siblings. It points to restatesProperty for why $ref and nullable must also agree.
| // TestOneOf_CoDeclaredBranchSayingMoreDeclaresShape holds a branch that writes | ||
| // anything the body does not to the degraded lowering, and pins each way of | ||
| // doing so a looser comparison would read as a restatement. | ||
| func TestOneOf_CoDeclaredBranchSayingMoreDeclaresShape(t *testing.T) { |
There was a problem hiding this comment.
nit: nothing pins typeKeywords as a closed list. Adding minLength to it leaves go test ./compilers/openapi/internal/schema ./compilers/openapi/e2e green, though the output changes:
S:
type: object
properties: {a: {type: string, minLength: 1, description: d}}
oneOf:
- {properties: {a: {type: string, minLength: 1}}, required: [a]}
- required: [a]This head reports openapi/degraded-construct; with the mutant it reports openapi/validation-only-keyword. ConstraintBesideTheType misses it because its body writes no minLength, so the comparison fails either way.
Rule: CLAUDE.md, "Break what a test claims to catch". This test's comment says it pins each way a looser comparison would read as a restatement.
Fix: add a row for this spec. That branch says nothing the body does not, so if it should count as a repeat, the other fix is to widen the list. Either way a row should pin the choice.
There was a problem hiding this comment.
Kept the closed list and pinned it, in 686c158. Widening it would mean deciding, keyword by keyword and version by version, which keywords read their siblings, as additionalProperties reads properties, which is a design change of its own rather than a fix here.
Your mutant was one of several: adding items, enum, const, dependentRequired, pattern, maxLength, description or properties to typeKeywords also left the suite green. ConstraintTheBodyAlsoWrites (your spec) and four sibling rows, ConstTheBodyAlsoWrites, EnumTheBodyAlsoWrites, ItemsTheBodyAlsoWrites and KeywordTheModelHasNoFieldForTheBodyAlsoWrites, each redden when minLength, const, enum, items or dependentRequired is added; the clean tree passes. Removing $ref, format or nullable already reddened a row. Adding pattern or maxLength (the same kind as minLength), description or properties is still not caught by a row of its own.
| // shape the body does not: those that can repeat it, and `required` and `not`, | ||
| // which only narrow it whatever they hold. Any other says something the body may | ||
| // not, so the branch keeps the verdict declaresShape gives it. | ||
| var restatingBranchKeywords = []string{"type", "nullable", "properties", "required", "not", "title", "description"} |
There was a problem hiding this comment.
suggestion: title and description pass here but other annotations do not, so the two spellings this PR aligns still differ beside them. {type: object, required: [a], $comment: hi} stays degraded while {required: [a], $comment: hi} is validation_only, and the same holds for deprecated and an x- extension.
This is not a defect: it matches main, the rule's closed list covers it, and no branch in the 28 remaining inline-degraded schemas on the golden spec writes one. These keywords say nothing about shape either, so they are candidates if the list grows.
There was a problem hiding this comment.
Agreed it is not a defect. On the head, {type: object, required: [a], $comment: hi} degrades while {required: [a], $comment: hi} is validation_only, and the same holds for deprecated: true and an x- extension; the first form degrades at main too. The list is unchanged here, and the updated description now states this as a deliberate limit.
|
issue: the description no longer matches Rule:
On #620, the opening line says the issue stays open, but what it asks for is done: Fix: edit the description to match the head, and use |
|
nit: the description does not use the form Fix: use Summary and Test plan, keep the deliberate limits, and drop the review history, in the same edit as the corrections above. |
|
nit: the follow-up comment says none of its items are filed, and
Fix: file each, along with the skills items the comment lists, and cite the |
docs/compilers/openapi.md conflicted in the Cases list of union-beside-structure: main added keywords-at-every-outcome and this branch added union-restated-branches, so the list keeps both. The union-restated-branches golden takes the unresolved auth ID space main added beside openapi, as every other conformance golden does.
|
Answering the review summary and the three comments on the description and the follow-ups:
|
Closes #620.
Summary
Two spellings of "exactly one of
htmlandurl" lowered differently. Branches writing onlyrequiredkept theoneOfasvalidation_only; branches also restating a property the body declares kept it asdegraded_lowering, with "a branch is written inline, so it names no referent". A branch that repeats the body declares no shape of its own, so it now lowers like the bare form.branchDeclaresShapeincompilers/openapi/internal/schema/union.gonow takes the body and asksrestatesBody. A branch repeats the body when:type,nullable,properties,required,not,titleanddescription, whererequiredandnotnarrow as they do in a bare branch;typeit writes, if any, is the body's, withnullableagreeing;propertiesdeclares, either written alike, or written in nothing but$ref,type,formatandnullable, each as the body writes it, with the two agreeing on$refandnullable.Declarations are compared on raw nodes with
yml.EqualNodes, which is whyinternal/archtestallows that import for this package: the typed model has no field for some keywords (docs/parser-library.md#unmodeled-keywords), soSchema.IsEqualcalls a branch writing one equal to a body without it.A consumer gets one
Model, the union verbatim underUnmodeledwith reasonvalidation_only, one infoopenapi/validation-only-keywordand noopenapi/degraded-construct. Nothing is lost either way: only the reason and the diagnostic change. The IR's shape does not change, so there is noIRVersionbump.Deliberate limits, each stated in
docs/compilers/openapi.md#union-beside-structure:minProperties; a branch writing onlyrequiredandminPropertiesis stillvalidation_only. The closed list is what stopsitems,formatorminLengthbeside a repeatedtypefrom reading as validation logic.$ref,type,formatandnullablekeeps the branch degraded even when written as the body writes it, unless the whole declaration is written alike. Anotinside a property is a repeat only in that case.titleanddescription($comment,deprecated,x-extensions) beside a repeatedtypekeep the branch degraded, as atmain.allOfhas no one declaration to repeat, so such a branch stays degraded while its bare form isvalidation_only.typeagainst a body with notypestays degraded, since it narrows the body.Test plan
make gateexits 0 on 686c158, lint included, with all 18589 statements covered.RenderandRenderBarelower alike, pinned by the conformance caseunion-restated-branches, which also holdsRenderDocumented,RenderExclusiveand aconstbranch that stays degraded.TestOneOf_CoDeclaredBranchSayingMoreDeclaresShapepins the closed property list: addingminLength,items,enum,constordependentRequiredtotypeKeywordsreddens a row each, and removing$ref,formatornullablereddens a row inTestOneOf_CoDeclaredRestatingBranchDeclaresNoShape.openapi.jsonat 319dc756), against a build ofmainat 3188471:jq -S 'del(.diagnostics)'shows 17reasonvalues moving fromdegraded_loweringtovalidation_onlyand no other IR difference. Lines matchinga branch is written inlinego from 45 to 28, and nobrowser-renderingrequest body is degraded.