test(conformance): add a cross-language conformance suite for the expression parser - #2497
test(conformance): add a cross-language conformance suite for the expression parser#2497diegolopezrm wants to merge 7 commits into
Conversation
…rror
A number literal the scanner accepts but neither language can parse, such
as `${1.2.3}`, left each implementation somewhere different: Dart threw a
`FormatException` from `num.parse` — an error outside the `A2uiError`
hierarchy, which `avoid_catching_errors` discourages catching — while
TypeScript's `Number()` returned NaN and handed it back as a parsed
value, silently placing a non-JSON value in the parse tree.
Both now throw `A2uiExpressionError`, like every other parse failure.
…parser The expression parser behind `formatString` is implemented once per client language, each a port of the others, and nothing compares them. This adds `core/expressions.yaml`: 30 cases covering literals, data bindings, function calls, nested interpolation, escaped markers and the parse errors, expressed once for every implementation to run. Cases are compared with adjacent literal parts joined, so a case fixes what a template means rather than how an implementation splits the literal text around its values. Errors use the suite's language-agnostic categories, with `ParseError` mapping to each SDK's expression error.
Adds the Dart harness for `core/expressions.yaml`. It locates the suite by walking up from the working directory, so it needs no configured path, and maps the suite's error categories onto `A2uiExpressionError`. 30 cases, all passing.
Adds the TypeScript harness for `core/expressions.yaml`, following the same shape as the Dart one: walk up to find the suite, map `ParseError` onto `A2uiExpressionError`, compare with adjacent literals joined. Pulls in js-yaml as a dev dependency to read the suite. 30 cases, all passing.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a shared conformance test suite for the client-side expression parser (core/expressions.yaml) along with corresponding test harnesses in both Dart and TypeScript. It also updates the expression parsers in both languages to handle invalid number literals. The review feedback highlights potential cross-language and platform-specific discrepancies in how numbers are parsed (e.g., differences between Dart VM, Dart Web, and JavaScript/TypeScript). To ensure strict behavioral parity, the reviewer suggests validating number literals against a standardized regular expression before conversion in both the TypeScript and Dart implementations.
| const text = scanner.input.substring(start, scanner.pos); | ||
| const value = Number(text); | ||
| if (!Number.isFinite(value)) { | ||
| throw new A2uiExpressionError(`Invalid number literal: '${text}'`); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
Using Number(text) and Number.isFinite(value) can lead to subtle cross-language and platform-specific discrepancies. For example, Number("1.") and Number(".5") are parsed as valid numbers in JavaScript/TypeScript, whereas in Dart (especially on the VM), num.tryParse("1.") and num.tryParse(".5") return null and will throw an error. Additionally, Number("") evaluates to 0 (which is finite), potentially allowing empty number literals to pass silently if the scanner doesn't advance.
To ensure strict 1:1 behavioral parity across all client implementations and platforms, we should validate the parsed number literal against a standardized decimal/integer regular expression (e.g., /^\d+(?:\.\d+)?$/) before converting it.
| const text = scanner.input.substring(start, scanner.pos); | |
| const value = Number(text); | |
| if (!Number.isFinite(value)) { | |
| throw new A2uiExpressionError(`Invalid number literal: '${text}'`); | |
| } | |
| return value; | |
| const text = scanner.input.substring(start, scanner.pos); | |
| if (!/^\d+(?:\.\d+)?$/.test(text)) { | |
| throw new A2uiExpressionError("Invalid number literal: '" + text + "'"); | |
| } | |
| return Number(text); |
References
- Maintain strict 1:1 structural and behavioral parity across different client implementations (such as Dart and TypeScript) to prevent subtle behavioral drift.
There was a problem hiding this comment.
Adopted the regex in both implementations, though the premise needs a correction.
num.tryParse("1.") returns 1.0 on the Dart VM, not null, and .5 returns 0.5. I also compiled the same code with dart2js and ran it under Node to check the VM/web split specifically — the two agree on every input tried:
| input | Dart VM | dart2js | Number() |
|---|---|---|---|
1. |
1.0 | 1 | 1 |
.5 |
0.5 | 0.5 | 0.5 |
"" |
null | null | 0 |
1.2.3 |
null | null | NaN |
So the drift you describe is not there today. .5 and "" also cannot reach this function — the scanner only enters it on a digit, and stops at anything that is not a digit or a point.
The underlying point stands anyway, and it is the better design: the accepted shape should be stated, not inherited from whichever number parser the platform ships. Both now use the grammar you suggested. The visible consequence is that ${1.} moves from accepted-by-accident to rejected in both, so I added a conformance case for it in core/expressions.yaml — that way the answer lives in the shared suite instead of in two platform parsers, and changing our minds means editing one case.
| final String text = scanner.input.substring(start, scanner.pos); | ||
| final num? value = num.tryParse(text); | ||
| if (value == null) { | ||
| throw A2uiExpressionError("Invalid number literal: '$text'"); | ||
| } | ||
| return value; |
There was a problem hiding this comment.
Using num.tryParse(text) can lead to platform-specific discrepancies between Dart VM and Dart Web (where it compiles to JavaScript's parseFloat/Number and might accept trailing or leading decimals like 1. or .5 that Dart VM rejects).
To guarantee strict 1:1 behavioral parity across all platforms and client implementations, we should validate the parsed number literal against a standardized decimal/integer regular expression before parsing it.
| final String text = scanner.input.substring(start, scanner.pos); | |
| final num? value = num.tryParse(text); | |
| if (value == null) { | |
| throw A2uiExpressionError("Invalid number literal: '$text'"); | |
| } | |
| return value; | |
| final String text = scanner.input.substring(start, scanner.pos); | |
| if (!RegExp(r"^\d+(?:\.\d+)?$").hasMatch(text)) { | |
| throw A2uiExpressionError("Invalid number literal: '$text'"); | |
| } | |
| return num.parse(text); |
References
- Maintain strict 1:1 structural and behavioral parity across different client implementations (such as Dart and TypeScript) to prevent subtle behavioral drift.
There was a problem hiding this comment.
Same change applied here; see the reply on the TypeScript side for the measurements, including a dart2js run that shows the VM and web behave the same on these inputs.
Both implementations delegated the shape of a number literal to their
platform's parser — `num.parse` and `Number()` — which agree today but
need not: each accepts inputs the grammar never intended, `Number('')`
being 0 among them, and neither is specified anywhere.
The accepted shape is now written out as digits, optionally followed by a
decimal point and more digits, identically in both. `${1.}` moves from
accepted-by-accident (1.0 in Dart, 1 in TypeScript) to rejected in both,
and a conformance case pins that so the answer lives in the suite rather
than in two platform number parsers.
Ran the suite's inputs through the Dart, TypeScript, Python and Swift
parsers. Two cases turned out to encode a choice rather than the shared
behaviour, and both are fixed here.
The number literal grammar rejected a trailing point (`${1.}`). All four
implementations accept it today, so this had turned a unanimous
behaviour into a 2-2 split — the grammar now reads digits, an optional
point, and optional further digits, keeping `${1.2.3}` rejected as all
four already do.
The empty template pinned `[""]`, which only Dart and TypeScript produce;
Python and Swift return `[]`. An empty literal carries no content either
way, so the harnesses now drop empty literals alongside joining adjacent
ones, and the case no longer pins a splitting choice.
All 31 cases now pass in all four implementations.
|
Update: I ran the suite's inputs through all four expression parser implementations in the repository — Two of them did not, and both were mine rather than theirs. The number literal grammar was wrong. I had it reject a trailing point (
So that change turned a unanimous behaviour into a 2-2 split, which is the opposite of what this PR is for. The grammar now reads digits, an optional point, and optional further digits — still stated in each parser rather than delegated to the platform's number parser, still rejecting The empty-template case pinned a splitting choice. It expected Both corrections point the same way: a conformance case should record what the implementations agree on, or a decision the maintainers have taken, and never a preference of whoever wrote the case. Checking against four implementations instead of two is what surfaced the difference, and it seems worth doing for any suite added here. Two things the same run turned up, which belong in their own issues rather than this PR:
|
…ser-conformance # Conflicts: # conformance/README.md # conformance/conformance_schema.json
github-actions
Bot
added
status: needs-triage
|
Hi @gspencergoog, I have pulled and verified the changes locally, and all tests pass as expected. Could you please review this PR? |
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
This is great! Thank you for implementing the conformance tests for this.
|
Once you update the changelog, I can commit this. |
| /// | ||
| /// Every client implementation accepts a trailing point (`1.`) today and none | ||
| /// accepts a second point (`1.2.3`), so the grammar is written to keep that. | ||
| final RegExp _numberLiteral = RegExp(r'^\d+\.?\d*$'); |
There was a problem hiding this comment.
This doesn't allow negative number literals. Or exponential notation. Or commas instead of decimals (locale support). (I don't think we actually support commas elsewhere yet either, but we shouldn't rule it out.)
There was a problem hiding this comment.
Oh, actually, commas can't work anyhow because of the parser (arguments are comma-separated). We'd have to do that in the presentation layer.
3 participants
Description
Addresses part of #2496.
The client-side expression parser behind
formatStringexists once per language, each implementation a close port of the others, and nothing compares them. Running the Dart and TypeScript parsers over the same 2,592 templates, they disagree on 408 — 137 of those on what a template actually means rather than on how the parts array is split.This PR does two things: it fixes the one disagreement that is unambiguously a bug in both, and it adds a conformance suite so the next drift is caught by CI instead of by someone diffing two parsers by hand.
1. Invalid number literals
A literal the scanner accepts but neither language can parse —
${1.2.3},${1..2}— ended somewhere different in each:FormatExceptionfromnum.parseA2uiExpressionErrorNumber()returnsNaN, handed back as a parsed valueA2uiExpressionErrorBoth were wrong in their own way.
FormatExceptionis outside theA2uiErrorhierarchy consumers catch, and oneavoid_catching_errorsdiscourages catching at all;NaNis not a JSON value and not a member ofDynamicValue, so web_core was placing a value in the parse tree that the protocol cannot express, silently. Every other parse failure in both raisesA2uiExpressionError, and now so does this one.2.
conformance/core/expressions.yamlA shared suite of 30 cases — literals, data bindings, function calls, nested interpolation, escaped markers, whitespace, and the five parse errors — expressed once and run by both clients. It follows the existing structure in
conformance/, andconformance/tests/test_conformance_yaml.pyvalidates it againstconformance_schema.jsonwith no change to that test.Supporting changes:
conformance_schema.json: adds theparse_expression_templateaction and its case shape, alongside the existing ones.conformance/README.md: documents the suite, the two client harnesses, and how to write cases for the new action.dart/a2ui_core/test/conformance/expressions_conformance_test.dart.renderers/web_core/src/v0_9/basic_catalog/expressions/expression_parser.conformance.test.ts.Both harnesses locate the suite by walking up from the test, so neither needs a configured path, and both map the suite's language-agnostic error categories onto their own types (
ParseError→A2uiExpressionError). js-yaml comes in as a web_core dev dependency, matching the approach in #2182.As far as I can tell these are the first conformance suites run by the client implementations —
conformance/is currently consumed by the Python and Kotlin agent SDKs — so the same pattern is available to Swift and Kotlin clients next.What the suite deliberately does not decide
Two of the divergences in #2496 are open questions rather than bugs, and this PR does not quietly settle either:
nullkeyword. No case uses it. The guide listsnullas a literal the parser must recognise whileDynamicValuehas no null branch, so any expectation I wrote would be me picking a side in a specification contradiction. [BUG]: Dart and web_core expression parsers disagree on 408 of 2,592 templates #2496 lays out the options.Verification
conformance:pytest conformance/tests/test_conformance_yaml.py— 9 passed, the new suite validating with the rest.dart testindart/a2ui_core— 104 passing (74 before, plus the 30 conformance cases).dart analyzereports the same two pre-existingunnecessary_library_directiveinfos as main and nothing new;dart formatclean.node --test "dist/**/*.test.js"inrenderers/web_core— 447 passing, 0 failing.eslintreports no new problems (the twono-explicit-anywarnings are pre-existing);prettier --checkclean on everything touched.nullkeyword left to [BUG]: Dart and web_core expression parsers disagree on 408 of 2,592 templates #2496.A note on the checklist below
As in #2491 and #2494, I have not touched
dart/a2ui_core/CHANGELOG.md; #2439 already adds a## 0.2.0section there and a competing one would only give that PR a conflict.renderers/web_core/CHANGELOG.mdI did leave alone deliberately as well — the web_core change here is test-only plus the number-literal fix, and I would rather add the entry in the wording you prefer. Happy to add both on request.Pre-launch Checklist
One time:
For this PR:
conformance/README.md)