From 3df761205171ba0babf7f75414abe6302d13138a Mon Sep 17 00:00:00 2001 From: Robin Scher Date: Mon, 24 Aug 2026 15:34:05 -0700 Subject: [PATCH] fix(expr): render lists per openjd-specifications#176 and bump the oracle pin to 0.11.5 --- Makefile | 2 +- internal/openjd/expr/doc.go | 43 +++--- internal/openjd/expr/funcsconv.go | 81 ++++++---- internal/openjd/expr/funcsreprshell.go | 35 ++++- .../expr/funcsreprshell_internal_test.go | 18 +++ internal/openjd/expr/value.go | 9 +- internal/openjd/expr/value_internal_test.go | 142 +++++++++++++++--- internal/openjd/paramjson.go | 28 ++-- internal/openjd/paramjson_test.go | 45 ++++-- internal/worker/fmtres/expres.go | 8 +- test/oracle/baseline-ops.txt | 97 +++++++++--- test/oracle/baseline.txt | 50 ------ third_party/openjd-specifications | 2 +- 13 files changed, 384 insertions(+), 176 deletions(-) diff --git a/Makefile b/Makefile index 63e295a0..76c94f91 100644 --- a/Makefile +++ b/Makefile @@ -218,7 +218,7 @@ test-conformance: ## Run the official OpenJD conformance suite (needs the pinned # unpinned upgrade could turn the differential test red without a single sqi # commit — and because a divergence report is meaningless without knowing # which build of the reference produced it. -OPENJD_MODEL_VERSION ?= 0.11.4 +OPENJD_MODEL_VERSION ?= 0.11.5 ORACLE_VENV := .venv-oracle .PHONY: expr-oracle-venv diff --git a/internal/openjd/expr/doc.go b/internal/openjd/expr/doc.go index 986266e6..c02498d4 100644 --- a/internal/openjd/expr/doc.go +++ b/internal/openjd/expr/doc.go @@ -975,26 +975,29 @@ // Value.String() rendered a list's string elements unquoted while // string()'s JSON row quoted them (see the float-passthrough bullet // above); that quoting gap closed in this wave, and the two were then -// measured to also agree byte-for-byte on ESCAPING for every case -// tried: an embedded double quote, non-ASCII, angle brackets and an -// ampersand, a newline, a backslash, tab, backspace, formfeed, carriage -// return, an emoji, U+2028/U+2029, and the empty string -// (TestValueString_VersusStringFunction). That is the full extent of -// the claim, and it must not be read as universal agreement: the two -// are INDEPENDENT implementations — Value.String() quotes a list's -// string elements with strconv.Quote (value.go), string()'s list row -// encodes with encoding/json and SetEscapeHTML(false) (funcsconv.go's -// writeJSONValue) — and a direct probe of the two encoders against each -// other, outside the set above, finds real divergences: on the C0 -// controls U+0000, U+0001 and U+001B, strconv.Quote emits its own -// hex-escape form where the JSON encoder emits a \u00XX escape; on -// vertical tab (U+000B), Quote emits its own short escape where JSON -// emits \u000b; on DEL (U+007F), Quote escapes it where JSON leaves it -// literal; and on invalid UTF-8, Quote preserves the raw bytes where -// JSON substitutes U+FFFD — all four measured directly against both -// functions and pinned as regression tests, not just measured once -// (TestValueString_DivergesFromStringFunctionOutsideMeasuredSet). Do not -// widen the agreement claim past the set it was measured on. +// measured to also agree byte-for-byte on ESCAPING for thirteen cases: +// an embedded double quote, non-ASCII, angle brackets and an ampersand, +// a newline, a backslash, tab, backspace, formfeed, carriage return, an +// emoji, U+2028/U+2029, and the empty string +// (TestValueString_VersusStringFunction). That claim was deliberately +// NARROW, because the two were independent implementations — +// Value.String() used strconv.Quote while string()'s list row used +// encoding/json with SetEscapeHTML(false) — and a direct probe outside +// the measured set found real divergences on the C0 controls, vertical +// tab, DEL and invalid UTF-8, pinned in the opposite direction. +// +// THAT IS NO LONGER THE SHAPE OF IT. openjd-specifications#176 added +// the rule that format-string interpolation "uses this same conversion" +// as string() and that the result "must parse as JSON". Go's spelling +// of a control character (\x01, and \a/\v for two JSON does not name) +// is not JSON at all, so every one of those divergences was a defect +// rather than a permitted difference. Both renderers now call +// funcsconv.go's jsonQuoteElement, so the agreement holds BY +// CONSTRUCTION for every input rather than by measurement over a set, +// and TestValueString_ListQuotingIsJSONEverywhere pins the six cases +// that used to diverge. The separator still differs from +// internal/openjd's canonical STORAGE form ("," there, ", " here), and +// paramjson.go states that difference where it lives. // // unique() CHARGES PER COMPARISON, NOT PER ELEMENT — and the reason is // a prediction that measurement overturned, not a preference. C1 diff --git a/internal/openjd/expr/funcsconv.go b/internal/openjd/expr/funcsconv.go index c8fa3d90..3a6a717c 100644 --- a/internal/openjd/expr/funcsconv.go +++ b/internal/openjd/expr/funcsconv.go @@ -175,27 +175,29 @@ var convFuncs = map[string][]Shape{ return String(s), nil }, }, - // RFC 0006 calls this "the JSON string representation", and it is a - // separate implementation from Value.String() -- two renderings, two - // functions, on purpose: this row encodes with encoding/json and - // SetEscapeHTML(false) (writeJSONValue), while Value.String() quotes a - // list's string elements with strconv.Quote (value.go). + // RFC 0006 calls this "the JSON string representation". This row and + // Value.String() are two renderings for two purposes, but they now + // share ONE quoting rule: both send a list's string-like elements + // through jsonQuoteElement (encoding/json with SetEscapeHTML(false)). // - // CORRECTION (final whole-branch review, sub-project E1): an earlier - // revision said Value.String() "renders a list's string elements - // unquoted ('[a, b]') as a diagnostic form, which is a known - // divergence deferred to sub-project E". That was true when written - // and is FALSE now -- sub-project E1 CLOSED the divergence rather than - // deferring it, and Value.String() has quoted list string elements - // since. What is true today: the two renderings agree byte-for-byte on - // quoting AND on escaping for every case measured - // (TestValueString_VersusStringFunction's thirteen), but they remain - // independent implementations and do NOT agree universally -- they - // diverge on the C0 controls, vertical tab, DEL and invalid UTF-8, - // pinned in the opposite direction by - // TestValueString_DivergesFromStringFunctionOutsideMeasuredSet. Do not - // restate the old claim, and do not widen the new one past the set it - // was measured on; doc.go carries the full statement. + // HISTORY, because two earlier revisions of this comment were each + // true when written and false later. (1) The oldest said + // Value.String() "renders a list's string elements unquoted" as a + // diagnostic form -- sub-project E1 closed that. (2) Its replacement + // said the two were INDEPENDENT implementations that agreed on the + // thirteen cases measured (TestValueString_VersusStringFunction) and + // provably diverged outside them -- on the C0 controls, vertical tab, + // DEL and invalid UTF-8 -- and warned against widening the claim. + // That was correct until openjd-specifications#176, which states that + // format-string interpolation "uses this same conversion" and that + // the result "must parse as JSON": Go's \x00 and \v forms are not + // JSON, so the divergence became a defect and the two renderers were + // merged onto one quoter. The agreement is now universal BY + // CONSTRUCTION rather than by measurement, and + // TestValueString_ListQuotingIsJSONEverywhere pins the six cases that + // used to diverge. What still differs is the SEPARATOR in + // internal/openjd's canonical storage form ("," there, ", " here) -- + // see paramjson.go. // // Cost{ArgElements: {0}} — a DELIBERATE divergence from the reference, // which measures a flat 1 for string([1,2,3]) AND for a 10-element @@ -348,6 +350,37 @@ func jsonList(v Value) (string, error) { return b.String(), nil } +// jsonQuoteElement renders one string-like list element as a JSON string. +// +// The HTML-escaping note above this function's only other caller applies +// here: json.Marshal escapes "<", ">" and "&" by default, because its output +// is meant to be safe inside an HTML document, which is not this context. +// Neither the reference implementation nor Python's json.dumps does that, and +// RFC 0006 asks for "the JSON string representation" without qualification. +// An Encoder with SetEscapeHTML(false) is the supported way to turn it off; +// it appends a newline, which is trimmed. +// +// Value.String() renders a list's elements through this same function, and +// must keep doing so: openjd-specifications#176 states that format-string +// interpolation with surrounding text "uses this same conversion", so +// "items: {{ MyList }}" and "items: " + string(MyList) are required to agree. +// They are two renderers sharing one quoting rule, not two rules -- see +// TestValueString_ListQuotingIsJSONEverywhere, which pins the six classes of +// input (C0 controls, vertical tab, DEL, invalid UTF-8) where a second rule +// would show through. +func jsonQuoteElement(s string) string { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + enc.SetEscapeHTML(false) + // Encode reports unsupported types, encoding cycles and writer failures, + // none of which a string written into a bytes.Buffer can produce. + // Invalid UTF-8 is not an error either -- the encoder substitutes U+FFFD + // -- so there is no failure left for a caller to handle, and an + // infallible signature is what lets Value.String() share this function. + _ = enc.Encode(s) //nolint:errcheck // cannot fail for a string into a buffer; see above + return string(bytes.TrimRight(buf.Bytes(), "\n")) +} + // writeJSONValue is jsonList's recursive worker. func writeJSONValue(b *strings.Builder, v Value) error { switch v.Type.Code { @@ -371,13 +404,7 @@ func writeJSONValue(b *strings.Builder, v Value) error { // rendering into a surprising one. An Encoder with SetEscapeHTML(false) // is the supported way to turn it off; it appends a newline, which is // trimmed. - var buf bytes.Buffer - enc := json.NewEncoder(&buf) - enc.SetEscapeHTML(false) - if err := enc.Encode(v.s); err != nil { - return err - } - b.Write(bytes.TrimRight(buf.Bytes(), "\n")) + b.WriteString(jsonQuoteElement(v.s)) default: // bool, int, float and null all render bare, and Value.String() // already spells each of them the way JSON does. diff --git a/internal/openjd/expr/funcsreprshell.go b/internal/openjd/expr/funcsreprshell.go index 0b2bbaac..501ddf59 100644 --- a/internal/openjd/expr/funcsreprshell.go +++ b/internal/openjd/expr/funcsreprshell.go @@ -83,11 +83,12 @@ var reprShellFuncs = map[string][]Shape{ }}, // DIVERGENCE from the reference: see this var block's own COST comment. {Params: []Type{ListOf(varT)}, Ret: TString, Cost: Cost{ArgElements: []int{0}}, Fn: func(args []Value) (Value, error) { - body, err := joinValues(args[0].AsList(), ", ", pwshElement) + elems := args[0].AsList() + body, err := joinValues(elems, ", ", pwshElement) if err != nil { return Value{}, err } - return boundedString("@(" + body.AsStr() + ")") + return boundedString("@(" + pwshUnaryComma(elems) + body.AsStr() + ")") }}, }, } @@ -152,6 +153,9 @@ func pwshQuote(s string) string { // runnable PowerShell for anything that expects array elements. Recursing // through pwshElement instead builds a nested "@(...)" literal, matching how // the top-level ListOf(varT) row itself is built. +// +// A nested list also picks up its own unary comma when it needs one, since +// the decision is per-list; see pwshUnaryComma. func pwshElement(v Value) string { switch v.Type.Code { case CodeBool: @@ -164,12 +168,33 @@ func pwshElement(v Value) string { case CodeNull: return "$null" case CodeList: - parts := make([]string, len(v.AsList())) - for i, elem := range v.AsList() { + elems := v.AsList() + parts := make([]string, len(elems)) + for i, elem := range elems { parts[i] = pwshElement(elem) } - return "@(" + strings.Join(parts, ", ") + ")" + return "@(" + pwshUnaryComma(elems) + strings.Join(parts, ", ") + ")" default: return pwshQuote(v.String()) } } + +// pwshUnaryComma returns the "," that section 2.2.6 requires in front of a +// one-element array whose only element is itself an array, and "" for every +// other list. +// +// PowerShell flattens "@(@(1, 2))" to "@(1, 2)", so the nesting is lost on +// the round trip; the unary comma operator, "@(,@(1, 2))", preserves it. The +// test is on the element COUNT, not on depth: "@(@(1, 2), @(3))" needs no +// comma because two elements already force an array, and a one-element list +// of SCALARS must not get one -- "@('a')" is unambiguous already. +// +// This is a spec rule sqi missed until openjd-specifications#176 stated it +// outright; the reference implementation refuses lists nested more than two +// deep, so it cannot answer the recursive case at all. +func pwshUnaryComma(elems []Value) string { + if len(elems) == 1 && elems[0].Type.Code == CodeList { + return "," + } + return "" +} diff --git a/internal/openjd/expr/funcsreprshell_internal_test.go b/internal/openjd/expr/funcsreprshell_internal_test.go index 4d64b84f..8f950c93 100644 --- a/internal/openjd/expr/funcsreprshell_internal_test.go +++ b/internal/openjd/expr/funcsreprshell_internal_test.go @@ -100,6 +100,24 @@ func TestReprPwsh(t *testing.T) { // list of text, not a nested array. A nested list must become a // nested "@(...)" array literal instead. {"nested list becomes a nested array literal", `repr_pwsh([['a'], ['b']])`, "@(@('a'), @('b'))"}, + // Section 2.2.6, as restated by openjd-specifications#176: a + // ONE-element list whose element is itself a list takes the unary + // comma form, because "@(@(1, 2))" flattens to "@(1, 2)" under + // PowerShell's array-flattening rules while "@(,@(1, 2))" preserves + // the nesting. The rule is about the number of ELEMENTS, not the + // depth: a two-element list of lists needs no comma, and a + // one-element list of SCALARS must not get one (@('a') is already + // unambiguous, and @(,'a') would be a different, wronger thing to + // write). + {"single nested list takes the unary comma form", `repr_pwsh([[1, 2]])`, "@(,@(1, 2))"}, + {"single nested EMPTY list takes the unary comma form", `repr_pwsh([[]])`, "@(,@())"}, + {"two nested lists take no comma", `repr_pwsh([[1, 2], [3]])`, "@(@(1, 2), @(3))"}, + {"single scalar element takes no comma", `repr_pwsh(['a'])`, "@('a')"}, + // The rule recurses: each list decides for itself. The reference + // implementation refuses three levels of nesting outright ("Lists + // may be nested at most 2 levels deep"), so this row's ground truth + // is section 2.2.6's own wording, not the oracle. + {"unary comma applies at depth too", `repr_pwsh([[['a']], [['b']]])`, "@(@(,@('a')), @(,@('b')))"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/openjd/expr/value.go b/internal/openjd/expr/value.go index 23982c81..cece64b8 100644 --- a/internal/openjd/expr/value.go +++ b/internal/openjd/expr/value.go @@ -274,7 +274,14 @@ func (v Value) String() string { // the regex family and the path .parts/.suffixes properties. switch elem.Type.Code { case CodeString, CodePath, CodeRangeExpr: - parts[i] = strconv.Quote(elem.s) + // jsonQuoteElement, NOT strconv.Quote: Go's quoting spells a + // control character "\x01" and has "\a"/"\v" forms JSON does + // not define, so it produced output no JSON parser accepts -- + // which openjd-specifications#176 forbids outright, and which + // also made this rendering disagree with string()'s for the + // same list. Sharing funcsconv.go's quoter is what keeps + // section 2.2.1's "same conversion" true by construction. + parts[i] = jsonQuoteElement(elem.s) default: parts[i] = elem.String() } diff --git a/internal/openjd/expr/value_internal_test.go b/internal/openjd/expr/value_internal_test.go index 44c480c4..3aee284c 100644 --- a/internal/openjd/expr/value_internal_test.go +++ b/internal/openjd/expr/value_internal_test.go @@ -2,7 +2,10 @@ package expr -import "testing" +import ( + "encoding/json" + "testing" +) func TestValue_Constructors(t *testing.T) { tests := []struct { @@ -373,29 +376,50 @@ func TestValueString_VersusStringFunction(t *testing.T) { } } -// TestValueString_DivergesFromStringFunctionOutsideMeasuredSet pins the four -// classes of input where Value.String() (value.go's strconv.Quote) and -// string(list)'s JSON row (funcsconv.go's writeJSONValue, an -// encoding/json.Encoder with SetEscapeHTML(false)) are independent -// implementations that provably do NOT agree, so doc.go's narrow "agrees on -// every case exercised" claim stays testable in both directions rather than -// only the agreeing one. +// TestValueString_ListQuotingIsJSONEverywhere pins the rule +// openjd-specifications#176 added to section 2.2.1: a list's string and path +// elements are double-quoted with `"`, `\` and every character below U+0020 +// escaped, "the result must parse as JSON", and format-string interpolation +// with surrounding text -- which renders through Value.String() -- uses "this +// same conversion", so `"items: {{ MyList }}"` and `"items: " + string(MyList)` +// agree. // -// Constructed directly through the List/String value constructors rather -// than through Eval: an EXPR source string must itself be valid UTF-8, so -// the invalid-UTF-8 case cannot be expressed as parseable source text at -// all. Building every case the same way keeps the six comparable. -func TestValueString_DivergesFromStringFunctionOutsideMeasuredSet(t *testing.T) { +// This test REPLACES TestValueString_DivergesFromStringFunctionOutsideMeasuredSet, +// which pinned the opposite: six classes of input where Value.String() +// (strconv.Quote, GO syntax) and string(list)'s JSON row provably did not +// agree. That divergence was defensible while the specification only said +// "the JSON string representation" of the string() row and said nothing at +// all about the interpolation row; #176 states both, and Go's spelling of a +// control character -- "\x01", and "\a"/"\v" for two JSON does not name -- +// is not JSON at all. So each of those six now has one right answer, and the +// old test's own closing instruction ("update doc.go's divergence claim if +// this is now correct") is what is being carried out here. +// +// Constructed through the List/String value constructors rather than through +// Eval: an EXPR source string must itself be valid UTF-8, so the invalid-UTF-8 +// case cannot be written as parseable source at all. Building every case the +// same way keeps them comparable. +func TestValueString_ListQuotingIsJSONEverywhere(t *testing.T) { tests := []struct { name string s string + want string }{ - {"C0 control U+0000", "\x00"}, - {"C0 control U+0001", "\x01"}, - {"C0 control U+001B (ESC)", "\x1b"}, - {"vertical tab U+000B", "\v"}, - {"DEL U+007F", "\x7f"}, - {"invalid UTF-8", "\xff\xfe"}, + {"C0 control U+0000", "\x00", `["\u0000"]`}, + {"C0 control U+0001", "\x01", `["\u0001"]`}, + {"C0 control U+001B (ESC)", "\x1b", `["\u001b"]`}, + {"vertical tab U+000B", "\v", `["\u000b"]`}, + // DEL is not a C0 control, so #176's "below U+0020" does not reach + // it and it stays literal -- which is also what the reference + // implementation renders (probed at openjd-model 0.11.5). repr_json + // is the spelling that escapes it. + {"DEL U+007F", "\x7f", "[\"\u007f\"]"}, + // Invalid UTF-8 has no JSON spelling; the encoder substitutes + // U+FFFD REPLACEMENT CHARACTER, and writes it as the six-character + // \ufffd ESCAPE rather than as the rune itself. The point of the row + // is that both renderings make the SAME substitution, not that the + // bytes survive. + {"invalid UTF-8", "\xff\xfe", `["\ufffd\ufffd"]`}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -405,10 +429,84 @@ func TestValueString_DivergesFromStringFunctionOutsideMeasuredSet(t *testing.T) if err != nil { t.Fatalf("jsonList(%q): %v", tt.s, err) } - if valueStr == jsonStr { - t.Errorf("Value.String() and string(list) unexpectedly AGREE on %q (both %s); "+ - "update doc.go's divergence claim if this is now correct", tt.s, valueStr) + if valueStr != jsonStr { + t.Errorf("Value.String() = %s; string(list) = %s; want the two to agree", valueStr, jsonStr) + } + if valueStr != tt.want { + t.Errorf("Value.String() = %s; want %s", valueStr, tt.want) + } + var parsed []string + if err := json.Unmarshal([]byte(valueStr), &parsed); err != nil { + t.Errorf("Value.String() = %s, which is not valid JSON: %v", valueStr, err) } }) } } + +// TestStringConversion_MatchesSpecFixture transcribes the expectations of +// conformance-tests/2023-09/EXPR/jobs/expr2.2.1--string-conversion-list-escaping.test.yaml, +// added to the specification by openjd-specifications#176. +// +// It is transcribed rather than scored because sqi's conformance harness +// collects job_templates and env_templates only -- the jobs/ suite executes a +// real task and asserts its stdout, which the harness has no runner for. That +// makes these rows the same kind of ground truth sub-project D's 31 +// apply_path_mapping expectations are: authored by the specification, checked +// here by hand, and invisible to every automated suite otherwise. +// +// The PATHS rows matter most, because they pin the INTERPOLATED rendering +// rather than string()'s: the fixture writes "{{ paths }}" with no call, which +// is Value.String() on a bare list. +func TestStringConversion_MatchesSpecFixture(t *testing.T) { + t.Run("string() rows", func(t *testing.T) { + tests := []struct{ name, src, want string }{ + {"QUOTE", `string(['a"b'])`, `["a\"b"]`}, + {"BACKSLASH", `string(['a\\b'])`, `["a\\b"]`}, + {"NEWLINE", `string(['a\nb'])`, `["a\nb"]`}, + {"TAB", `string(['a\tb'])`, `["a\tb"]`}, + {"NUL", `string(['\x00'])`, `["\u0000"]`}, + {"NESTED", `string([['a"b']])`, `[["a\"b"]]`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v, err := Eval(tt.src, nil, TString) + if err != nil { + t.Fatalf("Eval(%s): %v", tt.src, err) + } + if got := v.AsStr(); got != tt.want { + t.Errorf("Eval(%s) = %s; want %s", tt.src, got, tt.want) + } + var parsed any + if err := json.Unmarshal([]byte(v.AsStr()), &parsed); err != nil { + t.Errorf("Eval(%s) = %s, which is not valid JSON: %v", tt.src, v.AsStr(), err) + } + }) + } + }) + + // The fixture builds its list in TEMPLATE-scoped let so path() uses the + // POSIX flavor, then renders it in each host's own flavor. sqi's + // path_format is an evaluation option rather than a host default (doc.go's + // PATH FORMAT bullet), so the two host rows are two evaluations here. + t.Run("PATHS rows", func(t *testing.T) { + const src = `[path("/out/a.exr"), path("/out/b.exr")]` + for _, tt := range []struct { + name string + format PathFormat + want string + }{ + {"output_posix", PathPOSIX, `["/out/a.exr", "/out/b.exr"]`}, + {"output_windows", PathWindows, `["\\out\\a.exr", "\\out\\b.exr"]`}, + } { + t.Run(tt.name, func(t *testing.T) { + v, err := Eval(src, nil, TAny, WithPathFormat(tt.format)) + if err != nil { + t.Fatalf("Eval(%s): %v", src, err) + } + if got := v.String(); got != tt.want { + t.Errorf("Value.String() = %s; want %s", got, tt.want) + } + }) + } + }) +} diff --git a/internal/openjd/paramjson.go b/internal/openjd/paramjson.go index 20f28246..ab652f60 100644 --- a/internal/openjd/paramjson.go +++ b/internal/openjd/paramjson.go @@ -18,15 +18,25 @@ import ( // so a typed model representation would have to serialize at every boundary // anyway. One representation, defined once, at the top. // -// THE STORED FORM IS NOT THE INTERPOLATED FORM. A list rendered into a command -// line goes through expr.Value.String(), which quotes elements with -// strconv.Quote -- Go syntax, identical to JSON for ordinary text and -// different for control characters (Go writes \x00 where JSON requires a -// escape, and \x00 is not valid JSON at all). Section 1.3.2 governs -// interpolation; this file governs storage. They are two renderings for two -// purposes and must not be assumed interchangeable -- -// TestListDefault_JSONIsNotValueString pins the difference so it stays a -// documented fact rather than a latent surprise for sub-project F2. +// THE STORED FORM IS NOT THE INTERPOLATED FORM, but the gap is now narrower +// than it was. A list rendered into a command line goes through +// expr.Value.String(). That used to quote elements with strconv.Quote -- Go +// syntax, identical to JSON for ordinary text and different for control +// characters, where Go writes \x00 and JSON requires \u0000 -- so the two +// forms could not even be compared as text. openjd-specifications#176 then +// stated that interpolation and string() must use the same conversion and +// that the result must parse as JSON, and expr's renderer moved onto +// encoding/json (with HTML escaping off, exactly as marshalCanonical below +// uses it), so the ELEMENT QUOTING is now the same rule on both sides. +// +// What still differs is the SEPARATOR: canonical JSON writes ",", while +// section 2.2.1's rendering writes ", ". A single-element list therefore +// renders identically in both forms and a multi-element one does not. +// Section 1.3.2 governs interpolation; this file governs storage. They remain +// two renderings for two purposes and must not be assumed interchangeable -- +// TestListDefault_JSONVersusValueString pins both halves, the agreement and +// the remaining difference, so each stays a documented fact rather than a +// latent surprise. // isListParamType reports whether t is one of RFC 0007's list types. func isListParamType(t JobParamType) bool { diff --git a/internal/openjd/paramjson_test.go b/internal/openjd/paramjson_test.go index ee49bac1..8498cf78 100644 --- a/internal/openjd/paramjson_test.go +++ b/internal/openjd/paramjson_test.go @@ -157,17 +157,19 @@ func TestListDefault_ListTypeWithoutEXPRStillRejected(t *testing.T) { } } -// TestListDefault_JSONIsNotValueString pins the divergence paramjson.go's -// header describes, so it is a documented fact rather than a latent surprise: -// the STORED form is JSON, the INTERPOLATED form is expr.Value.String() (Go -// syntax, via strconv.Quote). They agree on ordinary text and separate on -// control characters — JSON requires a six-character \u0000 escape where Go -// writes \x00, and \x00 is not valid JSON at all. +// TestListDefault_JSONVersusValueString pins what paramjson.go's header +// describes: the stored form and the interpolated form now share their +// element QUOTING and still differ in their SEPARATOR. // -// Neither is wrong. They are two renderings for two purposes, and sub-project -// F2 must not assume a value read from the store can be compared against an -// interpolated one as text. -func TestListDefault_JSONIsNotValueString(t *testing.T) { +// The quoting half used to be a divergence -- the stored form was JSON while +// expr.Value.String() used strconv.Quote, so a control character came out +// \x00 on one side and \u0000 on the other, and \x00 is not valid JSON at +// all. openjd-specifications#176 removed that freedom by requiring the +// interpolated form to parse as JSON and to match string()'s conversion, and +// expr now renders through the same encoder. The separator half is untouched: +// canonical JSON has no space after the comma and section 2.2.1's rendering +// does. +func TestListDefault_JSONVersusValueString(t *testing.T) { const nul = "a\x00b" stored, err := encodeListDefault([]any{nul}, JobParamTypeListString) @@ -176,11 +178,26 @@ func TestListDefault_JSONIsNotValueString(t *testing.T) { } rendered := expr.List(expr.TString, []expr.Value{expr.String(nul)}).String() - if stored == rendered { - t.Fatalf("stored and interpolated renderings agree (%s); this test exists "+ - "because they do not, and something has changed", stored) + // One element: no separator to disagree about, so the two forms must now + // be byte-identical. This is the assertion that would have failed before + // the #176 fix. + if stored != rendered { + t.Errorf("stored = %s, interpolated = %s; a single-element list must render "+ + "identically now that both quote through encoding/json", stored, rendered) + } + + // Two elements: the separator is the one remaining difference. + storedPair, err := encodeListDefault([]any{"a", "b"}, JobParamTypeListString) + if err != nil { + t.Fatalf("encode pair: %v", err) + } + renderedPair := expr.List(expr.TString, []expr.Value{expr.String("a"), expr.String("b")}).String() + if storedPair != `["a","b"]` { + t.Errorf("stored pair = %s, want [\"a\",\"b\"] (canonical JSON, no space)", storedPair) + } + if renderedPair != `["a", "b"]` { + t.Errorf("interpolated pair = %s, want [\"a\", \"b\"] (section 2.2.1, with space)", renderedPair) } - t.Logf("stored=%q interpolated=%q — two forms, two purposes", stored, rendered) // The stored form must be valid JSON that round-trips; that is the whole // point of choosing it for storage. diff --git a/internal/worker/fmtres/expres.go b/internal/worker/fmtres/expres.go index 3d6f702b..0981a4db 100644 --- a/internal/worker/fmtres/expres.go +++ b/internal/worker/fmtres/expres.go @@ -39,9 +39,11 @@ package fmtres // - Anything else -- literal text, a reference embedded inside other // text, or more than one reference -- evaluates each reference // unconstrained (expr.TAny) and converts its NATURAL result to text via -// Value.String(), which funcsconv.go's own string() row documents as -// agreeing byte-for-byte with the language's string() function for -// every case that has been checked. checkFormatString takes exactly +// Value.String(), which agrees byte-for-byte with the language's +// string() function -- openjd-specifications#176 requires exactly that +// ("this same conversion"), and expr enforces it by routing both +// renderings through one quoter rather than by measuring two. +// checkFormatString takes exactly // this same branch for the identical position and ignores its own // target parameter there, for the identical reason: the result is // converted to a string regardless of what the reference itself diff --git a/test/oracle/baseline-ops.txt b/test/oracle/baseline-ops.txt index 2c9ec1da..abe63bd3 100644 --- a/test/oracle/baseline-ops.txt +++ b/test/oracle/baseline-ops.txt @@ -792,37 +792,88 @@ path | string :: path('s3://b//').relative_to(path('s3://b//')) # relative_to sums both operands' byte charges; reference takes the max (see section header). path | string :: path('s3://b///d').relative_to(path('s3://b//')) -# ── sqi is right: string(list) charges per element; reference is flat 1 ───── +# ── string(list): sqi omits the reference's RESULT unit and its recursion ──── # -# Rule 2's enumeration is introduced with "such as", so the general -# "iterates every element of a list" clause is not closed by the named -# list, and writeJSONValue really does walk every element of the list -# argument to render it -- confirmed against the spec text at -# third_party/openjd-specifications/wiki/2026-02-Expression-Language.md: -# 1084-1087. sqi charges rule 1 (1) plus ArgElements (the list's own -# length, NOT recursively flattened for a nested list); the reference -# never charges more than 1 regardless of list size. "string(flatten(...))" -# combines this with flatten()'s own charge, which the reference already -# matches exactly (flatten is not itself in this baseline) -- the whole -# diff there is string(list)'s own over-count for a 4-element result. - -# string(list)'s per-element ArgElements charge; reference is flat 1 (see section header). +# REWRITTEN 2026-08-24 on the 0.11.5 pin bump (openjd-expr 0.4.0). This section +# used to read "sqi is right: string(list) charges per element; reference is +# flat 1", and every word of that was true against 0.3.0 -- sqi charged MORE +# than the reference, whose flat 1 ignored list length entirely. openjd-rs#336 +# changed the reference's counting and the direction INVERTED without a single +# entry going stale: the three-direction rule cannot see a reason going false +# while the divergence itself persists. That is the second occurrence of the +# trap the upstream tracker's section 6a recorded on the previous bump -- read +# the DELTAS, not just the pass line. +# +# What the reference charges now, measured directly at openjd-model 0.11.5: +# +# string([]) 2 = 1 call + 0 elements + 1 result +# string([1]) 3 = 1 + 1 + 1 +# string([1, 2, 3, 4, 5]) 7 = 1 + 5 + 1 +# string([[1], [2]]) 6 = 1 + (2 outer + 2 inner) + 1 +# string([[1,2,3], [4,5,6]]) 10 = 1 + (2 outer + 6 inner) + 1 +# +# -- one per element counted RECURSIVELY, plus a unit for the RESULT string. +# sqi charges rule 1 (1) plus ArgElements, the argument list's own length, not +# recursed, and nothing for the result. So sqi is now BELOW the reference on +# every entry in this section, where it used to be above. +# +# TWO divergences now, adjudicated separately: +# +# 1. The RESULT unit -- sqi is right to omit it. Rule 3 charges "when a +# function processes a string or path VALUE", which is about what goes in, +# not what comes out. This is the identical ruling the repr_* section +# below already records, made there on the 0.11.4 bump when openjd-rs#284 +# added result units to those five functions. One rule, two sections. +# +# 2. The RECURSION -- sqi UNDER-charges, and this half is a known gap in sqi +# rather than a ruling in our favour. sqi's own reading of rule 2 is that +# the general "iterates through every element of a list" clause applies +# here because writeJSONValue walks every element; that walk descends into +# nested lists, so the same reading charges their elements too. Closing it +# means giving Cost.ArgElements a recursive variant, and ArgElements is a +# shared primitive whose other rows (the five repr_*, join, and the rest) +# each carry their own separately measured reference behavior -- changing +# it inside a pin bump would move all of them at once. Deferred, and +# recorded here rather than silently kept. Under-charging is the SAFE +# direction: it can only accept an expression a limit would otherwise +# reject, never reject a legitimate one. +# +# Rule 2's enumeration is introduced with "such as", so the general clause is +# not closed by the named list, and writeJSONValue really does walk every +# element of the list argument to render it -- confirmed against the spec text +# at third_party/openjd-specifications/wiki/2026-02-Expression-Language.md, +# section 1.3.10 rule 2. "string(flatten(...))" combines this with flatten()'s +# own charge, which the reference already matches exactly (flatten is not +# itself in this baseline). + +# The EMPTY list: no elements for the two sides to disagree about, so the +# whole divergence here is the reference's result unit (see section header). +string | list[nulltype] :: string([]) + +# go=2 ref=3, one element: the difference is the reference's result unit alone +# (see section header). +string | list[string] :: string(['ac&d']) + +# go=3 ref=4, two elements: the difference is the reference's result unit alone +# (see section header). string | list[int] :: string([1, 2]) -# string(list)'s per-element ArgElements charge; reference is flat 1 (see section header). +# go=3 ref=4: same shape as the entry above, string elements (see section +# header). string | list[string] :: string(["a", "b"]) -# string(list)'s per-element ArgElements charge; reference is flat 1 (see section header). -string | list[list[int]] :: string([[1], [2]]) - -# string(list)'s per-element ArgElements charge; reference is flat 1 (see section header). +# go=3 ref=4: same shape again, bool elements (see section header). string | list[bool] :: string([true, false]) -# string(list)'s per-element ArgElements charge; reference is flat 1 (see section header). -string | list[string] | list[list[string]] :: string(flatten([["-e", "A=1"], ["-e", "B=2"]])) +# go=3 ref=6, the only entry that shows BOTH halves at once: one result unit +# plus the two INNER elements sqi does not recurse into (see section header, +# divergence 2 -- the known gap). +string | list[list[int]] :: string([[1], [2]]) -# string(list)'s per-element ArgElements charge; reference is flat 1 (see section header). -string | list[string] :: string(['ac&d']) +# go=12 ref=13: flatten()'s own charge is common ground and matches exactly; +# the whole remaining difference is string(list)'s result unit, and there is no +# recursion term because flatten's result is already flat (see section header). +string | list[string] | list[list[string]] :: string(flatten([["-e", "A=1"], ["-e", "B=2"]])) # ── the repr_* family: sqi charges its ARGUMENT, the reference its RESULT ─── # diff --git a/test/oracle/baseline.txt b/test/oracle/baseline.txt index 5f8d42b0..f5ae39fc 100644 --- a/test/oracle/baseline.txt +++ b/test/oracle/baseline.txt @@ -220,19 +220,6 @@ bool | string :: isalnum('٣') # title's word boundary on the same rune: see the explanation above. string :: title('²x y') -# ── the reference is wrong: a negative maxsplit discards the string ────────── -# -# RFC 0006 documents maxsplit as "at most maxsplit times" and defines nothing -# below zero, so unlimited — Python's rule — is the only reading with support. -# The reference returns an EMPTY LIST, discarding the input entirely. Reported -# in expr-tracker.md's upstream ledger. - -# Negative maxsplit: see the shared explanation above. -list[string] | string | int :: split('a,b,c', ',', -1) - -# Negative maxsplit, rsplit direction: see the shared explanation above. -list[string] | string | int :: rsplit('a b c', ' ', -1) - # ── sqi is right: the intersection rule rejects what one engine alone accepts ─ # # RFC 0006 states the accepted regular-expression syntax is the INTERSECTION of @@ -447,28 +434,6 @@ string :: repr_sh("it's") # shared explanation above. string :: repr_py("it's") -# ── sqi is right: re_split's negative maxsplit discards the string, too ────── -# -# A DIFFERENT rule from the split()/rsplit() negative-maxsplit entries above, -# reached by the same reference bug — do not fold this into that shared -# explanation, and do not "fix" one function to match the other. C2's -# split()/rsplit() follow str.split's own convention, where a negative -# maxsplit means UNLIMITED; re_split follows RFC 0006's regex-function text -# ("at most maxsplit times", nothing defined below zero) and Python's -# re.split, where a negative maxsplit means NO split — the whole string comes -# back as its own one-element list. The reference returns an EMPTY LIST for -# re_split too, discarding the input entirely, which is wrong under either -# reading of "at most maxsplit times": zero matches its rule (the string is -# an available zero-split outcome, not a null result), and it does not match -# str.split's convention either, since re_split does not follow that one. -# Reported in expr-tracker.md's upstream ledger alongside the split()/ -# rsplit() case. - -# Negative maxsplit, re_split direction: sqi returns the string unsplit -# ("['a1b2c']") per Python's re.split rule; the reference discards it -# ("[]"). See the shared explanation above. -list[string] | string | int :: re_split('a1b2c', '\d', -1) - # ═══ C4: the path family ═════════════════════════════════════════════════════ # # Everything below is a path case. The reference implementation's path family @@ -497,21 +462,6 @@ path | list[string] :: path(['//', 'a']) # explanation above. bool | path | list[string] | string :: path(path('//a/b').parts) == path('//a/b') -# ── sqi is right: path([]) is Path(), which is "." ─────────────────────────── -# -# Two specification rules meet here. RFC 0006 line 755 defines the list row as -# "Construct path from components (like Path(*parts) in Python)", and Path() -# with no components is PurePosixPath("."). Expression-Language line 846 makes -# list[nulltype] implicitly convertible to list[T] for any T, which is what -# lets the empty literal reach that row at all — the same rule that makes -# [].join(sep) resolve, already relied on by C2's join. The reference has no -# list[nulltype] row and rejects the call outright. - -# The empty-list constructor: sqi returns "." per Path(*[]), the reference -# reports "No matching signature for path(list[nulltype])". See the -# explanation above. -path | list[nulltype] :: path([]) - # ── sqi is right: a URI keeps the empty component a trailing slash leaves ──── # # Expression-Language section 1.2.1 says a URI's path portion is parsed "without diff --git a/third_party/openjd-specifications b/third_party/openjd-specifications index be0aefb8..1e4d49e9 160000 --- a/third_party/openjd-specifications +++ b/third_party/openjd-specifications @@ -1 +1 @@ -Subproject commit be0aefb83e4d5b15aa89ed81f67424e85908282d +Subproject commit 1e4d49e937a80b47d33b4fa32e7f60ec565f7592