From 176cd167103049e0a089b3f7f51776b8ac77f903 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 14:09:57 -0400 Subject: [PATCH 01/14] Absorb a terminator-line comment into the statement it ends Closes #47. A statement's region ends at its terminator, so a comment after the semicolon on the same line sat in the gap. That is invisible while the object is copied, because the gap is copied too, and wrong the moment it is reformatted: the writer emits its own field comment and the author's then arrives from the gap on the line below. It is not even a duplicate, because the ordinary writer drops the unit, so `!- North Axis {deg}` reads as a stray fragment under `!- North Axis`. The comment written on this file's walk defended the old behaviour on the ground that the alternative is a writer guessing which comments belong to which object. That defence is right about the general case and does not apply to this one: "on the same line as the terminator" is a positional fact, and it is the one case where the owner is not in question. The writer had just emitted its own version of the very comment it then copied. It reads as rarer than it is. IDFEditor writes a field comment on every line of every object including the terminator line, so about half the statements in a typical file gained a stray line the moment they were edited. THE FIRST LANGUAGE ALREADY DID THIS. Its concrete syntax tree runs a node to the end of the line, so it absorbed the comment and replaced it, and it drops the comment on removal too. This is TypeScript catching up rather than a new rule, which is why no divergence entry is added and none goes stale. Three consequences, decided rather than discovered: - A file written unchanged is byte-identical still: the extent grows and the gap shrinks by exactly the same characters. - Removing an object takes that comment with it, where it used to be left on a line of its own describing a field that no longer exists. The test that pinned the old behaviour now pins this one. - A comment on the next line, or after a blank one, stays in the gap. That is where deciding which object a comment belongs to actually becomes a guess. --- packages/core/src/preserve/write.ts | 61 ++++++++++++++++++++++----- packages/core/tests/preserve.test.ts | 62 +++++++++++++++++++++++++--- 2 files changed, 108 insertions(+), 15 deletions(-) diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index 0f0cd64..f64bada 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -12,10 +12,10 @@ import { isUntouched, type PreservedSource } from './source.js'; * is what makes the one-object diff structural rather than careful, and it is why nothing is * reordered, no header is added and the version statement is not moved. * - * A statement's region ends at its terminator, so a comment trailing the semicolon on the same - * line is in the gap: removing an object leaves it, and reformatting one leaves it below the new - * text. Behaviour rather than defects, and the alternative is a writer that guesses which comments - * are about which object. + * A statement's region ends at its terminator, and a comment after that semicolon on the SAME LINE + * is absorbed into the statement rather than left in the gap. See {@link extentEnds}. A comment on + * the next line, or after a blank one, stays in the gap, which is where the guessing problem about + * which object a comment belongs to actually starts. * * @internal */ @@ -26,6 +26,7 @@ export function writePreserved( ): string { const text = source.layer.text; const statements = source.layer.statements; + const ends = extentEnds(source); const parts: string[] = []; // Everything before the first statement, which for a file with none is the whole text: an empty @@ -33,12 +34,9 @@ export function writePreserved( parts.push(text.slice(0, statements[0]?.region.start ?? text.length)); for (let index = 0; index < statements.length; index += 1) { - const statement = statements[index]!; - parts.push(statementPart(source, index, text, options)); + parts.push(statementPart(source, index, text, ends[index]!, options)); // Unconditional: the gap is emitted whether the statement was copied, reformatted or dropped. - parts.push( - text.slice(statement.region.end, statements[index + 1]?.region.start ?? text.length) - ); + parts.push(text.slice(ends[index]!, statements[index + 1]?.region.start ?? text.length)); } appendNewObjects(document, source, parts, options); @@ -54,10 +52,11 @@ function statementPart( source: PreservedSource, index: number, text: string, + end: number, options: ObjectWriteOptions ): string { const statement = source.layer.statements[index]!; - const verbatim = text.slice(statement.region.start, statement.region.end); + const verbatim = text.slice(statement.region.start, end); const anchored = source.anchors[index]; if (anchored === undefined) return verbatim; // Removal is answered from ownership: `remove` already clears the owner, and recording it on @@ -100,3 +99,45 @@ function lastNonEmpty(parts: readonly string[]): string { } return ''; } + +/** + * Where each statement's text ends for this walk: its terminator, or the comment on that same line. + * + * A comment after the semicolon with nothing but horizontal whitespace between them is the last + * field's comment. Leaving it in the gap is invisible while the statement is copied, because the + * gap is copied too, and wrong the moment it is reformatted: the writer emits its own field comment + * and the author's then arrives from the gap on the line below, so the output carries a line nobody + * wrote. It is not even a duplicate, because the ordinary writer drops the unit the original + * usually carries, so it reads as a stray fragment. + * + * This is not the writer guessing which comment belongs to which object. "On the same line as the + * terminator" is a positional fact, and it is the one case where the owner is not in question. + * + * Three consequences, decided rather than discovered: + * + * - A file written unchanged is byte-identical still. The extent grows and the gap shrinks by + * exactly the same characters, so the concatenation does not move. + * - Removing an object takes that comment with it, where it used to be left on a line of its own + * describing a field that no longer exists. + * - Reformatting replaces it, which is the defect this closes. + * + * The tokens are in source order and so are the statements, so one cursor walks both. + */ +function extentEnds(source: PreservedSource): number[] { + const { statements, tokens, text } = source.layer; + const ends = statements.map((statement) => statement.region.end); + + let token = 0; + for (let index = 0; index < statements.length; index += 1) { + const end = ends[index]!; + while (token < tokens.length && tokens.startAt(token) < end) token += 1; + if (token >= tokens.length || tokens.kindAt(token) !== 'comment') continue; + + // Horizontal whitespace only. A line feed between the two puts the comment on its own line, + // which makes it a comment about whatever comes next and none of this statement's business. + const between = text.slice(end, tokens.startAt(token)); + if (between.includes('\n') || between.trim() !== '') continue; + ends[index] = tokens.endAt(token); + } + return ends; +} diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index af9bee6..5cc3633 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -200,14 +200,66 @@ describe('one field changes and one object looks changed', () => { // The removed statement's extent goes and the gaps around it do not: the blank line that // separated the two zones is in a gap and belongs to no object. // - // A statement's region ends at its TERMINATOR, so a comment trailing the semicolon on the same - // line is in the gap too, and it survives the removal. That is a consequence of the definition - // of text that belongs to no object, and it is behaviour rather than a defect: the alternative - // is a writer that decides which comments are about which object, which is a guess. - expect(written).toBe(MODEL.replace('Zone,\n Zone Two, !- Name\n 1.0E-5;', '')); + // The comment on the TERMINATOR's line goes with the object (idfkit-js#47). It is the last + // field's comment, and the field no longer exists, so leaving it behind would strand a line + // describing something that is gone. A comment on its own line, or after a blank one, stays: + // that is where deciding which object a comment belongs to becomes a guess. + expect(written).toBe( + MODEL.replace( + 'Zone,\n Zone Two, !- Name\n 1.0E-5; !- Direction of Relative North', + '' + ) + ); + // Zone One still carries its own, on its own terminator line. expect(written).toContain('!- Direction of Relative North'); }); + it('does not leave the old terminator comment below a reformatted object', () => { + // idfkit-js#47. A statement's region ends at its terminator, so a comment after the semicolon + // on the same line used to sit in the gap. Invisible while the object is copied, because the + // gap is copied too; wrong the moment it is reformatted, because the writer emits its own + // field comment and the author's then arrives on the line below. + // + // It is not even a duplicate: the ordinary writer drops the unit, so `!- North Axis {deg}` + // reads as a stray fragment under `!- North Axis`. IDFEditor writes one of these on every + // line of every object, so about half the statements in a typical file were affected. + const text = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0; !- North Axis {deg}', + '', + ].join('\n'); + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + const written = writeIdf(document); + + expect(written).not.toContain('{deg}'); + expect(written.match(/!- North Axis/g)).toHaveLength(1); + }); + + it('leaves a comment on its own line where it is', () => { + // The boundary of the rule above. Only the terminator's own line is absorbed; a comment on the + // next line is about whatever follows it and is nobody's to move. + const text = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0;', + '! a note about what comes next', + '', + 'Timestep, 6;', + '', + ].join('\n'); + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + expect(writeIdf(document)).toContain('! a note about what comes next'); + }); + it('appends a new object at the end, formatted', () => { const document = read(); document.add('Zone', 'Zone Three'); From 9f914d39a51e4bf4665b2e016420d44a87902be4 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 14:30:07 -0400 Subject: [PATCH 02/14] Keep the author's field comments on an object the writer reformats Supersedes the first attempt at #47, which deleted the comment instead. That attempt absorbed a terminator-line comment into the statement so it could not arrive twice, and reached agreement with Python by matching it on the more destructive of two behaviours. A reader pointed out that the example used `!- North Axis {deg}`, which the writer can regenerate, so deletion and regeneration looked identical. A genuinely custom comment told them apart: before, as shipped in 0.3.0-rc.1: survives, on a line of its own below after that attempt: deleted now: kept, in place, once An edit asks for a value to be re-rendered and not for the object's comments to be rebuilt. Rebuilding them destroyed the field's unit, which the generated label does not carry, and any note the author wrote there. `writeObject` takes the author's comment per field where there is one, and the layer supplies them: a field's comment is the one after its delimiter on the same line, which is the same positional rule the terminator case established, generalised. A comment on its own line belongs to no field and stays in the gap. This subsumes the duplicate. The comment is emitted once, by the writer, in the place it was written, so there is nothing left in the gap to arrive below it. --- packages/core/src/preserve/write.ts | 50 +++++++++++++++++++++++++++- packages/core/src/write/idf.ts | 18 +++++++++- packages/core/tests/preserve.test.ts | 40 +++++++++++++++++++++- 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index f64bada..216f2be 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -63,7 +63,10 @@ function statementPart( // the object would mean holding a reference to something the document has let go. if (anchored[OWNER] === undefined) return ''; if (isUntouched(anchored, source)) return verbatim; - return writeObject(anchored, options); + // Re-render the VALUES, and keep the author's comments. An edit asks for the first and never for + // the second, and rebuilding a comment destroys whatever the schema cannot regenerate: a note to + // a colleague, and the field's unit, which the ordinary label does not carry. + return writeObject(anchored, { ...options, fieldComments: fieldComments(source, index) }); } /** @@ -141,3 +144,48 @@ function extentEnds(source: PreservedSource): number[] { } return ends; } + +/** + * The author's comment for each of a statement's fields, positionally, where one exists. + * + * A field's comment is the one after its separator on the same line, which is the same positional + * rule {@link extentEnds} uses for the terminator and is the convention every writer of these files + * follows. A comment on its own line belongs to no field and is left in the gap. + * + * Positional against `Statement.fields`, which is positional against the cells `writeObject` emits: + * the name first for a named type, then the fixed fields in order. An object that gained fields + * runs past the end of this list and generates the rest, which is right, since the author never + * wrote a comment for a field that was not there. + * + * The last entry is the comment on the terminator's line, so this subsumes the duplicate that + * {@link extentEnds} exists to prevent: the comment is emitted once, by the writer, in place. + */ +function fieldComments(source: PreservedSource, index: number): (string | undefined)[] { + const { statements, tokens, text } = source.layer; + const fields = statements[index]!.fields; + const comments: (string | undefined)[] = new Array(fields.length).fill( + undefined + ); + + // One cursor over the tokens, which are in source order, as the fields are. + let token = 0; + for (let at = 0; at < fields.length; at += 1) { + const end = fields[at]!.end; + while (token < tokens.length && tokens.startAt(token) < end) token += 1; + // The separator or terminator that closes the field sits between it and its comment, and is a + // token of its own. Step over it; a field's comment is the next thing after it. + while ( + token < tokens.length && + (tokens.kindAt(token) === 'separator' || tokens.kindAt(token) === 'terminator') + ) { + token += 1; + } + if (token >= tokens.length || tokens.kindAt(token) !== 'comment') continue; + const between = text.slice(end, tokens.startAt(token)); + // Horizontal whitespace and the delimiter only. A line feed puts the comment on its own line, + // where it belongs to no field. + if (between.includes('\n') || between.replace(/[,;]/g, '').trim() !== '') continue; + comments[at] = text.slice(tokens.startAt(token), tokens.endAt(token)).trimEnd(); + } + return comments; +} diff --git a/packages/core/src/write/idf.ts b/packages/core/src/write/idf.ts index 2c955a6..f326f93 100644 --- a/packages/core/src/write/idf.ts +++ b/packages/core/src/write/idf.ts @@ -182,6 +182,20 @@ export interface ObjectWriteOptions { indent: string; /** Put the whole object on one line. See `WriteIdfOptions.compressed`. */ compressed?: boolean; + /** + * The author's own comment for each field, positionally, when there is one to reuse. + * + * Supplied by the preserving writer and by nothing else. Re-rendering an object's VALUES is what + * an edit asks for; rebuilding its comments is not, and doing it anyway destroys anything the + * schema cannot regenerate — a note to a colleague, and the field's unit, which `humanize` does + * not emit. An entry is the whole comment as written, from its `!` onward. + * + * Positional, and shorter than the cells whenever the object gained fields, in which case the + * ones past the end are generated as they always were. + * + * @internal + */ + fieldComments?: readonly (string | undefined)[]; } /** Serialize one object. */ @@ -240,8 +254,10 @@ export function writeObject(obj: IdfObject, options: ObjectWriteOptions): string lines.push(body); return; } + // The author's comment where there is one, this writer's where there is not. + const comment = options.fieldComments?.[index] ?? `!- ${cell.label}`; const padding = ' '.repeat(Math.max(1, options.commentColumn - body.length)); - lines.push(`${body}${padding}!- ${cell.label}`); + lines.push(`${body}${padding}${comment}`); }); return `${lines.join('\n')}\n`; diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index 5cc3633..1cde927 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -214,6 +214,43 @@ describe('one field changes and one object looks changed', () => { expect(written).toContain('!- Direction of Relative North'); }); + it("keeps the author's comments on an object it reformats", () => { + // The values are what an edit asks to re-render. The comments are not, and rebuilding them + // destroys whatever the schema cannot regenerate: a note to a colleague, and the field's unit, + // which the generated label does not carry. Both are kept, in place, exactly as written. + const text = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0, !- North Axis {deg}', + ' City; !- VERIFY WITH CLIENT before the Feb review', + '', + ].join('\n'); + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + const written = writeIdf(document); + + expect(written).toContain('!- North Axis {deg}'); + expect(written).toContain('!- VERIFY WITH CLIENT before the Feb review'); + // Once each, not twice: the writer emits the author's comment in place of its own, so there is + // nothing left in the gap to arrive on the line below (idfkit-js#47). + expect(written.match(/!- North Axis/g)).toHaveLength(1); + expect(written.match(/VERIFY WITH CLIENT/g)).toHaveLength(1); + // The value is the one thing that did change. + expect(written).toContain('42.0'); + expect(written).not.toContain(' 0,'); + }); + + it('generates a comment only for a field the author never wrote one for', () => { + const text = ['Version, 26.1;', '', 'Building,', ' My Building;', ''].join('\n'); + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + expect(writeIdf(document)).toContain('!- North Axis'); + }); + it('does not leave the old terminator comment below a reformatted object', () => { // idfkit-js#47. A statement's region ends at its terminator, so a comment after the semicolon // on the same line used to sit in the gap. Invisible while the object is copied, because the @@ -236,8 +273,9 @@ describe('one field changes and one object looks changed', () => { const written = writeIdf(document); - expect(written).not.toContain('{deg}'); + // Once, in place, with the author's unit intact. It used to arrive a second time from the gap. expect(written.match(/!- North Axis/g)).toHaveLength(1); + expect(written).toContain('!- North Axis {deg}'); }); it('leaves a comment on its own line where it is', () => { From 91b6928b3cb4e4ce88f9a941a958f10ebbb0544e Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 14:42:16 -0400 Subject: [PATCH 03/14] Emit no line break the statement's extent did not cover A reformatted object grew the file by a blank line, and would grow it again on every save. A statement's extent ends at its terminator, or at the comment on that same line, and in neither case includes the line break: the break is the first character of the gap. `writeObject` ends with one because it also writes whole documents, so the break went in twice. This was there before the comment work and was invisible. The misplaced terminator comment sat between the two breaks, so it read as one blank line; removing the comment left the second break with nothing in front of it. Found by a consumer measuring a one-field edit: 476 lines out against 475 in, on a file where the requirement being written against is that a one-object edit changes only the lines of that object. A blank line after the object is a line outside it. --- packages/core/src/preserve/write.ts | 12 ++++++++++- packages/core/tests/preserve.test.ts | 31 ++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index 216f2be..c385ecf 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -66,7 +66,17 @@ function statementPart( // Re-render the VALUES, and keep the author's comments. An edit asks for the first and never for // the second, and rebuilding a comment destroys whatever the schema cannot regenerate: a note to // a colleague, and the field's unit, which the ordinary label does not carry. - return writeObject(anchored, { ...options, fieldComments: fieldComments(source, index) }); + // + // Without its trailing newline. A statement's extent ends at its terminator, or at the comment on + // that same line, and in neither case does it include the line break: the break is the first + // character of the gap. `writeObject` ends with one because it is also used to write whole + // documents, so emitting it here would put the break in twice and grow the file by a blank line + // per reformatted object. Every object in a file, edited and saved twice, would grow it twice. + const written = writeObject(anchored, { + ...options, + fieldComments: fieldComments(source, index), + }); + return written.endsWith('\n') ? written.slice(0, -1) : written; } /** diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index 1cde927..effa55c 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -243,6 +243,37 @@ describe('one field changes and one object looks changed', () => { expect(written).not.toContain(' 0,'); }); + it('adds no line to the file for an object it reformats', () => { + // A statement's extent ends at its terminator, or at the comment on that same line, and never + // includes the line break: the break is the first character of the gap. `writeObject` ends with + // one because it also writes whole documents, so emitting it here put the break in twice and + // grew the file by a blank line per reformatted object — compounding on every save. + // + // It was there before the comment work and was invisible: the misplaced terminator comment sat + // in the gap between the two breaks, so it read as one blank line. Fixing that exposed this. + const text = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0; !- North Axis {deg}', + '', + 'Timestep, 6;', + '', + ].join('\n'); + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + const written = writeIdf(document); + + expect(written.split('\n')).toHaveLength(text.split('\n').length); + expect(written).not.toContain('\n\n\n'); + // And again, on the output, because the growth compounded rather than saturating. + const reread = parseIdf(written, v26, { strict: false, preserveFormatting: true }).document; + reread.require('Building', 'My Building').set('north_axis', 43); + expect(writeIdf(reread).split('\n')).toHaveLength(text.split('\n').length); + }); + it('generates a comment only for a field the author never wrote one for', () => { const text = ['Version, 26.1;', '', 'Building,', ' My Building;', ''].join('\n'); const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); From 4ab90e53630c7517853babe909d6e023e40b7418 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 15:09:04 -0400 Subject: [PATCH 04/14] Keep everything on a reformatted object except its values Mirrors idfkit's change of the same name; the two behave identically on every case measured. A comment on its own line INSIDE an object was lost when the object was reformatted. One between two objects is carried by the gap and was always safe; one inside is carried by nothing. It is now emitted with the field below it. A field the author left bare gained a generated label. Absence is as much a thing the author wrote as the words are, so bare stays bare, and `fieldComments: 'generate'` is the escape hatch for a caller who wants the file annotated: it adds labels and never costs a comment line. `changedObjects()` answers which objects a preserving write will rewrite, which a consumer cannot derive: a rename clears the record on every object that referred to the renamed one, so counting from an edit log reports one where the answer is nine. CI is red until governance-2026.15 is published and pinned. The naming gate reads the built surface and fires on the change that adds a name, which is the register-lands-first rule doing its job. --- packages/core/src/document.ts | 26 ++++- packages/core/src/preserve/write.ts | 82 +++++++++----- packages/core/src/write/idf.ts | 80 ++++++++++++-- packages/core/tests/preserve.test.ts | 154 ++++++++++++++++++++++++++- 4 files changed, 303 insertions(+), 39 deletions(-) diff --git a/packages/core/src/document.ts b/packages/core/src/document.ts index 19489fa..138caca 100644 --- a/packages/core/src/document.ts +++ b/packages/core/src/document.ts @@ -3,7 +3,7 @@ import type { Schema, SlimType } from '@idfkit/schemas'; import { IdfCollection } from './collection.js'; import { DATA, KEY, NAME, OWNER, SHAPE, SOURCE } from './internal.js'; import { IdfObject, type FieldValues, type ObjectOwner, type StoredValue } from './object.js'; -import type { PreservedSource } from './preserve/source.js'; +import { isUntouched, type PreservedSource } from './preserve/source.js'; import { ReferenceGraph } from './references.js'; import type { AnyTypeMap, ObjectOf, TypeNameOf, UntypedMap, ValuesOf } from './typemap.js'; @@ -301,6 +301,30 @@ export class IdfDocument implements ObjectOwn for (const collection of this.#collections.values()) yield* collection; } + /** + * Every object a preserving write will write afresh rather than reproduce. + * + * Empty for a document read with `preserveFormatting` and not edited since. Every object for a + * document read without it, because there is nothing to reproduce. + * + * `rawText` answers whether a write will preserve at all. This answers how much of the file it + * will change, which is what a save button has to put to a user out loud, and it is the part a + * consumer cannot work out for itself: a rename clears the record on every object that referred + * to the renamed one, so counting from your own edit log reports one where the answer is nine. + * + * A generator, so listing what is about to be reformatted is as easy as counting it: + * + * ```ts + * const changed = [...document.changedObjects()]; + * if (changed.length > 0) warn(`Saving will rewrite ${changed.length} objects.`); + * ``` + */ + *changedObjects(): Generator { + for (const obj of this.objects()) { + if (!isUntouched(obj, this.#source)) yield obj; + } + } + /** Reference targets that no object provides. */ danglingReferences(): ReturnType { const valid = new Set(); diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index c385ecf..33ea924 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -1,6 +1,6 @@ import { OWNER } from '../internal.js'; import type { IdfObject } from '../object.js'; -import { writeObject, type ObjectWriteOptions } from '../write/idf.js'; +import { writeObject, type FieldAnnotation, type ObjectWriteOptions } from '../write/idf.js'; import { isUntouched, type PreservedSource } from './source.js'; /** @@ -74,7 +74,7 @@ function statementPart( // per reformatted object. Every object in a file, edited and saved twice, would grow it twice. const written = writeObject(anchored, { ...options, - fieldComments: fieldComments(source, index), + annotations: annotations(source, index), }); return written.endsWith('\n') ? written.slice(0, -1) : written; } @@ -156,46 +156,72 @@ function extentEnds(source: PreservedSource): number[] { } /** - * The author's comment for each of a statement's fields, positionally, where one exists. + * What the author wrote around each of a statement's fields, positionally. * - * A field's comment is the one after its separator on the same line, which is the same positional - * rule {@link extentEnds} uses for the terminator and is the convention every writer of these files - * follows. A comment on its own line belongs to no field and is left in the gap. + * Two kinds, and the second is the one nothing else carries. A field's own comment is the one after + * its delimiter on the same line, which is the convention every writer of these files follows and + * the only case where which field a comment belongs to is not a guess. A comment on its OWN line + * inside the statement belongs to the field below it, and it is lost the moment the object is + * reformatted unless it is emitted with that field: a comment between two statements is carried by + * the gap, and one inside a statement is not. * * Positional against `Statement.fields`, which is positional against the cells `writeObject` emits: - * the name first for a named type, then the fixed fields in order. An object that gained fields - * runs past the end of this list and generates the rest, which is right, since the author never - * wrote a comment for a field that was not there. + * the name first for a named type, then the fixed fields in order. An object that gained a field + * runs past the end of this list, and a field with no entry has no author to be faithful to. * - * The last entry is the comment on the terminator's line, so this subsumes the duplicate that - * {@link extentEnds} exists to prevent: the comment is emitted once, by the writer, in place. + * A field the author left bare gets an entry with no `trailing`, which is how "written bare" is + * told apart from "not written by this author at all". Absence is as much a thing the author wrote + * as the words are. */ -function fieldComments(source: PreservedSource, index: number): (string | undefined)[] { +function annotations(source: PreservedSource, index: number): FieldAnnotation[] { const { statements, tokens, text } = source.layer; - const fields = statements[index]!.fields; - const comments: (string | undefined)[] = new Array(fields.length).fill( - undefined - ); + const statement = statements[index]!; + const fields = statement.fields; + const built: FieldAnnotation[] = fields.map(() => ({ before: [], trailing: undefined })); - // One cursor over the tokens, which are in source order, as the fields are. + // One cursor over the tokens, which are in source order, as the fields are. Every comment + // between the previous field's delimiter and this one's value stands on its own line above it. let token = 0; + let previousEnd = statement.typeName.end; for (let at = 0; at < fields.length; at += 1) { - const end = fields[at]!.end; - while (token < tokens.length && tokens.startAt(token) < end) token += 1; - // The separator or terminator that closes the field sits between it and its comment, and is a - // token of its own. Step over it; a field's comment is the next thing after it. + const field = fields[at]!; + const before: string[] = []; + + while (token < tokens.length && tokens.startAt(token) < previousEnd) token += 1; + while (token < tokens.length && tokens.startAt(token) < field.start) { + if ( + tokens.kindAt(token) === 'comment' && + !onSameLine(text, previousEnd, tokens.startAt(token)) + ) { + before.push(text.slice(tokens.startAt(token), tokens.endAt(token)).trimEnd()); + } + token += 1; + } + + // Past the value now: step over the delimiter that closes it and take the comment after it. + while (token < tokens.length && tokens.startAt(token) < field.end) token += 1; while ( token < tokens.length && (tokens.kindAt(token) === 'separator' || tokens.kindAt(token) === 'terminator') ) { token += 1; } - if (token >= tokens.length || tokens.kindAt(token) !== 'comment') continue; - const between = text.slice(end, tokens.startAt(token)); - // Horizontal whitespace and the delimiter only. A line feed puts the comment on its own line, - // where it belongs to no field. - if (between.includes('\n') || between.replace(/[,;]/g, '').trim() !== '') continue; - comments[at] = text.slice(tokens.startAt(token), tokens.endAt(token)).trimEnd(); + let trailing: string | undefined; + if ( + token < tokens.length && + tokens.kindAt(token) === 'comment' && + onSameLine(text, field.end, tokens.startAt(token)) + ) { + trailing = text.slice(tokens.startAt(token), tokens.endAt(token)).trimEnd(); + } + + built[at] = { before, trailing }; + previousEnd = field.end; } - return comments; + return built; +} + +/** Whether two offsets sit on one line, which is what makes a comment a field's rather than its own. */ +function onSameLine(text: string, from: number, to: number): boolean { + return !text.slice(from, to).includes('\n'); } diff --git a/packages/core/src/write/idf.ts b/packages/core/src/write/idf.ts index f326f93..553e674 100644 --- a/packages/core/src/write/idf.ts +++ b/packages/core/src/write/idf.ts @@ -74,6 +74,20 @@ export interface WriteIdfOptions { * @defaultValue undefined, meaning decide */ preserveFormatting?: boolean; + /** + * What to do about a field the author deliberately left without a comment. + * + * Only meaningful on the preserving path, which is the only path that knows what the author + * wrote. `'preserve'` leaves a bare field bare, because absence is as much a thing the author + * wrote as the words are. `'generate'` labels it, for a caller who wants the file annotated. + * + * Neither setting touches the author's own comment lines. A comment between two objects is + * carried by the text between them, and a comment on its own line inside an object is emitted + * with the field below it, so asking for labels adds them and never costs a line. + * + * @defaultValue 'preserve' + */ + fieldComments?: 'preserve' | 'generate'; } /** @@ -98,6 +112,7 @@ export function writeIdf( comments: options.comments ?? true, commentColumn: options.commentColumn ?? 30, indent: options.indent ?? ' ', + labelBareFields: options.fieldComments === 'generate', }); } @@ -183,19 +198,51 @@ export interface ObjectWriteOptions { /** Put the whole object on one line. See `WriteIdfOptions.compressed`. */ compressed?: boolean; /** - * The author's own comment for each field, positionally, when there is one to reuse. + * What the author wrote around each field, positionally. * * Supplied by the preserving writer and by nothing else. Re-rendering an object's VALUES is what - * an edit asks for; rebuilding its comments is not, and doing it anyway destroys anything the - * schema cannot regenerate — a note to a colleague, and the field's unit, which `humanize` does - * not emit. An entry is the whole comment as written, from its `!` onward. + * an edit asks for; rebuilding what surrounds them is not, and doing it anyway destroys anything + * the schema cannot regenerate. + * + * Positional, and shorter than the cells whenever the object gained fields. A field past the end + * has no author to be faithful to, so it is labelled as it always was. * - * Positional, and shorter than the cells whenever the object gained fields, in which case the - * ones past the end are generated as they always were. + * @internal + */ + annotations?: readonly FieldAnnotation[]; + /** + * Label a field the author deliberately left bare. + * + * `false` is faithful and is what the preserving path asks for: a field written without a comment + * was written that way on purpose, and absence is as much a thing the author wrote as the words + * are. `true` restores the ordinary writer's behaviour of labelling every field, for a caller who + * wants the file annotated. + * + * Either way the author's own standalone comment lines are emitted, so turning this on adds + * labels and never costs a line. * * @internal */ - fieldComments?: readonly (string | undefined)[]; + labelBareFields?: boolean; +} + +/** + * What the author wrote around one field. + * + * @internal + */ +export interface FieldAnnotation { + /** + * Comment lines standing on their own above this field, in order, exactly as written. + * + * These live INSIDE the statement, so unlike a comment between two statements they are not + * carried by the gap and are lost the moment the object is reformatted unless they are emitted + * here. `! this value came from the 2019 survey` is the shape, and it is the author's note about + * the field below it. + */ + readonly before: readonly string[]; + /** The comment after this field's delimiter on the same line, if the author wrote one. */ + readonly trailing: string | undefined; } /** Serialize one object. */ @@ -254,8 +301,23 @@ export function writeObject(obj: IdfObject, options: ObjectWriteOptions): string lines.push(body); return; } - // The author's comment where there is one, this writer's where there is not. - const comment = options.fieldComments?.[index] ?? `!- ${cell.label}`; + // The author's own lines above the field, which live inside the statement and are carried by + // nothing else. + const annotation = options.annotations?.[index]; + for (const line of annotation?.before ?? []) lines.push(`${options.indent}${line}`); + + // The author's comment where there is one. Where the author left the field bare, nothing, + // unless the caller asked for a label. Where there is no author at all, because the object + // gained this field, the label as always. + const comment = + annotation === undefined + ? `!- ${cell.label}` + : (annotation.trailing ?? + (options.labelBareFields === true ? `!- ${cell.label}` : undefined)); + if (comment === undefined) { + lines.push(body); + return; + } const padding = ' '.repeat(Math.max(1, options.commentColumn - body.length)); lines.push(`${body}${padding}${comment}`); }); diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index effa55c..8fc9cdc 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -1,6 +1,6 @@ import { beforeAll, describe, expect, it } from 'vitest'; -import { parseEpJson, parseIdf, writeEpJson, writeIdf } from '@idfkit/core'; +import { parseEpJson, parseIdf, scanIdf, writeEpJson, writeIdf } from '@idfkit/core'; import type { Schema } from '@idfkit/schemas'; import { schema, syntaxFixture, syntaxFixtures } from './helpers.js'; @@ -600,3 +600,155 @@ describe('the object notation preserves on all-or-nothing terms', () => { expect(written).toContain('Zone,'); }); }); + +describe('which objects a preserving write will rewrite', () => { + // A consumer cannot derive this from its own edit log. A rename clears the record on every object + // that referred to the renamed one, so an editor counting its own edits reports one where the + // answer is three here and nine on a real model. + const REFERENCED = [ + 'Version, 26.1;', + '', + 'Zone, ZONE ONE;', + '', + 'BuildingSurface:Detailed,', + ' S1, Wall, C1, ZONE ONE, , Outdoors, , SunExposed, WindExposed, , ,', + ' 0,0,0, 1,0,0;', + '', + 'BuildingSurface:Detailed,', + ' S2, Wall, C1, ZONE ONE, , Outdoors, , SunExposed, WindExposed, , ,', + ' 0,0,0, 1,0,0;', + '', + ].join('\n'); + + const read = (preserve = true) => + parseIdf(REFERENCED, v26, { strict: false, preserveFormatting: preserve }).document; + + it('yields nothing for a document nobody has edited', () => { + expect([...read().changedObjects()]).toEqual([]); + }); + + it('yields every object a rename rewrote, not just the renamed one', () => { + const document = read(); + document.rename(document.require('Zone', 'ZONE ONE'), 'RENAMED'); + + expect([...document.changedObjects()].map((o) => o.typeName).sort()).toEqual([ + 'BuildingSurface:Detailed', + 'BuildingSurface:Detailed', + 'Zone', + ]); + }); + + it('yields nothing after a write of the value already held', () => { + const document = read(); + const zone = document.require('Zone', 'ZONE ONE'); + zone.set('multiplier', zone.get('multiplier') ?? undefined); + + expect([...document.changedObjects()]).toEqual([]); + }); + + it('yields every object for a document read without preservation', () => { + // There is nothing to reproduce, so a write rewrites the file entirely. + const document = read(false); + + expect([...document.changedObjects()]).toHaveLength(document.size); + }); + + it('agrees with what the writer actually reproduces', () => { + // The claim is only worth making if the writer honours it. After the rename, the three objects + // it touched are named and the Version is not, so the Version must come back from its own + // characters and the other three must not. + const document = read(); + document.rename(document.require('Zone', 'ZONE ONE'), 'RENAMED'); + const written = writeIdf(document); + const changed = [...document.changedObjects()]; + + expect(changed.map((o) => o.typeName)).not.toContain('Version'); + expect(written).toContain('Version, 26.1;'); + // And every object it DID name was rewritten: none of them survives as its original text. + expect(written).not.toContain('Zone, ZONE ONE;'); + expect(changed).toHaveLength(3); + }); +}); + +describe('what survives when the writer rewrites an object', () => { + // An edit asks for the VALUES to be re-rendered. Everything else the author wrote is theirs, and + // that includes the absence of a comment on a field they left bare. + const SOURCE = [ + 'Version, 26.1;', + '', + '! a note between objects', + 'Building,', + ' My Building, !- Name', + ' ! this value came from the 2019 survey', + ' 0, !- North Axis {deg}', + ' Suburbs;', + '', + 'Timestep, 6;', + '', + ].join('\n'); + + const edited = (options = {}) => { + const { document } = parseIdf(SOURCE, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + return writeIdf(document, options); + }; + + it('keeps a units annotation the generated label would drop', () => { + expect(edited()).toContain('!- North Axis {deg}'); + }); + + it('keeps a comment on its own line inside the object', () => { + // The one comment nothing else carries: it is inside the object rather than between two, so + // the gap does not reach it and reformatting destroyed it. + expect(edited()).toContain('! this value came from the 2019 survey'); + }); + + it('keeps a comment between two objects', () => { + expect(edited()).toContain('! a note between objects'); + }); + + it('leaves a field the author left bare bare', () => { + // Absence is as much a thing the author wrote as the words are. + const written = edited(); + + expect(written).toMatch(/Suburbs;\s*$/m); + expect(written).not.toContain('!- Terrain'); + }); + + it('labels a bare field on request, and still costs no comment line', () => { + const written = edited({ fieldComments: 'generate' }); + + expect(written).toContain('!- Terrain'); + expect(written).toContain('! this value came from the 2019 survey'); + expect(written).toContain('! a note between objects'); + expect(written).toContain('!- North Axis {deg}'); + }); + + it('changes the value and nothing else', () => { + const written = edited(); + + expect(written).toContain('42.0'); + expect(written).not.toContain(' 0,'); + }); + + it('attaches a comment by its delimiter, not by counting lines', () => { + // Several fields share a line in real files: a surface's vertices are routinely written three + // to a line with one comment for the triple. Counting lines mis-attaches every comment after + // the first such line. + const text = [ + 'Version, 26.1;', + '', + 'Building,', + ' Packed, City, 0.04, !- three fields, one comment', + ' 0.4; !- and another', + '', + ].join('\n'); + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'Packed').set('terrain', 'Suburbs'); + + const written = writeIdf(document); + + expect(written).toContain('!- three fields, one comment'); + expect(written).toContain('!- and another'); + }); +}); From 5419f5759b6ff7405276fa89e44ba3f3355f8da3 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 15:15:47 -0400 Subject: [PATCH 05/14] Say what changedObjects() does not answer It yields objects a write will REWRITE, and a consumer reading the old comment could take it for everything that will differ. A removal separates the two: the removed object is no longer in the document to be yielded, so this returns nothing for a write that changes the file, and treating an empty result as 'the file is unchanged' is wrong on every removal. Comparing the write with rawText is the question that answers; this one is how much is being written afresh. Found by a consumer whose save outcome already did it correctly and who pointed out the next one would not. --- packages/core/src/document.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/core/src/document.ts b/packages/core/src/document.ts index 138caca..a25c1a6 100644 --- a/packages/core/src/document.ts +++ b/packages/core/src/document.ts @@ -307,10 +307,16 @@ export class IdfDocument implements ObjectOwn * Empty for a document read with `preserveFormatting` and not edited since. Every object for a * document read without it, because there is nothing to reproduce. * - * `rawText` answers whether a write will preserve at all. This answers how much of the file it - * will change, which is what a save button has to put to a user out loud, and it is the part a - * consumer cannot work out for itself: a rename clears the record on every object that referred - * to the renamed one, so counting from your own edit log reports one where the answer is nine. + * `rawText` answers whether a write will preserve at all. This answers how many objects it will + * REWRITE, and it is the part a consumer cannot work out for itself: a rename clears the record on + * every object that referred to the renamed one, so counting from your own edit log reports one + * where the answer is nine. + * + * **It is not "everything that will differ", and a removal is the case that separates the two.** + * An object removed from the document is no longer in it to be yielded, so this can return nothing + * for a write that changes the file. A consumer treating an empty result as "the file is + * unchanged" would be wrong on every removal. To ask whether the file will differ at all, compare + * the write with `rawText`; ask this for how much of it is being written afresh. * * A generator, so listing what is about to be reformatted is as easy as counting it: * From 610a1466e41f00209f8a7c38a1ffc8ab0a5ec743 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 15:54:01 -0400 Subject: [PATCH 06/14] Keep values on the line the author put them on A reformatted object was written one value per line whatever the source said. Measured across the 693 EnergyPlus 22.1.0 example files, that is not a corner case: 21.5 percent of statements group several values on a line, 690 of the 693 files contain at least one, and a full reformat of the corpus would add 20.2 percent to its line count with 89.6 percent of that from this one shape. A four-line surface became twelve lines. `FieldAnnotation` gains `startsLine`, which the delimiter scan already knew and was discarding, and the emitter builds a line rather than pushing one per field. Editing one wall of 1ZoneUncontrolled leaves the file at 462 lines rather than 470. The type name is the line under construction rather than something pushed ahead of it, so an object written `Timestep,4;` comes back on one line. That is another 11.3 percent of statements and the case that surprises on a file with no geometry in it: nobody thinks of `Timestep,4;` as formatting they chose. That restructuring broke `comments: false` output, which emitted the type name last and no longer parsed. The corpus caught it; the test that granted the output form now reparses what it got. --- packages/core/src/preserve/write.ts | 10 ++++- packages/core/src/write/idf.ts | 65 ++++++++++++++++++++-------- packages/core/tests/preserve.test.ts | 51 ++++++++++++++++++++++ 3 files changed, 107 insertions(+), 19 deletions(-) diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index 33ea924..5ae930e 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -177,7 +177,11 @@ function annotations(source: PreservedSource, index: number): FieldAnnotation[] const { statements, tokens, text } = source.layer; const statement = statements[index]!; const fields = statement.fields; - const built: FieldAnnotation[] = fields.map(() => ({ before: [], trailing: undefined })); + const built: FieldAnnotation[] = fields.map(() => ({ + before: [], + trailing: undefined, + startsLine: true, + })); // One cursor over the tokens, which are in source order, as the fields are. Every comment // between the previous field's delimiter and this one's value stands on its own line above it. @@ -215,7 +219,9 @@ function annotations(source: PreservedSource, index: number): FieldAnnotation[] trailing = text.slice(tokens.startAt(token), tokens.endAt(token)).trimEnd(); } - built[at] = { before, trailing }; + // Whether the author began a line with this field, which is what keeps a vertex written + // `0,0,4.572,` on one line rather than three. + built[at] = { before, trailing, startsLine: !onSameLine(text, previousEnd, field.start) }; previousEnd = field.end; } return built; diff --git a/packages/core/src/write/idf.ts b/packages/core/src/write/idf.ts index 553e674..5112c5c 100644 --- a/packages/core/src/write/idf.ts +++ b/packages/core/src/write/idf.ts @@ -243,6 +243,15 @@ export interface FieldAnnotation { readonly before: readonly string[]; /** The comment after this field's delimiter on the same line, if the author wrote one. */ readonly trailing: string | undefined; + /** + * Whether the author began a new line with this field. + * + * False for the second and third coordinate of a vertex written `0,0,4.572,` on one line. Writing + * one value per line regardless turns a four-line surface into twelve, which is the most visible + * thing a reformat does to a geometry file: 21.5 percent of the statements in the 693 EnergyPlus + * example files group values this way, and 690 of those files contain at least one. + */ + readonly startsLine: boolean; } /** Serialize one object. */ @@ -292,35 +301,57 @@ export function writeObject(obj: IdfObject, options: ObjectWriteOptions): string return `${obj.typeName},${cells.map((cell) => cell.value).join(',')};`; } - const lines: string[] = [`${obj.typeName},`]; + const lines: string[] = []; + + // A line under construction, so fields the author wrote together stay together. Flushed when the + // next field opens a line of its own, and once at the end. + // + // It starts as the TYPE NAME rather than the type name being pushed straight out, so that an + // object the author wrote entirely on one line, `Timestep,4;`, can come back on one line. That is + // 11.3 percent of the statements in the example files, and it is the case that surprises on a + // file with no geometry in it: nobody thinks of `Timestep,4;` as formatting they chose. + let open = `${obj.typeName},`; + let openComment: string | undefined; + const flush = (): void => { + if (open === '') return; + if (openComment === undefined) { + lines.push(open); + } else { + const padding = ' '.repeat(Math.max(1, options.commentColumn - open.length)); + lines.push(`${open}${padding}${openComment}`); + } + open = ''; + openComment = undefined; + }; cells.forEach((cell, index) => { const terminator = index === cells.length - 1 ? ';' : ','; - const body = `${options.indent}${cell.value}${terminator}`; - if (!options.comments) { - lines.push(body); - return; - } - // The author's own lines above the field, which live inside the statement and are carried by - // nothing else. const annotation = options.annotations?.[index]; - for (const line of annotation?.before ?? []) lines.push(`${options.indent}${line}`); + // No annotation means no author to be faithful to, so one value per line as this writer always + // did. Field 0 is the one that decides whether the object opens on the type name's own line. + const opensLine = annotation === undefined || annotation.startsLine; + + if (opensLine) { + flush(); + for (const line of annotation?.before ?? []) lines.push(`${options.indent}${line}`); + open = `${options.indent}${cell.value}${terminator}`; + } else { + open = `${open} ${cell.value}${terminator}`; + } + if (!options.comments) return; // The author's comment where there is one. Where the author left the field bare, nothing, - // unless the caller asked for a label. Where there is no author at all, because the object - // gained this field, the label as always. + // unless the caller asked for a label. Where there is no author at all, the label as always. + // On a line carrying several values only the last has a comment, which is what the author + // wrote and what the delimiter rule recovers. const comment = annotation === undefined ? `!- ${cell.label}` : (annotation.trailing ?? (options.labelBareFields === true ? `!- ${cell.label}` : undefined)); - if (comment === undefined) { - lines.push(body); - return; - } - const padding = ' '.repeat(Math.max(1, options.commentColumn - body.length)); - lines.push(`${body}${padding}${comment}`); + if (comment !== undefined) openComment = comment; }); + flush(); return `${lines.join('\n')}\n`; } diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index 8fc9cdc..4d94d56 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -527,6 +527,9 @@ describe('asking for two contradictory things is refused', () => { const bare = writeIdf(kept(), { preserveFormatting: true, comments: false }); expect(bare).not.toBe(MODEL); expect(bare).not.toContain('!-'); + // And it still loads. The type name is the line the field loop starts with, so a branch that + // returns before flushing it emits it last, which parses as nothing at all. + expect(() => parseIdf(bare, v26, { strict: false })).not.toThrow(); }); it('raises nothing when preservation is asked for on a document read without it', () => { @@ -752,3 +755,51 @@ describe('what survives when the writer rewrites an object', () => { expect(written).toContain('!- and another'); }); }); + +describe('the line the author put a value on', () => { + // 21.5% of the statements in the 693 EnergyPlus example files write several values to a line, + // and 690 of those files contain at least one. Writing one value per line regardless turns a + // four-line surface into twelve, which is the most visible thing a reformat does to geometry. + const GROUPED = [ + 'Version, 26.1;', + '', + 'BuildingSurface:Detailed,', + ' S1, Wall, C1, Z1, , Outdoors, , SunExposed, WindExposed, , ,', + ' 0, 0, 4.572, !- X,Y,Z ==> Vertex 1 {m}', + ' 0, 0, 0; !- X,Y,Z ==> Vertex 2 {m}', + '', + ].join('\n'); + + it('keeps values the author grouped onto one line', () => { + const { document } = parseIdf(GROUPED, v26, { strict: false, preserveFormatting: true }); + document.require('BuildingSurface:Detailed', 'S1').set('sun_exposure', 'NoSun'); + + const written = writeIdf(document); + + expect(written.split('\n')).toHaveLength(GROUPED.split('\n').length); + expect(written).toMatch(/0\.0, 0\.0, 4\.572,\s+!- X,Y,Z ==> Vertex 1 \{m\}/); + expect(written).toMatch(/0\.0, 0\.0, 0\.0;\s+!- X,Y,Z ==> Vertex 2 \{m\}/); + }); + + it('keeps a whole object the author wrote on one line', () => { + // The cheaper case, and the one that surprises on a file with no geometry in it: 11.3% of + // statements are written this way, and nobody thinks of `Timestep,4;` as formatting they chose. + const text = 'Version, 26.1;\n\nTimestep,4;\n'; + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + [...document.all('Timestep')][0]!.set('number_of_timesteps_per_hour', 6); + + const written = writeIdf(document); + + expect(written).toContain('Timestep,'); + expect(written.split('\n')).toHaveLength(text.split('\n').length); + }); + + it('gives a field the author never wrote a line of its own', () => { + // No author to be faithful to, so the writer's own habit applies. + const text = 'Version, 26.1;\n\nBuilding,\n My Building;\n'; + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + expect(writeIdf(document)).toMatch(/\n\s+42\.0;\s+!- North Axis/); + }); +}); From 44bf5e3c4cdf6e2a792c692ab584061800f54f4a Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 16:41:32 -0400 Subject: [PATCH 07/14] Put the comment where EnergyPlus puts it, and keep the blanks the author wrote Two defects a peer found by forcing 263,471 objects from the 693 EnergyPlus 22.1.0 example files through the writer and attributing every difference. Both predate the preserving writer; both are the kind that only shows once a write is expected to give the file back. THE COLUMN. `commentColumn` is documented as a column and was applied as an index, so `!-` landed one place right of where the files it imitates put it. Across 1,504,802 comment lines whose content came back byte-identical, 91 percent moved by exactly one. `1ZoneUncontrolled.idf` writes the marker at index 29 on 223 of its 231 commented lines; this writer wrote 30. On a preserving write that is the difference that shows. A rewritten object's comments stood one column clear of every untouched object around it, so every save left a visible seam at the edit. THE BLANKS. The writer stops at the last field that is SET, so a run of commas the author wrote out is dropped and the field-name comments go with them. One `Sizing:System` went from 38 lines to 22 on a single-field edit; corpus-wide it is 20,571 lines, more than any other difference a rewrite makes, led by ComponentCost:LineItem at 4,548 and Coil:Heating:Water at 2,475. A field written out as a blank is as much a thing the author wrote as a field left bare of its comment, and that second rule is one this path already follows. Reaching the opposite answer on the first was an inconsistency, not a decision. Only on the preserving path: the annotations are the author's own field count and are absent everywhere else, so a write with nothing to reproduce trims as it always has. The extensible rule still wins over both, since a missing fixed slot lands every group in the wrong position. The tutorial's rendered output moves one column with everything else. --- docs/tutorials/first-model.md | 16 +++---- packages/core/src/write/idf.ts | 29 ++++++++++-- packages/core/tests/docs-snippets.test.ts | 2 +- packages/core/tests/preserve.test.ts | 54 +++++++++++++++++++++++ packages/core/tests/write.test.ts | 11 +++-- 5 files changed, 97 insertions(+), 15 deletions(-) diff --git a/docs/tutorials/first-model.md b/docs/tutorials/first-model.md index 139dade..2d5a776 100644 --- a/docs/tutorials/first-model.md +++ b/docs/tutorials/first-model.md @@ -201,14 +201,14 @@ cat office.idf ```idf Zone, - Open Plan, !- Name - , !- Direction of Relative North - , !- X Origin - , !- Y Origin - , !- Z Origin - , !- Type - 1, !- Multiplier - 2.7; !- Ceiling Height + Open Plan, !- Name + , !- Direction of Relative North + , !- X Origin + , !- Y Origin + , !- Z Origin + , !- Type + 1, !- Multiplier + 2.7; !- Ceiling Height ``` Your zone is called `Open Plan`, and so is the wall's `Zone Name` further down diff --git a/packages/core/src/write/idf.ts b/packages/core/src/write/idf.ts index 5112c5c..b795f0b 100644 --- a/packages/core/src/write/idf.ts +++ b/packages/core/src/write/idf.ts @@ -13,7 +13,13 @@ export interface WriteIdfOptions { */ comments?: boolean; /** - * Column the field-name comments are aligned to. + * Column the field-name comments are aligned to, counting from 1 as an editor does. + * + * The default puts `!-` where EnergyPlus itself puts it. Its own example files write the marker + * at column 30 on 223 of the 231 commented lines of `1ZoneUncontrolled.idf`, and matching that + * is what keeps a rewritten object flush with the untouched objects around it: on a preserving + * write, a column of its own would leave a visible seam at every edit. + * * @defaultValue 30 */ commentColumn?: number; @@ -193,6 +199,7 @@ function decidePreservation( export interface ObjectWriteOptions { comments: boolean; + /** Where `!-` goes, counted from 1. See {@link WriteIdfOptions.commentColumn}. */ commentColumn: number; indent: string; /** Put the whole object on one line. See `WriteIdfOptions.compressed`. */ @@ -271,7 +278,20 @@ export function writeObject(obj: IdfObject, options: ObjectWriteOptions): string // and a run of bare commas is noise. But IDF is positional: if extensible // groups follow, every fixed slot must be emitted or the groups land one // field early and each value is read into the wrong slot on the way back in. - const lastFixed = groups.length > 0 ? fixed.length - 1 : lastSetIndex(obj, fixed); + // + // The annotations are the third case, and the reason is the one that governs this whole path: a + // field the author WROTE OUT as a blank is as much a thing the author wrote as a field left + // bare of its comment. Dropping it takes the author's `!- Cooling Design Capacity Method` with + // it, so a single-field edit shortens one Sizing:System from 38 lines to 22. Across the 693 + // example files that is 20,571 lines, more than any other difference a rewrite makes. + // + // Only on this path. A write with no author behind it keeps trimming, as it always has. + const authored = options.annotations?.length ?? 0; + const lastAuthored = authored - (obj.isNamed ? 1 : 0) - 1; + const lastFixed = + groups.length > 0 + ? fixed.length - 1 + : Math.min(fixed.length - 1, Math.max(lastSetIndex(obj, fixed), lastAuthored)); for (let i = 0; i <= lastFixed; i += 1) { const field = fixed[i]!; cells.push({ value: formatValue(definition, field, obj.get(field)), label: humanize(field) }); @@ -317,7 +337,10 @@ export function writeObject(obj: IdfObject, options: ObjectWriteOptions): string if (openComment === undefined) { lines.push(open); } else { - const padding = ' '.repeat(Math.max(1, options.commentColumn - open.length)); + // Minus one because the option is a COLUMN, counted from 1, and this is an offset into a + // string, counted from 0. Applying it as an offset put every comment one column right of + // where the files being imitated put it. + const padding = ' '.repeat(Math.max(1, options.commentColumn - 1 - open.length)); lines.push(`${open}${padding}${openComment}`); } open = ''; diff --git a/packages/core/tests/docs-snippets.test.ts b/packages/core/tests/docs-snippets.test.ts index 121c76f..3eaa780 100644 --- a/packages/core/tests/docs-snippets.test.ts +++ b/packages/core/tests/docs-snippets.test.ts @@ -250,7 +250,7 @@ describe('docs/tutorials/first-model.md', () => { // which is what stops the vertices shifting a field early. const outPath = join(dir, 'office.idf'); await saveIdf(doc, outPath); - expect(readFileSync(outPath, 'latin1')).toContain('Open Plan, !- Name'); + expect(readFileSync(outPath, 'latin1')).toContain('Open Plan, !- Name'); // Step 8: reading it back. const reloaded = await loadIdf(outPath); diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index 4d94d56..9988785 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -803,3 +803,57 @@ describe('the line the author put a value on', () => { expect(writeIdf(document)).toMatch(/\n\s+42\.0;\s+!- North Axis/); }); }); + +describe('the fields the author wrote out as blanks', () => { + // The writer stops at the last field that is SET, so a run of explicit commas the author wrote + // is dropped and their field-name comments go with them. A single-field edit took one + // Sizing:System from 38 lines to 22; across the 693 example files it is 20,571 lines, more than + // any other difference a rewrite makes. A field written out as a blank is as much a thing the + // author wrote as a field left bare of its comment, which is the rule this path already follows. + const BLANKS = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0.0, !- North Axis {deg}', + ' , !- Terrain', + ' , !- Loads Convergence Tolerance Value', + ' , !- Temperature Convergence Tolerance Value', + ' ; !- Solar Distribution', + '', + ].join('\n'); + + it('keeps them, and their comments, through an edit', () => { + const { document } = parseIdf(BLANKS, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + const written = writeIdf(document); + + expect(written.split('\n')).toHaveLength(BLANKS.split('\n').length); + expect(written).toContain('!- Terrain'); + expect(written).toContain('!- Solar Distribution'); + }); + + it('still trims them where there is no author to be faithful to', () => { + // A document read without preservation has nothing to reproduce, so the ordinary writer's + // habit applies and a run of bare commas stays out of the output. + const { document } = parseIdf(BLANKS, v26, { strict: false }); + + expect(writeIdf(document)).not.toContain('!- Solar Distribution'); + }); +}); + +describe('the column the comment goes in', () => { + it('puts the marker where EnergyPlus puts it, so a rewrite leaves no seam', () => { + // `1ZoneUncontrolled.idf` writes `!-` at index 29 on 223 of its 231 commented lines. The + // option is a COLUMN, counted from 1, and was being applied as an index. + const text = 'Version, 26.1;\n\nBuilding,\n My Building,\n 0.0;\n'; + const { document } = parseIdf(text, v26, { strict: false, preserveFormatting: true }); + document.require('Building', 'My Building').set('north_axis', 42); + + for (const line of writeIdf(document).split('\n')) { + const marker = line.indexOf('!-'); + if (marker > 0) expect(marker).toBe(29); + } + }); +}); diff --git a/packages/core/tests/write.test.ts b/packages/core/tests/write.test.ts index 42aaf16..93d3650 100644 --- a/packages/core/tests/write.test.ts +++ b/packages/core/tests/write.test.ts @@ -142,7 +142,12 @@ describe('writer defaults are pinned (FR-017)', () => { expect(fieldLines.every((l) => l.startsWith(' ') && !l.startsWith(' '))).toBe(true); }); - it('puts the comment at column 30', () => { + it('puts the comment at column 30, which is index 29', () => { + // Where EnergyPlus itself writes it: `1ZoneUncontrolled.idf` puts `!-` at index 29 on 223 of + // its 231 commented lines. The default used to be applied as an INDEX, which put every line + // this writer produced one place right of the files it imitates. On a preserving write that + // is the difference that shows, because a rewritten object's comments then stand one column + // clear of every untouched object around it and each save leaves a visible seam. const text = writeIdf(model(v26)); let checked = 0; @@ -151,8 +156,8 @@ describe('writer defaults are pinned (FR-017)', () => { if (marker <= 0) continue; // Only lines the padding actually reached: a value longer than the column pushes the comment // right, and that overflow behaviour is itself one of the seven differences. - if (line.slice(0, marker).trimEnd().length < 30) { - expect(marker).toBe(30); + if (line.slice(0, marker).trimEnd().length < 29) { + expect(marker).toBe(29); checked += 1; } } From 20453c57f753bfe815cc45efc988e343f8a0df12 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 16:52:45 -0400 Subject: [PATCH 08/14] Say where an object's characters were `changedObjects()` was published for a consumer that could not use it. Turning an edit into the smallest possible change to a file takes three things: WHICH objects will be rewritten, WHAT text each becomes, and WHERE the old one was. The first two were public. The third was reachable only through the anchoring, which is internal on purpose, so a consumer had to write the whole file and diff it, which is the work `changedObjects()` exists to avoid. Registered in idfkit/idfkit-conformance#6, and found by the language server team reading the branch before it merged, which is what asking them to read it was for. Two things it is NOT. It is not `SOURCE`. That symbol is cleared the moment an object is touched, because its absence is what marks the object for rewriting, so it is useless for locating exactly the objects worth locating. `ORIGIN` is the same number recorded once and never cleared, and `regionOf` still checks anchor identity, so an object carrying an index from a file it is no longer in is not handed a range from this one. It is not `statement.region`. The extent a preserving write replaces reaches past the semicolon to a comment on the terminator's own line, which is the last field's comment. A consumer replacing the shorter range would leave that comment behind, describing a field that had just moved, which is the defect `extentEnds` exists to close. Handing out a range the writer does not use would have reopened it outside the writer. Computed once per document, since the retained source does not change after the read. What the method does NOT settle is where the replacement TEXT comes from, and `writeObject` is not the answer: a preserving write hands it the author's own per-field annotations, which are internal, so options built by hand come back with the author's units and notes as generated labels. The doc comment says so rather than leaving a consumer to discover it. That gap is open. --- packages/core/src/document.ts | 65 +++++++++++++++++++++++- packages/core/src/internal.ts | 11 ++++ packages/core/src/object.ts | 4 +- packages/core/src/parse/idf.ts | 5 +- packages/core/src/preserve/write.ts | 9 +++- packages/core/tests/preserve.test.ts | 75 ++++++++++++++++++++++++++++ 6 files changed, 165 insertions(+), 4 deletions(-) diff --git a/packages/core/src/document.ts b/packages/core/src/document.ts index a25c1a6..a8901e9 100644 --- a/packages/core/src/document.ts +++ b/packages/core/src/document.ts @@ -1,10 +1,12 @@ import type { Schema, SlimType } from '@idfkit/schemas'; import { IdfCollection } from './collection.js'; -import { DATA, KEY, NAME, OWNER, SHAPE, SOURCE } from './internal.js'; +import { DATA, KEY, NAME, ORIGIN, OWNER, SHAPE, SOURCE } from './internal.js'; import { IdfObject, type FieldValues, type ObjectOwner, type StoredValue } from './object.js'; import { isUntouched, type PreservedSource } from './preserve/source.js'; +import { extentEnds } from './preserve/write.js'; import { ReferenceGraph } from './references.js'; +import type { Region } from './syntax/region.js'; import type { AnyTypeMap, ObjectOf, TypeNameOf, UntypedMap, ValuesOf } from './typemap.js'; /** @@ -331,6 +333,67 @@ export class IdfDocument implements ObjectOwn } } + /** + * Where an object's characters sit in {@link rawText}, or `undefined` if they sit nowhere. + * + * `undefined` for an object added since the read, for a document read without preservation, and + * for one read from the object notation, which has no statements to point at and preserves + * all-or-nothing. + * + * This is what makes {@link changedObjects} usable. Turning an edit into the smallest possible + * change to a file takes three things: WHICH objects will be rewritten, WHAT text each becomes, + * and WHERE the old one was. Without the third a consumer has to write the whole file and diff + * it, which is the work `changedObjects` exists to avoid. + * + * The range is where the object WAS, and stays answerable after it changes. That is the case it + * is for: the objects worth locating are the ones being rewritten. + * + * ```ts + * for (const obj of document.changedObjects()) { + * const at = document.regionOf(obj); + * if (at === undefined) continue; // added since the read; there is no old text to replace + * edits.push({ range: at, newText: replacementFor(obj) }); + * } + * ``` + * + * **Where the replacement text comes from is not settled by this method, and `writeObject` is + * not the answer.** A preserving write hands that function the author's own per-field comments, + * which are internal, so calling it with options built by hand produces text that differs from + * what {@link writeIdf} would have produced for the same object: the author's units and notes + * come back as generated labels. A consumer that needs the two to agree has to take the whole + * file from `writeIdf`. This method locates the edit; producing its text for a single object is + * a gap that is open, and it is recorded rather than papered over. + * + * The end of the range is the WRITER's, which is not always the semicolon: a comment on the + * terminator's own line is that statement's last field's comment and a preserving write replaces + * it. A range that stopped at the semicolon would leave it behind, describing a field that had + * just moved. + * + * Offsets, not a line and column: `Region` carries the conversion, and a consumer that wants one + * has the text to compute it from, while going the other way costs a scan. + */ + #extents: number[] | undefined; + + regionOf(obj: IdfObject): Region | undefined { + const source = this.#source; + if (source === undefined) return undefined; + const at = obj[ORIGIN]; + if (at === undefined) return undefined; + // The identity check that guards `isUntouched`, for the same reason: an object carrying an + // index from a file it is no longer in would otherwise be handed a range from this one. + if (source.anchors[at] !== obj) return undefined; + // The object notation records one anchor per object and no statement, so there is nothing here + // to point at. Preservation is all-or-nothing there and a per-object range would be a fiction. + const statement = source.layer.statements[at]; + if (statement === undefined) return undefined; + // The END is the writer's, not the statement's. A comment on the terminator's own line is that + // statement's last field's comment and the preserving write replaces it; a range stopping at + // the semicolon would leave it behind, on a line describing a field that had just moved. + // Computed once per document, since the retained source does not change after the read. + this.#extents ??= extentEnds(source); + return { start: statement.region.start, end: this.#extents[at] ?? statement.region.end }; + } + /** Reference targets that no object provides. */ danglingReferences(): ReturnType { const valid = new Set(); diff --git a/packages/core/src/internal.ts b/packages/core/src/internal.ts index e2d41fc..a9d8e71 100644 --- a/packages/core/src/internal.ts +++ b/packages/core/src/internal.ts @@ -39,3 +39,14 @@ export const KEY = Symbol('idfkit.key'); * new value and back again is unchanged by comparison and touched in truth. */ export const SOURCE = Symbol('idfkit.source'); +/** + * Which statement an object was READ from, kept whether or not it has since changed. + * + * `SOURCE` is cleared the moment an object is touched, because its absence is what marks the + * object as needing to be rewritten. That makes it useless for saying where the old characters + * were, which is exactly the question a consumer building a minimal edit has to ask about a + * CHANGED object. This is the same number, recorded once and never cleared. + * + * @internal + */ +export const ORIGIN = Symbol('idfkit.origin'); diff --git a/packages/core/src/object.ts b/packages/core/src/object.ts index 1fccf82..13d44cb 100644 --- a/packages/core/src/object.ts +++ b/packages/core/src/object.ts @@ -1,7 +1,7 @@ import type { SlimField, SlimType } from '@idfkit/schemas'; import { ExtensibleList } from './extensible.js'; -import { DATA, KEY, NAME, OWNER, SHAPE, SOURCE } from './internal.js'; +import { DATA, KEY, NAME, ORIGIN, OWNER, SHAPE, SOURCE } from './internal.js'; import { shapeFor, type ObjectShape } from './shape.js'; /** A scalar field value. `undefined` means the field is absent. */ @@ -56,6 +56,7 @@ export class IdfObject { declare [KEY]: string; /** Index into the document's preserved anchors, or `undefined` once anything has changed this. */ declare [SOURCE]: number | undefined; + declare [ORIGIN]: number | undefined; /** * Objects are built through `IdfObject.create`, never `new`, because each @@ -83,6 +84,7 @@ export class IdfObject { // it exists (FR-007). `clone` builds through here too, which is why a copy is touched as well: // it is a different object from the one the characters describe. Object.defineProperty(obj, SOURCE, { value: undefined, writable: true }); + Object.defineProperty(obj, ORIGIN, { value: undefined, writable: true }); for (const [field, value] of Object.entries(values)) { if (value === undefined || value === null) continue; diff --git a/packages/core/src/parse/idf.ts b/packages/core/src/parse/idf.ts index d337687..d8752b5 100644 --- a/packages/core/src/parse/idf.ts +++ b/packages/core/src/parse/idf.ts @@ -1,7 +1,7 @@ import type { Schema, SlimType } from '@idfkit/schemas'; import { IdfDocument } from '../document.js'; -import { SOURCE } from '../internal.js'; +import { ORIGIN, SOURCE } from '../internal.js'; import type { ExtensibleGroup, FieldValues, IdfObject, StoredValue } from '../object.js'; import { statementIndexes } from '../preserve/source.js'; import { layerCollector } from '../syntax/layer.js'; @@ -143,6 +143,9 @@ export function parseIdf( if (at !== undefined) { anchors[at] = built; built[SOURCE] = at; + // The same number, kept past the first edit. `SOURCE` goes when the object is touched, + // which is what marks it for rewriting; this one answers where its characters WERE. + built[ORIGIN] = at; } // Reported after the object is built, never instead of building it: a value of the wrong diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index 5ae930e..c5f5b6a 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -135,8 +135,15 @@ function lastNonEmpty(parts: readonly string[]): string { * - Reformatting replaces it, which is the defect this closes. * * The tokens are in source order and so are the statements, so one cursor walks both. + * + * Exported so that `IdfDocument.regionOf` answers with the SAME extent this walk replaces. + * Handing a consumer `statement.region` instead would stop one character short of the + * terminator-line comment, and an edit built on it would leave that comment behind, which is + * the defect this function exists to close. + * + * @internal */ -function extentEnds(source: PreservedSource): number[] { +export function extentEnds(source: PreservedSource): number[] { const { statements, tokens, text } = source.layer; const ends = statements.map((statement) => statement.region.end); diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index 9988785..a1c2bd3 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -857,3 +857,78 @@ describe('the column the comment goes in', () => { } }); }); + +describe('where an object\'s characters were', () => { + // `changedObjects()` says WHICH objects a write will rewrite. Without saying WHERE the old ones + // are, a consumer building the smallest possible change has to write the whole file and diff it, + // which is the work that method exists to avoid. Found by the language server team reading the + // branch before it merged. + const TEXT = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0.0; !- North Axis {deg}', + '', + 'Timestep, 4;', + '', + ].join('\n'); + + const read = () => parseIdf(TEXT, v26, { strict: false, preserveFormatting: true }).document; + + it('locates an object that has not changed', () => { + const document = read(); + const at = document.regionOf(document.require('Building', 'My Building'))!; + + expect(document.rawText!.slice(at.start, at.end)).toBe( + 'Building,\n My Building, !- Name\n 0.0; !- North Axis {deg}' + ); + }); + + it('still locates it after it changes, which is the case it is for', () => { + // The objects worth locating are the ones being rewritten, and `SOURCE` is cleared the moment + // one is touched because its absence is what marks it. A second record answers this. + const document = read(); + const building = document.require('Building', 'My Building'); + const before = document.regionOf(building); + building.set('north_axis', 42); + + expect(document.regionOf(building)).toEqual(before); + expect([...document.changedObjects()]).toContain(building); + }); + + it('reaches past the semicolon to the comment the writer replaces', () => { + // Not `statement.region`, which stops at the terminator. A comment on the terminator's own line + // is that statement's last field's comment and a preserving write rewrites it; a consumer + // replacing the shorter range would leave it behind describing a field that had just moved. + const document = read(); + const at = document.regionOf(document.require('Building', 'My Building'))!; + + expect(document.rawText!.slice(at.start, at.end)).toContain('!- North Axis {deg}'); + }); + + it('answers nothing for an object added since the read', () => { + const document = read(); + const added = document.addRaw('Zone', 'Late Arrival', {}); + + expect(document.regionOf(added)).toBeUndefined(); + }); + + it('answers nothing for a document read without preservation', () => { + const document = parseIdf(TEXT, v26, { strict: false }).document; + + expect(document.regionOf(document.require('Building', 'My Building'))).toBeUndefined(); + }); + + it('locates each object separately, in source order', () => { + const document = read(); + const regions = [...document.objects()] + .map((obj) => document.regionOf(obj)) + .filter((region) => region !== undefined); + + expect(regions).toHaveLength(3); + for (let i = 1; i < regions.length; i += 1) { + expect(regions[i]!.start).toBeGreaterThanOrEqual(regions[i - 1]!.end); + } + }); +}); From 83784c5010d1dedd616fe916b5d9dc4fc6cb0c80 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 17:30:29 -0400 Subject: [PATCH 09/14] Give back the text that belongs in that range `regionOf` told a consumer where to put text it could not correctly generate. The one public function shaped to fill the hole is `writeObject`, and a preserving write hands that function the author's own per-field annotations, which are internal, so a caller building options by hand gets the author's units and notes back as generated labels. `!- North Axis {deg}` came back `!- North Axis`. A unit lost from an engineering model by an editor asked to save a file. The doc comment said so, in bold, which is not good enough: we had already agreed a doc comment is not a load-bearing place for a correctness constraint when the same argument was made about what `changedObjects()` does not answer. Two readers reached the same conclusion independently, from the language service and from the web editor, that the range alone changed nothing they would build. Registered in idfkit/idfkit-conformance#6. `renderStatement` is factored out of the preserving walk rather than reimplemented beside it. Two copies would be two answers to one question, and the question is which bytes go in the file. ONE OPTION, `fieldComments`, because it is the only one a preserving write honours. `indent`, `commentColumn`, `ordering` and `versionFirst` are refused by `writeIdf` alongside `preserveFormatting`; `comments: false` and `compressed` defeat preservation entirely and send the document down the formatting path. The first shape of this method took all of them, which would have let a caller render one object on terms the surrounding file was not written on, reintroducing the divergence one layer down. The test that matters splices every changed object's render into its own range and asserts the result equals `writeIdf` byte for byte. If that ever fails, an editor built on these three names is silently writing a different file. --- packages/core/src/document.ts | 51 ++++++++++++++++- packages/core/src/preserve/write.ts | 31 +++++++--- packages/core/tests/preserve.test.ts | 84 +++++++++++++++++++++++++++- 3 files changed, 157 insertions(+), 9 deletions(-) diff --git a/packages/core/src/document.ts b/packages/core/src/document.ts index a8901e9..beca923 100644 --- a/packages/core/src/document.ts +++ b/packages/core/src/document.ts @@ -4,9 +4,10 @@ import { IdfCollection } from './collection.js'; import { DATA, KEY, NAME, ORIGIN, OWNER, SHAPE, SOURCE } from './internal.js'; import { IdfObject, type FieldValues, type ObjectOwner, type StoredValue } from './object.js'; import { isUntouched, type PreservedSource } from './preserve/source.js'; -import { extentEnds } from './preserve/write.js'; +import { extentEnds, renderStatement } from './preserve/write.js'; import { ReferenceGraph } from './references.js'; import type { Region } from './syntax/region.js'; +import type { WriteIdfOptions } from './write/idf.js'; import type { AnyTypeMap, ObjectOf, TypeNameOf, UntypedMap, ValuesOf } from './typemap.js'; /** @@ -394,6 +395,54 @@ export class IdfDocument implements ObjectOwn return { start: statement.region.start, end: this.#extents[at] ?? statement.region.end }; } + /** + * One object, rendered exactly as a preserving write would render it. + * + * The text that belongs in the range {@link regionOf} returns, so the two compose into an edit + * that leaves the file byte for byte where {@link writeIdf} would have left it. `undefined` for + * an object the retained source does not hold, which is the same set `regionOf` declines. + * + * ```ts + * for (const obj of document.changedObjects()) { + * const at = document.regionOf(obj); + * const text = document.renderObject(obj); + * if (at === undefined || text === undefined) continue; // added since the read + * edits.push({ range: at, newText: text }); + * } + * ``` + * + * `writeObject` is not this, and that is the reason this exists. A preserving write hands that + * function the author's own per-field annotations, which are internal, so calling it with options + * built by hand comes back with the author's units and notes as generated labels: `!- North Axis + * {deg}` becomes `!- North Axis`. That is a unit lost from an engineering model by an editor + * asked to save a file, and no doc comment is a good enough guard against it. + * + * `fieldComments` is the ONLY option, because it is the only one a preserving write honours. + * `indent`, `commentColumn`, `ordering` and `versionFirst` are refused by {@link writeIdf} + * alongside `preserveFormatting`, and `comments: false` and `compressed` defeat preservation and + * send the whole document down the formatting path instead. Accepting any of them here would let + * a caller render one object on terms the surrounding file was not written on, which is the exact + * divergence this method exists to prevent. + * + * No trailing line break: the range this fills ends at the terminator, or at the comment on that + * line, and the break after it is the first character of what separates one object from the next, + * which a preserving write leaves in place. + */ + renderObject(obj: IdfObject, options: Pick = {}): string | undefined { + const source = this.#source; + if (source === undefined) return undefined; + const at = obj[ORIGIN]; + if (at === undefined || source.anchors[at] !== obj) return undefined; + // The same defaults `writeIdf` resolves for its preserving branch, which is the only branch + // that can reach this text. + return renderStatement(source, at, { + comments: true, + commentColumn: 30, + indent: ' ', + labelBareFields: options.fieldComments === 'generate', + }); + } + /** Reference targets that no object provides. */ danglingReferences(): ReturnType { const valid = new Set(); diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index c5f5b6a..4ccb84f 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -63,16 +63,33 @@ function statementPart( // the object would mean holding a reference to something the document has let go. if (anchored[OWNER] === undefined) return ''; if (isUntouched(anchored, source)) return verbatim; + return renderStatement(source, index, options); +} + +/** + * One statement's object, rendered the way this walk renders it. + * + * Factored out rather than inlined because `IdfDocument.renderObject` has to produce exactly this + * text: a consumer splicing something else into the range `regionOf` returns gets a file that + * differs from `writeIdf`, silently. Two copies of this would be two answers to one question. + * + * Without a trailing newline. A statement's extent ends at its terminator, or at the comment on + * that same line, and in neither case does it include the line break: the break is the first + * character of the gap. `writeObject` ends with one because it is also used to write whole + * documents, so emitting it here would put the break in twice and grow the file by a blank line + * per reformatted object. Every object in a file, edited and saved twice, would grow it twice. + * + * @internal + */ +export function renderStatement( + source: PreservedSource, + index: number, + options: ObjectWriteOptions +): string { // Re-render the VALUES, and keep the author's comments. An edit asks for the first and never for // the second, and rebuilding a comment destroys whatever the schema cannot regenerate: a note to // a colleague, and the field's unit, which the ordinary label does not carry. - // - // Without its trailing newline. A statement's extent ends at its terminator, or at the comment on - // that same line, and in neither case does it include the line break: the break is the first - // character of the gap. `writeObject` ends with one because it is also used to write whole - // documents, so emitting it here would put the break in twice and grow the file by a blank line - // per reformatted object. Every object in a file, edited and saved twice, would grow it twice. - const written = writeObject(anchored, { + const written = writeObject(source.anchors[index]!, { ...options, annotations: annotations(source, index), }); diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index a1c2bd3..83c5ae2 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -1,6 +1,6 @@ import { beforeAll, describe, expect, it } from 'vitest'; -import { parseEpJson, parseIdf, scanIdf, writeEpJson, writeIdf } from '@idfkit/core'; +import { parseEpJson, parseIdf, scanIdf, writeEpJson, writeIdf, writeObject } from '@idfkit/core'; import type { Schema } from '@idfkit/schemas'; import { schema, syntaxFixture, syntaxFixtures } from './helpers.js'; @@ -932,3 +932,85 @@ describe('where an object\'s characters were', () => { } }); }); + +describe('the text that belongs in that range', () => { + // The third leg. Knowing WHICH objects change and WHERE the old text is buys a consumer nothing + // while producing the new text for ONE object has no correct form: `writeObject` called with + // options built by hand comes back with the author's units as generated labels, because the + // annotations a preserving write hands it are internal. + const TEXT = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0.0; !- North Axis {deg}', + '', + 'Timestep, 4;', + '', + ].join('\n'); + + const read = () => parseIdf(TEXT, v26, { strict: false, preserveFormatting: true }).document; + + it('keeps the unit that the ordinary per-object writer drops', () => { + const document = read(); + const building = document.require('Building', 'My Building'); + building.set('north_axis', 42); + + expect(document.renderObject(building)).toContain('!- North Axis {deg}'); + // What a consumer would have had to reach for, and what it costs. + expect(writeObject(building, { comments: true, commentColumn: 30, indent: ' ' })).not.toContain( + '{deg}' + ); + }); + + it('splices into its own range to give back exactly what a whole write gives back', () => { + // The claim the three names make together, pinned as one assertion. If this ever fails, an + // editor built on them is silently writing a different file from the one `writeIdf` writes. + const document = read(); + document.require('Building', 'My Building').set('north_axis', 42); + [...document.all('Timestep')][0]!.set('number_of_timesteps_per_hour', 6); + + let spliced = document.rawText!; + // Back to front, so an earlier edit does not move a later range. + const changed = [...document.changedObjects()] + .map((obj) => ({ at: document.regionOf(obj)!, text: document.renderObject(obj)! })) + .sort((a, b) => b.at.start - a.at.start); + expect(changed).toHaveLength(2); + for (const { at, text } of changed) { + spliced = spliced.slice(0, at.start) + text + spliced.slice(at.end); + } + + expect(spliced).toBe(writeIdf(document)); + }); + + it('takes the one option a preserving write honours, and no others', () => { + // `indent`, `commentColumn`, `ordering` and `versionFirst` are refused by `writeIdf` alongside + // `preserveFormatting`; `comments: false` and `compressed` defeat preservation entirely and + // send the document down the formatting path. Accepting any of them here would render one + // object on terms the surrounding file was not written on. + // A source with a field the author wrote BARE, which is what the option is about. + const bare = 'Version, 26.1;\n\nBuilding,\n My Building, !- Name\n 0.0, !- North Axis {deg}\n City;\n'; + const { document } = parseIdf(bare, v26, { strict: false, preserveFormatting: true }); + const building = document.require('Building', 'My Building'); + building.set('north_axis', 42); + + // Bare stays bare, and asking for labels is the one thing that changes. + expect(document.renderObject(building)).not.toContain('!- Terrain'); + expect(document.renderObject(building, { fieldComments: 'generate' })).toContain('!- Terrain'); + }); + + it('ends where the range ends, with no line break of its own', () => { + const document = read(); + + expect(document.renderObject(document.require('Building', 'My Building'))).not.toMatch(/\n$/); + }); + + it('answers nothing for an object the retained source does not hold', () => { + const document = read(); + + expect(document.renderObject(document.addRaw('Zone', 'Late Arrival', {}))).toBeUndefined(); + expect(parseIdf(TEXT, v26, { strict: false }).document.renderObject( + parseIdf(TEXT, v26, { strict: false }).document.require('Building', 'My Building') + )).toBeUndefined(); + }); +}); From 55d3b6299efb463df6334f6261c6562e2754b47f Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 17:50:31 -0400 Subject: [PATCH 10/14] Derive each fact about a statement once A cleanup pass over the preserving writer. No output moves: both corpora pass with the same counts and all 779 tests hold. THE ONE THAT MATTERED. `annotations` started its token cursor at zero and seeked forward to the statement it wanted, which is a full prefix scan of the token stream per statement and quadratic in the file. Reformatting every object of HospitalLowEnergy.idf, 6,874 statements over 233,925 tokens, took 647 ms. It now takes 76 ms. Neither benchmark covers it. Both time an UNCHANGED preserving write, where every statement is copied verbatim and `annotations` is never reached, so the budget gate would have held at any cost on this path. I nearly missed it twice: my first A/B showed no difference because the harness set each field to the value it already held, which the writer correctly treats as no edit at all, so I was timing the same untouched path the benchmarks time. The fix is real; the first measurement of it was not. `extentEnds` and the cursor now come from one memo on the retained source, which never changes after the read. Before this, the extent was derived in two places with two lifetimes: `writePreserved` recomputed it on every write, and `regionOf` cached it in a field of its own. One immutable derivation with two caches is a disagreement waiting to happen, and the doc comment saying `extentEnds` was exported "so regionOf answers with the same extent" was the tell. THE REST, none of which changes behaviour: The anchor rule lives once, as `originOf`, beside the `isUntouched` it mirrors. `regionOf` and `renderObject` are documented as declining the same set and were deciding it separately. `preservingOptions` resolves what a preserving write resolves. `renderObject` had its own copy of the defaults, including the comment column whose value carries a six-line argument from the example corpus; moving that argument would have left the accessor on the old number, silently breaking the byte-for-byte agreement its own doc comment promises. A private field sat between `regionOf`'s doc comment and `regionOf`, so the whole comment, worked example included, documented the field and the method shipped bare. Removing the field fixed it. `annotations` no longer pre-fills entries the loop overwrites unconditionally: a placeholder is a second, contradictory statement of what an entry defaults to. The `ORIGIN` comment no longer names the strip tag in prose. `stripInternal` matches the tag as TEXT anywhere in a comment, so explaining the hazard triggered it, dropped the symbol from the emitted types and broke the build. The comment now says so without saying it. Two tests read one fixture rather than two identical copies, so a change to it cannot leave them asserting against different files while both pass. --- packages/core/src/document.ts | 41 +++++--------- packages/core/src/internal.ts | 7 ++- packages/core/src/preserve/source.ts | 21 ++++++- packages/core/src/preserve/write.ts | 83 ++++++++++++++++++++-------- packages/core/src/write/idf.ts | 45 ++++++++++++--- packages/core/tests/preserve.test.ts | 69 +++++++++++------------ 6 files changed, 171 insertions(+), 95 deletions(-) diff --git a/packages/core/src/document.ts b/packages/core/src/document.ts index beca923..eecd5d2 100644 --- a/packages/core/src/document.ts +++ b/packages/core/src/document.ts @@ -3,11 +3,11 @@ import type { Schema, SlimType } from '@idfkit/schemas'; import { IdfCollection } from './collection.js'; import { DATA, KEY, NAME, ORIGIN, OWNER, SHAPE, SOURCE } from './internal.js'; import { IdfObject, type FieldValues, type ObjectOwner, type StoredValue } from './object.js'; -import { isUntouched, type PreservedSource } from './preserve/source.js'; -import { extentEnds, renderStatement } from './preserve/write.js'; +import { isUntouched, originOf, type PreservedSource } from './preserve/source.js'; +import { derivedOf, renderStatement } from './preserve/write.js'; import { ReferenceGraph } from './references.js'; import type { Region } from './syntax/region.js'; -import type { WriteIdfOptions } from './write/idf.js'; +import { preservingOptions, type WriteIdfOptions } from './write/idf.js'; import type { AnyTypeMap, ObjectOf, TypeNameOf, UntypedMap, ValuesOf } from './typemap.js'; /** @@ -373,16 +373,10 @@ export class IdfDocument implements ObjectOwn * Offsets, not a line and column: `Region` carries the conversion, and a consumer that wants one * has the text to compute it from, while going the other way costs a scan. */ - #extents: number[] | undefined; - regionOf(obj: IdfObject): Region | undefined { const source = this.#source; - if (source === undefined) return undefined; - const at = obj[ORIGIN]; - if (at === undefined) return undefined; - // The identity check that guards `isUntouched`, for the same reason: an object carrying an - // index from a file it is no longer in would otherwise be handed a range from this one. - if (source.anchors[at] !== obj) return undefined; + const at = originOf(obj, source); + if (source === undefined || at === undefined) return undefined; // The object notation records one anchor per object and no statement, so there is nothing here // to point at. Preservation is all-or-nothing there and a per-object range would be a fiction. const statement = source.layer.statements[at]; @@ -390,9 +384,7 @@ export class IdfDocument implements ObjectOwn // The END is the writer's, not the statement's. A comment on the terminator's own line is that // statement's last field's comment and the preserving write replaces it; a range stopping at // the semicolon would leave it behind, on a line describing a field that had just moved. - // Computed once per document, since the retained source does not change after the read. - this.#extents ??= extentEnds(source); - return { start: statement.region.start, end: this.#extents[at] ?? statement.region.end }; + return { start: statement.region.start, end: derivedOf(source).ends[at] ?? statement.region.end }; } /** @@ -428,19 +420,16 @@ export class IdfDocument implements ObjectOwn * line, and the break after it is the first character of what separates one object from the next, * which a preserving write leaves in place. */ - renderObject(obj: IdfObject, options: Pick = {}): string | undefined { + renderObject( + obj: IdfObject, + options: Pick = {} + ): string | undefined { const source = this.#source; - if (source === undefined) return undefined; - const at = obj[ORIGIN]; - if (at === undefined || source.anchors[at] !== obj) return undefined; - // The same defaults `writeIdf` resolves for its preserving branch, which is the only branch - // that can reach this text. - return renderStatement(source, at, { - comments: true, - commentColumn: 30, - indent: ' ', - labelBareFields: options.fieldComments === 'generate', - }); + const at = originOf(obj, source); + if (source === undefined || at === undefined) return undefined; + // The options `writeIdf` resolves for its preserving branch, resolved by the same function, so + // the two cannot disagree about the bytes. + return renderStatement(source, at, preservingOptions(options)); } /** Reference targets that no object provides. */ diff --git a/packages/core/src/internal.ts b/packages/core/src/internal.ts index a9d8e71..f9bc981 100644 --- a/packages/core/src/internal.ts +++ b/packages/core/src/internal.ts @@ -47,6 +47,11 @@ export const SOURCE = Symbol('idfkit.source'); * were, which is exactly the question a consumer building a minimal edit has to ask about a * CHANGED object. This is the same number, recorded once and never cleared. * - * @internal + * Deliberately untagged, as `SOURCE` above is. `stripInternal` drops a declaration whose JSDoc + * carries the tag, and `object.ts` declares a property keyed on this symbol, so tagging it emits a + * `.d.ts` that references a symbol its own module no longer declares and the build stops. Being + * absent from the package index is what makes this module internal, and that part holds. + * + * The tag is matched as TEXT anywhere in the comment, so naming it here would strip this too. */ export const ORIGIN = Symbol('idfkit.origin'); diff --git a/packages/core/src/preserve/source.ts b/packages/core/src/preserve/source.ts index ecd278d..30c5568 100644 --- a/packages/core/src/preserve/source.ts +++ b/packages/core/src/preserve/source.ts @@ -1,4 +1,4 @@ -import { SOURCE } from '../internal.js'; +import { ORIGIN, SOURCE } from '../internal.js'; import type { IdfObject } from '../object.js'; import type { RawObject } from '../parse/lexer.js'; import type { SyntaxLayer } from '../syntax/layer.js'; @@ -77,6 +77,25 @@ export function isWholeDocumentUntouched( return true; } +/** + * The statement an object was read from, or `undefined` if this source did not read it. + * + * The identity check is the one `isUntouched` makes below, for the same reason: an object carrying + * an index from a file it is no longer in would otherwise be answered from this one. It is stated + * once here because `regionOf` and `renderObject` both decline exactly this set, and a rule two + * methods share is a rule one of them will eventually be fixed without. + * + * Reads `ORIGIN`, not `SOURCE`: the question is where the characters WERE, which stays answerable + * after the object changes, and changing it is what clears `SOURCE`. + * + * @internal + */ +export function originOf(obj: IdfObject, source: PreservedSource | undefined): number | undefined { + if (source === undefined) return undefined; + const at = obj[ORIGIN]; + return at !== undefined && source.anchors[at] === obj ? at : undefined; +} + export function isUntouched(obj: IdfObject, source: PreservedSource | undefined): boolean { if (source === undefined) return false; const at = obj[SOURCE]; diff --git a/packages/core/src/preserve/write.ts b/packages/core/src/preserve/write.ts index 4ccb84f..ccbd284 100644 --- a/packages/core/src/preserve/write.ts +++ b/packages/core/src/preserve/write.ts @@ -26,7 +26,7 @@ export function writePreserved( ): string { const text = source.layer.text; const statements = source.layer.statements; - const ends = extentEnds(source); + const ends = derivedOf(source).ends; const parts: string[] = []; // Everything before the first statement, which for a file with none is the whole text: an empty @@ -131,16 +131,17 @@ function lastNonEmpty(parts: readonly string[]): string { } /** - * Where each statement's text ends for this walk: its terminator, or the comment on that same line. + * The two facts about each statement that are derived from the token stream, computed together. * - * A comment after the semicolon with nothing but horizontal whitespace between them is the last + * `ends` is where a statement's text ends FOR THE WRITER, which is not always its terminator. A + * comment after the semicolon with nothing but horizontal whitespace between them is the last * field's comment. Leaving it in the gap is invisible while the statement is copied, because the * gap is copied too, and wrong the moment it is reformatted: the writer emits its own field comment * and the author's then arrives from the gap on the line below, so the output carries a line nobody * wrote. It is not even a duplicate, because the ordinary writer drops the unit the original * usually carries, so it reads as a stray fragment. * - * This is not the writer guessing which comment belongs to which object. "On the same line as the + * That is not the writer guessing which comment belongs to which object. "On the same line as the * terminator" is a positional fact, and it is the one case where the owner is not in question. * * Three consequences, decided rather than discovered: @@ -151,32 +152,68 @@ function lastNonEmpty(parts: readonly string[]): string { * describing a field that no longer exists. * - Reformatting replaces it, which is the defect this closes. * - * The tokens are in source order and so are the statements, so one cursor walks both. + * `firstToken` is where each statement's tokens begin. `annotations` used to start its cursor at + * zero and seek forward, which is a full prefix scan of the token stream per statement: quadratic + * in the file, and invisible to the benchmarks because they time an UNCHANGED write, where no + * statement is reformatted and `annotations` is never reached. On a ten thousand statement model + * that is the difference between milliseconds and seconds. * - * Exported so that `IdfDocument.regionOf` answers with the SAME extent this walk replaces. - * Handing a consumer `statement.region` instead would stop one character short of the - * terminator-line comment, and an edit built on it would leave that comment behind, which is - * the defect this function exists to close. + * Both come from one pass with monotone cursors, because the statements and the tokens are both in + * source order. Memoised per retained source, which never changes after the read, so the walk and + * the two document accessors share one answer rather than each deriving it. + */ +interface Derived { + /** Where each statement's text ends for the writer. */ + readonly ends: readonly number[]; + /** The first token at or after each statement's type name, as a cursor for `annotations`. */ + readonly firstToken: readonly number[]; +} + +const derived = new WeakMap(); + +/** + * The derived facts for one retained source, computed once. * - * @internal + * Exported so `IdfDocument.regionOf` answers with the SAME extent this walk replaces. Handing a + * consumer `statement.region` instead would stop short of the terminator-line comment, and an edit + * built on it would leave that comment behind, which is the defect the extent exists to close. */ -export function extentEnds(source: PreservedSource): number[] { +export function derivedOf(source: PreservedSource): Derived { + let found = derived.get(source); + if (found === undefined) { + found = compute(source); + derived.set(source, found); + } + return found; +} + +function compute(source: PreservedSource): Derived { const { statements, tokens, text } = source.layer; const ends = statements.map((statement) => statement.region.end); + const firstToken: number[] = new Array(statements.length); - let token = 0; + // Two cursors rather than one, because they track different points and both only move forward. + let atStatement = 0; + let atExtent = 0; for (let index = 0; index < statements.length; index += 1) { + const statement = statements[index]!; + + while (atStatement < tokens.length && tokens.startAt(atStatement) < statement.typeName.end) { + atStatement += 1; + } + firstToken[index] = atStatement; + const end = ends[index]!; - while (token < tokens.length && tokens.startAt(token) < end) token += 1; - if (token >= tokens.length || tokens.kindAt(token) !== 'comment') continue; + while (atExtent < tokens.length && tokens.startAt(atExtent) < end) atExtent += 1; + if (atExtent >= tokens.length || tokens.kindAt(atExtent) !== 'comment') continue; // Horizontal whitespace only. A line feed between the two puts the comment on its own line, // which makes it a comment about whatever comes next and none of this statement's business. - const between = text.slice(end, tokens.startAt(token)); + const between = text.slice(end, tokens.startAt(atExtent)); if (between.includes('\n') || between.trim() !== '') continue; - ends[index] = tokens.endAt(token); + ends[index] = tokens.endAt(atExtent); } - return ends; + return { ends, firstToken }; } /** @@ -201,15 +238,15 @@ function annotations(source: PreservedSource, index: number): FieldAnnotation[] const { statements, tokens, text } = source.layer; const statement = statements[index]!; const fields = statement.fields; - const built: FieldAnnotation[] = fields.map(() => ({ - before: [], - trailing: undefined, - startsLine: true, - })); + // Every entry is assigned in the loop below, so there is nothing to pre-fill: a placeholder + // would be a second, contradictory statement of what an entry defaults to. + const built: FieldAnnotation[] = new Array(fields.length); // One cursor over the tokens, which are in source order, as the fields are. Every comment // between the previous field's delimiter and this one's value stands on its own line above it. - let token = 0; + // It starts where this statement starts rather than at zero: seeking from the front of the file + // made a whole-document reformat quadratic in the token count. + let token = derivedOf(source).firstToken[index]!; let previousEnd = statement.typeName.end; for (let at = 0; at < fields.length; at += 1) { const field = fields[at]!; diff --git a/packages/core/src/write/idf.ts b/packages/core/src/write/idf.ts index b795f0b..d6a3440 100644 --- a/packages/core/src/write/idf.ts +++ b/packages/core/src/write/idf.ts @@ -114,20 +114,15 @@ export function writeIdf( ): string { const preserved = decidePreservation(document, options); if (preserved !== undefined) { - return writePreserved(document, preserved, { - comments: options.comments ?? true, - commentColumn: options.commentColumn ?? 30, - indent: options.indent ?? ' ', - labelBareFields: options.fieldComments === 'generate', - }); + return writePreserved(document, preserved, preservingOptions(options)); } const compressed = options.compressed ?? false; // Compressed output has no comments by definition. Asking for both is not an error, because the // narrower request is unambiguous: comments cannot survive a single-line object. const comments = compressed ? false : (options.comments ?? true); - const commentColumn = options.commentColumn ?? 30; - const indent = options.indent ?? ' '; + const commentColumn = options.commentColumn ?? DEFAULT_COMMENT_COLUMN; + const indent = options.indent ?? DEFAULT_INDENT; const versionFirst = options.versionFirst ?? true; const ordering = options.ordering ?? 'source'; @@ -197,6 +192,35 @@ function decidePreservation( return source; } +/** Where `!-` goes, counted from 1. See {@link WriteIdfOptions.commentColumn}. */ +export const DEFAULT_COMMENT_COLUMN = 30; + +/** The indent before each field line. See {@link WriteIdfOptions.indent}. */ +export const DEFAULT_INDENT = ' '; + +/** + * The options a PRESERVING write resolves, which is the only set a preserved object can be + * rendered on. + * + * `IdfDocument.renderObject` has to produce exactly the bytes this walk produces, so it resolves + * its options here rather than restating the defaults. Restating them is how the two drift: the + * comment column has a reason behind its value, and moving that reason in one place while the + * other kept the old number would break the byte-for-byte agreement `renderObject` promises. + * + * The controls are not read from the caller because a preserving write refuses them. `indent`, + * `commentColumn`, `ordering` and `versionFirst` throw when set alongside `preserveFormatting`, + * and `comments: false` and `compressed` defeat preservation and send the document down the + * formatting path, so on this path they are always their defaults. + */ +export function preservingOptions(options: WriteIdfOptions): ObjectWriteOptions { + return { + comments: true, + commentColumn: DEFAULT_COMMENT_COLUMN, + indent: DEFAULT_INDENT, + labelBareFields: options.fieldComments === 'generate', + }; +} + export interface ObjectWriteOptions { comments: boolean; /** Where `!-` goes, counted from 1. See {@link WriteIdfOptions.commentColumn}. */ @@ -286,6 +310,11 @@ export function writeObject(obj: IdfObject, options: ObjectWriteOptions): string // example files that is 20,571 lines, more than any other difference a rewrite makes. // // Only on this path. A write with no author behind it keeps trimming, as it always has. + // + // Counted as an INDEX into the fixed fields, with the name subtracted out, because the + // annotations lead with the name. Python states the same rule as a count of emitted values with + // the name included. Both are right against their own annotations and neither would notice if + // the other's convention moved, so a change to either belongs in both. const authored = options.annotations?.length ?? 0; const lastAuthored = authored - (obj.isNamed ? 1 : 0) - 1; const lastFixed = diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index 83c5ae2..9ab3fd9 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -858,26 +858,35 @@ describe('the column the comment goes in', () => { }); }); +/** + * The file the three composing accessors are exercised against. + * + * `changedObjects`, `regionOf` and `renderObject` are one capability in three names, and they are + * tested against one file so that a change to the fixture cannot leave two suites quietly asserting + * against different text while both pass. The author's unit on the terminator line is the load + * bearing part: it is what a rewrite used to destroy and what the range has to reach past. + */ +const COMPOSING = [ + 'Version, 26.1;', + '', + 'Building,', + ' My Building, !- Name', + ' 0.0; !- North Axis {deg}', + '', + 'Timestep, 4;', + '', +].join('\n'); + +const composing = () => parseIdf(COMPOSING, v26, { strict: false, preserveFormatting: true }).document; + describe('where an object\'s characters were', () => { // `changedObjects()` says WHICH objects a write will rewrite. Without saying WHERE the old ones // are, a consumer building the smallest possible change has to write the whole file and diff it, // which is the work that method exists to avoid. Found by the language server team reading the // branch before it merged. - const TEXT = [ - 'Version, 26.1;', - '', - 'Building,', - ' My Building, !- Name', - ' 0.0; !- North Axis {deg}', - '', - 'Timestep, 4;', - '', - ].join('\n'); - - const read = () => parseIdf(TEXT, v26, { strict: false, preserveFormatting: true }).document; it('locates an object that has not changed', () => { - const document = read(); + const document = composing(); const at = document.regionOf(document.require('Building', 'My Building'))!; expect(document.rawText!.slice(at.start, at.end)).toBe( @@ -888,7 +897,7 @@ describe('where an object\'s characters were', () => { it('still locates it after it changes, which is the case it is for', () => { // The objects worth locating are the ones being rewritten, and `SOURCE` is cleared the moment // one is touched because its absence is what marks it. A second record answers this. - const document = read(); + const document = composing(); const building = document.require('Building', 'My Building'); const before = document.regionOf(building); building.set('north_axis', 42); @@ -901,27 +910,27 @@ describe('where an object\'s characters were', () => { // Not `statement.region`, which stops at the terminator. A comment on the terminator's own line // is that statement's last field's comment and a preserving write rewrites it; a consumer // replacing the shorter range would leave it behind describing a field that had just moved. - const document = read(); + const document = composing(); const at = document.regionOf(document.require('Building', 'My Building'))!; expect(document.rawText!.slice(at.start, at.end)).toContain('!- North Axis {deg}'); }); it('answers nothing for an object added since the read', () => { - const document = read(); + const document = composing(); const added = document.addRaw('Zone', 'Late Arrival', {}); expect(document.regionOf(added)).toBeUndefined(); }); it('answers nothing for a document read without preservation', () => { - const document = parseIdf(TEXT, v26, { strict: false }).document; + const document = parseIdf(COMPOSING, v26, { strict: false }).document; expect(document.regionOf(document.require('Building', 'My Building'))).toBeUndefined(); }); it('locates each object separately, in source order', () => { - const document = read(); + const document = composing(); const regions = [...document.objects()] .map((obj) => document.regionOf(obj)) .filter((region) => region !== undefined); @@ -938,21 +947,9 @@ describe('the text that belongs in that range', () => { // while producing the new text for ONE object has no correct form: `writeObject` called with // options built by hand comes back with the author's units as generated labels, because the // annotations a preserving write hands it are internal. - const TEXT = [ - 'Version, 26.1;', - '', - 'Building,', - ' My Building, !- Name', - ' 0.0; !- North Axis {deg}', - '', - 'Timestep, 4;', - '', - ].join('\n'); - - const read = () => parseIdf(TEXT, v26, { strict: false, preserveFormatting: true }).document; it('keeps the unit that the ordinary per-object writer drops', () => { - const document = read(); + const document = composing(); const building = document.require('Building', 'My Building'); building.set('north_axis', 42); @@ -966,7 +963,7 @@ describe('the text that belongs in that range', () => { it('splices into its own range to give back exactly what a whole write gives back', () => { // The claim the three names make together, pinned as one assertion. If this ever fails, an // editor built on them is silently writing a different file from the one `writeIdf` writes. - const document = read(); + const document = composing(); document.require('Building', 'My Building').set('north_axis', 42); [...document.all('Timestep')][0]!.set('number_of_timesteps_per_hour', 6); @@ -1000,17 +997,17 @@ describe('the text that belongs in that range', () => { }); it('ends where the range ends, with no line break of its own', () => { - const document = read(); + const document = composing(); expect(document.renderObject(document.require('Building', 'My Building'))).not.toMatch(/\n$/); }); it('answers nothing for an object the retained source does not hold', () => { - const document = read(); + const document = composing(); expect(document.renderObject(document.addRaw('Zone', 'Late Arrival', {}))).toBeUndefined(); - expect(parseIdf(TEXT, v26, { strict: false }).document.renderObject( - parseIdf(TEXT, v26, { strict: false }).document.require('Building', 'My Building') + expect(parseIdf(COMPOSING, v26, { strict: false }).document.renderObject( + parseIdf(COMPOSING, v26, { strict: false }).document.require('Building', 'My Building') )).toBeUndefined(); }); }); From 6e4b292276838d3467cca00785b41dfafb56dee3 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 17:54:03 -0400 Subject: [PATCH 11/14] Run the formatter over what the cleanup pass touched Prettier's own check, which `npm run lint` does not include, so the two files the last commit reflowed only failed once CI reached them. --- packages/core/src/document.ts | 5 ++++- packages/core/tests/preserve.test.ts | 22 +++++++++++++--------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/core/src/document.ts b/packages/core/src/document.ts index eecd5d2..c44d226 100644 --- a/packages/core/src/document.ts +++ b/packages/core/src/document.ts @@ -384,7 +384,10 @@ export class IdfDocument implements ObjectOwn // The END is the writer's, not the statement's. A comment on the terminator's own line is that // statement's last field's comment and the preserving write replaces it; a range stopping at // the semicolon would leave it behind, on a line describing a field that had just moved. - return { start: statement.region.start, end: derivedOf(source).ends[at] ?? statement.region.end }; + return { + start: statement.region.start, + end: derivedOf(source).ends[at] ?? statement.region.end, + }; } /** diff --git a/packages/core/tests/preserve.test.ts b/packages/core/tests/preserve.test.ts index 9ab3fd9..8832bd4 100644 --- a/packages/core/tests/preserve.test.ts +++ b/packages/core/tests/preserve.test.ts @@ -877,9 +877,10 @@ const COMPOSING = [ '', ].join('\n'); -const composing = () => parseIdf(COMPOSING, v26, { strict: false, preserveFormatting: true }).document; +const composing = () => + parseIdf(COMPOSING, v26, { strict: false, preserveFormatting: true }).document; -describe('where an object\'s characters were', () => { +describe("where an object's characters were", () => { // `changedObjects()` says WHICH objects a write will rewrite. Without saying WHERE the old ones // are, a consumer building the smallest possible change has to write the whole file and diff it, // which is the work that method exists to avoid. Found by the language server team reading the @@ -955,9 +956,9 @@ describe('the text that belongs in that range', () => { expect(document.renderObject(building)).toContain('!- North Axis {deg}'); // What a consumer would have had to reach for, and what it costs. - expect(writeObject(building, { comments: true, commentColumn: 30, indent: ' ' })).not.toContain( - '{deg}' - ); + expect( + writeObject(building, { comments: true, commentColumn: 30, indent: ' ' }) + ).not.toContain('{deg}'); }); it('splices into its own range to give back exactly what a whole write gives back', () => { @@ -986,7 +987,8 @@ describe('the text that belongs in that range', () => { // send the document down the formatting path. Accepting any of them here would render one // object on terms the surrounding file was not written on. // A source with a field the author wrote BARE, which is what the option is about. - const bare = 'Version, 26.1;\n\nBuilding,\n My Building, !- Name\n 0.0, !- North Axis {deg}\n City;\n'; + const bare = + 'Version, 26.1;\n\nBuilding,\n My Building, !- Name\n 0.0, !- North Axis {deg}\n City;\n'; const { document } = parseIdf(bare, v26, { strict: false, preserveFormatting: true }); const building = document.require('Building', 'My Building'); building.set('north_axis', 42); @@ -1006,8 +1008,10 @@ describe('the text that belongs in that range', () => { const document = composing(); expect(document.renderObject(document.addRaw('Zone', 'Late Arrival', {}))).toBeUndefined(); - expect(parseIdf(COMPOSING, v26, { strict: false }).document.renderObject( - parseIdf(COMPOSING, v26, { strict: false }).document.require('Building', 'My Building') - )).toBeUndefined(); + expect( + parseIdf(COMPOSING, v26, { strict: false }).document.renderObject( + parseIdf(COMPOSING, v26, { strict: false }).document.require('Building', 'My Building') + ) + ).toBeUndefined(); }); }); From 16d65b11d3cf05fea04e2a0ec65831b745479540 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 17:57:57 -0400 Subject: [PATCH 12/14] Say where the preserving write stops being the cheap one A peer measuring across four models found that "a preserving write is 7x to 45x faster than a formatting one" is an UNEDITED-document number and does not hold in general. It inverts once most objects have changed: this path renders each changed object and walks the tiling, which is strictly more work than formatting alone. On a 13 MB model with every object edited it is 409 ms against 104 ms. Not a defect and not worth a guard. What a caller actually pays is nought to a hundred objects edited, which is 0.1 ms to 4.4 ms, and a whole model rewrite is what `preserveFormatting: false` is for. But an unstated crossover is how somebody benchmarks the wrong path and reports the wrong number, which is exactly what happened, so the doc comment says it. Both libraries, same words. --- packages/core/src/write/idf.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/core/src/write/idf.ts b/packages/core/src/write/idf.ts index d6a3440..d7744d0 100644 --- a/packages/core/src/write/idf.ts +++ b/packages/core/src/write/idf.ts @@ -77,6 +77,13 @@ export interface WriteIdfOptions { * or `comments: false`, which ask for a different output FORM the source was never going to * express, so producing it is honest. * + * Cheapest where it is used: a write that reproduces most of the file copies text rather than + * building it. It CROSSES OVER once most objects have changed, because this path renders each of + * them AND walks the tiling, which is more work than formatting alone. On a 13 MB model with + * every object edited it is roughly four times slower than a formatting write. Nothing to guard + * against, since an edit touches a handful of objects and a whole model rewrite is what + * `preserveFormatting: false` is for, but worth knowing before timing the wrong one. + * * @defaultValue undefined, meaning decide */ preserveFormatting?: boolean; From 6aff858d99354a943f662c4598ffafd656016129 Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 18:09:21 -0400 Subject: [PATCH 13/14] Adopt conformance-2026.11 and governance-2026.15 The runner level first. The case set is unchanged from 2026.10, 69 cases and 211 assertions, and this library sees no new expectation. What it sees is a runner that stops reporting a false failure on preserve-edit-one-field, and the terminator-comment rule mirrored into the JavaScript runner so the two agree about where a statement's text ends. The governance level carries the five names this feature adds, none of which renames anything: changedObjects, regionOf, Region, renderObject and fieldComments. The naming gate passes with 136 of 136 public names resolving. That order is the rule rather than an accident. The entries were published in idfkit-conformance and the tag cut from its main before this pin moved, so a gate here could never have gone green against something a reviewer had not seen. --- packages/core/package.json | 4 ++-- packages/core/src/conformance.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/package.json b/packages/core/package.json index 096b241..59876f5 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -42,8 +42,8 @@ "node": ">=20" }, "idfkit": { - "conformance": "conformance-2026.10", - "governance": "governance-2026.14" + "conformance": "conformance-2026.11", + "governance": "governance-2026.15" }, "dependencies": { "@idfkit/schemas": "0.0.0" diff --git a/packages/core/src/conformance.ts b/packages/core/src/conformance.ts index f946424..092f732 100644 --- a/packages/core/src/conformance.ts +++ b/packages/core/src/conformance.ts @@ -17,4 +17,4 @@ * This is not a version number and it is not compared to one. Two installed libraries agree on the * formats when they declare the same level, whatever their own versions say (FR-025). */ -export const CONFORMANCE_LEVEL = 'conformance-2026.10'; +export const CONFORMANCE_LEVEL = 'conformance-2026.11'; From 1a791b4f648b8ee48157f83f6f4e2163cdf8e1db Mon Sep 17 00:00:00 2001 From: Samuel Letellier-Duchesne Date: Sun, 6 Sep 2026 18:14:47 -0400 Subject: [PATCH 14/14] Attest conformance-2026.11 as the level both libraries publish at The constant FR-044 rests on, moved only after checking the other library rather than after moving this one's pin. Evidence taken today against the corpus checked out AT THE TAG, not read off a badge: idfkit-js packages/core/package.json idfkit.conformance = conformance-2026.11 idfkit pyproject.toml [tool.idfkit.conformance] = conformance-2026.11 idfkit-js npm run check:release PASS at that level idfkit uv run python scripts/check_release_conformance.py PASS at that level 2026.11 changes no case. 69 cases and 211 assertions, as 2026.10 had; what moved is the runners, which stopped reporting a false failure on preserve-edit-one-field. So the precondition is met on the same evidence it was, rather than on a weaker one. The gate caught this: bumping the pin without moving the constant failed `emit-conformance.mjs --check`, which is the cheap failure it was added to produce after the pair went stale once between 2026.7 and 2026.8 and surfaced only when a release was attempted. --- scripts/check-publication.mjs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/scripts/check-publication.mjs b/scripts/check-publication.mjs index a9521cd..7e2114c 100644 --- a/scripts/check-publication.mjs +++ b/scripts/check-publication.mjs @@ -109,18 +109,19 @@ class CannotRun extends Error {} * asserted by the run in that repository and is not observable from here. So the level is written * down by someone who checked both, and moving it is the act of re-attesting. * - * T101 named conformance-2026.6, the level that proved the Tier 1 port. This is 2026.10, the level - * that carries the preserved-text assertion with the second language's divergence entries removed, - * and the evidence for it, taken rather than recalled: + * T101 named conformance-2026.6, the level that proved the Tier 1 port. This is 2026.11, the level + * whose runners stop reporting a false failure on preserve-edit-one-field, and the evidence for it, + * taken rather than recalled: * - * idfkit-js packages/core/package.json idfkit.conformance = conformance-2026.10 - * idfkit pyproject.toml [tool.idfkit.conformance] level = conformance-2026.10 + * idfkit-js packages/core/package.json idfkit.conformance = conformance-2026.11 + * idfkit pyproject.toml [tool.idfkit.conformance] level = conformance-2026.11 * idfkit-js npm run check:release green at that level * idfkit uv run python scripts/check_release_conformance.py green at that level * - * Both were run against the corpus checkout on the day this moved, rather than read off a CI - * badge, because the two levels that preceded this one were cut hours apart and a badge would have - * been reporting the older of them. + * Both were run against the corpus checked out AT THE TAG on the day this moved, rather than read + * off a CI badge, because the two levels that preceded 2026.10 were cut hours apart and a badge + * would have been reporting the older of them. 2026.11 changes no case: 69 cases and 211 + * assertions, as 2026.10 had, so the precondition is met on the same evidence it was. * * Each level since 2026.6 contains all of it and adds cases, so the precondition is met more * strongly rather than less. @@ -131,7 +132,7 @@ class CannotRun extends Error {} * the pin on every run, so the next time the two part company it fails a cheap gate on the change * that caused it rather than a release months later. */ -const REQUIRED_CONFORMANCE = 'conformance-2026.10'; +const REQUIRED_CONFORMANCE = 'conformance-2026.11'; /** The distribution gates, precondition 4. Order is cheapest first. */ const DISTRIBUTION_GATES = [