From 8deddb79bb5c24b4bb62beeac1f6bf4f2e48a2d0 Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Fri, 28 Aug 2026 09:32:22 +0300 Subject: [PATCH 1/3] fix(data,export): collapse the STEP string escaper's two TS copies into one (#3300) @ifc-lite/data's step-serializers.ts and @ifc-lite/export's step-serialization.ts each kept a byte-identical escapeStepString (backslash/quote doubling, \X2\/\X4\ non-ASCII directives, one space per control char). export already depends on data with no cycle, so data now exports the function and export re-exports it instead of keeping its own body; every existing call site is unaffected. The rust implementation (ifc_lite_export::step_text::escape) stays separate -- a wasm adapter to share it is out of scope here -- and stays pinned by a hand-kept vector test rather than shared code. #3284's parity test, which asserted the two TS copies agreed with Rust, is gone from the export package (there is nothing left to duplicate there) and the coverage lives once in data's step-serializers.test.ts, which also gains direct tests of escapeStepString now that it is exported. Verified today's three implementations agree on backslash, apostrophe, non-ASCII, an already-escaped-looking sequence, and the empty string before collapsing. The encoding package's separate encodeIfcString is a genuinely different, partial implementation (whole-Latin-1 \X\HH range, no '' doubling) and is left alone per the issue. Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436 --- .changeset/collapse-step-escaper-copies.md | 10 +++ packages/data/src/index.ts | 1 + packages/data/src/step-serializers.test.ts | 50 +++++++++++-- packages/data/src/step-serializers.ts | 12 ++- .../export/src/step-serialization.test.ts | 73 +++---------------- packages/export/src/step-serialization.ts | 71 +++--------------- scripts/api-surface.json | 1 + 7 files changed, 86 insertions(+), 132 deletions(-) create mode 100644 .changeset/collapse-step-escaper-copies.md diff --git a/.changeset/collapse-step-escaper-copies.md b/.changeset/collapse-step-escaper-copies.md new file mode 100644 index 0000000000..af4a133050 --- /dev/null +++ b/.changeset/collapse-step-escaper-copies.md @@ -0,0 +1,10 @@ +--- +'@ifc-lite/data': patch +'@ifc-lite/export': patch +--- + +The STEP string escaper (`escapeStepString`) existed twice in TypeScript: once, private, in `@ifc-lite/data`'s `step-serializers.ts`, and again, exported, in `@ifc-lite/export`'s `step-serialization.ts`. The two bodies were identical — same backslash/quote doubling, same `\X2\`/`\X4\` non-ASCII directive thresholds, same one-space-per-control-character rule. `@ifc-lite/export` already depends on `@ifc-lite/data` with no cycle, so there was no reason for the second copy. + +`@ifc-lite/data` now exports `escapeStepString` as the single implementation; `@ifc-lite/export`'s `step-serialization.ts` re-exports it instead of keeping its own copy, so every existing call site is unaffected. The Rust implementation, `ifc_lite_export::step_text::escape`, stays separate — sharing it with TypeScript would need a wasm adapter, which is a bigger change than this one — and continues to be pinned by a hand-kept vector test rather than shared code. + +A prior fix (#3284) added a test in each of the two TypeScript files asserting that both matched the Rust half's output on the same inputs; with only one TypeScript implementation left, that duplication is gone and the coverage lives once, in `@ifc-lite/data`'s `step-serializers.test.ts`. diff --git a/packages/data/src/index.ts b/packages/data/src/index.ts index 833218b965..8b2e09c04e 100644 --- a/packages/data/src/index.ts +++ b/packages/data/src/index.ts @@ -56,6 +56,7 @@ export { generateHeader, generateStepFileWithRegistry, parseStepValue, + escapeStepString, } from './step-serializers.js'; export type { StepValue, diff --git a/packages/data/src/step-serializers.test.ts b/packages/data/src/step-serializers.test.ts index a3f610189d..3598ef1312 100644 --- a/packages/data/src/step-serializers.test.ts +++ b/packages/data/src/step-serializers.test.ts @@ -5,6 +5,7 @@ import { describe, it, expect } from 'vitest'; import { enumVal, + escapeStepString, formatStepReal, generateHeader, generateStepFileWithRegistry, @@ -233,13 +234,16 @@ describe('toStepLineWithRegistry / generateStepFileWithRegistry', () => { /** - * The `@ifc-lite/data` serializer's escaper is the second TS copy of the rule - * pinned in `@ifc-lite/export`'s `step-serialization.test.ts` and in - * `ifc_lite_export::step_text::escape`: ONE SPACE PER CONTROL CHARACTER, not - * one per run (#3284 item 2). `escapeStepString` is module-private here, so a - * run reaches it through the two public funnels that use it — `serializeValue` - * for an attribute value and `generateHeader` for a header field. The expected - * strings are the Rust half's observed output for the same inputs. + * `escapeStepString` is now the ONE TypeScript implementation (#3300): + * `@ifc-lite/export`'s `step-serialization.ts` re-exports this symbol rather + * than keeping its own copy, so this suite is the only place TS-side coverage + * needs to live. `ifc_lite_export::step_text::escape` stays a separate, + * hand-kept Rust implementation — a wasm adapter to share it is out of scope + * here — so the rule below (ONE SPACE PER CONTROL CHARACTER, not one per run, + * #3284 item 2) is pinned against the Rust half's observed output rather than + * shared code. Exercised through the two public funnels that use it — + * `serializeValue` for an attribute value and `generateHeader` for a header + * field — plus `escapeStepString` itself directly, now that it is exported. */ describe('control-character runs are one space each (#3284, parity with the Rust escape)', () => { const RUST_VECTORS: ReadonlyArray = [ @@ -275,3 +279,35 @@ describe('control-character runs are one space each (#3284, parity with the Rust } }); }); + + +/** + * `escapeStepString` exported directly (#3300): the two TS copies of the + * encode half collapsed into this one, re-exported by `@ifc-lite/export`. + * Awkward inputs a caller could plausibly pass through -- a bare backslash, + * an apostrophe, a BMP non-ASCII character, text that already looks like a + * `\X2\` directive, and the empty string. + */ +describe('escapeStepString direct (#3300)', () => { + it('doubles a lone backslash', () => { + expect(escapeStepString('a\\b')).toBe('a\\\\b'); + }); + + it('doubles a lone apostrophe', () => { + expect(escapeStepString("a'b")).toBe("a''b"); + }); + + it('encodes a non-ASCII character as an \\X2\\ directive', () => { + expect(escapeStepString('\u00C4')).toBe('\\X2\\00C4\\X0\\'); + }); + + it('doubles the backslashes of text that already looks like a directive', () => { + // The input is literal text, not an actual directive: every backslash in + // it must be doubled like any other, or a reader would decode it as one. + expect(escapeStepString('a\\X2\\00FC\\X0\\b')).toBe('a\\\\X2\\\\00FC\\\\X0\\\\b'); + }); + + it('returns the empty string unchanged', () => { + expect(escapeStepString('')).toBe(''); + }); +}); diff --git a/packages/data/src/step-serializers.ts b/packages/data/src/step-serializers.ts index 36b394f9d0..cda0f5b5fe 100644 --- a/packages/data/src/step-serializers.ts +++ b/packages/data/src/step-serializers.ts @@ -188,10 +188,16 @@ export function serializeValue(value: StepValue): string { * `\X2\HHHH\X0\`, `\X4\HHHHHHHH\X0\`), never a raw byte, and buildingSMART * says the same for IFC2X3/IFC4/IFC4X3. A reader decoding as ISO-8859-1 (what * the base standard and most consumers assume) turns raw UTF-8 into mojibake or - * a broken parse: IfcOpenShell#699/#1016, files rejected by Solibri. Matches - * `@ifc-lite/export`'s `escapeStepString`. + * a broken parse: IfcOpenShell#699/#1016, files rejected by Solibri. + * + * This is the SINGLE TypeScript implementation (#3300): `@ifc-lite/export`'s + * `step-serialization.ts` re-exports this symbol rather than keeping its own + * copy. `ifc_lite_export::step_text::escape` is a separate, hand-kept Rust + * implementation — collapsing that one too would need a wasm adapter, which + * is a bigger change than this issue's scope, so the TS/Rust agreement is + * pinned by a vector test instead of shared code. */ -function escapeStepString(str: string): string { +export function escapeStepString(str: string): string { const escaped = str .replace(/\\/g, '\\\\') // Backslash .replace(/'/g, "''") // Single quote diff --git a/packages/export/src/step-serialization.test.ts b/packages/export/src/step-serialization.test.ts index 0b58481842..19288cd9a0 100644 --- a/packages/export/src/step-serialization.test.ts +++ b/packages/export/src/step-serialization.test.ts @@ -196,69 +196,18 @@ describe('toStepRealScaled', () => { }); }); -describe('escapeStepString non-ASCII encoding (ISO 10303-21 6.3.3.4)', () => { - // ISO 10303-21 restricts a string literal's plain-text bytes to the "basic - // graphic" range 32-126; anything outside it is a control directive - // (\X\HH, \X2\HHHH\X0\, \X4\HHHHHHHH\X0\), never a raw byte. buildingSMART's - // own IFC string-encoding guidance states the same for IFC2X3/IFC4/IFC4X3: - // "characters ... represented by decimal value 32 to 126 ... any other - // character ... has to be encoded" (e.g. German 'Ä' as '\X2\00C4\X0\'). - // A reader that treats the file bytes as ISO-8859-1 (the byte encoding real - // consumers - and the base standard - assume) turns a raw UTF-8 multi-byte - // sequence into mojibake or an outright parse break; this is a reported, - // reproduced defect in real IFC tooling (IfcOpenShell#699, files rejected - // by Solibri) for exactly this shape of writer bug. - it('encodes a BMP character as \\X2\\HHHH\\X0\\, not raw UTF-8', () => { - expect(escapeStepString('Trümpler')).toBe('Tr\\X2\\00FC\\X0\\mpler'); - }); - - it('encodes a non-BMP character (emoji) as \\X4\\HHHHHHHH\\X0\\', () => { - expect(escapeStepString('😀')).toBe('\\X4\\0001F600\\X0\\'); - }); - - it('leaves printable ASCII untouched', () => { - expect(escapeStepString('plain text 123')).toBe('plain text 123'); - }); -}); - - /** - * A run of control characters becomes ONE SPACE PER CHARACTER (#3284 item 2). - * - * The expectations below are not invented: they are the observed output of the - * Rust half, `ifc_lite_export::step_text::escape`, over the same six inputs — - * the doc comment on each escaper claims it "matches" the other, and until - * this fix the TS `/[\x00-\x1F\x7F]+/g` collapsed `"a\t\t\tb"` to `'a b'` - * while Rust wrote `'a b'`. ISO 10303-21 6.3.3.4 mandates neither (it only - * bars the control byte from the literal), so the tie is broken by the parity - * claim and by information loss: collapsing discards the run's length. + * `escapeStepString` no longer has an implementation here (#3300): this + * package re-exports `@ifc-lite/data`'s. Full coverage -- non-ASCII `\X2\`/ + * `\X4\` directives, one-space-per-control-char (#3284), and the vector + * comparison against `ifc_lite_export::step_text::escape` -- lives once, in + * `packages/data/src/step-serializers.test.ts`. This is a wiring check that + * the re-export resolves to a working function, not a second copy of that + * suite. */ -describe('escapeStepString control-character runs (#3284, parity with the Rust escape)', () => { - // label, input, and the output the Rust half printed for that input. - const RUST_VECTORS: ReadonlyArray = [ - ['tab run', 'a\t\t\tb', 'a b'], - ['crlf', 'a\r\nb', 'a b'], - ['mixed C0 + DEL', 'a\u0000\u000B\u001F\u007Fb', 'a b'], - ['single control char', 'a\tb', 'a b'], - ['quote doubling around a run', "O'Brien\t\tx", "O''Brien x"], - // Negative control: no control characters at all, byte-identical output. - ['no control chars', 'plain text 123', 'plain text 123'], - ]; - - it.each(RUST_VECTORS)('%s escapes exactly as the Rust half does', (_label, input, expected) => { - expect(escapeStepString(input)).toBe(expected); - }); - - it('preserves the length of every control run and emits no control byte', () => { - // Both directions of the rule in one place: the output must have the same - // length as the input (one space per control character), and must contain - // no control character (a run left intact would also keep its length, so - // neither half alone is sufficient). - for (const n of [1, 2, 3, 8]) { - const escaped = escapeStepString(`a${'\n'.repeat(n)}b`); - expect(escaped).toBe(`a${' '.repeat(n)}b`); - // eslint-disable-next-line no-control-regex - expect(escaped).not.toMatch(/[\u0000-\u001F\u007F]/); - } +describe('escapeStepString (re-exported from @ifc-lite/data, #3300)', () => { + it('escapes quotes, backslashes and non-ASCII the way @ifc-lite/data does', () => { + expect(escapeStepString("O'Brien\\x")).toBe("O''Brien\\\\x"); + expect(escapeStepString('\u00C4')).toBe('\\X2\\00C4\\X0\\'); }); }); diff --git a/packages/export/src/step-serialization.ts b/packages/export/src/step-serialization.ts index bf4315681f..47a0117c0d 100644 --- a/packages/export/src/step-serialization.ts +++ b/packages/export/src/step-serialization.ts @@ -22,7 +22,17 @@ */ import { serializeValue, SCHEMA_REGISTRY, type IfcAttributeValue } from '@ifc-lite/parser'; -import { QuantityType, formatStepReal } from '@ifc-lite/data'; +import { QuantityType, formatStepReal, escapeStepString } from '@ifc-lite/data'; + +/** + * Re-exported so every existing `import { escapeStepString } from + * './step-serialization.js'` call site keeps working. The implementation + * lives once, in `@ifc-lite/data` (#3300) — this package used to keep its + * own byte-identical copy, which is exactly the duplication + * `packages/codegen/test/serialization-generator.test.ts` already forbids + * for the schema-agnostic serializer bundles. + */ +export { escapeStepString }; /** EXPRESS base primitives a defined type ultimately resolves to. */ const EXPRESS_PRIMITIVES = new Set(['BOOLEAN', 'LOGICAL', 'INTEGER', 'REAL', 'NUMBER', 'STRING', 'BINARY']); @@ -115,65 +125,6 @@ export function serializeTypedMarker(type: string, value: string | number | bool return `${token}(${serializeInnerByBase(value, resolveExpressBase(type))})`; } -/** - * Escape a string for STEP format: backslash and single-quote doubling, plus - * non-ASCII directive encoding. - * - * Control characters (CR/LF and other C0 codes, plus DEL) are each replaced by - * ONE space — a run of n control characters becomes n spaces, not one — so - * every generated STEP entity stays on one physical line and round-trips - * through the line-oriented merge/convert paths without losing length. - * Collapsing a run (`/[...]+/`, which this did until #3284) disagreed with - * `ifc_lite_export::step_text::escape`, which has always mapped each control - * character to its own space: the same value written by the two halves came - * out different. ISO 10303-21 6.3.3.4 mandates neither, but preserving the - * count loses no information. - * - * ISO 10303-21 6.3.3.4 restricts a string literal's plain-text bytes to the - * "basic graphic" range 32-126; every other character is a control directive - * (`\X\HH`, `\X2\HHHH\X0\`, `\X4\HHHHHHHH\X0\`), never a raw byte — and - * buildingSMART's IFC string-encoding guidance states the same for the - * IFC2X3/IFC4/IFC4X3 schemas this exporter targets: characters outside - * decimal 32-126 "have to be encoded" (e.g. 'Ä' as `\X2\00C4\X0\`). A reader - * that treats the file's bytes as ISO-8859-1 — the byte encoding the base - * standard and most real consumers assume for these schemas — turns a raw - * UTF-8 multi-byte sequence into mojibake or a broken parse; this exact - * writer shape is a reported, reproduced defect in real IFC tooling - * (IfcOpenShell#699/#1016; files rejected by Solibri). Encoded one character - * at a time (never batching a run into one `\X2\...\X0\` block) to keep the - * encoder trivially correct; ISO 10303-21 allows either. - */ -export function escapeStepString(str: string): string { - const escaped = str - .replace(/\\/g, '\\\\') - .replace(/'/g, "''") - // eslint-disable-next-line no-control-regex - .replace(/[\x00-\x1F\x7F]/g, ' '); - return encodeNonAsciiStepDirectives(escaped); -} - -/** - * Replace every character outside the STEP "basic graphic" range (32-126) - * with its `\X2\`/`\X4\` directive. Must run AFTER backslash-doubling: the - * directive's own backslashes are literal syntax the reader expects - * undoubled, so escaping non-ASCII first would corrupt them when the - * backslash-doubling pass ran afterward. - */ -function encodeNonAsciiStepDirectives(str: string): string { - let out = ''; - for (const ch of str) { - const cp = ch.codePointAt(0)!; - if (cp >= 32 && cp <= 126) { - out += ch; - } else if (cp <= 0xFFFF) { - out += `\\X2\\${cp.toString(16).toUpperCase().padStart(4, '0')}\\X0\\`; - } else { - out += `\\X4\\${cp.toString(16).toUpperCase().padStart(8, '0')}\\X0\\`; - } - } - return out; -} - /** * Convert a number to a valid STEP REAL literal. * diff --git a/scripts/api-surface.json b/scripts/api-surface.json index 79d3519a1b..ea4aa5dbf4 100644 --- a/scripts/api-surface.json +++ b/scripts/api-surface.json @@ -1020,6 +1020,7 @@ "entityTableFromColumns: function", "entityTableToColumns: function", "enumVal: function", + "escapeStepString: function", "exactNameOfRow: function", "exactTypeName: function", "findAttribute: function", From fc70ee987feacf4b4d6768f7b39b9a976ba0ae6c Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Fri, 28 Aug 2026 09:49:40 +0300 Subject: [PATCH 2/3] fix(data): trim the escaper doc comment back under the module-size budget `step-serializers.ts` sits at a 459-line budget and the #3300 note pushed it to 465, failing the 'Check TypeScript module size ratchet' step in Node tests. Trimmed the added comment rather than raising the budget: a reviewer flagged a budget raise on a sibling PR today, and the house rule treats a raise as a last resort needing written justification. The note still says what matters -- this is the one TS implementation, export re-exports it, and a vector test pins parity with the separate Rust escaper. --- packages/data/src/step-serializers.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/packages/data/src/step-serializers.ts b/packages/data/src/step-serializers.ts index cda0f5b5fe..7cde56a27f 100644 --- a/packages/data/src/step-serializers.ts +++ b/packages/data/src/step-serializers.ts @@ -188,14 +188,8 @@ export function serializeValue(value: StepValue): string { * `\X2\HHHH\X0\`, `\X4\HHHHHHHH\X0\`), never a raw byte, and buildingSMART * says the same for IFC2X3/IFC4/IFC4X3. A reader decoding as ISO-8859-1 (what * the base standard and most consumers assume) turns raw UTF-8 into mojibake or - * a broken parse: IfcOpenShell#699/#1016, files rejected by Solibri. - * - * This is the SINGLE TypeScript implementation (#3300): `@ifc-lite/export`'s - * `step-serialization.ts` re-exports this symbol rather than keeping its own - * copy. `ifc_lite_export::step_text::escape` is a separate, hand-kept Rust - * implementation — collapsing that one too would need a wasm adapter, which - * is a bigger change than this issue's scope, so the TS/Rust agreement is - * pinned by a vector test instead of shared code. + * a broken parse: IfcOpenShell#699/#1016, files rejected by Solibri. The one TS + * implementation (#3300); export re-exports it, a vector test pins Rust parity. */ export function escapeStepString(str: string): string { const escaped = str From 3bbfd414d0bb7d374a84083bc66385e3b4f6c5a3 Mon Sep 17 00:00:00 2001 From: Petru Conduraru Date: Fri, 28 Aug 2026 13:13:35 +0300 Subject: [PATCH 3/3] fix(data): bump the escaper changeset to minor, not patch louistrue: this PR adds a new public export (escapeStepString) to @ifc-lite/data's index.ts and api-surface.json, which is an additive API change and therefore minor, not patch, per the same rule already enforced across the fb55a6ea7 changeset bumps. --- .changeset/collapse-step-escaper-copies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/collapse-step-escaper-copies.md b/.changeset/collapse-step-escaper-copies.md index af4a133050..43f6b91496 100644 --- a/.changeset/collapse-step-escaper-copies.md +++ b/.changeset/collapse-step-escaper-copies.md @@ -1,5 +1,5 @@ --- -'@ifc-lite/data': patch +'@ifc-lite/data': minor '@ifc-lite/export': patch ---