diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a28e6298..559e4680 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,14 +140,29 @@ jobs: run: make vet # Uses the official golangci-lint GitHub Action, which handles binary - # download, caching, and version resolution automatically. - # Configuration is read from .golangci.yml at the repo root. + # download and caching. Configuration is read from .golangci.yml at the + # repo root. + # + # version is PINNED on purpose. Left unset the action resolves "latest", + # so every new golangci-lint release lands on unrelated PRs as a red CI + # run: v2.13.1 enabled the modernize/errorsastype analyzer and broke a + # branch that had touched none of the reported code. Bump this + # deliberately, in its own commit, with the fallout fixed alongside it. + # + # 2.13.1 is also a FLOOR, not just a pin: .golangci.yml excludes + # errors.AsType from errcheck by function name, and errcheck before + # 2.13.1 cannot resolve a generic function's name, so on an older + # golangci-lint that exclusion silently fails to match and every + # errors.AsType call site is reported. Keep docs/development.md's + # required-version note in step with this value. + # # Explicit path patterns mirror the Makefile LINT_PKGS variable: they # exclude web/node_modules/ (third-party JS packages that happen to # contain Go code and are not part of the sqi codebase). - name: golangci-lint uses: golangci/golangci-lint-action@v9 with: + version: v2.13.1 args: --timeout=5m ./cmd/... ./internal/... ./pkg/... ./test/... ./web # The internal/openjd/expr path-engine differentials shell out to python3 diff --git a/.golangci.yml b/.golangci.yml index f8630676..e4d65499 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -113,6 +113,19 @@ linters: errcheck: check-type-assertions: true # catch unchecked x.(T) check-blank: true + exclude-functions: + # errors.AsType[E](err) returns (E, bool). The first result is the + # extracted error value, not a failure signal — the bool is. Discarding + # it as `_, ok := errors.AsType[...](err)` is the idiomatic form when + # only the match matters, but check-blank sees a blank-assigned error + # and reports it. Exclude the function rather than turning check-blank + # off, so a genuine `_ = f()` is still caught. + # + # Requires golangci-lint >= 2.13.1: earlier errcheck cannot resolve a + # generic function's name (it reports "Error return value is not + # checked" with no name), so this entry never matches and every + # errors.AsType call site is flagged. + - errors.AsType # gosec: suppress a few low-signal rules gosec: diff --git a/Makefile b/Makefile index 8d5495d8..76fe390c 100644 --- a/Makefile +++ b/Makefile @@ -214,7 +214,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.1 +OPENJD_MODEL_VERSION ?= 0.11.4 ORACLE_VENV := .venv-oracle .PHONY: expr-oracle-venv @@ -237,7 +237,24 @@ test-expr-oracle: ## Differential-test the EXPR evaluator against the OpenJD ref $(MAKE) --no-print-directory expr-oracle-venv || \ { echo "could not install the reference implementation — skipping the expression oracle"; exit 0; }; \ fi - go test $(TEST_FLAGS) -tags oracle -run 'TestExprOracle' -v -timeout 5m ./test/oracle/ +# An EXISTING venv is not evidence of the RIGHT venv: the guard above only +# creates one when it is missing, so before this check a raised +# OPENJD_MODEL_VERSION left the suite grading against the previous reference +# with nothing red to say so. Reinstall on mismatch, and let the test itself +# assert the version it actually spoke to (SQI_EXPR_ORACLE_EXPECT_VERSION) so +# the guarantee survives a hand-run `go test` too. Skipped entirely when +# SQI_EXPR_ORACLE_PYTHON points the harness at an interpreter we do not own. + @if [ -x "$(ORACLE_VENV)/bin/python3" ] && [ -z "$$SQI_EXPR_ORACLE_PYTHON" ]; then \ + have=$$($(ORACLE_VENV)/bin/python3 -c \ + 'import importlib.metadata as m; print(m.version("openjd-model"))' 2>/dev/null); \ + if [ "$$have" != "$(OPENJD_MODEL_VERSION)" ]; then \ + echo "$(ORACLE_VENV) has openjd-model $$have, pin is $(OPENJD_MODEL_VERSION) — reinstalling"; \ + $(MAKE) --no-print-directory expr-oracle-venv || \ + { echo "could not install the pinned reference implementation"; exit 1; }; \ + fi; \ + fi + SQI_EXPR_ORACLE_EXPECT_VERSION=$$([ -n "$$SQI_EXPR_ORACLE_PYTHON" ] || echo $(OPENJD_MODEL_VERSION)) \ + go test $(TEST_FLAGS) -tags oracle -run 'TestExprOracle' -v -timeout 5m ./test/oracle/ # Validates the PUBLISHED preset library against the validator in this working # tree. It exists because a validator change can silently invalidate content diff --git a/docs/development.md b/docs/development.md index 5b5cb243..f0e9b91b 100644 --- a/docs/development.md +++ b/docs/development.md @@ -14,7 +14,7 @@ guides for extending the worker. | Node.js ≥ 24 with npm ≥ 11 (see `.nvmrc` and `web/package.json` `engines`) | Build the web UI bundle embedded in `sqi-server` (`make build` runs it) | [nodejs.org](https://nodejs.org/) or `nvm use` | | `gofumpt` | Stricter formatter (superset of `gofmt`) | `go install mvdan.cc/gofumpt@latest` | | `goimports` | Import organizer | `go install golang.org/x/tools/cmd/goimports@latest` | -| `golangci-lint` | Linter suite | [golangci-lint.run/usage/install](https://golangci-lint.run/usage/install/) | +| `golangci-lint` ≥ 2.13.1 (CI pins this exact version; see below) | Linter suite | [golangci-lint.run/usage/install](https://golangci-lint.run/usage/install/) | | `lefthook` | Git hook runner | `go install github.com/evilmartians/lefthook@latest` | | `pkgsite` | Local pkg.go.dev docs server | `go install golang.org/x/pkgsite/cmd/pkgsite@latest` | | Docker (optional) | Build and run the container image; also runs the real-directory LDAP tests (`make test-ldap`), the real-provider SSO tests (`make test-oidc`), and the real-root run-as-user isolation tests (`make test-isolation`), all of which skip cleanly without it | [docs.docker.com](https://docs.docker.com/get-docker/), or `brew install colima docker && colima start` | @@ -22,6 +22,14 @@ guides for extending the worker. `gofumpt`, `goimports`, and `golangci-lint` are required at commit time via pre-commit hooks. Install them before running `make hooks`. +**`golangci-lint` 2.13.1 is a hard floor, not a suggestion.** `.golangci.yml` +excludes `errors.AsType` from `errcheck` by function name, and `errcheck` before +2.13.1 cannot resolve a *generic* function's name — it reports `Error return +value is not checked` with no name at all, so the exclusion never matches and +every `errors.AsType` call site in the repo is reported. On an older +golangci-lint `make lint` fails on code CI considers clean. CI pins the same +version in `.github/workflows/ci.yml`; keep the two in step when bumping. + --- ## First-time setup @@ -715,6 +723,10 @@ db := t.TempDir() + "/test.db" [official instructions](https://golangci-lint.run/usage/install/). The `go install golangci-lint` method is not supported by the project. +**`make lint` reports `Error return value is not checked` on `errors.AsType`** +— your `golangci-lint` predates 2.13.1. Check with `golangci-lint version` and +upgrade; see the version floor noted under [Prerequisites](#prerequisites). + **`gofumpt` or `goimports` not found after installing** — ensure `$(go env GOPATH)/bin` is on your `$PATH`: ```sh diff --git a/internal/api/ws.go b/internal/api/ws.go index 6d84f910..907f846a 100644 --- a/internal/api/ws.go +++ b/internal/api/ws.go @@ -381,8 +381,7 @@ func (wc *wsConn) readLoop(ctx context.Context) { // logReadError logs a WebSocket read error at the appropriate level. func (wc *wsConn) logReadError(ctx context.Context, err error) { - var closeErr websocket.CloseError - if errors.As(err, &closeErr) { + if closeErr, ok := errors.AsType[websocket.CloseError](err); ok { wc.logger.DebugContext( ctx, "ws: client closed connection", slog.Int("code", int(closeErr.Code)), diff --git a/internal/openjd/deadline_test.go b/internal/openjd/deadline_test.go index 6b59d13c..67ae4495 100644 --- a/internal/openjd/deadline_test.go +++ b/internal/openjd/deadline_test.go @@ -401,8 +401,7 @@ func TestCheckExpressionsAtSubmit_DeadlineIsNotASubmitValidationError(t *testing if !errors.Is(serr, expr.ErrDeadlineExceeded) { t.Fatalf("error = %v, want it to wrap expr.ErrDeadlineExceeded", serr) } - var sve *SubmitValidationError - if errors.As(serr, &sve) { + if _, ok := errors.AsType[*SubmitValidationError](serr); ok { t.Errorf("error = %v, want NOT a *SubmitValidationError: a wall-clock stop "+ "is not the submitter's fault", serr) } @@ -587,8 +586,7 @@ func TestSubmit_DeadlineIsNotASubmitValidationError(t *testing.T) { if !errors.Is(err, expr.ErrDeadlineExceeded) { t.Fatalf("error = %v, want it to wrap expr.ErrDeadlineExceeded", err) } - var sve *SubmitValidationError - if errors.As(err, &sve) { + if _, ok := errors.AsType[*SubmitValidationError](err); ok { t.Errorf("error = %v, want NOT a *SubmitValidationError: a wall-clock stop is "+ "the server giving up, not the submitter's fault, and that type is what "+ "internal/api turns into a 4xx", err) diff --git a/internal/openjd/expr/coerce.go b/internal/openjd/expr/coerce.go index ee5add91..4c67c9e8 100644 --- a/internal/openjd/expr/coerce.go +++ b/internal/openjd/expr/coerce.go @@ -5,19 +5,40 @@ package expr import ( "errors" "fmt" + "sort" "strconv" ) -// This file implements section 1.2.3, implicit type coercion. +// This file implements section 1.2.3, implicit type coercion, as RFC 0005 +// restated it in openjd-specifications#175 (merged 2026-08-19). // -// The spec's rules are phrased against what the TARGET does not include — "int -// to float when the target types do not include int" — so every rule below asks -// includes() about the target rather than examining the source alone. +// TWO STEPS, IN ORDER. Satisfaction asks whether the result's type already +// satisfies the target, and if so the value is used UNCHANGED and nothing is +// converted (satisfies). Otherwise conversion walks the target's DESTINATIONS +// in an order fixed by the result's own type, first success wins, and a +// destination that fails is not an error so long as a later one succeeds +// (scalarDestinations, orderedListDestinations). Non-list destinations precede +// list ones, always. // -// Two entry points, because two callers need different things. Shape matching -// (shape.go) asks only whether a type COULD reach a declared parameter type: it -// must not convert anything before a shape is chosen, and on the unresolved path -// there is no value to convert. coerce() performs the conversion afterward. +// The older reading is still visible in this file's shape and should not be +// mistaken for the current one: it phrased every rule against what the target +// does NOT include ("int to float when the target types do not include int"), +// which is how a satisfaction check looks when it is spelled out one rule at a +// time. It also had no answer when a target offered two candidates of the same +// kind, and gave up rather than choosing -- singleScalarTarget, still used by +// the promotion path below, is that giving-up. +// +// THREE PREDICATES, NOT TWO, and the difference matters: +// +// - coerce() converts a VALUE against a target type. +// - coercibleToTarget() is coerce()'s type-level twin, for the unresolved +// path where there is no value to convert. +// - coercible() belongs to a DIFFERENT MECHANISM: the coercion that +// resolves a function call and promotes 1 to 1.0 in "1 + 2.0" (promotable, +// shape.go). RFC 0005 says in as many words that #175 does not touch it, +// so it keeps the older rules deliberately. Routing call dispatch through +// the destination table would make it depend on a target the caller never +// supplied. // includes reports whether target admits a value whose type code is c, looking // through union members and through an unresolved constraint. @@ -40,8 +61,17 @@ func includes(target Type, c Code) bool { } // coercible reports whether a value of type from can be implicitly converted to -// the target type to, per section 1.2.3. It answers at the type level only, and -// performs nothing. +// the type to, under the rules RFC 0005 applies while RESOLVING A FUNCTION CALL +// -- the mechanism that promotes 1 to 1.0 in "1 + 2.0". It answers at the type +// level only, and performs nothing. +// +// It is NOT the predicate behind target-type coercion; that is +// coercibleToTarget, and since openjd-specifications#175 the two genuinely +// differ. The clearest case: coercible(int, "float | int") is FALSE, because +// nothing needs converting, while coercibleToTarget says TRUE because the +// target admits an int outright. Both answers are right for their own question. +// Its only callers are promotable() and shape matching; do not reach for it +// from a target-type path. func coercible(from, to Type) bool { if from.Equal(to) || to.Code == CodeAny { return true @@ -243,6 +273,392 @@ func scalarCoercible(from, to Code) bool { return false } +// satisfies reports whether a result of type from already SATISFIES target, in +// RFC 0005's sense: the value is used unchanged and no conversion is attempted. +// +// The relation is DIRECTIONAL and the specification says so explicitly: an int +// satisfies any, and any does not satisfy an int. It is deliberately not the +// symmetric matching that binds type variables during signature matching, which +// would accept a list[T1] target by binding T1 and then discarding the binding. +// Keep the two apart; shape.go owns the symmetric one. +func satisfies(from, to Type) bool { + switch to.Code { + case CodeAny: + return true + case CodeUnresolved: + c, ok := unresolvedConstraint(to) + return ok && satisfies(from, c) + case CodeUnion: + for _, m := range to.Params { + if satisfies(from, m) { + return true + } + } + return false + } + if fromElem, ok := listParam(from); ok { + if toElem, ok := listParam(to); ok { + return satisfies(fromElem, toElem) + } + return false + } + return from.Equal(to) +} + +// listParam returns t's element type when t is literally a list, without +// looking through unions or unresolved constraints. +// +// listElem() does look through both, and answers a different question: "which +// single list type does this TARGET offer". Satisfaction needs the plain one -- +// a union is decomposed by satisfies() itself, one member at a time, so a +// union-aware accessor here would answer for the wrong type. +func listParam(t Type) (Type, bool) { + if t.Code == CodeList && len(t.Params) == 1 { + return t.Params[0], true + } + return Type{}, false +} + +// scalarDestinations is RFC 0005's destination-order table, restated by +// openjd-specifications#175. Two principles fix the order, and both matter: +// a value stays within its own kind before it becomes text, and a conversion +// that CAN FAIL is attempted before one that always succeeds -- a universal +// fallback tried first would make every destination after it unreachable. +// +// The range_expr row's list[int] destination is not here: non-list destinations +// come before list ones, so it is attempted by coerce() after this table is +// exhausted, which is why a target offering both string and list[int] gets the +// string. +func scalarDestinations(from Code) []Code { + switch from { + case CodeBool: + return []Code{CodeString} + case CodeInt: + return []Code{CodeFloat, CodeString} + case CodeFloat: + return []Code{CodeInt, CodeString} + case CodeString: + // int before float because every string that parses as an int also + // parses as a float; bool and range_expr are selective parses; path + // last because every string is a valid path. + return []Code{CodeInt, CodeFloat, CodeBool, CodeRangeExpr, CodePath} + case CodePath: + return []Code{CodeString} + case CodeRangeExpr: + return []Code{CodeString} + } + return nil +} + +// convertScalar performs one destination's conversion, or reports that this +// destination does not take the value. A failure here is not fatal: coerce() +// tries the next destination, and only an exhausted list is an error. +func convertScalar(v Value, to Code) (Value, error) { + switch to { + case CodeString: + return String(v.String()), nil + case CodePath: + return Value{Type: TPath, s: v.AsStr()}, nil + case CodeInt: + return toInt(v) + case CodeFloat: + return toFloat(v) + case CodeBool: + if v.Type.Code != CodeString { + return Value{}, fmt.Errorf("%s %w to bool", v.Type, errNotCoercible) + } + return boolFromString(v.AsStr()) + case CodeRangeExpr: + if v.Type.Code != CodeString { + return Value{}, fmt.Errorf("%s %w to range_expr", v.Type, errNotCoercible) + } + return RangeExpr(v.AsStr()) + } + return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, to) +} + +// coerceByDestination runs step 2 of RFC 0005's coercion on a scalar result: +// each destination the target offers, in the table's order, first success wins. +// A destination that fails is not an error so long as a later one succeeds. +// +// The three results are distinct and the caller needs all three. ok reports a +// conversion; a nil error with ok false means the target offered this value no +// scalar destination at all, so another rule (the list ones) may still apply; a +// non-nil error means every offered destination was attempted and failed, and +// that error is the LAST one's, which is the most specific thing there is to +// say. Returning a generic "cannot be coerced" there would throw away +// boolFromString's own message for a string that is not a bool spelling. +func coerceByDestination(v Value, target Type) (Value, bool, error) { + var lastErr error + for _, to := range scalarDestinations(v.Type.Code) { + // nulltype, type variables, noreturn and unresolved contribute no + // destination; includes() answers this for the scalar codes in the + // table because none of them carries type parameters. + if !includes(target, to) { + continue + } + out, err := convertScalar(v, to) + if err == nil { + return out, true, nil + } + lastErr = err + } + return Value{}, false, lastErr +} + +// coercibleToTarget is the TYPE-level twin of coerce(): it answers whether a +// result of type from could reach target under RFC 0005's two steps, without +// converting anything and without a value to convert. +// +// It is deliberately NOT coercible(). The specification keeps two mechanisms +// apart and #175 changed only one of them: target-type coercion (this one, the +// one a template FIELD applies to an expression's result) versus the coercion +// that resolves a function call and promotes 1 to 1.0 in "1 + 2.0" (coercible, +// promotable, shape.go). Routing call promotion through the destination table +// would make dispatch depend on a target the caller never supplied, so the two +// predicates stay separate even though they overlap heavily. +func coercibleToTarget(from, to Type) bool { + if satisfies(from, to) { + return true + } + // Unresolved is transparent on both sides: what a placeholder can reach is + // decided by its constraint, and a target that is itself unresolved + // constrains no more tightly than its constraint does. + if c, ok := unresolvedConstraint(from); ok { + return coercibleToTarget(c, to) + } + if c, ok := unresolvedConstraint(to); ok { + return coercibleToTarget(from, c) + } + // A union SOURCE is usable only where every member would be: it is some one + // of its members, decided at runtime, so a target that would reject any one + // of them cannot safely receive it. + if from.Code == CodeUnion { + for _, m := range from.Params { + if !coercibleToTarget(m, to) { + return false + } + } + return true + } + // null reaches only a target that already admits it; no conversion produces + // null, so nulltype is never a destination. + if from.Code == CodeNull { + return includes(to, CodeNull) + } + for _, d := range scalarDestinations(from.Code) { + if includes(to, d) { + return true + } + } + return hasListDestination(from, to) +} + +// hasListDestination is coercibleToTarget's list half, split out to keep each +// function inside the complexity budget. It answers the same question one level +// down: does the target offer a list destination this type could reach? +func hasListDestination(from, to Type) bool { + // range_expr -> list[int] is accepted exactly when a list[int] value would + // satisfy the destination, and it is the only list destination a non-list + // source has. + if from.Code == CodeRangeExpr { + for _, elem := range listDestinations(to) { + if satisfies(TInt, elem) { + return true + } + } + return false + } + srcElem, ok := listParam(from) + if !ok { + return false + } + for _, elem := range listDestinations(to) { + // list[nulltype] -> list[T] for any T: the empty list literal has no + // element that could fail, so every list destination takes it. + if srcElem.Code == CodeNull || coercibleToTarget(srcElem, elem) { + return true + } + } + return false +} + +// narrowedUnionConstraint is narrowedConstraint's existential half: each member +// that can reach the target contributes what it would become, and the ones that +// cannot are discarded rather than failing the whole coercion. Coercing +// unresolved[int | string] to an int target therefore yields unresolved[int]. +func narrowedUnionConstraint(from, target Type) (Type, bool) { + var out []Type + for _, m := range from.Params { + if satisfies(m, target) { + out = append(out, m) + continue + } + if n, ok := narrowedConstraint(m, target); ok { + out = append(out, n) + } + } + if len(out) == 0 { + return Type{}, false + } + return UnionOf(out...), true +} + +// narrowedConstraint is the type-level half of RFC 0005's "Coercion of +// Unresolved Values": what a PLACEHOLDER's constraint becomes when it is +// coerced, given that there is no payload to decide which destination wins. +// +// It narrows to the UNION of every destination with a type-level rule rather +// than betting on one, because the invariant the specification states has two +// halves and a guess breaks the second: the narrowed constraint must satisfy +// the target, AND the concrete result's type must satisfy the narrowed +// constraint. unresolved[float] against "int | string" narrows to +// unresolved[int | string] -- a 3.0 payload takes float->int and a 3.5 payload +// fails it and falls through to string, and both outcomes lie inside that. +// +// A union CONSTRAINT is existential, and this is the one place where a union on +// the source side does not mean "every member must clear the bar": a constraint +// is a set of possibilities, not a value, so members that cannot coerce are +// discarded rather than failing the whole thing. coercibleToTarget's own +// union-source rule is the opposite for the opposite reason -- a union-typed +// VALUE is some one of its members, chosen at runtime, so a target that would +// reject any one of them cannot safely receive it. +func narrowedConstraint(from, target Type) (Type, bool) { + if from.Code == CodeUnion { + return narrowedUnionConstraint(from, target) + } + var dests []Type + for _, d := range scalarDestinations(from.Code) { + if includes(target, d) { + dests = append(dests, Type{Code: d}) + } + } + if from.Code == CodeRangeExpr { + for _, elem := range listDestinations(target) { + if satisfies(TInt, elem) { + // Materializing a range only ever produces a list[int], so the + // constraint narrows to that and not to the destination. + dests = append(dests, ListOf(TInt)) + break + } + } + } + if srcElem, ok := listParam(from); ok { + for _, elem := range orderedListDestinations(srcElem, target) { + if srcElem.Code == CodeNull || coercibleToTarget(srcElem, elem) { + dests = append(dests, ListOf(elem)) + } + } + } + if len(dests) == 0 { + return Type{}, false + } + return UnionOf(dests...), true +} + +// listDestinations returns the element type of every list the target offers, in +// the target's own normalized member order -- which RFC 0005 defines (type +// parameters sorted alphabetically, nulltype last), so it is a stated order and +// not this function's choice. +func listDestinations(target Type) []Type { + switch target.Code { + case CodeUnresolved: + if c, ok := unresolvedConstraint(target); ok { + return listDestinations(c) + } + case CodeUnion: + var out []Type + for _, m := range target.Params { + out = append(out, listDestinations(m)...) + } + return out + case CodeList: + if len(target.Params) == 1 { + return []Type{target.Params[0]} + } + } + return nil +} + +// orderedListDestinations is the list[S] row of RFC 0005's destination table: +// "list destinations in S's order, applied to their element types", so +// list[float] against "list[int] | list[string]" attempts list[int] first. +// +// The sort is STABLE and unranked destinations keep their normalized position, +// which is what makes the empty list work without a second rule: list[nulltype] +// has no destination order of its own (scalarDestinations(nulltype) is empty), +// so every candidate is unranked and the union's normalized member order stands +// -- exactly what the specification says the empty list's nominal element type +// follows. +func orderedListDestinations(srcElem, target Type) []Type { + cands := listDestinations(target) + order := scalarDestinations(srcElem.Code) + rank := func(t Type) int { + for i, c := range order { + if t.Code == c { + return i + } + } + return len(order) + } + sort.SliceStable(cands, func(i, j int) bool { return rank(cands[i]) < rank(cands[j]) }) + return cands +} + +// convertListTo performs one list destination's elementwise conversion. A +// failure is that destination's, not the coercion's: the caller tries the next. +func convertListTo(v Value, elem Type) (Value, error) { + elems := v.AsList() + if err := checkElementCount(len(elems)); err != nil { + return Value{}, err + } + out := make([]Value, len(elems)) + for i, e := range elems { + converted, err := coerce(e, elem) + if err != nil { + return Value{}, fmt.Errorf("element %d: %w", i, err) + } + out[i] = converted + } + return List(elem, out), nil +} + +// coerceByListDestination runs the list destinations, after every scalar one has +// been tried and failed. The three results mean what coerceByDestination's do. +func coerceByListDestination(v Value, target Type) (Value, bool, error) { + // range_expr -> list[int] is the one list conversion from a non-list source, + // and it is accepted exactly when a list[int] value would satisfy the + // destination: list[int], list[any] and list[int | string], but not + // list[float] or list[string]. Implicit rules do not chain, so the + // materialized list is not widened element-wise afterwards. + if v.Type.Code == CodeRangeExpr { + for _, elem := range listDestinations(target) { + if !satisfies(TInt, elem) { + continue + } + ints, err := rangeInts(v) + if err != nil { + return Value{}, false, err + } + return List(TInt, intValues(ints)), true, nil + } + return Value{}, false, nil + } + srcElem, ok := listParam(v.Type) + if !ok { + return Value{}, false, nil + } + var lastErr error + for _, elem := range orderedListDestinations(srcElem, target) { + out, err := convertListTo(v, elem) + if err == nil { + return out, true, nil + } + lastErr = err + } + return Value{}, false, lastErr +} + // errNotCoercible is the sentinel behind every "cannot be coerced" report, so a // caller can distinguish an inapplicable conversion from a conversion that // applied and then failed on the value. @@ -285,12 +701,19 @@ func Coerce(v Value, target Type) (Value, error) { return coerce(v, target) } // plain error and the evaluator attaches the offset of the construct that // failed. func coerce(v Value, target Type) (Value, error) { - if v.Type.Equal(target) || target.Code == CodeAny { - return v, nil - } if v.IsUnresolved() { return coerceUnresolved(v, target) } + // Step 1, satisfaction: a result whose type the target already admits is + // used UNCHANGED, and step 2 is never reached for it. This subsumes three + // carve-outs the older reading needed separately -- the Equal/any early + // return, directUnionMember, and coerceList's "the target admits the list + // without naming an element type" branch -- and it fixes what they got + // wrong between them: a list[int] against a list[any] target kept its own + // list[int] type here only by accident of which branch caught it first. + if satisfies(v.Type, target) { + return v, nil + } if c, ok := unresolvedConstraint(target); ok { return coerce(v, c) } @@ -312,189 +735,72 @@ func coerce(v Value, target Type) (Value, error) { // and listElem reports a union naming two DIFFERENT list types as not // list-shaped at all, which is why "[1.0, 2.0]" was rejected by the target // "list[float] | list[int]" that literally names its type. - if directUnionMember(target, v.Type) { - return v, nil + // Step 2, conversion, for a scalar result: the destination table, in order. + // It runs before the list branch because RFC 0005 puts every non-list + // destination ahead of every list one -- which is what makes a range_expr + // against a target offering both string and list[int] become the string, + // and the list[int] destination unreachable there. + out, ok, convErr := coerceByDestination(v, target) + if ok { + return out, nil + } + if convErr != nil { + return Value{}, convErr + } + // The list destinations, attempted only after every scalar one, because + // RFC 0005 puts the whole non-list group first. The gate is on the SOURCE: + // a plain scalar reaching a target that merely CONTAINS a list type -- + // "T? | list[T]", which section 1.3.2 makes the target of every template + // "args" item -- has no list conversion to perform, and sending it here + // used to reach AsList() and PANIC. range_expr is the one non-list source + // with a list destination. + out, ok, listErr := coerceByListDestination(v, target) + if ok { + return out, nil + } + if listErr != nil { + return Value{}, listErr } - // The three list rules of section 1.2.3: elementwise conversion, the empty - // list, and range_expr -> list[int]. - // - // The gate is on the SOURCE, not on either side. A list-shaped TARGET alone - // is not enough: a plain scalar reaching a target that merely CONTAINS a list - // type — "T? | list[T]", which section 1.3.2 makes the target of every - // template "args" item, so it is the first shape a caller will construct — - // has no list conversion to perform at all and belongs on the scalar path - // below. Sending it here instead reached coerceList's AsList() and PANICKED. - // range_expr is the one non-list source with a list conversion, and only when - // the target really is list-shaped; a range_expr against a string target is - // section 1.2.3's range_expr -> string rule, which is the scalar path's. - _, srcIsList := listElem(v.Type) - dstElem, dstIsList := listElem(target) - if srcIsList || (v.Type.Code == CodeRangeExpr && dstIsList) { - if !coercible(v.Type, target) { - return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) - } - return coerceList(v, target, dstElem, dstIsList) - } - // The general applicability check, with a direct-membership carve-out: - // coercible alone is too strict here because it deliberately reports false - // for a scalar that is already a direct (if ambiguous) member of a union - // target — that case needs no conversion at all, which is exactly why - // coercible refuses it. targetScalarCode alone is too permissive: its - // final catch-all resolves "which code to become" without ever asking - // whether that direction is legal, which let bool/int/float -> path reach - // AsStr() and panic. This keeps every real conversion validated by the - // already-vetted coercible/scalarCoercible pair. - if !includes(target, v.Type.Code) && !coercible(v.Type, target) { - return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) - } - return coerceScalar(v, target) + return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) } // coerceUnresolved coerces a PLACEHOLDER, which has no value to convert. // Coercing one narrows its constraint and it stays a placeholder — which is // what lets a type check proceed through a coercion boundary. func coerceUnresolved(v Value, target Type) (Value, error) { - // The direct-membership carve-out, the exact counterpart of coerce()'s own - // (directUnionMember, below), and missing here until EXPR sub-project - // E4b's whole-branch review. coercible answers "does a CONVERSION apply" - // and is pinned false for a type a union target already admits unchanged, - // so asking it alone made a PLACEHOLDER strictly harder to coerce than a - // concrete value of the very same type: coerce() reaches - // directUnionMember before it ever consults coercible, and a concrete - // string against "int | list[int] | range_expr | string" therefore passed - // while unresolved[string] against the identical target was rejected. - // - // Phase 1 is exactly where that asymmetry bites: every job parameter is an - // unresolved placeholder at template-upload time (symbolsFor, exprcheck.go), - // so section 1.3.12's four-member INT range union — the first target in - // this codebase with more than one scalar member — rejected - // range: "{{Param.Frames}}" at upload and then accepted the identical - // expression at submit once Frames was bound. Reported at review as - // "declaring EXPR removes base-spec capability at this field". + // Step 1 applies to a PLACEHOLDER too, and RFC 0005 says what it leaves + // behind: "Satisfaction leaves the value alone, so its constraint keeps the + // source type" -- an unresolved[list[int]] against a list[any] target stays + // unresolved[list[int]] rather than widening, matching the concrete + // list[int] that would be returned unchanged. // - // Narrowing to the whole union (below) rather than to the member is the - // same thing every other success here does: a placeholder carries a - // constraint, not a decision. - if c, ok := unresolvedConstraint(v.Type); ok && directUnionMember(target, c) { - return Unresolved(target), nil - } - if !coercible(v.Type, target) { - return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) + // This subsumes the directUnionMember carve-out that stood here, and it is + // the same asymmetry that carve-out was added (in E4b's whole-branch review) + // to close: coerce() reaches satisfaction before it ever consults coercible, + // so asking coercible alone made a placeholder strictly harder to coerce + // than a concrete value of the very same type. Phase 1 is where that bites, + // because every job parameter is a placeholder at template-upload time, and + // section 1.3.12's four-member INT range union is the first target here with + // more than one scalar member: range: "{{Param.Frames}}" was rejected at + // upload and the identical expression accepted at submit. + if c, ok := unresolvedConstraint(v.Type); ok && satisfies(c, target) { + return v, nil } if target.Code == CodeUnresolved { + if !coercibleToTarget(v.Type, target) { + return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) + } return Value{Type: target}, nil } - return Unresolved(target), nil -} - -// directUnionMember reports whether target is a union that names t exactly as -// one of its own members, looking through an unresolved constraint. -// -// This is deliberately NOT coercible's question. coercible answers "does a -// conversion apply", and its own tests pin it to false for a type the target -// already admits unchanged — coercible(int, "float | int") is false precisely -// because nothing needs converting. That is the answer coerce() needs here too, -// with the opposite consequence: nothing to convert means pass the value -// through, not refuse it. -func directUnionMember(target, t Type) bool { - if c, ok := unresolvedConstraint(target); ok { - return directUnionMember(c, t) - } - return target.Code == CodeUnion && containsType(target.Params, t) -} - -// coerceScalar performs a scalar conversion whose applicability coercible has -// already confirmed. It resolves which scalar code to aim for, then converts. -func coerceScalar(v Value, target Type) (Value, error) { - to, ok := targetScalarCode(v, target) + c, ok := unresolvedConstraint(v.Type) if !ok { return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) } - if to == v.Type.Code { - return v, nil - } - switch to { - case CodeString: - return String(v.String()), nil - case CodePath: - return Value{Type: TPath, s: v.AsStr()}, nil - case CodeInt: - return toInt(v) - case CodeFloat: - return toFloat(v) - } - return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) -} - -// coerceList performs the list conversions of section 1.2.3, having already -// confirmed with coercible that the conversion is legal. -// -// dstElem/dstIsList are passed in rather than recomputed: the caller needed them -// to decide this branch applied at all. -func coerceList(v Value, target, dstElem Type, dstIsList bool) (Value, error) { - // The invariant this function is written against, stated where it is relied - // upon: only a list source or a range_expr source has a list conversion, and - // the AsList() below is unchecked precisely because of it. A scalar arriving - // here used to panic there rather than being reported, so the guard is an - // error and not an assertion. - if _, ok := listElem(v.Type); !ok && v.Type.Code != CodeRangeExpr { + narrowed, ok := narrowedConstraint(c, target) + if !ok { return Value{}, fmt.Errorf("%s %w to %s", v.Type, errNotCoercible, target) } - // range_expr -> list[int]: expand, then convert elementwise in case the - // target's element type is not int (list[float], say). - if v.Type.Code == CodeRangeExpr { - ints, err := rangeInts(v) - if err != nil { - return Value{}, err - } - return coerceList(List(TInt, intValues(ints)), target, dstElem, dstIsList) - } - // A list value whose type already satisfies the target needs no conversion. - // listElem's element-aware comparison is what makes this safe where the - // scalar path's code-only includes() would not be: it cannot confuse a - // list[string] with a list[int] inside a union target. - if !dstIsList { - // The target admits the list without naming an element type — TAny, or - // a union in which the list is a direct member. - return v, nil - } - elems := v.AsList() - if err := checkElementCount(len(elems)); err != nil { - return Value{}, err - } - out := make([]Value, len(elems)) - for i, elem := range elems { - converted, err := coerce(elem, dstElem) - if err != nil { - return Value{}, fmt.Errorf("element %d: %w", i, err) - } - out[i] = converted - } - return List(dstElem, out), nil -} - -// targetScalarCode picks the scalar code v should become. The conditional rules -// of section 1.2.3 win over the single-scalar catch-all where they apply, in the -// same order coercibleConditional checks them. -func targetScalarCode(v Value, target Type) (Code, bool) { - switch v.Type.Code { - case CodeInt: - if includes(target, CodeFloat) && !includes(target, CodeInt) { - return CodeFloat, true - } - case CodePath: - if includes(target, CodeString) && !includes(target, CodePath) { - return CodeString, true - } - case CodeRangeExpr: - if includes(target, CodeString) && !includes(target, CodeRangeExpr) { - return CodeString, true - } - } - if includes(target, v.Type.Code) { - return v.Type.Code, true - } - return singleScalarTarget(target) + return Unresolved(narrowed), nil } // toInt implements float/string -> int, which section 1.2.3 requires to be diff --git a/internal/openjd/expr/coerce_internal_test.go b/internal/openjd/expr/coerce_internal_test.go index 9bb836ff..495b24ef 100644 --- a/internal/openjd/expr/coerce_internal_test.go +++ b/internal/openjd/expr/coerce_internal_test.go @@ -34,6 +34,15 @@ func TestIncludes(t *testing.T) { } } +// TestCoercible covers the CALL-RESOLUTION predicate, not target-type coercion. +// +// The distinction is new with openjd-specifications#175 and the table below +// reads oddly without it: "int stays int when the target admits int" expects +// FALSE, which is the right answer to "does a conversion apply" and the wrong +// answer to "can an int reach this target" (it can -- by satisfying it, which +// is TestCoerceDestinationOrder's first case). coercible() answers the former +// for promotable()/shape.go, and RFC 0005 explicitly leaves that mechanism +// alone; coercibleToTarget() answers the latter. func TestCoercible(t *testing.T) { tests := []struct { name string @@ -272,7 +281,12 @@ func TestCoerce_Rejected(t *testing.T) { {"non-numeric string to float", String("nothing"), "float", "cannot be parsed"}, // Not coercible at all. {"int to bool", Int(1), "bool", "cannot be coerced"}, - {"string to bool", String("true"), "bool", "cannot be coerced"}, + // MOVED by openjd-specifications#175: string -> bool is now one of the + // destination table's conversions, taking the same case-insensitive + // spellings as the explicit bool() of RFC 0006, so String("true") is no + // longer rejected -- see TestCoerceDestinationOrder. What is still + // rejected is a string that is not one of those spellings. + {"an unspellable string to bool", String("maybe"), "bool", "cannot convert"}, {"null to a scalar", Null(), "int", "cannot be coerced"}, {"int to a list", Int(1), "list[int]", "cannot be coerced"}, // Regression: coerceScalar's switch has a case CodePath that calls @@ -500,10 +514,15 @@ func TestCoercibleMatchesCoerce(t *testing.T) { // element-aware one: it is what admits a list value that IS one // of a union target's members, which includes() above cannot // answer for exactly the reason the paragraph above gives. - canDo := coercible(v.Type, target) || v.Type.Equal(target) || - target.Code == CodeAny || - (v.Type.Code != CodeList && includes(target, v.Type.Code)) || - directUnionMember(target, v.Type) + // UPDATED for openjd-specifications#175: the predicate this + // invariant is stated against is coercibleToTarget, the + // type-level twin of coerce(), and the pile of carve-outs that + // stood here -- Equal, any, includes-by-code, directUnionMember + // -- were the old reading's way of spelling SATISFACTION. They + // are one call now, and coercible() is no longer part of this + // invariant at all: it belongs to call-argument promotion, a + // separate mechanism #175 explicitly does not touch. + canDo := coercibleToTarget(v.Type, target) got, coerceErr := coerce(v, target) switch { case canDo && coerceErr != nil: @@ -567,36 +586,77 @@ func resultTypeAdmitted(got Value, target Type) bool { // scalar target AND a list element type, so asking the scalar question first // would answer for a list value with the wrong rule — "is a list a string or a // float", trivially no — and shadow the list rule coerce actually applied. +// fallibleDestination reports whether converting from a value of type from to +// destination d can fail on the VALUE rather than on the types -- "3.75" to int, +// "maybe" to bool. Everything reaches string, and every string reaches path, so +// those two never fail. +func fallibleDestination(from, d Code) bool { + switch d { + case CodeString: + return false + case CodePath: + return false + case CodeInt: + return from == CodeString || from == CodeFloat + case CodeFloat, CodeBool, CodeRangeExpr: + return from == CodeString + } + return false +} + func valueMayNotFit(v Value, target Type) bool { if _, srcIsList := listElem(v.Type); !srcIsList { - if to, ok := singleScalarTarget(target); ok { - switch to { - case CodeInt, CodeFloat: - return v.Type.Code == CodeString || v.Type.Code == CodeFloat + // RESTATED for openjd-specifications#175. coerce() now walks the + // destination table and fails only when EVERY destination the target + // offers has failed, so the type-level predicate and the value-level + // one can disagree only when every offered destination is a fallible + // conversion. One infallible destination anywhere in the list (any + // value reaches string; any string reaches path) means coerce() had a + // way through and a failure is a real disagreement. + // + // The old form asked singleScalarTarget, which is the wrong question + // now: it gives up when a union offers two scalars, which is precisely + // the case #175 made coercible. + offered := 0 + for _, d := range scalarDestinations(v.Type.Code) { + if !includes(target, d) { + continue + } + offered++ + if !fallibleDestination(v.Type.Code, d) { + return false } } + return offered > 0 + } + // The list rule performs the very same per-element conversion the bare-scalar + // case above already covers, so a list conversion fails on a value for + // exactly the reason a scalar one does -- and, since #175, over exactly the + // same set of destinations: every list destination the target offers, each + // judged by its ELEMENT type. One list destination whose elements convert + // infallibly (list[string] takes anything) means coerce() had a way through. + elemFrom, srcOK := listParam(v.Type) + if !srcOK { return false } - // Section 1.2.3's list rule, "list[T] -> list[U] when each element T can be - // coerced to U", performs the very same per-element scalar conversion the - // bare-scalar case above already covers — so a list conversion can fail on - // a value for exactly the reason a scalar one can: an element that is a - // string/float landing in a list[int]/list[float] target may not fit - // (float 1.5 -> int, string "a" -> int/float), even though coercible - // correctly permits the type-level list[T] -> list[U] conversion. This is - // the same exception as the scalar case, just applied elementwise; it is - // not a new kind of failure. - if elemFrom, srcOK := listElem(v.Type); srcOK { - if elemTo, dstOK := listElem(target); dstOK { - if to, ok := singleScalarTarget(elemTo); ok { - switch to { - case CodeInt, CodeFloat: - return elemFrom.Code == CodeString || elemFrom.Code == CodeFloat - } + // The empty list literal has no element that could fail, so no list + // destination can reject it on a value. + if elemFrom.Code == CodeNull { + return false + } + offered := 0 + for _, elemTo := range listDestinations(target) { + for _, d := range scalarDestinations(elemFrom.Code) { + if !includes(elemTo, d) { + continue + } + offered++ + if !fallibleDestination(elemFrom.Code, d) { + return false } } } - return false + return offered > 0 } // TestCoerceUnresolved_DirectUnionMember pins the carve-out coerceUnresolved @@ -626,11 +686,17 @@ func TestCoerceUnresolved_DirectUnionMember(t *testing.T) { {"int constraint, 4-member range union", TInt, rangeField, true}, {"range_expr constraint, 4-member range union", TRangeExpr, rangeField, true}, {"list[int] constraint, 4-member range union", ListOf(TInt), rangeField, true}, - // Not a member and not coercible into one: bool has no conversion to - // int, string is ambiguous with two scalar members present, and there - // is no bool rule at all. - {"bool constraint, 4-member range union", TBool, rangeField, false}, - {"float constraint, 4-member range union", TFloat, rangeField, false}, + // CHANGED by openjd-specifications#175. These two used to be errors, + // for a reason that was true of the old wording and is not of the new: + // "string is ambiguous with two scalar members present". Ambiguity is + // exactly what the destination table resolves -- bool's only + // destination is string, and float's are int then string, all of which + // this union offers -- so both now coerce, and a template field typed + // like section 1.3.12's INT range accepts a placeholder of either. + // This is acceptance-widening only: no target that used to take a + // placeholder stops taking one. + {"bool constraint, 4-member range union", TBool, rangeField, true}, + {"float constraint, 4-member range union", TFloat, rangeField, true}, // The narrower, single-scalar unions that already worked keep working // through coercible's own catch-all, not through the new carve-out. {"string constraint, string? | list[string]", TString, UnionOf(OptionalOf(TString), ListOf(TString)), true}, diff --git a/internal/openjd/expr/coercedest_internal_test.go b/internal/openjd/expr/coercedest_internal_test.go new file mode 100644 index 00000000..0562dfc9 --- /dev/null +++ b/internal/openjd/expr/coercedest_internal_test.go @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package expr + +import "testing" + +// TestCoerceDestinationOrder pins RFC 0005's "Implicit Type Coercion" as it was +// restated by openjd-specifications#175 (merged 2026-08-19): coercion asks +// SATISFACTION first, and only then converts toward the target's DESTINATIONS, +// in an order fixed by the result's type. +// +// The old wording had no answer when a target offered more than one candidate +// of the same kind, and sqi's reading of it -- singleScalarTarget, which gives +// up the moment two members disagree -- therefore REJECTED those targets +// outright. Every "converts" case below is one the merged text accepts and this +// package used to refuse, so this test is an acceptance-widening test: nothing +// sqi accepts today may start failing. +// +// Each case is either quoted from the merged section's own examples or derived +// from its destination-order table, and the "why" column says which. +func TestCoerceDestinationOrder(t *testing.T) { + tests := []struct { + name string + value Value + target string // ParseType, so the table reads as the specification does + want string // Value.String() of the result + wantTy string // the result's type, so a pass-through is distinguishable + }{ + // ── Satisfaction comes first, so an admitted type is never converted ── + { + name: "int satisfies a union naming int and is not stringified", + value: Int(5), + target: "int | string", + want: "5", wantTy: "int", + }, + { + name: "list[int] satisfies list[any] by element satisfaction", + value: List(TInt, []Value{Int(1), Int(2)}), + target: "list[any]", + want: "[1, 2]", wantTy: "list[int]", + }, + { + name: "null satisfies an optional target", + value: Null(), + target: "string?", + want: "null", wantTy: "nulltype", + }, + + // ── Conversion: the destination table, one row at a time ───────────── + // int -> float, then string. The spec states this example verbatim. + { + name: "int prefers float over string", + value: Int(5), + target: "float | string", + want: "5.0", wantTy: "float", + }, + // float -> int, then string. Both halves are the spec's own example. + { + name: "a whole float takes the int destination", + value: Float(3.0), + target: "int | string", + want: "3", wantTy: "int", + }, + { + name: "a fractional float fails int and falls through to string", + value: Float(3.5), + target: "int | string", + want: "3.5", wantTy: "string", + }, + // string -> int, float, bool, range_expr, path. The first two are the + // spec's example; the string's own lexical form routes it. + { + name: "a string that parses as an int takes int before float", + value: String("5"), + target: "int | float", + want: "5", wantTy: "int", + }, + { + name: "a string that parses only as a float takes float", + value: String("5.0"), + target: "int | float", + want: "5.0", wantTy: "float", + }, + { + name: "a string reaches bool, a destination the old rules had no conversion for", + value: String("yes"), + target: "bool", + want: "true", wantTy: "bool", + }, + { + name: "a string reaches range_expr, also new", + value: String("1-5"), + target: "range_expr", + want: "1-5", wantTy: "range_expr", + }, + { + name: "bool and range_expr are tried before path", + value: String("1-5"), + target: "bool | path | range_expr", + want: "1-5", wantTy: "range_expr", + }, + { + name: "path is the universal fallback, so a word reaches it", + value: String("shot010"), + target: "int | path", + want: "shot010", wantTy: "path", + }, + + // ── nulltype is never a destination ────────────────────────────────── + { + name: "a string is not converted toward a nulltype member", + value: String("null"), + target: "int | nulltype", + want: "", wantTy: "", // expected to fail + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target, err := ParseType(tt.target) + if err != nil { + t.Fatalf("ParseType(%q) = %v", tt.target, err) + } + got, err := coerce(tt.value, target) + if tt.wantTy == "" { + if err == nil { + t.Fatalf("coerce(%s, %s) = %s; want an error", tt.value, tt.target, got) + } + return + } + if err != nil { + t.Fatalf("coerce(%s, %s) = %v; want %s : %s", tt.value, tt.target, err, tt.want, tt.wantTy) + } + if got.String() != tt.want || got.Type.String() != tt.wantTy { + t.Errorf("coerce(%s, %s) = %s : %s; want %s : %s", + tt.value, tt.target, got, got.Type, tt.want, tt.wantTy) + } + }) + } +} diff --git a/internal/openjd/expr/coercedestlist_internal_test.go b/internal/openjd/expr/coercedestlist_internal_test.go new file mode 100644 index 00000000..b9ddd2c8 --- /dev/null +++ b/internal/openjd/expr/coercedestlist_internal_test.go @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package expr + +import "testing" + +// TestCoerceListDestinations pins the list half of RFC 0005's destination +// table, as restated by openjd-specifications#175: +// +// | list[S] | list destinations in S's order, applied to their element types | +// +// and, for the empty list, "the nominal element type it carries follows the +// first list destination in the union's normalized member order" -- an order +// the RFC defines outright (type parameters sorted alphabetically, nulltype +// last), so it is a rule and not an implementation detail. +// +// The old reading had no list-destination ORDER because it had no notion of +// more than one list destination: listElem() reports a target naming two +// different list types as not list-shaped at all, which is why the cases below +// either failed or, worse, passed the value through unconverted. +func TestCoerceListDestinations(t *testing.T) { + tests := []struct { + name string + value Value + target string + want string + wantTy string + }{ + { + name: "list[float] tries list[int] first, because int leads float's own order", + value: List(TFloat, []Value{Float(1.0), Float(2.0)}), + target: "list[int] | list[string]", + want: "[1, 2]", wantTy: "list[int]", + }, + { + name: "a fractional element fails list[int] and the next destination takes it", + value: List(TFloat, []Value{Float(1.5)}), + target: "list[int] | list[string]", + want: `["1.5"]`, wantTy: "list[string]", + }, + { + name: "a failed list destination is not an error when a later one converts", + value: List(TString, []Value{String("1"), String("x")}), + target: "list[int] | list[path]", + want: `["1", "x"]`, wantTy: "list[path]", + }, + { + name: "a single list destination still converts elementwise", + value: List(TInt, []Value{Int(1), Int(2)}), + target: "list[string]", + want: `["1", "2"]`, wantTy: "list[string]", + }, + { + name: "the empty list takes the first list destination in normalized order", + value: List(TNull, nil), + target: "list[string] | list[int]", + want: "[]", wantTy: "list[int]", + }, + { + name: "no list destination converts, so the coercion fails", + value: List(TString, []Value{String("x")}), + target: "list[int]", + want: "", wantTy: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target, err := ParseType(tt.target) + if err != nil { + t.Fatalf("ParseType(%q) = %v", tt.target, err) + } + got, err := coerce(tt.value, target) + if tt.wantTy == "" { + if err == nil { + t.Fatalf("coerce(%s, %s) = %s : %s; want an error", tt.value, tt.target, got, got.Type) + } + return + } + if err != nil { + t.Fatalf("coerce(%s, %s) = %v; want %s : %s", tt.value, tt.target, err, tt.want, tt.wantTy) + } + if got.String() != tt.want || got.Type.String() != tt.wantTy { + t.Errorf("coerce(%s, %s) = %s : %s; want %s : %s", + tt.value, tt.target, got, got.Type, tt.want, tt.wantTy) + } + }) + } +} + +// TestCoerceRejectsUnusableDestinations pins RFC 0005's exclusion list: "a type +// variable, noreturn, unresolved[T], or a list parameterized by any of those +// contributes no destination, so a target composed only of such types cannot be +// coerced to at all." +// +// The list[T1] row is the one the specification states as a MUST, and it says +// why: the symmetric matching that binds type variables during signature +// matching would accept such a target by binding T1 and then discarding the +// binding. Satisfaction is directional and must not be used for that. +func TestCoerceRejectsUnusableDestinations(t *testing.T) { + tests := []struct { + name string + value Value + target Type + }{ + {"a bare type variable offers nothing", Int(1), Type{Code: CodeVarT1}}, + {"noreturn offers nothing", Int(1), TNoReturn}, + {"a list of a type variable offers nothing", List(TInt, []Value{Int(1)}), ListOf(Type{Code: CodeVarT1})}, + {"a union of only unusable types offers nothing", Int(1), UnionOf(TNoReturn, Type{Code: CodeVarT})}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := coerce(tt.value, tt.target) + if err == nil { + t.Fatalf("coerce(%s, %s) = %s : %s; want an error", tt.value, tt.target, got, got.Type) + } + }) + } +} + +// TestCoerceUnresolvedNarrowing pins the constraint a PLACEHOLDER carries away +// from a coercion, which #175 restated: "conversion narrows to the union of +// every destination with a type-level rule, rather than betting on any one of +// them -- anything narrower would misdescribe some resolved value." +// +// The invariant behind it, stated by the RFC and worth keeping in mind when +// reading these cases: the narrowed constraint always satisfies the target, AND +// the concrete result's type always satisfies the narrowed constraint. Narrowing +// unresolved[float] against "int | string" to unresolved[int] would break the +// second half, because a 3.5 payload fails float->int and lands on string. +func TestCoerceUnresolvedNarrowing(t *testing.T) { + tests := []struct { + name string + constraint Type + target string + want string + }{ + { + name: "satisfaction keeps the source constraint", + constraint: ListOf(TInt), + target: "list[any]", + want: "unresolved[list[int]]", + }, + { + name: "a single destination narrows to exactly that type", + constraint: TInt, + target: "string", + want: "unresolved[string]", + }, + { + name: "two destinations narrow to their union, not to a guess", + constraint: TFloat, + target: "int | string", + want: "unresolved[int | string]", + }, + { + name: "range_expr against a list target narrows to what materializing produces", + constraint: TRangeExpr, + target: "list[any]", + want: "unresolved[list[int]]", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + target, err := ParseType(tt.target) + if err != nil { + t.Fatalf("ParseType(%q) = %v", tt.target, err) + } + got, err := coerce(Unresolved(tt.constraint), target) + if err != nil { + t.Fatalf("coerce(unresolved[%s], %s) = %v; want %s", tt.constraint, tt.target, err, tt.want) + } + if got.Type.String() != tt.want { + t.Errorf("coerce(unresolved[%s], %s) = %s; want %s", + tt.constraint, tt.target, got.Type, tt.want) + } + }) + } +} diff --git a/internal/openjd/expr/eval_internal_test.go b/internal/openjd/expr/eval_internal_test.go index c53e8792..7b39763d 100644 --- a/internal/openjd/expr/eval_internal_test.go +++ b/internal/openjd/expr/eval_internal_test.go @@ -822,8 +822,7 @@ func TestEval_RecursionDepthIsBounded(t *testing.T) { if !strings.Contains(err.Error(), "nested too deeply") { t.Fatalf("Eval of a %d-deep %s error = %q, want it to mention nesting depth", maxEvalDepth+10, tc.name, err.Error()) } - var e2 *Error - if !errors.As(err, &e2) { + if _, ok := errors.AsType[*Error](err); !ok { t.Fatalf("Eval of a %d-deep %s returned %T, want an *Error carrying a position", maxEvalDepth+10, tc.name, err) } }) diff --git a/internal/openjd/expr/funcsstrpad.go b/internal/openjd/expr/funcsstrpad.go index 7267aa95..ce685368 100644 --- a/internal/openjd/expr/funcsstrpad.go +++ b/internal/openjd/expr/funcsstrpad.go @@ -131,11 +131,20 @@ func padWidth(ec evalCtx, s string, width int64) (padLen int, need bool, err err // padString implements ljust, rjust and center over a space fill. // -// center splits by FLOOR division and leaves the remainder on the right, so -// center("ab", 7) is " ab ". CPython disagrees — it biases the split with -// "marg & width & 1" and answers " ab " — and the reference implementation -// does not. We follow the reference, which is also the simpler rule; matching -// CPython here would create an oracle divergence for no reason. +// center follows CPython: the padding is split by floor division and then +// biased by "marg & width & 1", so center("ab", 7) is " ab " — three spaces +// left, two right — and not the plain floor split's " ab ". The bias only +// fires when the padding and the width are both odd. +// +// CHANGED 2026-08-19, and the reason it changed is the reason it was ever the +// other way. sqi used the plain floor split because the REFERENCE did, and this +// comment said so: "matching CPython here would create an oracle divergence for +// no reason." openjd-expr 0.3.0 (openjd-model 0.11.3+) moved center to +// CPython's rule, so the plain split became the divergence — caught by the +// oracle on the 0.11.4 pin bump, as a NEW value divergence on center('ab', 7). +// RFC 0006 says only "Center, pad with spaces to width" and does not settle the +// tie, so with the spec silent the tie-break follows the library the function +// is modeled on, which is now what the reference does too. func padString(ec evalCtx, s string, width int64, side padSide) (Value, error) { n, need, err := padWidth(ec, s, width) if err != nil { @@ -148,7 +157,9 @@ func padString(ec evalCtx, s string, width int64, side padSide) (Value, error) { case padLeft: return String(strings.Repeat(" ", n) + s), nil case padCenter: - left := n / 2 + // CPython's str.center bias: floor split, plus one on the left when the + // padding and the width are both odd. + left := n/2 + int(int64(n)&width&1) return String(strings.Repeat(" ", left) + s + strings.Repeat(" ", n-left)), nil default: return String(s + strings.Repeat(" ", n)), nil diff --git a/internal/openjd/expr/funcsstrpad_internal_test.go b/internal/openjd/expr/funcsstrpad_internal_test.go index 8b21e931..8d65b25f 100644 --- a/internal/openjd/expr/funcsstrpad_internal_test.go +++ b/internal/openjd/expr/funcsstrpad_internal_test.go @@ -10,11 +10,14 @@ import ( // TestPaddingFunctions covers ljust, rjust, center and zfill. // // Two rows are easy to get wrong in opposite directions: -// - center splits the padding by FLOOR division and puts the remainder on the -// right, so center('ab',7) is ' ab '. CPython answers ' ab ' — it -// biases the split with marg & width & 1 — and the reference does not. -// Do not "correct" this toward Python; it would manufacture an oracle -// divergence out of nothing. +// - center biases the split with CPython's "marg & width & 1", so +// center('ab',7) is ' ab ' — three spaces left, two right — rather than +// the floor split's ' ab '. It only shows up when the padding and the +// width are both odd; center('abc',4) and center('é',5) are the same under +// either rule. sqi used the floor split until 2026-08-19 for one reason +// only, recorded in funcsstrpad.go: the reference did, and matching CPython +// would have manufactured an oracle divergence. openjd-expr 0.3.0 moved to +// CPython's rule, so that reason now argues the other way. // - a width at or below the current length returns the input UNCHANGED, which // is what makes a negative width a harmless no-op. The reference panics on // a negative width instead. @@ -26,7 +29,7 @@ func TestPaddingFunctions(t *testing.T) { }{ {"ljust pads on the right", `ljust('ab', 5)`, "ab "}, {"rjust pads on the left", `rjust('ab', 5)`, " ab"}, - {"center splits by floor, remainder right", `center('ab', 7)`, " ab "}, + {"center biases the odd space left, as CPython does", `center('ab', 7)`, " ab "}, {"center with one to spare", `center('abc', 4)`, "abc "}, {"center counts codepoints", `center('é', 5)`, " é "}, {"ljust below the width is unchanged", `ljust('abcd', 2)`, "abcd"}, diff --git a/internal/openjd/expr/paramtypes.go b/internal/openjd/expr/paramtypes.go index 0a0d1cb7..2075cc1f 100644 --- a/internal/openjd/expr/paramtypes.go +++ b/internal/openjd/expr/paramtypes.go @@ -20,7 +20,7 @@ import ( // declared type binds Param and RawParam to the SAME type. That rule is // stated in as many words by Template Schemas §7.3.1's value-reference // table — "This is the same as RawParam. for all parameter types -// except PATH" (wiki/2023-09-Template-Schemas.md:1991) — NOT by +// except PATH" (wiki/2023-09-Template-Schemas.md:1993) — NOT by // Expression-Language §1.2.2, which an earlier revision of this comment cited // for it; the substance was right and the citation was wrong. §1.2.2's own // contribution is the type table above plus the PATH prose ("Param. @@ -70,7 +70,7 @@ func JobParamTypes(declared string) (paramType, rawType Type) { // the parameter with relevant path mapping rules applied to it" and // Task.RawParam. as "the value of the parameter as it was defined, // with no path mapping rules applied" -// (wiki/2023-09-Template-Schemas.md:1246-1247). An earlier revision of this +// (wiki/2023-09-Template-Schemas.md:1248-1249). An earlier revision of this // comment attributed those two quotes to §1.2.2, which does not contain // them. It is a VALUE difference, not a type difference; both remain path. // diff --git a/internal/openjd/expr/parser_internal_test.go b/internal/openjd/expr/parser_internal_test.go index 3065b61a..514093e4 100644 --- a/internal/openjd/expr/parser_internal_test.go +++ b/internal/openjd/expr/parser_internal_test.go @@ -483,8 +483,7 @@ func TestParse_RejectsPythonBeyondEXPR(t *testing.T) { if err == nil { t.Fatalf("Parse(%q) accepted the expression (root %T); want a syntax error", tt.src, e.Root()) } - var pe *Error - if !errors.As(err, &pe) { + if _, ok := errors.AsType[*Error](err); !ok { t.Errorf("error is %T; want *Error carrying a position", err) } }) @@ -766,8 +765,7 @@ func TestParse_NestingDepthIsBounded(t *testing.T) { t.Fatalf("Parse of %d-deep %q error = %q, want it to mention nesting depth", tc.depth, tc.name, err.Error()) } - var e *Error - if !errors.As(err, &e) { + if _, ok := errors.AsType[*Error](err); !ok { t.Fatalf("Parse of %d-deep %q returned %T, want an *Error carrying a position", tc.depth, tc.name, err) } diff --git a/internal/openjd/exprcheck.go b/internal/openjd/exprcheck.go index 43912b45..513c44b7 100644 --- a/internal/openjd/exprcheck.go +++ b/internal/openjd/exprcheck.go @@ -243,12 +243,12 @@ func bindEmbeddedFileSymbols(files []EmbeddedFile, prefix string, syms expr.MapS // The restriction itself is the SPECIFICATION's: the wiki's FunctionLibrary // entry for with_host_context() is a library "with host-only functions like // apply_path_mapping() enabled" -// (third_party/openjd-specifications/wiki/2026-02-Expression-Language.md:514). +// (third_party/openjd-specifications/wiki/2026-02-Expression-Language.md:527). // // That "like" is the whole reason this is a SET rather than a name // comparison, and the specification is the only source needed for it. RFC // 0005's "Host-Context Function Availability" section -// (third_party/openjd-specifications/rfcs/0005-expression-language.md:1022) +// (third_party/openjd-specifications/rfcs/0005-expression-language.md:1163) // says the same thing at more length -- "Certain functions ... For example, // apply_path_mapping()" -- but that section exists ONLY in the RFC, which is // the proposal behind the specification and not the specification itself; do diff --git a/internal/openjd/exprcheck_test.go b/internal/openjd/exprcheck_test.go index d979c598..be48b4a1 100644 --- a/internal/openjd/exprcheck_test.go +++ b/internal/openjd/exprcheck_test.go @@ -697,9 +697,22 @@ func TestCheckParameterSpaceExpressions_RangeTargetTypes(t *testing.T) { // INT/CHUNK[INT] must name their own: section 1.3.12's RangeString // note makes an int and a string LEGAL at that position (they are // range TEXT -- see rangeExprFieldType), so those two rows need a - // scalar that is neither, and a bool is the one scalar with no - // conversion into any member of "int | string | range_expr | - // list[int]". + // scalar that is neither. + // + // It used to be "{{ true }}", on the ground that "a bool is the one + // scalar with no conversion into any member". openjd-specifications#175 + // ended that: bool's destination is string, the target offers string, + // so a bool now becomes the range TEXT "true" -- which is not a valid + // and is rejected by EXPANSION, exactly as "{{ 'abc' }}" + // always has been (resolve substitutes the text without parsing it; + // ExpandParameterSpace is the only layer that reads it -- pinned by + // TestExpandParameterSpace_NonRangeTextIsRejected). The checker's job + // at this position is the type, not the text. + // + // null is what is left, and it is left for a stated reason rather than + // by elimination: no conversion produces null, so nulltype is never a + // destination, and a target that does not name nulltype cannot receive + // one. wholeShapeReject string entryAccept string entryReject string // "" when there is no type-mismatch (as opposed to shape-mismatch) case @@ -707,7 +720,7 @@ func TestCheckParameterSpaceExpressions_RangeTargetTypes(t *testing.T) { { name: "INT", typ: TaskParamTypeInt, wholeAccept: "{{ [1, 2, 3] }}", entryAccept: "{{ 5 }}", - wholeShapeReject: "{{ true }}", + wholeShapeReject: "{{ null }}", // A list of strings: string->int coercion needs a value that // parses as an int, and "a" does not -- so this is rejected on // the VALUE, not merely the type (list[string] -> list[int] is @@ -726,7 +739,7 @@ func TestCheckParameterSpaceExpressions_RangeTargetTypes(t *testing.T) { { name: "CHUNK[INT]", typ: TaskParamTypeChunkInt, wholeAccept: "{{ [1, 2, 3] }}", entryAccept: "{{ 5 }}", - wholeShapeReject: "{{ true }}", + wholeShapeReject: "{{ null }}", wholeReject: "{{ ['a'] }}", entryReject: "{{ 2.5 }}", }, diff --git a/internal/openjd/fmtstring/fmtstring_test.go b/internal/openjd/fmtstring/fmtstring_test.go index f49c7309..fd6b97a4 100644 --- a/internal/openjd/fmtstring/fmtstring_test.go +++ b/internal/openjd/fmtstring/fmtstring_test.go @@ -213,8 +213,7 @@ func TestResolveMalformed(t *testing.T) { if err == nil { t.Fatalf("Resolve(%q) returned no error", tc.input) } - var malformed *fmtstring.MalformedError - if !errors.As(err, &malformed) { + if _, ok := errors.AsType[*fmtstring.MalformedError](err); !ok { t.Fatalf("Resolve(%q) error is not *MalformedError: %T (%v)", tc.input, err, err) } }) @@ -331,8 +330,7 @@ func TestReferencesMalformed(t *testing.T) { if err == nil { t.Errorf("References(%q) returned no error", input) } - var malformed *fmtstring.MalformedError - if !errors.As(err, &malformed) { + if _, ok := errors.AsType[*fmtstring.MalformedError](err); !ok { t.Errorf("References(%q) error is not *MalformedError: %T (%v)", input, err, err) } } diff --git a/internal/openjd/resolve_test.go b/internal/openjd/resolve_test.go index 5297283e..5ffaf336 100644 --- a/internal/openjd/resolve_test.go +++ b/internal/openjd/resolve_test.go @@ -1367,3 +1367,49 @@ func TestResolveParameterSpaceParams_NonLoneRangeExprEmbeddedRangeExprValue(t *t }) } } + +// TestResolveParameterSpaceParams_NonRangeTextIsRejectedAtResolve pins where a +// scalar that COERCES into an INT range field but does not spell a range is +// caught. +// +// openjd-specifications#175 moved that boundary. The whole-field target for an +// INT parameter is "int | string | range_expr | list[int]", and under the old +// coercion rules a bool reached none of those members, so "{{ true }}" was +// rejected by the expression CHECKER at upload. Under the merged rules bool's +// destination is string, the target offers string, and the checker now accepts +// it -- exactly as it has always accepted "{{ 'abc' }}", which is equally not a +// range. +// +// So the rejection has to still happen further along. It does, and NOT where +// the first draft of this test looked: ResolveParameterSpaceParams substitutes +// the expression and leaves the TEXT in RangeExpr without parsing it -- it +// accepts "{{ true }}" and "{{ 'abc' }}" alike, and always has for the string. +// The parse is ExpandParameterSpace's, through range.go's parseIntRangeExpr. +// So the layering is: the checker rules on the TYPE, resolve substitutes the +// TEXT, and expansion is the only place that reads it. This test asserts the +// last of those, for both scalars, so the boundary #175 moved stays covered. +func TestExpandParameterSpace_NonRangeTextIsRejected(t *testing.T) { + for _, tc := range []struct { + name, body string + }{ + {"a bool, which #175 made the checker accept", "{{ true }}"}, + {"a string, which the checker has always accepted", "{{ 'abc' }}"}, + } { + t.Run(tc.name, func(t *testing.T) { + tmpl := &openjd.JobTemplate{Extensions: []string{"EXPR"}} + ps := &openjd.StepParameterSpace{ + TaskParameterDefinitions: []openjd.TaskParamDefinition{ + {Name: "P", Type: openjd.TaskParamTypeInt, RangeExpr: ptr(tc.body)}, + }, + } + resolved, errs := openjd.ResolveParameterSpaceParams(tmpl, nil, ps, nil) + if len(errs) != 0 { + t.Fatalf("ResolveParameterSpaceParams(%q) = %v; want it to substitute and defer", tc.body, errs) + } + tasks, err := openjd.ExpandParameterSpace(resolved) + if err == nil { + t.Fatalf("ExpandParameterSpace(%q) = %v with no error; want the text rejected", tc.body, tasks) + } + }) + } +} diff --git a/internal/openjd/scope.go b/internal/openjd/scope.go index 27bec055..6e5072ac 100644 --- a/internal/openjd/scope.go +++ b/internal/openjd/scope.go @@ -65,7 +65,7 @@ func (s Scope) String() string { // It gates the host-context-only functions. The specification does NOT commit // to a single such function: its FunctionLibrary.with_host_context() enables // "host-only functions like apply_path_mapping()" (wiki -// 2026-02-Expression-Language.md:514). apply_path_mapping is the only one in +// 2026-02-Expression-Language.md:527). apply_path_mapping is the only one in // sqi's registry today because it is the only function that reads session // state; the gate is a set (hostOnlyFunctions, exprcheck.go) rather than a // name comparison so a second entrant costs one edit. (RFC 0005's diff --git a/internal/openjd/submit_test.go b/internal/openjd/submit_test.go index 78247c7e..9abf6636 100644 --- a/internal/openjd/submit_test.go +++ b/internal/openjd/submit_test.go @@ -241,8 +241,7 @@ func TestSubmitter_Submit_BadYAML(t *testing.T) { if err == nil { t.Fatal("expected error for bad YAML, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -268,8 +267,7 @@ func TestSubmitter_Submit_ValidationError(t *testing.T) { if err == nil { t.Fatal("expected validation error, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -305,8 +303,7 @@ func TestSubmitter_Submit_UnregisteredStorageLocation(t *testing.T) { if err == nil { t.Fatal("expected error for unregistered storage location, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -381,8 +378,7 @@ func TestSubmitter_Submit_JobEnvMissingDefaultRoot(t *testing.T) { if err == nil { t.Fatal("expected error: job-level ref to a location with no default root") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -424,8 +420,7 @@ func TestSubmitter_Submit_JobParamDefaultMissingDefaultRoot(t *testing.T) { if err == nil { t.Fatal("expected error: job-param-default ref to a location with no default root") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -570,8 +565,7 @@ func TestSubmitter_Submit_MissingRequiredParam(t *testing.T) { if err == nil { t.Fatal("expected error for missing required parameter, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -593,8 +587,7 @@ func TestSubmitter_Submit_InvalidParamValue(t *testing.T) { if err == nil { t.Fatal("expected error for invalid INT value, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -617,8 +610,7 @@ func TestSubmitter_Submit_UnknownParam(t *testing.T) { if err == nil { t.Fatal("expected error for unknown parameter, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -797,8 +789,7 @@ func TestSubmitter_Submit_ParamInRangeExpr_ExceedsValueLimit(t *testing.T) { if err == nil { t.Fatal("expected SubmitValidationError for resolved range exceeding the value limit, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -833,8 +824,7 @@ func TestSubmitter_Submit_ParamInRangeExpr_UnknownParam(t *testing.T) { if err == nil { t.Fatal("expected SubmitValidationError for unknown {{Param.Missing}}, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Errorf("expected SubmitValidationError, got %T: %v", err, err) } } @@ -1102,8 +1092,7 @@ steps: if err == nil { t.Fatal("expected an error for an EXPR template dividing by a zero-valued parameter, got nil") } - var ve *openjd.SubmitValidationError - if !errors.As(err, &ve) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Fatalf("expected SubmitValidationError, got %T: %v", err, err) } if !strings.Contains(err.Error(), "division by zero") { diff --git a/internal/product/exprpresetcost_test.go b/internal/product/exprpresetcost_test.go index 17d1c71b..c3f7dd6e 100644 --- a/internal/product/exprpresetcost_test.go +++ b/internal/product/exprpresetcost_test.go @@ -123,8 +123,7 @@ func TestExprPresetSubmitsAtCeiling(t *testing.T) { if errors.Is(err, expr.ErrDeadlineExceeded) { t.Fatalf("failed on the wall-clock deadline, not the operation limit: %v", err) } - var verr *openjd.SubmitValidationError - if !errors.As(err, &verr) { + if _, ok := errors.AsType[*openjd.SubmitValidationError](err); !ok { t.Fatalf("want a *openjd.SubmitValidationError (the client-fault channel), got %T: %v", err, err) } if !strings.Contains(err.Error(), "operation limit exceeded") { diff --git a/internal/store/migrations/migrations_test.go b/internal/store/migrations/migrations_test.go index 943f06ff..2c3fc8d6 100644 --- a/internal/store/migrations/migrations_test.go +++ b/internal/store/migrations/migrations_test.go @@ -41,8 +41,7 @@ func TestFS_OpenAppleDoubleIsHidden(t *testing.T) { if err == nil { t.Fatal("Open of AppleDouble file: want error, got nil") } - var perr *fs.PathError - if !errors.As(err, &perr) { + if _, ok := errors.AsType[*fs.PathError](err); !ok { t.Fatalf("want *fs.PathError, got %T", err) } } diff --git a/test/conformance/baseline.txt b/test/conformance/baseline.txt index 79f4b808..204d619b 100644 --- a/test/conformance/baseline.txt +++ b/test/conformance/baseline.txt @@ -66,3 +66,32 @@ EXPR/job_templates/3.6--let-host-context-symbols.yaml # first step, which references Job.Name and Step.Name from .let # and .let, is accepted; only the SimpleAction half is unreachable. EXPR/job_templates/7.3.1--job-step-name-in-step-let.yaml + +# 2026-08-19, submodule bump 42a1fb6 -> be0aefb: this fixture is NEW upstream, +# added by openjd-specifications PR #172 ("do not cap expansion +# at the list-form limit"), which also rewrote 3.4--max-range-items.yaml and +# 3.4--too-many-range-items.invalid.yaml to use explicit LISTS of 1024 / 1025 +# values (both still pass here — only the new range-EXPRESSION case fails). +# +# The change is a base-spec acceptance widening: §3.4's "at most 1024 values" +# now bounds the LIST form only, so `range: "1-5000"` — one +# naming 5000 values — is valid where it previously was not. The bump also +# added the normative sentence behind it, at +# wiki/2023-09-Template-Schemas.md:1155: the number of values an +# expands to "is not constrained by this specification", and an implementation +# needing to bound Step size "is expected to do so with its own limit on the +# Task count, not by constraining this expression". sqi already HAS those +# count limits — maxTasksPerStep and maxTasksPerJob — which is what makes the +# fix tractable, and also what makes it a policy decision rather than a typo. sqi applies the +# cap to BOTH forms: taskParamValueCount (validate.go) counts a RangeExpr +# arithmetically and hands the same maxTaskParamValues (1024) gate the count, +# so the fixture is rejected on +# `/steps/0/parameterSpace/taskParameterDefinitions/0/range: at most 1024 +# values are allowed per task parameter (got 5000)` — MEASURED, not inferred. +# +# Deliberately NOT fixed in passing. Lifting the cap for the expression form is +# an acceptance change in internal/openjd — templates rejected today would start +# producing tasks — and it lands on the same policy surface as maxTasksPerStep +# and maxTasksPerJob, which is exactly the reasoning CLAUDE.md records for the +# three intrange divergences. It wants its own decision, not a drive-by. +base/job_templates/3.4--wide-int-range-expression.yaml diff --git a/test/conformance/report.go b/test/conformance/report.go index 6bff7679..903f6fc9 100644 --- a/test/conformance/report.go +++ b/test/conformance/report.go @@ -13,8 +13,18 @@ import ( type Group struct { // Name is "/", e.g. "base/job_templates". Name string - // Passed and Failed count live tests only. - Passed, Failed int + // Passed counts live tests that passed. + Passed int + // Baselined and Regressed split the live FAILURES, and the split is the + // whole point of this type: a failure listed in the baseline has been + // adjudicated and written down, while one that is not is a break. Both are + // failures and both are counted in the live total; only one of them is news. + // + // They were a single Failed field until 2026-08-19, and FormatRollup + // labeled all of it "baselined" — so the run that first met an unlisted + // failure summarized it as "449/450 pass 1 baselined" directly above the + // line calling the same fixture a REGRESSION. + Baselined, Regressed int // NotApplicable counts tests for extensions sqi has not registered, or for // a document kind sqi does not implement at all (env_templates). These // are never folded into Passed — see StateNotApplicable. @@ -22,7 +32,22 @@ type Group struct { } // Rollup tallies results per "/" directory, sorted by name. -func Rollup(results []Result) []Group { +// +// The baseline is a parameter rather than something the caller applies +// afterwards because the tally cannot be honest without it: "failed" and +// "failed in a way we already accepted" are different facts about a run. +// +// It classifies through DiffBaseline rather than re-deriving membership from +// the map, so the counts here and the REGRESSION lines a caller prints from the +// same diff are one judgement rendered twice. Re-deriving would be two lines of +// code and a standing opportunity for them to disagree. +func Rollup(results []Result, baseline map[string]struct{}) []Group { + regressions, _, _ := DiffBaseline(results, baseline) + isRegression := make(map[string]bool, len(regressions)) + for _, id := range regressions { + isRegression[id] = true + } + byName := map[string]*Group{} for _, r := range results { name := filepath.ToSlash(filepath.Dir(r.ID())) @@ -36,8 +61,10 @@ func Rollup(results []Result) []Group { g.NotApplicable++ case r.Passed: g.Passed++ + case isRegression[r.ID()]: + g.Regressed++ default: - g.Failed++ + g.Baselined++ } } @@ -53,7 +80,11 @@ func Rollup(results []Result) []Group { // // Not-applicable counts are reported in their own column and never as a pass // ratio, so an unimplemented extension or unimplemented document kind (such as -// env_templates) can never look like a green one. +// env_templates) can never look like a green one. Regressions are reported in +// their own words and in CAPITALS, ahead of the baselined count, for the same +// reason: this table is the first thing a reader sees, and the one number on it +// that means "something broke" must not be spelled like the one that means +// "something is known and accepted". func FormatRollup(groups []Group) string { width := 0 for _, g := range groups { @@ -64,15 +95,22 @@ func FormatRollup(groups []Group) string { var b strings.Builder for _, g := range groups { - live := g.Passed + g.Failed + live := g.Passed + g.Baselined + g.Regressed switch live { case 0: fmt.Fprintf(&b, "%-*s %s %d n/a — not implemented\n", width, g.Name, strings.Repeat(" ", 9), g.NotApplicable) default: fmt.Fprintf(&b, "%-*s %4d/%-4d pass", width, g.Name, g.Passed, live) - if g.Failed > 0 { - fmt.Fprintf(&b, " %d baselined", g.Failed) + if g.Regressed > 0 { + noun := "REGRESSION" + if g.Regressed > 1 { + noun = "REGRESSIONS" + } + fmt.Fprintf(&b, " %d %s", g.Regressed, noun) + } + if g.Baselined > 0 { + fmt.Fprintf(&b, " %d baselined", g.Baselined) } b.WriteByte('\n') } diff --git a/test/conformance/report_test.go b/test/conformance/report_test.go index 020bf0bb..b061de2b 100644 --- a/test/conformance/report_test.go +++ b/test/conformance/report_test.go @@ -14,17 +14,24 @@ func TestRollup(t *testing.T) { result("base/job_templates/a.yaml", conformance.StateLive, true), result("base/job_templates/b.yaml", conformance.StateLive, true), result("base/job_templates/c.yaml", conformance.StateLive, false), - result("base/env_templates/d.yaml", conformance.StateLive, false), - result("EXPR/job_templates/e.yaml", conformance.StateNotApplicable, false), + result("base/job_templates/d.yaml", conformance.StateLive, false), + result("base/env_templates/e.yaml", conformance.StateLive, false), result("EXPR/job_templates/f.yaml", conformance.StateNotApplicable, false), + result("EXPR/job_templates/g.yaml", conformance.StateNotApplicable, false), + } + baseline := map[string]struct{}{ + "base/job_templates/c.yaml": {}, } - groups := conformance.Rollup(results) + groups := conformance.Rollup(results, baseline) + // d.yaml fails and is NOT in the baseline, so it is a regression; c.yaml + // fails and is, so it is adjudicated. A tally that cannot tell them apart + // is the defect this split exists to fix. want := []conformance.Group{ {Name: "EXPR/job_templates", NotApplicable: 2}, - {Name: "base/env_templates", Failed: 1}, - {Name: "base/job_templates", Passed: 2, Failed: 1}, + {Name: "base/env_templates", Regressed: 1}, + {Name: "base/job_templates", Passed: 2, Baselined: 1, Regressed: 1}, } if len(groups) != len(want) { t.Fatalf("got %d groups, want %d: %+v", len(groups), len(want), groups) @@ -36,9 +43,42 @@ func TestRollup(t *testing.T) { } } +// TestRollup_AgreesWithDiffBaseline is the invariant behind the split: the +// rollup's regression count and the REGRESSION lines the suite prints beneath it +// come from the same judgement, so they can never contradict each other. +// +// They did contradict each other before, which is why this test exists: the +// rollup labeled every failure "baselined" no matter what the diff said, so a +// genuine regression was summarized as an accepted divergence one line above the +// text calling it a regression. +func TestRollup_AgreesWithDiffBaseline(t *testing.T) { + results := []conformance.Result{ + result("base/job_templates/a.yaml", conformance.StateLive, true), + result("base/job_templates/b.yaml", conformance.StateLive, false), + result("base/job_templates/c.yaml", conformance.StateLive, false), + result("EXPR/job_templates/d.yaml", conformance.StateLive, false), + result("EXPR/job_templates/e.yaml", conformance.StateNotApplicable, false), + } + baseline := map[string]struct{}{ + "base/job_templates/b.yaml": {}, + "EXPR/job_templates/d.yaml": {}, + } + + regressions, _, _ := conformance.DiffBaseline(results, baseline) + + total := 0 + for _, g := range conformance.Rollup(results, baseline) { + total += g.Regressed + } + if total != len(regressions) { + t.Errorf("rollup counts %d regressions, DiffBaseline reports %d (%v)", + total, len(regressions), regressions) + } +} + func TestFormatRollup_ShowsNotApplicableSeparately(t *testing.T) { out := conformance.FormatRollup([]conformance.Group{ - {Name: "base/job_templates", Passed: 438, Failed: 13}, + {Name: "base/job_templates", Passed: 438, Baselined: 13}, {Name: "EXPR/job_templates", NotApplicable: 209}, }) @@ -52,3 +92,36 @@ func TestFormatRollup_ShowsNotApplicableSeparately(t *testing.T) { t.Errorf("not-applicable tests are being shown as passes:\n%s", out) } } + +// TestFormatRollup_UnbaselinedFailureIsNotCalledBaselined pins the line that +// misled a reader on 2026-08-19: a new upstream fixture regressed and the +// summary read "449/450 pass 1 baselined" while the diff below it correctly +// said REGRESSION. The gate was sound; the sentence was false. +func TestFormatRollup_UnbaselinedFailureIsNotCalledBaselined(t *testing.T) { + out := conformance.FormatRollup([]conformance.Group{ + {Name: "base/job_templates", Passed: 449, Regressed: 1}, + }) + + if strings.Contains(out, "baselined") { + t.Errorf("an unbaselined failure is being reported as adjudicated:\n%s", out) + } + if !strings.Contains(out, "REGRESSION") { + t.Errorf("rollup does not call out the regression:\n%s", out) + } +} + +func TestFormatRollup_ReportsBothKindsOfFailure(t *testing.T) { + out := conformance.FormatRollup([]conformance.Group{ + {Name: "EXPR/job_templates", Passed: 206, Baselined: 3, Regressed: 2}, + }) + + if !strings.Contains(out, "206/211") { + t.Errorf("the live ratio must count both kinds of failure:\n%s", out) + } + if !strings.Contains(out, "2 REGRESSIONS") { + t.Errorf("rollup does not report the regressions:\n%s", out) + } + if !strings.Contains(out, "3 baselined") { + t.Errorf("rollup does not report the adjudicated failures:\n%s", out) + } +} diff --git a/test/conformance/suite_test.go b/test/conformance/suite_test.go index 51f518f5..dd93db10 100644 --- a/test/conformance/suite_test.go +++ b/test/conformance/suite_test.go @@ -98,12 +98,16 @@ func TestConformance_Templates(t *testing.T) { results = append(results, conformance.RunCase(tc, state, data)) } - t.Logf("\n%s", conformance.FormatRollup(conformance.Rollup(results))) - + // The baseline is loaded BEFORE the rollup is printed, because the rollup + // needs it: without it the summary cannot tell an adjudicated failure from + // a regression, and it used to call both "baselined". baseline, err := conformance.LoadBaseline(baselinePath) if err != nil { t.Fatalf("load baseline: %v", err) } + + t.Logf("\n%s", conformance.FormatRollup(conformance.Rollup(results, baseline))) + regressions, stale, orphaned := conformance.DiffBaseline(results, baseline) byID := map[string]conformance.Result{} diff --git a/test/oracle/baseline-ops.txt b/test/oracle/baseline-ops.txt index 3e9a8b10..2c9ec1da 100644 --- a/test/oracle/baseline-ops.txt +++ b/test/oracle/baseline-ops.txt @@ -234,13 +234,13 @@ int :: 0 or 99 string :: true and true # and/or is evaluated directly per RFC 0005 rule 6, not as a call (see section header). -int | bool :: true and 0 +int :: true and 0 # and/or is evaluated directly per RFC 0005 rule 6, not as a call (see section header). -string | bool :: false or 'fallback' +string :: false or 'fallback' # and/or is evaluated directly per RFC 0005 rule 6, not as a call (see section header). -int | nulltype :: null or 7 +int :: null or 7 # The conditional expression is evaluated directly, not as a call (see section header). int :: 1 if true else 2 @@ -338,6 +338,14 @@ bool | int | string | range_expr :: 5 in range_expr("3-9") # "not in" over a range_expr charges its full expansion; the reference does not scale (see section header). bool | int | string | range_expr :: 1 not in range_expr("3-9") +# ARRIVED ON THE 0.11.4 PIN BUMP, and only as an operation-count case: until +# openjd-expr 0.3.0 this expression diverged on VALUE (the reference rejected a +# float against a list[int] container; openjd-rs#276 made int/float comparison +# exact), so its count was never compared. The count divergence it turns out to +# have is the same flat +1 as the three entries above. +# "in"/"not in" reference over-count on top of an agreeing container-length charge (see section header). +bool :: 2.0 in [1, 2, 3] + # ── sqi is right: unary negation of a non-literal or float operand ────────── # # RFC 0005 rule 2 transforms USub into a __neg__ call, so rule 1's flat @@ -415,16 +423,16 @@ string | float | int :: zfill(-1.5, 7) # above is sound. No corpus case subscripts a range_expr. # List-subscript reference over-count (see section header above). -int | list[int] :: [10, 20, 30][0] +any :: [10, 20, 30][0] # List-subscript reference over-count (see section header above). -int | list[int] :: [10, 20, 30][2] +any :: [10, 20, 30][2] # List-subscript reference over-count (see section header above). -int | list[int] :: [10, 20, 30][-1] +any :: [10, 20, 30][-1] # List-subscript reference over-count (see section header above). -int | list[int] :: [10, 20, 30][-3] +any :: [10, 20, 30][-3] # ADDED by the final whole-branch review, sub-project E1, and in the OPPOSITE # direction from the four list entries above: on a RANGE_EXPR receiver sqi @@ -452,7 +460,7 @@ int | string | range_expr :: range_expr("3-9")[-1] # over-count documented in the section above; the diff is exactly double. # Two chained subscripts, each paying the subscript over-count once (see section header). -int | list[list[int]] | list[int] :: [[1, 2], [3, 4]][1][0] +any :: [[1, 2], [3, 4]][1][0] # ── sqi is right: slice's receiver-dependent charge; reference = 2 + len(result) ── # @@ -470,49 +478,49 @@ int | list[list[int]] | list[int] :: [[1, 2], [3, 4]][1][0] # finding its own count is exactly "2 + len(result)" throughout. # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][1:4] +any :: [10, 20, 30, 40, 50][1:4] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][:3] +any :: [10, 20, 30, 40, 50][:3] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][2:] +any :: [10, 20, 30, 40, 50][2:] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][:] +any :: [10, 20, 30, 40, 50][:] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][::2] +any :: [10, 20, 30, 40, 50][::2] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][::-1] +any :: [10, 20, 30, 40, 50][::-1] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][0:5:2] +any :: [10, 20, 30, 40, 50][0:5:2] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][-3:] +any :: [10, 20, 30, 40, 50][-3:] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][:-2] +any :: [10, 20, 30, 40, 50][:-2] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][1:99] +any :: [10, 20, 30, 40, 50][1:99] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][99:] +any :: [10, 20, 30, 40, 50][99:] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -list[int] | int | nulltype :: [10, 20, 30, 40, 50][3:1] +any :: [10, 20, 30, 40, 50][3:1] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -string | int | nulltype :: 'hello'[1:3] +string :: 'hello'[1:3] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -string | int | nulltype :: 'hello'[::-1] +string :: 'hello'[::-1] # Slice reference over-count relative to sqi's receiver-dependent charge (see section header). -string | int | nulltype :: 'hello'[1:99] +string :: 'hello'[1:99] # ── sqi is right: list/range equality scales with element count; reference is flat 2 ── # @@ -533,148 +541,6 @@ bool :: [1, 2] == [1, 3] # List-equality reference over-count for a non-empty list (see section header). bool :: [1, 2] == [1] -# ── sqi is right: join() charges rule 2 AND rule 3; reference omits rule 3 ── -# -# Section 1.3.10 names join() under BOTH rule 2 ("iterate lists") and rule -# 3 ("process ... a string ... such as ... join()"), so sqi charges -# ArgElements (the input list's length) and ResultBytes (the joined -# string's length /256, rounded up) -- ResultBytes is also the only shape -# the Cost mechanism supports, since chargeArgs reads args[i].s and cannot -# express a per-element byte sum. The reference charges rule 2 but omits -# rule 3's byte charge entirely, despite the spec naming join() under it by -# name. Every entry below is exactly sqi's extra ResultBytes unit (the -# joined output never exceeds 256 bytes in this corpus, so the charge is -# always ceil(len/256) = 1). - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(['a', 'b', 'c'], ',') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(['a', 'b'], '') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(['a'], ',') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(split('a;b;c', ';'), ',') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(split('a,b,,c', ','), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(split(' a b '), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | int :: join(split('a,b,c', ',', 1), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | int :: join(rsplit('a b c', ' ', 1), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(rsplit('a,b,c', ','), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | int :: join(split('a,b,c', ',', 9223372036854775807), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(re_findall('shot010_shot020', 'shot(\d+)'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] :: join(re_split('a1b2c', '\d'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | int :: join(re_split('a1b2c', '\d', 1), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | nulltype :: join(re_search('asset_v042', '_v(\d+)'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | nulltype :: join(re_search('-', r'[\w-]'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | nulltype :: join(re_search('-', r'[\w\-a]'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | nulltype :: join(re_search('b', r'[\wa-c]'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | nulltype :: join(re_search('-', r'[\W-]'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | nulltype :: join(re_search('-', r'[a\W-]'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | nulltype :: join(re_search('c', r'[a\Wc]'), '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('s3://bucket//').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('/a/b/c').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('a/b').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('/').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('//').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('//a/b').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('///a/b').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('/a/b/').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('/a/../b').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('/a/./b').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('s3://b/d/f').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('s3://b/d/').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('s3://b').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('s3://b/d//x').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('c.tar.gz').suffixes, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('..a.b').suffixes, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('s3://b/d/f.tar.gz').suffixes, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join((path('s3://b/d/') / 'f').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join((path('/a/b') / 'c').parts, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[path] | path :: join([path('/a'), path('/b')], ',') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[path] | path :: join([path('/a') / 'x'], ',') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('..b.c').suffixes, '|') - -# join()'s ResultBytes charge; the reference omits rule 3 for join() entirely (see section header). -string | list[string] | path :: join(path('.hidden.tar.gz').suffixes, '|') - # ── sqi is right: is_relative_to/relative_to sum both operands' bytes; reference uses max ── # # Both take two path/string operands, each processed under rule 3; the Cost @@ -958,51 +824,108 @@ string | list[string] | list[list[string]] :: string(flatten([["-e", "A=1"], ["- # string(list)'s per-element ArgElements charge; reference is flat 1 (see section header). string | list[string] :: string(['ac&d']) -# ── sqi is right: the repr_* family's list rows charge per element; reference is flat 1 ── +# ── the repr_* family: sqi charges its ARGUMENT, the reference its RESULT ─── # -# Rule 2 names all five repr_*() functions EXPLICITLY (repr_sh, repr_py, -# repr_json, repr_pwsh, repr_cmd) -- the strongest textual basis of any -# entry in this file, since there is no "such as" ambiguity to resolve. -# sqi charges rule 1 (1) plus ArgElements for a list-typed argument on each; -# the reference never charges more than a flat 1 regardless of list size. +# REWRITTEN 2026-08-19 on the 0.11.4 pin bump, because the claim this section +# used to make went false without a single entry going stale -- the failure +# mode this file has no automatic check for. It read "sqi charges rule 1 plus +# ArgElements for a list-typed argument; the reference never charges more than +# a flat 1 regardless of list size", and the seven list rows below measured +# go=3 ref=1. On openjd-expr 0.3.0 they measure go=3 ref=4. Still diverging, +# still by one, in the OPPOSITE direction and for a different reason. A reason +# that has quietly inverted is worse than a missing one, hence a rewrite. +# +# What each side charges now: +# +# sqi 1 (rule 1) + ArgElements on a list-typed argument (rule 2) +# reference the same, plus one unit for the string it BUILDS +# +# Rule 2 names all five repr_* functions explicitly (repr_sh, repr_py, +# repr_json, repr_pwsh, repr_cmd), so the per-element charge is common ground +# and is not what these entries record. +# +# Rule 3 names exactly ONE of the five, repr_sh, and charges "the length of the +# value" -- the value the function processes, which is its argument. On that +# reading repr_sh('') charges 0 for an empty argument, and every scalar row +# charges 0 because there is no string argument at all: repr_py(42) processes +# an int. sqi's 1 is rule 1 alone. The reference's extra unit is the length of +# what it PRODUCED -- a value rule 3 does not name -- and it charges it for the +# four functions rule 3 never mentions as readily as for repr_sh. +# +# This is new upstream behavior, not a long-standing disagreement, and it is +# worth reporting: see docs/superpowers/specs/openjd-upstream-tracker.md. -# repr_*()'s per-element ArgElements charge on a list argument; reference is flat 1 (see section header). +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). string | list[string] :: repr_cmd(['echo', 'hello & world']) -# repr_*()'s per-element ArgElements charge on a list argument; reference is flat 1 (see section header). +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). string | list[string] :: repr_pwsh(['a', 'b']) -# repr_*()'s per-element ArgElements charge on a list argument; reference is flat 1 (see section header). +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). string | list[int] :: repr_pwsh([1, 2]) -# repr_*()'s per-element ArgElements charge on a list argument; reference is flat 1 (see section header). +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). string | list[string] :: repr_py(['a', 'b']) -# repr_*()'s per-element ArgElements charge on a list argument; reference is flat 1 (see section header). +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). string | list[int] :: repr_py([1, 2]) -# repr_*()'s per-element ArgElements charge on a list argument; reference is flat 1 (see section header). +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). string | list[string] :: repr_json(['a', 'b']) -# repr_*()'s per-element ArgElements charge on a list argument; reference is flat 1 (see section header). +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). string | list[int] :: repr_json([1, 2]) -# ── sqi is right: zfill charges ResultBytes; the reference's own count does not track it ── -# -# zfill's Cost is Cost{ResultBytes: true} on all three overloads (string, -# int, float receiver), matching rule 3's "processing a string ... such -# as ... similar" -- zfill pads a string to a target width, real work -# proportional to the result's length. Probed directly against the -# reference: a non-empty base string ("a".zfill(N) for N in 3, 300, 1000) -# charges a flat 2 regardless of width, matching sqi exactly when the -# result stays under 256 bytes (call 1 + ResultBytes 1 = 2) -- so most -# zfill cases in this corpus do NOT diverge at all. The one entry below is -# the exception: zfilling an EMPTY base string, where the reference's own -# count drops to 1 instead of its usual 2, an implementation-specific edge -# case sqi's principled ResultBytes charge has no reason to reproduce. - -# zfill on an empty base string: the reference's own count drops below its usual charge (see section header). -string | int :: zfill('', 3) +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string :: repr_sh('') + +# repr_*() on a list: sqi's per-element rule-2 charge, plus the reference's new result-string unit (see section header). +string | list[string] :: repr_sh(['echo', 'hello world']) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | int :: repr_pwsh(42) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | int :: repr_pwsh(-1) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | float :: repr_pwsh(1.5) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | bool :: repr_pwsh(true) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | bool :: repr_pwsh(false) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | nulltype :: repr_py(null) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | bool :: repr_py(true) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | bool :: repr_py(false) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | int :: repr_py(42) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | float :: repr_py(1.5) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | float :: repr_py(1.0) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | nulltype :: repr_json(null) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | bool :: repr_json(true) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | int :: repr_json(42) + +# repr_*() on a scalar: sqi charges rule 1 alone, the reference adds a unit for the result string it builds (see section header). +string | float :: repr_json(1.5) # ── sqi is right: min/max's fixed 2- and 3-arg rows charge nothing; reference charges 1+N ── # @@ -1117,7 +1040,6 @@ list[int] :: unique([2, 1, 2, 3, 1]) # divergence, not a new question. list[string] :: unique(["a", "b", "a"]) - # round(2.0, 309) joins the positive-ndigits count family already recorded # above: the reference charges ceil(ndigits/256) on top of the call, tracking # the length of the RENDERED decimal string. That string lives on Value.fs, a @@ -1127,3 +1049,88 @@ list[string] :: unique(["a", "b", "a"]) # header for round above; this case is new only because the corpus had no # large-ndigits round case until the positive-branch overflow fix added one. int | float :: round(2.0, 309) + +# ── sqi is right: padding charges ONE length term; the reference now charges two ── +# +# NEW WITH openjd-expr 0.3.0 (openjd-model 0.11.3+), found on the 0.11.4 pin +# bump. It REPLACES the "zfill charges ResultBytes; the reference's own count +# does not track it" section this file used to carry: that section's single +# entry, zfill('', 3), stopped diverging and was removed on the same run that +# turned these fifteen long-agreeing cases red. The old section's claim -- "a +# non-empty base string charges a flat 2 regardless of width, matching sqi +# exactly, so most zfill cases in this corpus do NOT diverge at all" -- is +# simply no longer true of the reference. +# +# Section 1.3.10 rule 3 charges "the length OF THE VALUE divided by 256 +# (rounded up)": one value per call. sqi charges the value these functions +# PRODUCE -- Cost{ResultBytes: true} on ljust/rjust/center and on all three +# zfill overloads -- so its count is 1 + ceil(len(result)/256). +# +# The reference now charges TWO length terms. Probed directly against +# openjd-model 0.11.4, one call per probe: +# +# ljust(<300-byte literal>, 5) 3 = 1 + ceil(300/256) + ceil(0/256) +# ljust('a', 256) 3 = 1 + ceil(1/256) + ceil(255/256) +# ljust('a', 257) 3 = 1 + ceil(1/256) + ceil(256/256) +# ljust('a', 300) 4 = 1 + ceil(1/256) + ceil(299/256) +# ljust('a', 600) 5 = 1 + ceil(1/256) + ceil(599/256) +# +# -- the argument's length, PLUS the length of the padding it adds. Each term +# is individually rule-3-shaped, which is what makes this a two-value reading +# of a rule that names one value, rather than an arbitrary flat +1. +# +# The two counts CONVERGE on large inputs, because the padding added is then +# empty: ljust(<600-byte literal>, 5) is 4 on both sides. Every entry below is +# therefore a small-string case, where the argument and the padding each round +# up to a unit of their own and the reference's second term is visible. + +# This row only became count-comparable on the 0.11.4 bump: its VALUE diverged +# until center's tie-break was changed to CPython's bias in the same commit +# (funcsstrpad.go), so the count behind it had never been compared. +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: center('ab', 7) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: ljust('ab', 5) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: rjust('ab', 5) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: center('abc', 4) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: center('é', 5) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: zfill('42', 5) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: zfill('é', 3) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: zfill('-10', 4) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: zfill('+7', 5) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: zfill('-', 4) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: zfill(42, 5) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: zfill(-1, 3) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | float | int :: zfill(1.5, 6) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | float | int :: zfill(2.0, 6) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: '42'.zfill(5) + +# Padding: the reference charges the argument's length AND the added padding's; rule 3 names one value (see section header). +string | int :: (42).zfill(5) diff --git a/test/oracle/baseline.txt b/test/oracle/baseline.txt index b4766be7..5f8d42b0 100644 --- a/test/oracle/baseline.txt +++ b/test/oracle/baseline.txt @@ -19,295 +19,6 @@ # — this file is a list of open questions, and one answering itself is worth # knowing about. -# ── sqi is right: the reference mishandles and/or under a target type ──────── -# -# Three entries, one root cause, and the evidence is that the reference gets -# all three RIGHT when no target type is supplied: -# -# false or 'fallback' -> 'fallback' : string -# null or 7 -> 7 : int -# true and 0 -> 0 : int -# -# Supplying a target type changes the ANSWER, not merely its coercion. With -# target=string, "false or 'fallback'" returns "false" — the operand the spec -# says is discarded. The reference appears to type and/or statically as a union -# of both operand types and push the target coercion through that union, which -# both loses the short-circuit and, when a member cannot coerce (bool->int, -# nulltype->int), rejects an expression that has a perfectly good value. -# -# Spec section 2.1.6 is unambiguous, and states the rule as a value rule rather -# than a typing rule: -# -# a and b If a is null or false, return a; otherwise evaluate and return b -# a or b If a is null or false, evaluate and return b; otherwise return a -# -# and adds that they "are value-returning: they return one of their operands, -# not necessarily a bool". sqi evaluates the operator first and coerces the -# operand it actually returned, which is what those rules describe. -# -# Worth reporting upstream. Until then these stay as documentation that the -# disagreement is known and which side the spec is on. - -# and/or under a target type: the reference errors, sqi returns 0 per 2.1.6. -int :: true and 0 - -# and/or under a target type: the reference returns the DISCARDED operand -# ("false"), sqi returns 'fallback' per 2.1.6. -string :: false or 'fallback' - -# and/or under a target type: the reference errors on nulltype->int, sqi -# returns 7 per 2.1.6 — the null operand is the one being discarded. -int :: null or 7 - -# ── sqi is right: float exponent rendering ────────────────────────────────── -# -# sqi renders 1e-05, the reference renders 1e-5. Python's repr — which sqi's -# formatFloat deliberately reproduces, and which the spec's own float examples -# follow — gives 1e-05, so sqi matches the language the spec defines itself as -# a subset of and the reference does not. Note the reference is not internally -# consistent about this either: it renders the positive exponent as 1e+16, -# keeping both the sign and the two-digit field it drops here. -# -# The spec does not state a rendering for computed floats outside the fixed -# notation window, so this is a real ambiguity rather than a plain bug. If it is -# ever settled against sqi, the change is confined to formatFloat in value.go. - -# Float exponent rendering: sqi gives 1e-05 (Python's repr), reference 1e-5. -float :: 0.00001 * 1.0 - -# ── irrecoverable: an explicit target bypasses the 1.2.6 rule under test ───── -# -# These two cases are pinned to an "any" target on purpose, not out of -# convenience: they exist to exercise section 1.2.6's rules 2 and 4, the -# element-type unification that runs when a list literal has NO single -# list[T] target to aim at, and any concrete target we could give them instead -# would skip that code path rather than exercise it. -# -# In sqi (internal/openjd/expr/list.go), evalListLit calls listElemTarget(target) -# to decide how to type a list literal's elements. listElemTarget returns a -# concrete T whenever the target names exactly one list[T] — and when it does, -# every element is evaluated against T and coerced to it individually; the -# unifyElemTypes/unifyElemPair pair (rules 2-7) is never called at all, -# because evalListLit only reaches for it when elemTarget.Code == CodeAny. -# So target=list[float] on "[1, 2.0, 3]" would still land on 2.0:1.0:3.0 -# — the right elements, arrived at by direct per-element coercion instead of -# by 1.2.6-rule-2's int/float unification, which is exactly what these cases -# were added to check. Nested lists ("[[1], [2.0]]", rule 4) have the same -# problem one level down: a target of list[list[float]] hands each inner list -# literal an elemTarget of list[float] and coerces it directly, bypassing the -# unifyElemPair recursion into nested list types that rule 4 is testing. -# -# A union target was tried as a middle ground — list[int] | list[float] does -# not name a single list[T], so listElemTarget falls through to TAny and -# unifyElemTypes runs — but it fails on sqi's own side before getting anywhere -# near the reference: coerce() rejects a list[float] result against a -# list[float] | list[int] target ("list[float] cannot be coerced to -# list[float] | list[int]"), a separate, unrelated gap in union-coercion -# admissibility for list members that is not what this task is investigating. -# So "any" is the only target this corpus format can give these two cases that -# still exercises 1.2.6 rules 2/4, and the cost is that the reference cannot -# evaluate that target at all (see the general ANY-target note below). -# -# The reference's failure mode is the same one document further down for the -# 16 cases that WERE moved to a concrete target: an explicit target_type=ANY -# errors "Cannot coerce to any" for every value, scalar or list — even -# evaluate_expression("1", target_type=ExprType("any")) raises "Cannot coerce -# int to any", while omitting target_type entirely evaluates it fine. The -# Recommended Library Interface's ExprType table defines ANY as "Unconstrained -# type (matches anything)", and the ExprValue section says outright that -# "ANY, UNION, and UNRESOLVED are type-level constructs used during type -# checking. They do not appear as the type of a concrete ExprValue at runtime -# — values always have a specific concrete type." An ANY target is a no-op -# constraint by that definition; the reference treats it as a concrete -# coercion target with rules that were never written. sqi returns the value -# unchanged, in its own concrete type, which is what the definition requires. - -# Irrecoverable under any concrete target — see the shared explanation above. -# Tests 1.2.6 rule 2 (mixed int/float unifies to list[float]) via -# unifyElemPair; a concrete target would coerce each element independently -# instead and never call that function. -any :: [1, 2.0, 3] - -# Irrecoverable under any concrete target — see the shared explanation above. -# Tests 1.2.6 rule 4 (list[int] and list[float] elements unify to -# list[list[float]]) via unifyElemPair's nested-list branch; a concrete -# target of list[list[float]] would coerce each inner list independently -# instead and never call that function. -any :: [[1], [2.0]] - -# ── sqi is right: the reference mistypes subscript/slice under a target type ─ -# -# Every subscript and slice case below fails, and the cause is not specific to -# lists, to ANY, or to any one target type: it reproduces with a plain `int` -# target, with a matching `list[int]` target, and with a matching `string` -# target alike, and it disappears completely when target_type is omitted. -# Reproduced directly against openjd-model 0.11.1: -# -# evaluate_expression("[10,20,30][0]", target_type=ExprType("int")) -# -> Cannot coerce list[int] to int (WRONG: should be 10 : int) -# evaluate_expression("[10,20,30][0]", target_type=ExprType("list[int]")) -# -> Cannot coerce int to list[int] (WRONG: the *opposite* mismatch) -# evaluate_expression("[10,20,30][0]") -> 10 : int (correct, no target) -# evaluate_expression("X[1:4]", target_type=ExprType("list[int]")) -# with X bound to [10,20,30,40,50] : list[int] -# -> Cannot coerce int to list[int] (WRONG: a slice returns a list) -# -# The reference's static type-checking pass for a target type does not model -# what `__getitem__` (section 2.1.7) or slicing (section 2.1.8) does to the -# static type of a postfix expression — a subscript narrows list[T] to T, a -# slice keeps list[T] (or keeps string as string) — and substitutes some other -# type into the check instead, which is why the two experiments above report -# opposite, self-contradictory mismatches for the identical expression. Spec -# sections 2.1.7 and 2.1.8 define subscript and slice purely in terms of the -# operand types, with no carve-out for the presence of a target type, and -# section 1.3.1 says a target type merely "guides implicit type coercion" — it -# does not license the checker to compute a different result type. sqi threads -# the target type through evaluation and coerces the actual computed value, -# which is what these sections require. - -# Subscript under a target type: see the shared explanation above. -any :: [10, 20, 30][0] - -# Subscript under a target type. -any :: [10, 20, 30][2] - -# Subscript under a target type, negative index. -any :: [10, 20, 30][-1] - -# Subscript under a target type, negative index. -any :: [10, 20, 30][-3] - -# Subscript under a target type, chained on a nested list. -any :: [[1, 2], [3, 4]][1][0] - -# Subscript under a target type, string character access. -string :: 'hello'[0] - -# Subscript under a target type, negative string index. -string :: 'hello'[-1] - -# Slice under a target type. -any :: [10, 20, 30, 40, 50][1:4] - -# Slice under a target type, omitted start. -any :: [10, 20, 30, 40, 50][:3] - -# Slice under a target type, omitted stop. -any :: [10, 20, 30, 40, 50][2:] - -# Slice under a target type, full copy. -any :: [10, 20, 30, 40, 50][:] - -# Slice under a target type, stepped. -any :: [10, 20, 30, 40, 50][::2] - -# Slice under a target type, reversed. -any :: [10, 20, 30, 40, 50][::-1] - -# Slice under a target type, start/stop/step all given. -any :: [10, 20, 30, 40, 50][0:5:2] - -# Slice under a target type, negative start. -any :: [10, 20, 30, 40, 50][-3:] - -# Slice under a target type, negative stop. -any :: [10, 20, 30, 40, 50][:-2] - -# Slice under a target type, stop past the end (Python-style clamping, 2.1.8). -any :: [10, 20, 30, 40, 50][1:99] - -# Slice under a target type, start past the end. -any :: [10, 20, 30, 40, 50][99:] - -# Slice under a target type, start after stop (empty result). -any :: [10, 20, 30, 40, 50][3:1] - -# Slice under a target type, string slice. -string :: 'hello'[1:3] - -# Slice under a target type, reversed string slice. -string :: 'hello'[::-1] - -# Slice under a target type, string slice with stop past the end. -string :: 'hello'[1:99] - -# ── sqi is right: the reference requires an exact element type for 'in' ────── -# -# `2.0 in [1, 2, 3]` errors in the reference with "Cannot use 'in' operator -# with list[int] and float", and the same happens in reverse: `1 in -# [1.0, 2.0, 3.0]` also errors there. Section 2.1.3's signature is -# `__contains__(list: list[T], item: T) -> bool`, and membership is naturally -# an equality test — `item in list` means "some element equals item" — so it -# should follow the language's own equality rule rather than requiring an -# exact static type match. Section 1.2.5 defines that rule for `==`/`!=` -# specifically: `int` vs `float` is "Numeric comparison (e.g., 5 == 5.0 is -# true)", not an error. Applying that same rule to membership (a sound -# analogy, not something 1.2.5 states for `in` itself) makes 2.0 equal the -# int 2, and 2 is an element of the list, so `2.0 in [1, 2, 3]` must be true. -# The reference appears to require the item's static type to unify exactly -# with T before considering the membership test at all, instead of falling -# back to element-by-element equality. sqi's containsElem uses the same -# cross-type equality `==` uses (internal/openjd/expr/ops.go), which is what -# `in` needs to mean even though 1.2.5 does not name the operator directly. - -# 'in' with a float item against a list[int]: reference rejects the mixed -# types outright instead of applying 1.2.5's int/float equality rule. -bool :: 2.0 in [1, 2, 3] - -# ── sqi is right: the reference mistypes list comprehensions under a target type ─ -# -# Both cases below evaluate correctly and produce the expected nested list -# WITHOUT a target type — reproduced directly against openjd-model 0.11.1: -# -# evaluate_expression("[[x] for x in [1, 2]]") -# -> [[1], [2]] : list[list[int]] (correct) -# evaluate_expression("[[y for y in [1, 2]] for x in ['a', 'b']]") -# -> [[1, 2], [1, 2]] : list[list[int]] (correct) -# -# Supplying the matching target_type=list[list[int]] makes both fail, and the -# error's caret position shows why: it does not point at the comprehension -# body (the expression that actually produces the result), it points at an -# ELEMENT OF THE INPUT ITERABLE instead — -# -# evaluate_expression("[[x] for x in [1, 2]]", target_type=ExprType("list[list[int]]")) -# -> Cannot coerce int to list[int] -# [[x] for x in [1, 2]] -# ^ <- caret lands on "1", the iterable's element -# evaluate_expression("[[y for y in [1, 2]] for x in ['a', 'b']]", -# target_type=ExprType("list[list[int]]")) -# -> Cannot coerce string to list[int] -# [[y for y in [1, 2]] for x in ['a', 'b']] -# ^~~ <- caret lands on 'a', the iterable's element -# -# The reference appears to peel one list[] layer off the target (list[int]) -# and check the SOURCE ITERABLE's element type against it, rather than typing -# the loop BODY expression and coercing the comprehension's result to the -# target — a `for x in [1, 2]` clause has nothing to do with `list[int]`, and -# the second case shows the same mechanism confusing an unrelated outer -# iterable (['a', 'b'] : list[string]) with the target entirely, even though -# the outer loop variable x is not even referenced in the body. -# -# Section 1.3.7 defines comprehension evaluation with no carve-out for a -# target type, and section 1.3.1 says a target type merely "guides implicit -# type coercion" of the result — it does not license substituting some other -# expression's type into the check. sqi evaluates the comprehension body per -# element and coerces the assembled list to the target afterward, which is -# what those sections require; the third comprehension case (list[string] :: -# [s for s in ['a', 'b']] above) does not show this bug only because its loop -# body is the identity function over an iterable that already matches the -# target, which happens to sidestep the miscomputation. - -# Comprehension under a target type: reference checks the INPUT iterable's -# element type against the target's peeled element type instead of the loop -# body's type — see the shared explanation above. Correct absent a target. -list[list[int]] :: [[x] for x in [1, 2]] - -# Comprehension under a target type, nested comprehension whose outer loop -# variable is unused in the body — see the shared explanation above. The -# reference's error names the OUTER iterable's element type even though the -# outer variable never appears in the body. Correct absent a target. -list[list[int]] :: [[y for y in [1, 2]] for x in ['a', 'b']] - # ── sqi is right: the reference widens round(x, ndigits <= 0) to float ─────── # # RFC 0006's signature table is explicit: "round(x: float, ndigits: int) -> @@ -522,42 +233,6 @@ 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) -# ── the reference is wrong: it PANICS or miscounts on an out-of-range width ── -# -# A width at or below the current length is a no-op in sqi, since RFC 0006 -# declares no error condition for it and inventing a rejection would add a rule -# the specification does not have. -# -# The reference instead: -# ljust('ab', -3) -> "Expression operation count (72057594037927938) -# exceeded limit (10000000)" — the negative width -# reinterpreted as unsigned -# center('ab', -3) -> the same operation-count error -# zfill('ab', -3) -> a Rust PANIC, "capacity overflow" -# A panic is not a considered behavior, and it is the strongest evidence -# available that the reference is not authoritative on out-of-range arguments. -# It also has no string-size bound at all: zfill('a', 20000000) builds a 20 MB -# string there. All of it is in expr-tracker.md's upstream ledger. -# -# NOT listed here: ljust('a', 100000000), where the reference panics with -# "Formatting argument out of range" (openjd-expr-0.2.1/src/functions/ -# string.rs:374) and sqi returns errTooLarge. Both sides FAIL, and agree() -# treats two failures as agreement without comparing messages, so that case is -# green and belongs only in the corpus. -# -# These cases are only runnable at all because scripts/expr-oracle.py catches -# BaseException — pyo3 surfaces a Rust panic as PanicException, which a -# narrower except would let kill the whole run. - -# Negative width, ljust direction: see the shared explanation above. -string | int :: ljust('ab', -3) - -# Negative width, center direction: see the shared explanation above. -string | int :: center('ab', -3) - -# Negative width, zfill direction: see the shared explanation above. -string | int :: zfill('ab', -3) - # ── sqi is right: the intersection rule rejects what one engine alone accepts ─ # # RFC 0006 states the accepted regular-expression syntax is the INTERSECTION of diff --git a/test/oracle/corpus.txt b/test/oracle/corpus.txt index c496ed1f..0cbdec3d 100644 --- a/test/oracle/corpus.txt +++ b/test/oracle/corpus.txt @@ -26,23 +26,39 @@ # TestPathOperators_Windows. A URI is NOT a flavor — it is detected from the # text under any path_format — so URI cases below are genuinely measured. # -# TARGET TYPES MUST BE UNIONS FOR ANY CASE CONTAINING A CALL, A SUBSCRIPT OR A -# SLICE, and this is not a style preference. The reference implementation -# applies the supplied target_type to a construct's OPERANDS as well as to its -# result — reported upstream as openjd-rs#291, and observed here on call -# arguments, subscript receivers and indices, slice bounds, and the operand -# and/or discards. So "floor(3.7)" with the target "int" fails there, because -# the target lands on the 3.7. Give the target a member for every operand and -# the pushdown becomes harmless: +# MANY TARGET TYPES BELOW ARE UNIONS WHERE A SINGLE TYPE WOULD DO, AND THAT IS +# HISTORY, NOT STYLE — read this before "simplifying" one. +# +# Through openjd-expr 0.2.1 (openjd-model <= 0.11.2) the reference applied the +# supplied target_type to a construct's OPERANDS as well as to its result — +# reported upstream as openjd-rs#291, and observed here on call arguments, +# subscript receivers and indices, slice bounds, and the operand and/or +# discards. So "floor(3.7)" with the target "int" failed there, because the +# target landed on the 3.7. Giving the target a member for every operand made +# the pushdown harmless: # # floor(3.7) int | float # sum([1,2,3]) int | list[int] # range(5) list[int] | int # 'hello'[1:3] string | int | nulltype # -# A case written with the obvious single target produces a divergence that -# looks real and is not. If you add a case and it diverges, check the target -# before reaching for baseline.txt. +# openjd-rs#297 FIXED THAT, and it shipped in openjd-expr 0.3.0 (openjd-model +# 0.11.3+), so as of the 0.11.4 pin the workaround is no longer needed. What +# was done about it, 2026-08-19: +# +# - The "coverage recovered from the openjd-rs#291 target pushdown" section +# — 27 TWIN cases that existed ONLY to carry a union target alongside a +# baselined narrow-target original — was DELETED. Its originals evaluate +# live now, which is exactly the "fail loudly the day the defect is fixed" +# the twins' own note promised, so they are the surviving copy. +# - The remaining union targets (e.g. "bool | string" on isdigit) were LEFT +# ALONE. Tightening them is a mechanical but wide change: every id here is +# a key in baseline.txt and baseline-ops.txt, so narrowing a target renames +# the case and orphans its entries. It is a separate pass, deliberately not +# bundled with the pin bump. +# +# So a union target below means "written when the pushdown was live", not "this +# case needs a union". A NEW case does not need one. # # A handful of cases are in deliberately and are expected to diverge. They earn # their place by DOCUMENTING a gap or a disagreement rather than leaving it @@ -501,64 +517,6 @@ string | list[string] | list[list[string]] string(flatten([["-e", "A=1"], ["-e", int | list[int] min([len("abcd"), 3]) list[int] | int sorted(range(5, 0, -1)) -# ── coverage recovered from the openjd-rs#291 target pushdown ──────────────── -# -# Each case below is a TWIN of an entry in baseline.txt. The baselined original -# keeps its narrow target and stays baselined: it is the evidence for the -# upstream defect, and it is designed to fail loudly the day that defect is -# fixed. The twin supplies the union target the rule above describes, which -# makes the pushdown harmless and turns the case into live differential -# coverage — coverage sub-project B2 recorded as unobtainable. -# -# Do not "consolidate" the two. Deleting an original orphans its baseline -# entry, which is a hard error, and loses the record of why these targets look -# strange. - -# Subscript twins (baseline target was "any" — unusable on its own, see -# baseline.txt's shared explanation for the subscript/slice section; the -# reference also cannot evaluate against a bare "any" target at all, a -# separate, unrelated limitation documented next to the list-literal cases -# above). Each twin's target is the actual result type unioned with the -# receiver's type and int, per the subscript rule. -int | list[int] [10, 20, 30][0] -int | list[int] [10, 20, 30][2] -int | list[int] [10, 20, 30][-1] -int | list[int] [10, 20, 30][-3] -int | list[list[int]] | list[int] [[1, 2], [3, 4]][1][0] -string | int 'hello'[0] -string | int 'hello'[-1] - -# Slice twins, same reasoning: receiver type, int and nulltype (an omitted -# bound is null) join the result type. -list[int] | int | nulltype [10, 20, 30, 40, 50][1:4] -list[int] | int | nulltype [10, 20, 30, 40, 50][:3] -list[int] | int | nulltype [10, 20, 30, 40, 50][2:] -list[int] | int | nulltype [10, 20, 30, 40, 50][:] -list[int] | int | nulltype [10, 20, 30, 40, 50][::2] -list[int] | int | nulltype [10, 20, 30, 40, 50][::-1] -list[int] | int | nulltype [10, 20, 30, 40, 50][0:5:2] -list[int] | int | nulltype [10, 20, 30, 40, 50][-3:] -list[int] | int | nulltype [10, 20, 30, 40, 50][:-2] -list[int] | int | nulltype [10, 20, 30, 40, 50][1:99] -list[int] | int | nulltype [10, 20, 30, 40, 50][99:] -list[int] | int | nulltype [10, 20, 30, 40, 50][3:1] -string | int | nulltype 'hello'[1:3] -string | int | nulltype 'hello'[::-1] -string | int | nulltype 'hello'[1:99] - -# and/or twins: add the type of the operand the operator discards. -int | bool true and 0 -string | bool false or 'fallback' -int | nulltype null or 7 - -# Comprehension twins: add the element type and the iterable's type. The -# second case is doubly nested — the outer body is itself a comprehension, so -# its element type (list[int]) and its iterable's type (list[string]) join -# the inner comprehension's own element type (int); the inner iterable -# (list[int]) is already covered by the outer body's element type. -list[list[int]] | list[int] | int [[x] for x in [1, 2]] -list[list[int]] | list[int] | list[string] | int [[y for y in [1, 2]] for x in ['a', 'b']] - # ── C2: string case transforms (spec section 2.2.4) ────────────────────────── string upper('hello') string lower('HeLLo') diff --git a/test/oracle/oracle_test.go b/test/oracle/oracle_test.go index 49eaa754..74069dfc 100644 --- a/test/oracle/oracle_test.go +++ b/test/oracle/oracle_test.go @@ -78,6 +78,7 @@ func TestExprOracle_MatchesReferenceImplementation(t *testing.T) { version, refs := runOracle(t, root, python, cases) t.Logf("reference implementation: openjd-model %s (%d cases)", version, len(cases)) + assertPinnedVersion(t, version) var diverged, expected int var compared, opsDiverged, opsExpected int @@ -206,6 +207,34 @@ func evalGo(c exprCase) caseResult { return caseResult{ok: true, value: v.String(), typ: v.Type.String(), ops: ops} } +// assertPinnedVersion fails when the reference implementation that actually +// answered is not the one the Makefile pins. +// +// It exists because the pin was, until 2026-08-19, advisory in exactly the case +// that matters: test-expr-oracle creates .venv-oracle only when it is MISSING, +// so raising OPENJD_MODEL_VERSION against an existing venv changed nothing and +// the suite went on grading sqi against the old reference. Nothing failed. The +// version was already logged, which is how the mismatch was eventually noticed +// — by reading the log, not by a red test, which is the wrong way round for a +// harness whose whole purpose is to be exact about which build produced a +// divergence. +// +// The expectation is supplied by the Makefile rather than duplicated here, so +// there is one pin, not two. Unset means "no expectation" — a bare +// `go test -tags oracle`, or a run against SQI_EXPR_ORACLE_PYTHON, still works. +func assertPinnedVersion(t *testing.T, version string) { + t.Helper() + want := os.Getenv("SQI_EXPR_ORACLE_EXPECT_VERSION") + if want == "" || version == want { + return + } + t.Fatalf("reference implementation is openjd-model %s, but the pin is %s: "+ + "the venv predates the pin bump. Recreate it with "+ + "`rm -rf .venv-oracle && make expr-oracle-venv`, or clear "+ + "SQI_EXPR_ORACLE_EXPECT_VERSION to grade against whatever is installed.", + version, want) +} + // mustTarget parses c's target type for EvalForBalanceCheck, which needs a // expr.Type rather than the raw string evalGo works from. Called only on cases // where evalGo already succeeded, which means its own identical ParseType call diff --git a/third_party/openjd-specifications b/third_party/openjd-specifications index 42a1fb67..be0aefb8 160000 --- a/third_party/openjd-specifications +++ b/third_party/openjd-specifications @@ -1 +1 @@ -Subproject commit 42a1fb674c94aea586d4e7ebc5250f6706633ffe +Subproject commit be0aefb83e4d5b15aa89ed81f67424e85908282d