From d914af2e1c1e8394b24d5adaa02c0c00eb21a4ea Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 20 Sep 2026 11:19:17 -0400 Subject: [PATCH 1/4] fix(mobile): name the shape applyTerminalTheme writes through The anti-slop gate refused `loadThemeApplier(term: object)` in the theme test. `applyTerminalTheme` touches exactly two slots on the terminal it is handed, so `terminal-theme.ts` now exports that shape as `TerminalDocumentThemeTarget` and the test's parameter and both fixtures use it. The theme is optional on the way in because `applyTerminalTheme` is what writes it. No cast. The type is erased by the generator's transform, so the document is unchanged and the flip test's class table and the byte pin both still hold. Control: restoring the `object` parameter reproduces the finding at terminal-webview-theme.test.ts:35:33 and the gate exits 1; with the named type it exits 0. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- mobile/src/terminal/document/terminal-theme.ts | 10 +++++++++- mobile/src/terminal/terminal-webview-theme.test.ts | 9 ++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/mobile/src/terminal/document/terminal-theme.ts b/mobile/src/terminal/document/terminal-theme.ts index cd4eb5743a1..757e041e6a0 100644 --- a/mobile/src/terminal/document/terminal-theme.ts +++ b/mobile/src/terminal/document/terminal-theme.ts @@ -1,5 +1,5 @@ import { terminalBackgroundFallback } from './document-constants' -import { scope } from './document-scope' +import { scope, type TerminalDocumentTheme } from './document-scope' /** A terminal colour with no alpha: what the contrast maths works on. */ export type TerminalDocumentRgb = { r: number; g: number; b: number } @@ -152,6 +152,14 @@ export function normalizeTerminalTheme(input: TerminalDocumentThemeMessage) { return Object.assign({}, scope.defaultTheme, next) } +/** + * What `applyTerminalTheme` writes through. Both slots are written, so a target may arrive without + * a theme; nothing else on the terminal is touched. + */ +export type TerminalDocumentThemeTarget = { + options: { theme?: TerminalDocumentTheme; minimumContrastRatio: number } +} + export function applyTerminalTheme(input: TerminalDocumentThemeMessage) { scope.terminalThemeInput = input scope.terminalTheme = normalizeTerminalTheme(input) diff --git a/mobile/src/terminal/terminal-webview-theme.test.ts b/mobile/src/terminal/terminal-webview-theme.test.ts index ad80c297e34..2b1f5fcbffa 100644 --- a/mobile/src/terminal/terminal-webview-theme.test.ts +++ b/mobile/src/terminal/terminal-webview-theme.test.ts @@ -6,6 +6,7 @@ import { documentScopePreamble, generatedDocumentModule } from './document/generated-document-region.test-support' +import type { TerminalDocumentThemeTarget } from './document/terminal-theme' const themeSource = await generatedDocumentModule('terminal-theme') @@ -31,7 +32,7 @@ function loadContrastFloorResolver(): (bg: unknown) => number { return documentDeclaredFunction(loadThemeInjected(), 'resolveTerminalContrastFloor') } -function loadThemeApplier(term: object): (input: unknown) => void { +function loadThemeApplier(term: TerminalDocumentThemeTarget): (input: unknown) => void { const context = loadThemeInjected({ term, document: { @@ -78,9 +79,7 @@ describe('mobile terminal-webview contrast floor gate', () => { }) it('writes the resolved floor onto a live terminal when the theme changes', () => { - const term: { options: { theme?: unknown; minimumContrastRatio: number } } = { - options: { minimumContrastRatio: 1 } - } + const term: TerminalDocumentThemeTarget = { options: { minimumContrastRatio: 1 } } const applyTerminalTheme = loadThemeApplier(term) applyTerminalTheme({ theme: { background: '#ffffff' } }) @@ -93,7 +92,7 @@ describe('mobile terminal-webview contrast floor gate', () => { // #10754: the desktop user can lower or disable the floor. Mobile mirrors the desktop gate, so the // published value has to win here or the same session renders differently on the phone. describe('published desktop override', () => { - function applyOn(term: { options: { minimumContrastRatio: number } }, input: unknown): void { + function applyOn(term: TerminalDocumentThemeTarget, input: unknown): void { loadThemeApplier(term)(input) } From 8da7680c9b4451eea32fa4bc6b76807f776ea026 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 20 Sep 2026 11:35:10 -0400 Subject: [PATCH 2/4] refactor(mobile): drop the dead URL-tap constant and two stale reflow guards Round 1 fixes, all three folded here. 1. `URL_TAP_WEBVIEW_JS` is gone from terminal-webview-url-tap.ts, with `document/url-tap.test.ts` deleted alongside it. The document is generated from its modules now, so that constant was a second copy of the URL-tap group with no consumer but its own tests. terminal-webview-url-tap.test.ts's resolver harness reads the document's own text instead, the path-tap, url-tap, osc-link-tap and surface-tap modules in document order through `generatedDocumentModule`, which refuses unless the document carries each verbatim. Its 33 expects all stay. One mechanism-only assertion went with the file: `document/url-tap.test.ts`'s single `compareTerminalDocumentScripts` pin of the three emissions against the constant, which the flip test's whole-document pin already covers. The file's other exports stay. The deletion surfaced a third reader. terminal-webview-scroll-routing.test.ts concatenated terminal-webview-url-tap.ts into its `source`, and its `notify({ type: 'terminal-tap' });` assertion was matching the constant's single-quoted text, not the document. The read is dropped, since nothing else in that file needed it, and the assertion is the document's form: notify({ type: 'terminal-tap' }); -> notify({ type: "terminal-tap" }); Its 95 expects stay. Leaving the read in place would let a document assertion pass against a module source, which is the hazard this lane exists to remove. 2. terminal-webview-reflow.test.ts guarded a template placeholder that no longer exists, so it could not fail: expect(XTERM_HTML).not.toContain('TERMINAL_REFLOW_JS}') -> expect(XTERM_HTML.split(reflowSource).length - 1).toBe(1) Same intent against the generated document: the reflow module's emitted text is in the document exactly once. The case is renamed to say so and the comment above it describes the generator, not the deleted template. 3. Same file, the routine assertion still passed as a substring of the qualified call; qualified as line 30 already was: term.resize(nextCols, nextRows); -> scope.term.resize(nextCols, nextRows); Its 22 expects stay. Controls, each verified to have changed the file first, all red, tree green after restore: osc-link-tap return parsePathLineCol(value) -> url-tap test, 3 failed surface-tap notify({ type: 'terminal-tap' }) -> scroll-routing, 1 failed reflow scope.term.resize(nextCols, nextRows) -> reflow test, 2 failed module order 'reflow' listed twice -> reflow test, expected 2 to be 1 Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- mobile/src/terminal/document/url-tap.test.ts | 37 ---- .../terminal/terminal-webview-reflow.test.ts | 18 +- .../terminal-webview-scroll-routing.test.ts | 7 +- .../terminal/terminal-webview-url-tap.test.ts | 25 ++- .../src/terminal/terminal-webview-url-tap.ts | 209 ------------------ 5 files changed, 24 insertions(+), 272 deletions(-) delete mode 100644 mobile/src/terminal/document/url-tap.test.ts diff --git a/mobile/src/terminal/document/url-tap.test.ts b/mobile/src/terminal/document/url-tap.test.ts deleted file mode 100644 index 8b7f69e7a47..00000000000 --- a/mobile/src/terminal/document/url-tap.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { fileURLToPath } from 'node:url' -import { describe, expect, it } from 'vitest' -import { emitTerminalDocumentModule } from '../../../scripts/build-terminal-document-script.mjs' -import { URL_TAP_WEBVIEW_JS } from '../terminal-webview-url-tap' -import { compareTerminalDocumentScripts } from './terminal-document-equivalence.test-support' - -/** - * The URL-tap group is three modules, not one: at 303 lines it was over the file cap, and the - * document's own order interleaves the OSC 8 lookup with the file-URL parsing. The split follows - * that order, so the group's text is the three emissions joined. - */ -const modulePaths = ['./url-tap.ts', './osc-link-tap.ts', './surface-tap.ts'].map((relative) => - fileURLToPath(new URL(relative, import.meta.url)) -) - -describe('the url-tap group', () => { - it('emits the script the document carries, modulo the five normalisations', async () => { - const emitted = (await Promise.all(modulePaths.map(emitTerminalDocumentModule))).join('\n') - expect(compareTerminalDocumentScripts(URL_TAP_WEBVIEW_JS, emitted, 'scope')).toEqual({ - equivalent: true, - normalisations: { - // The terminal twice through its internals, and the captured OSC 8 links with their row - // offset; the two patterns and the length bound are build-time constants, not state. - qualifiedReferences: 10, - scopeFieldDeclarations: 0, - rebindings: 41, - bracedBodies: 25, - // Every read of xterm's internals, the two URL parses and the text capture. - unboundCatches: 6, - // All four take a digit run a capture group already matched. - numberProperties: 4, - shorthandProperties: 0, - unshadowedNames: 0 - } - }) - }) -}) diff --git a/mobile/src/terminal/terminal-webview-reflow.test.ts b/mobile/src/terminal/terminal-webview-reflow.test.ts index 17428d54e47..be24fbb3819 100644 --- a/mobile/src/terminal/terminal-webview-reflow.test.ts +++ b/mobile/src/terminal/terminal-webview-reflow.test.ts @@ -55,19 +55,15 @@ describe('terminal WebView reflow', () => { expect(htmlSource).toContain('notify({ type: "measure-result", cols: null, rows: null });') }) - // Why: the raw-source assertions above pass even if the reflow module is - // dropped from the XTERM_HTML concatenation (a broken/removed import or an - // emptied TERMINAL_REFLOW_JS leaves the `${...}` placeholder in the template - // but never injects the routine). That was the regression class reported when - // a sibling refactor extracted the tap dispatcher next to the reflow inject. - // Guard the *assembled* document so the routine and its dispatch are really - // present in what the WebView runs. + // Why: the assertions above read the reflow module's own emission, which still reads whole if + // the generator drops the module from the document or emits it twice. That was the regression + // class reported when a sibling refactor extracted the tap dispatcher next to reflow. Guard the + // assembled document so the routine, once, and its dispatch are really in what the WebView runs. describe('assembled XTERM_HTML', () => { - it('still injects the reflow routine (placeholder fully expanded)', () => { + it('carries the reflow routine exactly once', () => { expect(XTERM_HTML).toContain('function reflow(cols, rows) {') - expect(XTERM_HTML).toContain('term.resize(nextCols, nextRows);') - // No unexpanded template placeholder for the injected reflow JS. - expect(XTERM_HTML).not.toContain('TERMINAL_REFLOW_JS}') + expect(XTERM_HTML).toContain('scope.term.resize(nextCols, nextRows);') + expect(XTERM_HTML.split(reflowSource).length - 1).toBe(1) }) it('still routes the reflow message to the injected routine', () => { diff --git a/mobile/src/terminal/terminal-webview-scroll-routing.test.ts b/mobile/src/terminal/terminal-webview-scroll-routing.test.ts index 527bf79b43f..53d580cf196 100644 --- a/mobile/src/terminal/terminal-webview-scroll-routing.test.ts +++ b/mobile/src/terminal/terminal-webview-scroll-routing.test.ts @@ -2,12 +2,11 @@ import { readFileSync } from 'node:fs' import { describe, expect, it } from 'vitest' import { XTERM_HTML } from './terminal-webview-html' -// The in-WebView JS lives in terminal-webview-html.ts; the RN wrapper in -// TerminalWebView.tsx. Concatenate both so assertions resolve regardless of file. +// The RN wrapper and the pending-message queue are TypeScript; everything the WebView runs is the +// generated document. Concatenated so assertions resolve regardless of file. const source = readFileSync(new URL('./TerminalWebView.tsx', import.meta.url), 'utf8') + readFileSync(new URL('./terminal-webview-pending-messages.ts', import.meta.url), 'utf8') + - readFileSync(new URL('./terminal-webview-url-tap.ts', import.meta.url), 'utf8') + XTERM_HTML const sessionSource = readFileSync( new URL('../session/use-mobile-session-terminal-input.ts', import.meta.url), @@ -242,7 +241,7 @@ describe('TerminalWebView scroll routing', () => { expect(tapHandlerBlock).toContain( 'if (focusKeyboard || !isClickMouseTrackingMode(getMouseTrackingMode()))' ) - expect(tapHandlerBlock).toContain("notify({ type: 'terminal-tap' });") + expect(tapHandlerBlock).toContain('notify({ type: "terminal-tap" });') }) it('allows x10 mouse gesture reports through the mobile session gate', () => { diff --git a/mobile/src/terminal/terminal-webview-url-tap.test.ts b/mobile/src/terminal/terminal-webview-url-tap.test.ts index bac5d1c9cbf..edb6523993f 100644 --- a/mobile/src/terminal/terminal-webview-url-tap.test.ts +++ b/mobile/src/terminal/terminal-webview-url-tap.test.ts @@ -1,11 +1,13 @@ import { createContext, Script } from 'node:vm' import { describe, expect, it } from 'vitest' import type { TappedFilePath } from './terminal-path-tap' -import { generatedDocumentModule } from './document/generated-document-region.test-support' +import { + documentDeclaredFunction, + generatedDocumentModule +} from './document/generated-document-region.test-support' import { TERMINAL_HTTP_URL_MAX_LENGTH, TERMINAL_HTTP_URL_REGEX_SOURCE, - URL_TAP_WEBVIEW_JS, findFileUrlAtColumn, findUrlAtColumn, resolveTerminalOscFileTap, @@ -13,7 +15,12 @@ import { } from './terminal-webview-url-tap' import { XTERM_HTML } from './terminal-webview-html' -const pathTapSource = await generatedDocumentModule('path-tap') +// The three modules the document carries the URL-tap group as, in its own order. +const urlTapGroupSource = ( + await Promise.all( + ['path-tap', 'url-tap', 'osc-link-tap', 'surface-tap'].map(generatedDocumentModule) + ) +).join('\n') type FileTapResolverCase = { name: string @@ -101,19 +108,15 @@ function createInjectedFileTapResolvers(): { resolveTerminalFileUrlTap: InjectedFileTapResolver resolveTerminalOscFileTap: InjectedFileTapResolver } { - const context = createContext({ URL }) + const context: Record = createContext({ URL }) new Script( - `${pathTapSource}\n${URL_TAP_WEBVIEW_JS}\n` + + `${urlTapGroupSource}\n` + 'this.__resolveTerminalFileUrlTap = resolveTerminalFileUrlTap;\n' + 'this.__resolveTerminalOscFileTap = resolveTerminalOscFileTap;' ).runInContext(context) - const injected = context as { - __resolveTerminalFileUrlTap: InjectedFileTapResolver - __resolveTerminalOscFileTap: InjectedFileTapResolver - } return { - resolveTerminalFileUrlTap: injected.__resolveTerminalFileUrlTap, - resolveTerminalOscFileTap: injected.__resolveTerminalOscFileTap + resolveTerminalFileUrlTap: documentDeclaredFunction(context, '__resolveTerminalFileUrlTap'), + resolveTerminalOscFileTap: documentDeclaredFunction(context, '__resolveTerminalOscFileTap') } } diff --git a/mobile/src/terminal/terminal-webview-url-tap.ts b/mobile/src/terminal/terminal-webview-url-tap.ts index f5d416d5797..c63cf8d48d6 100644 --- a/mobile/src/terminal/terminal-webview-url-tap.ts +++ b/mobile/src/terminal/terminal-webview-url-tap.ts @@ -43,212 +43,3 @@ function findTerminalUrlAtColumn(lineText: string, col: number, source: string): } return null } - -export const URL_TAP_WEBVIEW_JS = ` - var URL_TAP_RE_SOURCE = ${JSON.stringify(TERMINAL_HTTP_URL_REGEX_SOURCE)}; - var FILE_URL_TAP_RE_SOURCE = ${JSON.stringify(TERMINAL_FILE_URL_REGEX_SOURCE)}; - var URL_TAP_MAX_LENGTH = ${TERMINAL_HTTP_URL_MAX_LENGTH}; - function findUrlAtColumn(lineText, col) { - return findTerminalUrlAtColumn(lineText, col, URL_TAP_RE_SOURCE); - } - function findFileUrlAtColumn(lineText, col) { - return findTerminalUrlAtColumn(lineText, col, FILE_URL_TAP_RE_SOURCE); - } - function findTerminalUrlAtColumn(lineText, col, source) { - if (typeof lineText !== 'string' || lineText.length === 0) return null; - var re = new RegExp(source, 'gi'); - var match; - while ((match = re.exec(lineText)) !== null) { - var end = match.index + match[0].length; - if (match[0].length <= URL_TAP_MAX_LENGTH && col >= match.index && col < end) return match[0]; - if (match[0].length === 0) re.lastIndex++; - } - return null; - } - function fileUrlAtViewportPoint(clientX, clientY) { - var cell = viewportToCell(clientX, clientY); - if (!cell) return null; - return findFileUrlAtColumn(getLineText(cell.row), cellColToStringIndex(cell.row, cell.col)); - } - function urlAtViewportPoint(clientX, clientY) { - var cell = viewportToCell(clientX, clientY); - if (!cell) return null; - // Map the cell column to a string index so wide chars earlier on the line - // don't shift the match column off the tapped URL. - return findUrlAtColumn(getLineText(cell.row), cellColToStringIndex(cell.row, cell.col)); - } - - // Why: OSC 8 links can render as labels like "#1234"; the URI lives in - // xterm's internal link service, so every access is guarded and falls through. - function oscLinkService() { - try { - var core = term && term._core; - if (!core) return null; - return core._oscLinkService - || (core._inputHandler && core._inputHandler._oscLinkService) - || null; - } catch (e) { return null; } - } - function oscLinkAtViewportPoint(clientX, clientY) { - try { - var cell = viewportToCell(clientX, clientY); - if (!cell) return null; - var line = term.buffer.active.getLine(cell.row); - if (!line) return null; - var urlId = oscLinkIdAtCell(line, cell.col); - if (!urlId) return initialOscLinkAtCell(cell.row, cell.col); - var svc = oscLinkService(); - if (!svc || !svc.getLinkData) return initialOscLinkAtCell(cell.row, cell.col); - var data = svc.getLinkData(urlId); - var uri = data && data.uri; - return terminalOscLinkTarget(uri); - } catch (e) { return null; } - } - function initialOscLinkAtCell(row, col) { - for (var i = 0; i < initialOscLinks.length; i++) { - var link = initialOscLinks[i]; - if (!link || typeof link.uri !== 'string') continue; - if (link.row < initialOscLinkRowOffset) continue; - var shiftedRow = link.row - initialOscLinkRowOffset; - if (shiftedRow === row && col >= link.startCol && col < link.endCol && initialOscLinkTextStillMatches(link, shiftedRow)) return terminalOscLinkTarget(link.uri); - } - return null; - } - function terminalOscLinkTarget(uri) { - if (typeof uri !== 'string') return null; - if (/^https?:/i.test(uri)) return { kind: 'url', url: uri }; - var fileTap = resolveTerminalOscFileTap(uri); - return fileTap ? { kind: 'file', fileTap: fileTap } : null; - } - function resolveTerminalOscFileTap(uri) { - return resolveTerminalFileUrlTap(uri) || parseOscPathLikeTarget(uri); - } - function resolveTerminalFileUrlTap(uri) { - var parsed; - try { - parsed = new URL(uri); - } catch (e) { - return null; - } - if (parsed.protocol !== 'file:') return null; - var filePath; - try { - filePath = decodeURIComponent(parsed.pathname || ''); - } catch (e) { - return null; - } - if (parsed.hostname && !isLocalFileUriHostname(parsed.hostname)) { - filePath = '//' + parsed.hostname + filePath; - } else if (/^\\/[A-Za-z]:\\//.test(filePath)) { - filePath = filePath.slice(1); - } - if (!filePath) return null; - var hashTarget = parseFileUrlLineHash(parsed.hash || ''); - if (hashTarget) { - return { pathText: filePath, line: hashTarget.line, column: hashTarget.column }; - } - if (/%3a/i.test(parsed.pathname || '')) { - return { pathText: filePath, line: null, column: null }; - } - return parseFilePathTrailingLineTarget(filePath) || { pathText: filePath, line: null, column: null }; - } - function isLocalFileUriHostname(hostname) { - var normalized = String(hostname).toLowerCase(); - return normalized === 'localhost' || normalized === '127.0.0.1' || normalized === '::1' || normalized === '[::1]'; - } - function parseOscPathLikeTarget(value) { - if (!/^(?:~[\\\\/]|[\\\\/]|\\.{1,2}[\\\\/]|[A-Za-z]:[\\\\/]|[A-Za-z0-9._-]+[\\\\/]|(?=[A-Za-z0-9._-]*\\.[A-Za-z0-9]))/.test(value)) return null; - return parsePathLineCol(value); - } - function parseFileUrlLineHash(hash) { - var match = /^#?L(\\d+)(?:C(\\d+))?$/i.exec(hash); - if (!match) return null; - var line = parseInt(match[1], 10); - var column = match[2] ? parseInt(match[2], 10) : null; - if (line < 1 || (column !== null && column < 1)) return null; - return { line: line, column: column }; - } - function parseFilePathTrailingLineTarget(filePath) { - var match = /^(.*?)(?::(\\d+))(?::(\\d+))?$/.exec(filePath); - if (!match || !match[1] || match[1].charAt(match[1].length - 1) === '/' || match[1].charAt(match[1].length - 1) === '\\\\') return null; - var line = parseInt(match[2], 10); - var column = match[3] ? parseInt(match[3], 10) : null; - if (line < 1 || (column !== null && column < 1)) return null; - return { pathText: match[1], line: line, column: column }; - } - function captureInitialOscLinkTexts() { - if (!Array.isArray(initialOscLinks)) return; - for (var i = 0; i < initialOscLinks.length; i++) { - var link = initialOscLinks[i]; - if (!link || typeof link.text === 'string') continue; - link.text = initialOscLinkTextAtRow(link, link.row); - } - } - function initialOscLinkTextStillMatches(link, row) { - if (typeof link.text !== 'string') return false; - return link.text.length > 0 && initialOscLinkTextAtRow(link, row) === link.text; - } - function initialOscLinkTextAtRow(link, row) { - try { - var lineText = getLineText(row); - var start = cellColToStringIndex(row, link.startCol); - var end = cellColToStringIndex(row, link.endCol); - return lineText.slice(start, end); - } catch (e) { - return ''; - } - } - function oscLinkIdAtCell(line, col) { - try { - var bufCell = line.getCell(col); - return bufCell && bufCell.extended && bufCell.extended.urlId ? bufCell.extended.urlId : 0; - } catch (e) { return 0; } - } - - function notifyTerminalSurfaceTap(originX, originY, focusKeyboard) { - var tappedOscLink = oscLinkAtViewportPoint(originX, originY); - if (tappedOscLink && tappedOscLink.kind === 'file') { - notify({ - type: 'terminal-file-tap', - pathText: tappedOscLink.fileTap.pathText, - line: tappedOscLink.fileTap.line, - column: tappedOscLink.fileTap.column - }); - return; - } - var tappedFileUrl = fileUrlAtViewportPoint(originX, originY); - var tappedFileUrlPath = tappedFileUrl ? resolveTerminalFileUrlTap(tappedFileUrl) : null; - if (tappedFileUrlPath) { - notify({ - type: 'terminal-file-tap', - pathText: tappedFileUrlPath.pathText, - line: tappedFileUrlPath.line, - column: tappedFileUrlPath.column - }); - return; - } - var tappedUrl = tappedOscLink && tappedOscLink.kind === 'url' ? tappedOscLink.url : urlAtViewportPoint(originX, originY); - if (tappedUrl) { - notify({ type: 'open-url', url: tappedUrl }); - return; - } - var tappedPath = filePathAtViewportPoint(originX, originY); - if (tappedPath) { - notify({ - type: 'terminal-file-tap', - pathText: tappedPath.pathText, - line: tappedPath.line, - column: tappedPath.column - }); - return; - } - var clickInput = buildMouseClickInput(originX, originY); - if (clickInput) { - notify({ type: 'terminal-input', bytes: clickInput }); - } - // Touch still needs native input focus after the TUI consumes its mouse click. - if (focusKeyboard || !isClickMouseTrackingMode(getMouseTrackingMode())) { - notify({ type: 'terminal-tap' }); - } - } -` From 35ad788f1740e15d5040a4dca8a10513594cfdfe Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 20 Sep 2026 11:56:02 -0400 Subject: [PATCH 3/4] refactor(mobile): retire the last module concatenator and guard the order list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 fixes, all five folded here. 1. Deleted terminal-webview-html-source.test-support.ts. `readTerminalWebViewHtmlSource()` had no consumers left once the behavioural tests moved to the generated document, and it was the last thing that built a document-shaped string by concatenating module sources — its filter admitted `.test-support.ts` files too, so it could have grown one. Confirmed by grep that the only occurrence of either name in the repository was its own declaration. 2. New document-module-order.test.ts asserts both directions: the non-test, non-test-support `.ts` files under `document/` are exactly `{document-scope} + TERMINAL_DOCUMENT_MODULE_ORDER + {document-constants}`, and no name is listed twice. `document-constants` is the one exception because it is never emitted: its exports are substituted into the modules that import them as literals, so the document carries its values without carrying the module. A module added here and forgotten there would be dead code that reads as live; a name left after its file goes makes the generator throw at build time rather than at review time. 3. terminal-document-flip.test.ts's docstring now carries the retirement policy from ruling 18: the test is the proof of the flip and holds only while no module changes, the first lane that must change one retires it together with `terminal-document-pre-flip-script.txt`, and the standing pin from then on is `terminal-document-identity.test.ts`, whose fixture regeneration is a review event. Comment only. 4. terminal-document-equivalence.test-support.ts said 57 reassigned variables and "Four classes and no others". It now says 73 declaration sites and eight classes, with each class's measured figure named. Two doc comments sat above the wrong declaration and were moved onto what they describe: the `NUMBER_GLOBALS` one down to that constant, and the printing one down to `significantTokens`, with `STRICT_DIRECTIVE` given its own line. 5. build-terminal-document-script.mjs substituted constants with `replaceAll(regexp, literal)`, where `$&`, `` $` ``, `$'` and `$n` in a constant's value are read as replacement patterns. The substitution is now `substituteDocumentConstants`, exported so it can be tested directly, and replaces with a function. Controls, each verified to have changed its input first, all red, tree green after restore: plant document/zz-planted-module.ts -> order guard, "+ zz-planted-module" drop 'wheel-scroll' from the order -> order guard, "+ wheel-scroll" revert to the string replacer -> 4 failed, "a $& b" became "a marker b" The `$n` case is deliberately absent from that table: the pattern has no capture group, so `$1` is already literal under either form and a case for it could not tell them apart. The document did not move. The byte golden, the digest and the flip test's class table are all unchanged. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../build-terminal-document-script.mjs | 20 +++++++-- .../build-terminal-document-script.test.ts | 23 +++++++++- .../document/document-module-order.test.ts | 43 +++++++++++++++++++ ...minal-document-equivalence.test-support.ts | 43 +++++++++++-------- .../document/terminal-document-flip.test.ts | 5 +++ ...rminal-webview-html-source.test-support.ts | 32 -------------- 6 files changed, 110 insertions(+), 56 deletions(-) create mode 100644 mobile/src/terminal/document/document-module-order.test.ts delete mode 100644 mobile/src/terminal/terminal-webview-html-source.test-support.ts diff --git a/mobile/scripts/build-terminal-document-script.mjs b/mobile/scripts/build-terminal-document-script.mjs index 6f82a967864..9fe3c9b0587 100644 --- a/mobile/scripts/build-terminal-document-script.mjs +++ b/mobile/scripts/build-terminal-document-script.mjs @@ -55,6 +55,21 @@ async function documentConstantSubstitutions() { return substitutions } +/** + * Replaces each constant's name with its literal. + * + * The replacement is a function, not the literal itself: as a string, `$&`, `` $` ``, `$'` and + * `$n` are replacement patterns, so a constant whose value contains one would be spliced with the + * match rather than written out. A function replacer has no such reading. + */ +export function substituteDocumentConstants(text, substitutions) { + let substituted = text + for (const [name, literal] of Object.entries(substitutions)) { + substituted = substituted.replaceAll(new RegExp(`\\b${name}\\b`, 'g'), () => literal) + } + return substituted +} + /** * Whether a line is a lint directive. * @@ -119,10 +134,7 @@ export async function emitTerminalDocumentModule(modulePath) { } kept.push(line.startsWith('export ') ? line.slice('export '.length) : line) } - let text = kept.join('\n') - for (const [name, literal] of Object.entries(await documentConstantSubstitutions())) { - text = text.replaceAll(new RegExp(`\\b${name}\\b`, 'g'), literal) - } + const text = substituteDocumentConstants(kept.join('\n'), await documentConstantSubstitutions()) const substituted = await esbuild.transform(text, { loader: 'js', format: 'esm', diff --git a/mobile/scripts/build-terminal-document-script.test.ts b/mobile/scripts/build-terminal-document-script.test.ts index 963871e25ca..f84e30de688 100644 --- a/mobile/scripts/build-terminal-document-script.test.ts +++ b/mobile/scripts/build-terminal-document-script.test.ts @@ -2,7 +2,10 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterAll, beforeAll, describe, expect, it } from 'vitest' -import { emitTerminalDocumentModule } from './build-terminal-document-script.mjs' +import { + emitTerminalDocumentModule, + substituteDocumentConstants +} from './build-terminal-document-script.mjs' import { terminalBackgroundFallback } from '../src/terminal/document/document-constants' /** @@ -82,3 +85,21 @@ describe('emitting one terminal document module', () => { ).toBe(' const R = /a/g;') }) }) + +describe('substituting a build-time constant', () => { + it('writes a value containing a replacement pattern out as it stands', () => { + // `$&` is the matched text to `String.replaceAll`'s string form, which would splice the + // constant's own name in here and ship a document that says something else. + const literal = JSON.stringify('a $& b') + expect(substituteDocumentConstants('const v = marker;', { marker: literal })).toBe( + 'const v = "a $& b";' + ) + }) + + // `$n` is not listed: the pattern has no capture group, so it is already literal under either + // form and a case for it could not tell them apart. + it.each([['$&'], ["$'"], ['$`']])('is not read as the replacement pattern %s', (pattern) => { + const literal = JSON.stringify(`x${pattern}y`) + expect(substituteDocumentConstants('marker', { marker: literal })).toBe(literal) + }) +}) diff --git a/mobile/src/terminal/document/document-module-order.test.ts b/mobile/src/terminal/document/document-module-order.test.ts new file mode 100644 index 00000000000..7a66e9ad29a --- /dev/null +++ b/mobile/src/terminal/document/document-module-order.test.ts @@ -0,0 +1,43 @@ +import { readdirSync } from 'node:fs' +import { describe, expect, it } from 'vitest' +import { + TERMINAL_DOCUMENT_MODULE_ORDER, + TERMINAL_DOCUMENT_SCOPE_MODULE +} from '../../../scripts/terminal-document-module-order.mjs' + +/** + * Every module in this directory is in the document, and everything in the order list is here. + * + * The generator emits exactly what the order list names, so a module added here and forgotten + * there is dead code that reads as live, and a name left in the list after its file goes makes the + * generator throw at build time rather than at review time. Both directions are asserted. + * + * `document-constants` is the one file that is deliberately not emitted: its exports are + * substituted into the modules that import them as literals, so the document carries its values + * without carrying the module. + */ +const NOT_EMITTED = 'document-constants' + +function documentModuleNames(): string[] { + return readdirSync(new URL('.', import.meta.url)) + .filter((entry) => entry.endsWith('.ts')) + .filter((entry) => !entry.endsWith('.test.ts') && !entry.endsWith('.test-support.ts')) + .map((entry) => entry.slice(0, -'.ts'.length)) + .sort() +} + +describe('the document module order', () => { + it('names every module the directory holds, and only those', () => { + const expected = [ + NOT_EMITTED, + TERMINAL_DOCUMENT_SCOPE_MODULE, + ...TERMINAL_DOCUMENT_MODULE_ORDER + ].sort() + expect(documentModuleNames()).toEqual(expected) + }) + + it('names each module once, so the generator cannot emit one twice', () => { + const listed = [TERMINAL_DOCUMENT_SCOPE_MODULE, ...TERMINAL_DOCUMENT_MODULE_ORDER] + expect(listed).toHaveLength(new Set(listed).size) + }) +}) diff --git a/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts b/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts index eb74c223e35..8a909dce265 100644 --- a/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts +++ b/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts @@ -5,10 +5,11 @@ import { transformSync } from 'esbuild' * Whether two versions of the in-WebView document script are the same program, allowing only the * scope qualifier that moving it into modules requires. * - * C7.1 turns the document's one 2,758-line IIFE into modules the web page can import. The 57 - * variables the script reassigns cannot stay free variables across ES modules — assigning an - * imported binding is a syntax error — so they become fields of one scope object, and every read - * and write of them gains a qualifier. Nothing else about the program may change. + * C7.1 turns the document's one 2,758-line IIFE into modules the web page can import. A variable + * the script assigns across what became a module boundary cannot stay a free variable — assigning + * an imported binding is a syntax error — so those become fields of one scope object, 73 + * declaration sites in all, and every read and write of them gains a qualifier. Nothing else about + * the program may change. * * Byte comparison cannot make that claim once the source is formatter-owned: `oxfmt` writes the * repository's style, which drops the semicolons the hand-written document carries, so the emitted @@ -23,10 +24,13 @@ import { transformSync } from 'esbuild' /** * The differences moving the script into modules is allowed to make, each counted on its own. * - * Four classes and no others. Three are the repository's own rules rewriting the document's ES5 - * style the moment its source is a linted module — measured, not assumed: `curly` braces 279 - * brace-less bodies, `no-unused-vars` unbinds 38 catch clauses, and 446 `var` declarators become - * `const`, `let` or a scope field. The fourth is the move itself. Semicolons and whitespace are the + * Eight classes and no others. Six are the repository's own rules and the printer rewriting the + * document's ES5 style the moment its source is a linted module — measured over the whole script, + * not assumed: `curly` braces 279 brace-less bodies, `no-unused-vars` unbinds 36 catch clauses, 373 + * `var` declarators become `const` or `let`, `unicorn/prefer-number-properties` moves 17 globals + * onto `Number`, the printer spells out 4 shorthand properties whose value gained a qualifier, and + * it stops renaming 7 bindings that are no longer shadows. The other two are the move itself: 609 + * qualified references and 73 declarations onto the scope. Semicolons and whitespace are the * formatter's and never reach the token stream at all. * * Counted separately because the flip commit pins each number: a total would let one class absorb @@ -57,14 +61,6 @@ export type TerminalDocumentNormalisations = { readonly unshadowedNames: number } -/** - * The globals `unicorn/prefer-number-properties` moves onto `Number`. - * - * Measured over the whole script: seventeen sites, and the rule is the only one of its kind that - * appears often enough to be worth matching. Each is equivalent here because every call is already - * behind a `typeof … === 'number'` check or is parsing a string, which is what the `Number` form - * does with no coercion of its own. - */ /** * Whether `printed` is the printer's disambiguated form of `original`: the same name with a decimal * suffix it appends when two bindings of that name are visible at once. @@ -76,6 +72,14 @@ function isPrinterDisambiguation(printed: string, original: string): boolean { return /^[2-9][0-9]*$/.test(printed.slice(original.length)) } +/** + * The globals `unicorn/prefer-number-properties` moves onto `Number`. + * + * Measured over the whole script: seventeen sites, and the rule is the only one of its kind that + * appears often enough to be worth matching. Each is equivalent here because every call is already + * behind a `typeof … === 'number'` check or is parsing a string, which is what the `Number` form + * does with no coercion of its own. + */ const NUMBER_GLOBALS = new Set(['isFinite', 'isNaN', 'parseInt', 'parseFloat']) export type TerminalDocumentEquivalence = @@ -105,16 +109,17 @@ function readDocumentToken(token: unknown): DocumentToken | null { return { label, text: value === undefined || value === null ? '' : String(value) } } +/** The directive prepended to both sides, and checked to have survived printing. */ +const STRICT_DIRECTIVE = 'use strict' + /** * Both sides are printed by the generator's own printer before being read. * * Otherwise every choice the printer makes — semicolons, property shorthand, quote style — reads as * a difference in the program, when it is a difference in who typed it. Printing both sides with * one printer removes that whole class by construction rather than by a rule per symptom, and - * leaves only what the four normalisations and the qualifier cover. + * leaves only what the eight counted classes cover. */ -const STRICT_DIRECTIVE = 'use strict' - function significantTokens(source: string): DocumentToken[] { // Read strict on both sides. A loose script has to defend Annex B's block-scoped function // declarations, and the printer does that by hoisting a `var` and renaming the function; a module diff --git a/mobile/src/terminal/document/terminal-document-flip.test.ts b/mobile/src/terminal/document/terminal-document-flip.test.ts index 76e65932b66..308e382b20d 100644 --- a/mobile/src/terminal/document/terminal-document-flip.test.ts +++ b/mobile/src/terminal/document/terminal-document-flip.test.ts @@ -19,6 +19,11 @@ import { compareTerminalDocumentScripts } from './terminal-document-equivalence. * * The scope object is the one thing the emitted script has that the document did not, so it is * pinned on its own below rather than folded into a count. + * + * Retirement, per ruling 18: this test is the proof of the flip and holds only while no module + * changes, so the first lane that must change one retires it together with + * `terminal-document-pre-flip-script.txt`, and the standing pin from then on is + * `terminal-document-identity.test.ts`, whose fixture regeneration is a review event. */ const preFlipScript = readFileSync( new URL('../terminal-document-pre-flip-script.txt', import.meta.url), diff --git a/mobile/src/terminal/terminal-webview-html-source.test-support.ts b/mobile/src/terminal/terminal-webview-html-source.test-support.ts deleted file mode 100644 index 342937fa116..00000000000 --- a/mobile/src/terminal/terminal-webview-html-source.test-support.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { readdirSync, readFileSync } from 'node:fs' - -const COMPOSER_FILE = './terminal-webview-html.ts' -const DOCUMENT_DIRECTORY = './document/' -/** The parts of the document that are still markup rather than program. */ -const SHELL_FILES = [ - './terminal-webview-html/document-shell.ts', - './terminal-webview-html/document-close.ts', - './terminal-webview-html/theme.ts' -] - -function readSource(relativePath: string): string { - return readFileSync(new URL(relativePath, import.meta.url), 'utf8') -} - -/** - * Reads the TypeScript source the in-WebView document is built from. - * - * Why a directory and not a list: the document's script is generated from every module under - * `document/`, so a new one cannot join the emitted document while staying invisible to the tests - * that search this source. - */ -export function readTerminalWebViewHtmlSource(): string { - const modules = readdirSync(new URL(DOCUMENT_DIRECTORY, import.meta.url)) - .filter((name) => name.endsWith('.ts') && !name.endsWith('.test.ts')) - .sort() - .map((name) => readSource(DOCUMENT_DIRECTORY + name)) - if (modules.length < 30) { - throw new Error(`expected the document's modules, found ${modules.length}`) - } - return [readSource(COMPOSER_FILE), ...SHELL_FILES.map(readSource), ...modules].join('\n') -} From 0ce0fc99a2966cb8e516ca34a6a537178c627bdc Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 20 Sep 2026 12:06:30 -0400 Subject: [PATCH 4/4] test(mobile): make the flip comparator refuse what it was accepting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 items 6 and 7, both in the equivalence instrument. 6. `isPrinterDisambiguation` accepted any `name2` facing `name` without proving the two were the same binding, so an unrelated rename ending in a digit would have been counted rather than refused. It is replaced by `UNSHADOWED_RENAMES`, an explicit list of pre-flip name, generated name and declaring module. The whole script has one entry: `term2` -> `term` in `query-reply`, which is the `term` parameter of `attachTerminalQueryReplyBridge` and its six uses, seven sites in all. That is stated in the docstring rather than encoded as a second pin, since the flip test already pins the total. 7. Brace absorption treated every unexpected `{` as a linter-added body and absorbed any later `}` while one was outstanding, so a bare block anywhere would have been swallowed. `isBraceableHeadBody` now requires the open to be the body of `if`, `for`, `while`, `else` or `do` — walking a `)` back to its `(` and reading the keyword before it — and `matchingCloseIndex` records the index the close must appear at, so the absorbed `}` is that body's own. That check had to move ahead of the equality check. Wherever a braced body ends a block, the baseline's next token is a `}` as well, so pairing them would consume the wrong one and leave the counts right for the wrong reason. Both refusals are tested over snippets: function f() { return value2; } vs return value; -> token 6: expected name value2, generated name value let value = 1; use(value); vs { let value = 1; } use(value); -> token 0: expected name let, generated { and the braceable heads are tested one by one, `if`, `for`, `while`, `if`/`else` and `do`, so the new rule is shown to accept every shape the `curly` rule produces and not only the one the document happens to exercise. Controls: restoring the shape rule fails the first refusal case and nothing else; restoring the accept-any-brace rule fails the second and nothing else. The eight counts did not move: 609, 73, 373, 279, 36, 17, 4, 7. Splitting out `terminal-document-tokens.test-support.ts` is not cosmetic. The tightened rules put the file over the 300-line cap, and a `max-lines` disable is forbidden, so the token reader moved to its own module: that side answers what a script says, and says nothing about which differences between two of them are allowed. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- ...minal-document-equivalence.test-support.ts | 226 +++++++++--------- .../terminal-document-equivalence.test.ts | 39 +++ .../terminal-document-tokens.test-support.ts | 94 ++++++++ 3 files changed, 246 insertions(+), 113 deletions(-) create mode 100644 mobile/src/terminal/document/terminal-document-tokens.test-support.ts diff --git a/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts b/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts index 8a909dce265..4a799bb1f25 100644 --- a/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts +++ b/mobile/src/terminal/document/terminal-document-equivalence.test-support.ts @@ -1,5 +1,8 @@ -import { tokenizer } from 'acorn' -import { transformSync } from 'esbuild' +import { + describeToken, + readScriptTokens, + type DocumentToken +} from './terminal-document-tokens.test-support' /** * Whether two versions of the in-WebView document script are the same program, allowing only the @@ -62,14 +65,28 @@ export type TerminalDocumentNormalisations = { } /** - * Whether `printed` is the printer's disambiguated form of `original`: the same name with a decimal - * suffix it appends when two bindings of that name are visible at once. + * The bindings the printer renamed on the baseline and leaves alone in the modules, listed. + * + * A parameter named for a document variable shadowed it while both lived in one function scope, so + * the printer gave the inner one a decimal suffix; once the outer name is a scope field there is no + * shadow and the inner one keeps its own name. Listed rather than matched by shape: a rule that + * accepted any `name2` facing `name` would also accept an unrelated rename that happens to end in a + * digit, which is a changed program, not a normalisation. + * + * One entry covers all seven sites the whole script has: the `term` parameter of + * `attachTerminalQueryReplyBridge` in `query-reply.ts` and its six uses. */ -function isPrinterDisambiguation(printed: string, original: string): boolean { - if (!printed.startsWith(original) || printed.length === original.length) { - return false - } - return /^[2-9][0-9]*$/.test(printed.slice(original.length)) +const UNSHADOWED_RENAMES: readonly { + readonly baseline: string + readonly generated: string + readonly module: string +}[] = [{ baseline: 'term2', generated: 'term', module: 'query-reply' }] + +/** Whether this exact baseline-to-generated pair is one of the listed unshadowed renames. */ +function isListedUnshadowedRename(baseline: string, generated: string): boolean { + return UNSHADOWED_RENAMES.some( + (entry) => entry.baseline === baseline && entry.generated === generated + ) } /** @@ -86,98 +103,70 @@ export type TerminalDocumentEquivalence = | { readonly equivalent: true; readonly normalisations: TerminalDocumentNormalisations } | { readonly equivalent: false; readonly reason: string } -/** One token as this comparison reads it: what kind it is, and the text it carried. */ -type DocumentToken = { readonly label: string; readonly text: string } - -/** - * Acorn's `Token` class declares `type`, `start` and `end` and not `value`, which it does carry, - * so the field is read through a narrowing check rather than asserted onto the declared type. - */ -function readDocumentToken(token: unknown): DocumentToken | null { - if (typeof token !== 'object' || token === null || !('type' in token) || !('value' in token)) { - return null - } - const type: unknown = token.type - if (typeof type !== 'object' || type === null || !('label' in type)) { - return null - } - const label: unknown = type.label - if (typeof label !== 'string') { - return null - } - const value: unknown = token.value - return { label, text: value === undefined || value === null ? '' : String(value) } -} - -/** The directive prepended to both sides, and checked to have survived printing. */ -const STRICT_DIRECTIVE = 'use strict' - -/** - * Both sides are printed by the generator's own printer before being read. - * - * Otherwise every choice the printer makes — semicolons, property shorthand, quote style — reads as - * a difference in the program, when it is a difference in who typed it. Printing both sides with - * one printer removes that whole class by construction rather than by a rule per symptom, and - * leaves only what the eight counted classes cover. - */ -function significantTokens(source: string): DocumentToken[] { - // Read strict on both sides. A loose script has to defend Annex B's block-scoped function - // declarations, and the printer does that by hoisting a `var` and renaming the function; a module - // does not, so one side would carry a rename the other cannot. Neither name escapes its block, so - // the two readings agree on behaviour and only the strict one can be compared. - const printed = transformSync(`'${STRICT_DIRECTIVE}';\n${source}`, { - loader: 'js', - target: 'chrome74', - minify: false - }).code - const kept: DocumentToken[] = [] - for (const raw of tokenizer(printed, { ecmaVersion: 2020 })) { - const token = readDocumentToken(raw) - if (token === null) { - throw new Error('acorn produced a token this comparison cannot read') - } - if (token.label === ';' || token.label === 'eof') { - continue - } - kept.push(token) - } - if (kept[0]?.text !== STRICT_DIRECTIVE) { - throw new Error('the strict directive this comparison prepends did not survive printing') - } - return kept.slice(1) -} - -/** - * The tokens of one side, or the reason it could not be read. - * - * A script that does not parse is a refusal with the printer's own message rather than an - * exception out of the comparison: a generator that emitted something broken should say so where - * the other differences are reported. - */ -function readScriptTokens( - source: string, - side: string -): { ok: true; tokens: DocumentToken[] } | { ok: false; reason: string } { - try { - return { ok: true, tokens: significantTokens(source) } - } catch (error) { - return { - ok: false, - reason: `${side} does not parse: ${error instanceof Error ? error.message.split('\n')[0] : String(error)}` - } - } -} - -function describeToken(token: DocumentToken | undefined): string { - return token === undefined ? '(end of script)' : `${token.label} ${token.text}`.trim() -} - /** * `baseline` is the script as it stood before the move, `candidate` the one the modules generate. * * The qualifier is read from `qualifier`, not assumed, so the test names the object it expects and * a rename cannot quietly satisfy this. */ +/** The statement heads `curly` braces: everything whose body may be a single unbraced statement. */ +const BRACEABLE_HEAD_KEYWORDS = new Set(['if', 'for', 'while']) + +/** + * Whether the `{` at `open` is the body of a braceable head rather than some other block. + * + * `else` and `do` are followed by their body directly. The rest put a parenthesised head first, so + * the `)` is walked back to its `(` and the keyword before that is what decides. Without this a + * bare block anywhere in the generated script would be absorbed as a linter-added body, when it is + * a statement the baseline does not have. + */ +function isBraceableHeadBody(tokens: readonly DocumentToken[], open: number): boolean { + const previous = tokens[open - 1] + if (previous === undefined) { + return false + } + if (previous.label === 'else' || previous.label === 'do') { + return true + } + if (previous.label !== ')') { + return false + } + let depth = 0 + for (let i = open - 1; i >= 0; i--) { + const label = tokens[i]?.label + if (label === ')') { + depth += 1 + continue + } + if (label === '(') { + depth -= 1 + if (depth === 0) { + return BRACEABLE_HEAD_KEYWORDS.has(tokens[i - 1]?.label ?? '') + } + } + } + return false +} + +/** The index of the `}` closing the `{` at `open`, or -1 when the generated script has none. */ +function matchingCloseIndex(tokens: readonly DocumentToken[], open: number): number { + let depth = 0 + for (let i = open; i < tokens.length; i++) { + const label = tokens[i]?.label + if (label === '{') { + depth += 1 + continue + } + if (label === '}') { + depth -= 1 + if (depth === 0) { + return i + } + } + } + return -1 +} + export function compareTerminalDocumentScripts( baseline: string, candidate: string, @@ -201,15 +190,23 @@ export function compareTerminalDocumentScripts( let numberProperties = 0 let shorthandProperties = 0 let unshadowedNames = 0 - // Braces arrive in pairs around one statement, so a counter is enough: a close is only ever - // absorbed while an inserted open is outstanding, which bounds how far this can mask a real one. - let openInsertedBraces = 0 + // The generated index each inserted `{` expects its `}` at, innermost last. Recording the index + // rather than counting means an absorbed close is the one that closes that body and no other. + const insertedBraceCloses: number[] = [] let lastMatched: DocumentToken | undefined let left = 0 let right = 0 while (left < before.length && right < after.length) { const expected = before[left] const actual = after[right] + // Ahead of the equality check on purpose: the baseline's next token is a `}` too wherever a + // braced body ends a block, and this index is known to close the inserted body, so matching + // them as a pair would consume the wrong one and leave the counts right for the wrong reason. + if (actual.label === '}' && insertedBraceCloses.at(-1) === right) { + insertedBraceCloses.pop() + right += 1 + continue + } if (expected.label === actual.label && expected.text === actual.text) { lastMatched = expected left += 1 @@ -221,7 +218,7 @@ export function compareTerminalDocumentScripts( if ( expected.label === 'name' && actual.label === 'name' && - isPrinterDisambiguation(expected.text, actual.text) + isListedUnshadowedRename(expected.text, actual.text) ) { unshadowedNames += 1 lastMatched = actual @@ -299,16 +296,16 @@ export function compareTerminalDocumentScripts( left += 3 continue } - if (actual.label === '{') { - bracedBodies += 1 - openInsertedBraces += 1 - right += 1 - continue - } - if (actual.label === '}' && openInsertedBraces > 0) { - openInsertedBraces -= 1 - right += 1 - continue + // `if (a) b;` -> `if (a) { b; }`: the body the repository's `curly` rule braced. Only a + // braceable head's body qualifies, and only that body's own close is absorbed. + if (actual.label === '{' && isBraceableHeadBody(after, right)) { + const close = matchingCloseIndex(after, right) + if (close !== -1) { + bracedBodies += 1 + insertedBraceCloses.push(close) + right += 1 + continue + } } return { equivalent: false, @@ -316,8 +313,8 @@ export function compareTerminalDocumentScripts( } } // A body braced at the very end of the script leaves its close after the baseline has run out. - while (openInsertedBraces > 0 && after[right]?.label === '}') { - openInsertedBraces -= 1 + while (insertedBraceCloses.at(-1) === right && after[right]?.label === '}') { + insertedBraceCloses.pop() right += 1 } if (left !== before.length || right !== after.length) { @@ -326,8 +323,11 @@ export function compareTerminalDocumentScripts( reason: `length: ${before.length - left} token(s) left in the baseline, ${after.length - right} in the generated script` } } - if (openInsertedBraces !== 0) { - return { equivalent: false, reason: `${openInsertedBraces} inserted brace(s) never closed` } + if (insertedBraceCloses.length !== 0) { + return { + equivalent: false, + reason: `${insertedBraceCloses.length} inserted brace(s) never closed` + } } return { equivalent: true, diff --git a/mobile/src/terminal/document/terminal-document-equivalence.test.ts b/mobile/src/terminal/document/terminal-document-equivalence.test.ts index 538155343ec..ebc8a4e6be4 100644 --- a/mobile/src/terminal/document/terminal-document-equivalence.test.ts +++ b/mobile/src/terminal/document/terminal-document-equivalence.test.ts @@ -144,6 +144,45 @@ describe('terminal document script equivalence', () => { ).toEqual({ ...NONE, scopeFieldDeclarations: 1, unshadowedNames: 2 }) }) + it('refuses a numeric-suffix rename that is not a listed unshadowed binding', () => { + // The shape `value2` -> `value` is what the printer does to a shadow, but this pair is not one + // of the document's, so it is a renamed local: a changed program, not a normalisation. + expect( + normalisationsOf('function f() { return value2; }', 'function f() { return value; }') + ).toBe('token 6: expected name value2, generated name value') + }) + + it('refuses a bare block the baseline does not have', () => { + // A block that is nobody's body cannot be the `curly` rule's work, so absorbing it would hide + // a statement boundary the baseline never had. + expect(normalisationsOf('let value = 1; use(value);', '{ let value = 1; } use(value);')).toBe( + 'token 0: expected name let, generated {' + ) + }) + + it('counts a braced body only for a head that can carry an unbraced one', () => { + expect(normalisationsOf('if (a) b();', 'if (a) { b(); }')).toEqual({ + ...NONE, + bracedBodies: 1 + }) + expect(normalisationsOf('for (;;) b();', 'for (;;) { b(); }')).toEqual({ + ...NONE, + bracedBodies: 1 + }) + expect(normalisationsOf('while (a) b();', 'while (a) { b(); }')).toEqual({ + ...NONE, + bracedBodies: 1 + }) + expect(normalisationsOf('if (a) b(); else c();', 'if (a) { b(); } else { c(); }')).toEqual({ + ...NONE, + bracedBodies: 2 + }) + expect(normalisationsOf('do b(); while (a);', 'do { b(); } while (a);')).toEqual({ + ...NONE, + bracedBodies: 1 + }) + }) + it('refuses a changed literal', () => { expect(normalisationsOf('var a = 1;', 'var a = 2')).toBe( 'token 3: expected num 1, generated num 2' diff --git a/mobile/src/terminal/document/terminal-document-tokens.test-support.ts b/mobile/src/terminal/document/terminal-document-tokens.test-support.ts new file mode 100644 index 00000000000..4c47a6ca1aa --- /dev/null +++ b/mobile/src/terminal/document/terminal-document-tokens.test-support.ts @@ -0,0 +1,94 @@ +import { tokenizer } from 'acorn' +import { transformSync } from 'esbuild' + +/** + * Reading a version of the in-WebView document script as a token stream. + * + * Kept apart from the comparison that consumes it: this side answers what the script says, and + * says nothing about which differences between two of them are allowed. + */ +/** One token as this comparison reads it: what kind it is, and the text it carried. */ +export type DocumentToken = { readonly label: string; readonly text: string } + +/** + * Acorn's `Token` class declares `type`, `start` and `end` and not `value`, which it does carry, + * so the field is read through a narrowing check rather than asserted onto the declared type. + */ +function readDocumentToken(token: unknown): DocumentToken | null { + if (typeof token !== 'object' || token === null || !('type' in token) || !('value' in token)) { + return null + } + const type: unknown = token.type + if (typeof type !== 'object' || type === null || !('label' in type)) { + return null + } + const label: unknown = type.label + if (typeof label !== 'string') { + return null + } + const value: unknown = token.value + return { label, text: value === undefined || value === null ? '' : String(value) } +} + +/** The directive prepended to both sides, and checked to have survived printing. */ +const STRICT_DIRECTIVE = 'use strict' + +/** + * Both sides are printed by the generator's own printer before being read. + * + * Otherwise every choice the printer makes — semicolons, property shorthand, quote style — reads as + * a difference in the program, when it is a difference in who typed it. Printing both sides with + * one printer removes that whole class by construction rather than by a rule per symptom, and + * leaves only what the eight counted classes cover. + */ +function significantTokens(source: string): DocumentToken[] { + // Read strict on both sides. A loose script has to defend Annex B's block-scoped function + // declarations, and the printer does that by hoisting a `var` and renaming the function; a module + // does not, so one side would carry a rename the other cannot. Neither name escapes its block, so + // the two readings agree on behaviour and only the strict one can be compared. + const printed = transformSync(`'${STRICT_DIRECTIVE}';\n${source}`, { + loader: 'js', + target: 'chrome74', + minify: false + }).code + const kept: DocumentToken[] = [] + for (const raw of tokenizer(printed, { ecmaVersion: 2020 })) { + const token = readDocumentToken(raw) + if (token === null) { + throw new Error('acorn produced a token this comparison cannot read') + } + if (token.label === ';' || token.label === 'eof') { + continue + } + kept.push(token) + } + if (kept[0]?.text !== STRICT_DIRECTIVE) { + throw new Error('the strict directive this comparison prepends did not survive printing') + } + return kept.slice(1) +} + +/** + * The tokens of one side, or the reason it could not be read. + * + * A script that does not parse is a refusal with the printer's own message rather than an + * exception out of the comparison: a generator that emitted something broken should say so where + * the other differences are reported. + */ +export function readScriptTokens( + source: string, + side: string +): { ok: true; tokens: DocumentToken[] } | { ok: false; reason: string } { + try { + return { ok: true, tokens: significantTokens(source) } + } catch (error) { + return { + ok: false, + reason: `${side} does not parse: ${error instanceof Error ? error.message.split('\n')[0] : String(error)}` + } + } +} + +export function describeToken(token: DocumentToken | undefined): string { + return token === undefined ? '(end of script)' : `${token.label} ${token.text}`.trim() +}