diff --git a/docs/design/context-carriers.tdd.md b/docs/design/context-carriers.tdd.md index 76ee4c878..65587dccb 100644 --- a/docs/design/context-carriers.tdd.md +++ b/docs/design/context-carriers.tdd.md @@ -102,3 +102,7 @@ Third review follow-up. Review of that head found four more problems, each repro Two first-round findings had stayed open and still reproduced. A member call such as `records.require('./label')` matched the call pattern, because the dot satisfies a word boundary, so a self-contained object method failed every projection. And the dynamic-argument pattern allowed only one level of nested parentheses, so `require(path.join(__dirname, getName()))` produced no warning while `require(getName())` did. A seventh RED added 3 cases, all failing. GREEN skips calls preceded by a dot, including after whitespace or optional chaining, and finds each call's argument by balanced-paren scanning over the string-free skeleton. Rescanning the 802 files changed 6 results, each a newly reported nested dynamic argument in a docker eval check or a test, none of them in a carrier. The real registry is unchanged. Focused verification: 22 carrier cases, 12 compiler cases, 57 closure cases, 3 CI cases and 2 publish-surface cases pass with zero failures or skips. ESLint passes on the validator and the new test. The validator is CI-only and stays out of the packed surface, matching `scripts/ci/validate-context-profiles.js`. + +Fourth review follow-up. Three open review findings still let a carrier with a missing dependency pass the gate, each reproduced against the real registry first. Treating every `}` as a statement boundary, the third follow-up's fix, also caught object literals, so in `const ratio = {} / 2; require('../x')` the division was read as a regex that swallowed the `require`; a member named `of` (`a.of / 2`) did the same through the keyword list. The member-call guard treated the last dot of a spread as member access, so `[...require('../x')]` was skipped. And a literal first argument counted only when the call closed right after it, so `require('../x', null)` was demoted to a dynamic warning, which never fails. An eighth RED added 14 cases, 11 failing; the three controls (regex after a function body, a class body and a nested block) already passed. GREEN records whether each `{` opens a block or an object literal from the token before it, and only a block's `}` lets a regex follow. It matches keywords only when no member dot precedes them, ignores the dot of a spread, and resolves a call's first argument up to its first top-level comma, so `import('./data.json', { with: { type: 'json' } })` is an exact-path import, not a dynamic warning. As a side effect, `import(path.join(__dirname, …))`, which used to be skipped silently, now warns as dynamic, because `import()` takes a URL, not a path. Rescanning all 802 repository JavaScript files against the previous head changed no result. The real registry is unchanged: 35/35 resolved, zero failures or warnings. 71 closure cases pass. + +Scope. The gate checks static, relative module specifiers in planned JavaScript files, and nothing else. It does not see `require.resolve`, an aliased or `createRequire`-made `require`, files a script spawns or reads at run time, or `.sh` and `.py` scripts. It does not model `.node` addons or a directory's `package.json` `main`. Each of those can still ship a carrier that fails at load time. diff --git a/scripts/ci/validate-carrier-closure.js b/scripts/ci/validate-carrier-closure.js index b6e4682db..5c781ba7e 100644 --- a/scripts/ci/validate-carrier-closure.js +++ b/scripts/ci/validate-carrier-closure.js @@ -26,23 +26,29 @@ const BUILTINS = new Set(builtinModules); const SCRIPT_EXTENSIONS = new Set(['.js', '.cjs', '.mjs']); const MARK = '\u0000'; const SLOT = `${MARK}(\\d+)${MARK}`; -// `obj.require(`, `obj?.import(` and `obj . require(` are method calls, not module loads. -const NOT_MEMBER = '(?]|\b(?:return|typeof|case|in|of|new|delete|void|instanceof|do|else|yield|await))$/; +// A keyword, not a member of that name: `a.of / 2` divides. +const KEYWORD = `${NOT_MEMBER}(?]|${KEYWORD}(?:return|typeof|case|in|of|new|delete|void|instanceof|do|else|yield|await))$`); +// After these a `{` opens an object literal; after any other word it opens a block. +const OBJECT_KEYWORD = new RegExp( + `${KEYWORD}(?:return|typeof|case|in|of|new|delete|void|instanceof|yield|await)$`); +const CASE_LABEL = new RegExp(`${KEYWORD}(?:case\\b[^;{}]*|default\\s*):$`); const CONDITION_KEYWORD = /(?:^|[^\w$])(if|while|for|with)$/; /** Cook a raw string or template chunk the way the JavaScript parser would. */ @@ -73,6 +79,21 @@ function opensCondition(code) { return Boolean(match) && !code.slice(0, code.length - match[1].length).trimEnd().endsWith('.'); } +/** + * Whether a `{` after code opens a block (a slash after its `}` starts a + * regex) or an object literal (a slash after its `}` divides). `blocks` holds + * the kind of every enclosing brace, innermost last. + */ +function opensBlock(code, blocks) { + if (!code || code.endsWith('=>')) return true; + const last = code[code.length - 1]; + if (last === ')' || last === ';' || last === '}') return true; + if (last === '{') return blocks[blocks.length - 1] !== false; + if (last === ':') return blocks[blocks.length - 1] !== false && CASE_LABEL.test(code); + if (/[\w$]/.test(last)) return !OBJECT_KEYWORD.test(code); + return false; +} + function readRegexLiteral(source, start) { let index = start + 1; let inClass = false; @@ -102,6 +123,10 @@ function scanSource(rawSource) { // last one closed did: `/` after `if (...)` starts a regex, not division. const parens = []; let closedCondition = false; + // The same for braces: whether each open one began a block, and whether the + // last one closed did: `/` after `{}` divides, after `if (x) {}` it does not. + const blocks = []; + let closedBlock = false; let code = ''; let index = 0; // Slots cut from template literals; `from` and `import` need a quoted string. @@ -141,7 +166,9 @@ function scanSource(rawSource) { index = index === -1 ? source.length : index + 2; continue; } - if (char === '/' && (REGEX_PRECEDES.test(code.trimEnd()) || (closedCondition && code.trimEnd().endsWith(')')))) { + if (char === '/' && (REGEX_PRECEDES.test(code.trimEnd()) + || (closedCondition && code.trimEnd().endsWith(')')) + || (closedBlock && code.trimEnd().endsWith('}')))) { index = readRegexLiteral(source, index); code += ' '; continue; @@ -168,6 +195,8 @@ function scanSource(rawSource) { } if (char === '(') parens.push(opensCondition(code.trimEnd())); else if (char === ')') closedCondition = parens.pop() === true; + else if (char === '{') blocks.push(opensBlock(code.trimEnd(), blocks)); + else if (char === '}') closedBlock = blocks.pop() !== false; code += char; index += 1; } @@ -185,9 +214,11 @@ function collect(pattern, code, handler) { } /** - * The argument text of every `require(...)`/`import(...)` call, matched to - * its closing paren at any nesting depth. Strings are already slots, so a - * paren inside one cannot unbalance the count. + * The first argument of every `require(...)`/`import(...)` call, matched to + * its closing paren at any nesting depth and cut at the first top-level + * comma: Node resolves only the first argument, so `require('./x', null)` and + * `import('./x.json', { with })` still name `./x`. Strings are already slots, + * so a paren or comma inside one cannot unbalance the count. */ function callArguments(code) { const found = []; @@ -195,12 +226,15 @@ function callArguments(code) { const start = match.index + match[0].length; let depth = 1; let cursor = start; + let end = null; while (cursor < code.length && depth > 0) { - if (code[cursor] === '(') depth += 1; - else if (code[cursor] === ')') depth -= 1; + const char = code[cursor]; + if (char === '(' || char === '[' || char === '{') depth += 1; + else if (char === ')' || char === ']' || char === '}') depth -= 1; + else if (char === ',' && depth === 1 && end === null) end = cursor; cursor += 1; } - if (depth === 0) found.push(code.slice(start, cursor - 1).trim()); + if (depth === 0) found.push({ keyword: match[1], argument: code.slice(start, end ?? cursor - 1).trim() }); }); return found; } @@ -230,7 +264,6 @@ function extractReferences(rawSource) { packages.add(value.startsWith('@') ? value.split('/').slice(0, 2).join('/') : value.split('/')[0]); } }; - collect(CALL_PATTERN, code, match => record(literals[Number(match[2])], match[1] === 'import')); // Static `from` and side-effect `import` take only a quoted string, so a // template there (a tag named `from`, say) is not an import. for (const pattern of [FROM_PATTERN, SIDE_EFFECT_PATTERN]) { @@ -238,14 +271,19 @@ function extractReferences(rawSource) { if (!templates.has(Number(match[1]))) record(literals[Number(match[1])], true); }); } - collect(DIRNAME_JOIN_PATTERN, code, match => { - const segments = []; - collect(SLOT_PATTERN, match[1], slotMatch => segments.push(literals[Number(slotMatch[1])])); - specifiers.add(`./${segments.join('/')}`); - }); - for (const argument of callArguments(code)) { - if (!argument || new RegExp(`^${SLOT}$`).test(argument) || STATIC_JOIN_PATTERN.test(argument)) continue; - dynamic.add(renderExpression(argument, literals)); + for (const { keyword, argument } of callArguments(code)) { + if (!argument) continue; + const literal = LITERAL_PATTERN.exec(argument); + const join = keyword === 'require' && STATIC_JOIN_PATTERN.exec(argument); + if (literal) { + record(literals[Number(literal[1])], keyword === 'import'); + } else if (join) { + const segments = []; + collect(SLOT_PATTERN, join[1], slotMatch => segments.push(literals[Number(slotMatch[1])])); + specifiers.add(`./${segments.join('/')}`); + } else { + dynamic.add(renderExpression(argument, literals)); + } } return { specifiers: [...specifiers], imports: [...imports], packages: [...packages], dynamic: [...dynamic] }; }