Skip to content

issue-2088 plan: round-14 duplicate-declaration fail-safe in condition 4 narrows recall for a harmlessly-redeclared handler #2633

Description

@carlos-alm

What

docs/plans/issue-2088.md's escape analysis (condition 4 of computeObjectLiteralSiteEscapes, WU-2) resolves an identifier-valued pair or shorthand object-literal property to the same-file function it names via resolveIdentifierValueThisReference, using findTopLevelFunctionNodeByName to find that declaration among the module's top-level statements.

Round 14 (PR #2612) closed a soundness gap in that search: pre-round-14, it returned the FIRST top-level declaration of name it found and stopped immediately, even when a SECOND (or later) top-level declaration of the same name also existed — var run = () => {}; var run = function () { return this.alpha(); }; (legal in both ESM and CommonJS, since var permits redeclaration) and function run() { return 0; } function run() { return this.alpha(); } (legal in CommonJS/sloppy-mode scripts; a SyntaxError under an ES module or "use strict") both resolved to the FIRST, this-free declaration with full confidence, even though the runtime binding is always the LAST one.

The fix makes findTopLevelFunctionNodeByName count every top-level declaration of name (a function_declaration, or a variable_declarator under a lexical_declaration/variable_declaration, regardless of whether its own value is a recognised function shape) and return null — fail safe — the instant more than one exists, rather than returning the first and ignoring the rest.

This closes the soundness hole, but it is strictly more conservative than necessary: a name declared more than once at module level, where the LAST declaration is the only one actually in effect and is itself provably this-free, would, under a fuller fix, still allow T1 to fire; under this fix it does not — the site always escapes and falls back to T2 the moment a second top-level declaration of the name is found, regardless of which one actually resolves at runtime. The same over-conservatism also fires on a harmless var name; (no initializer) restating an existing function/var binding — a legal, common, semantically-inert JS idiom — since it still counts as "a second top-level declaration of name" under this fix.

Why this wasn't fixed with full resolution directly

Determining "the LAST declaration wins" correctly requires the search to keep scanning to the end of the file (already true of this fix) AND to specifically special-case a no-initializer var name; restatement so it does not wrongly count as an overriding redeclaration — additional logic on top of an already soundness-critical resolution chain this plan has found and fixed bugs in across rounds 7 through 14, for a redeclaration pattern that is itself unusual in modern code and, per the #1771/#1784 "restrict to the simplest syntactic shape" precedent already governing this function's own scope, was never asked to be resolved with full precision. Fail-safe on ANY duplicate was chosen instead, consistent with how this same function already treats a shadowed name (#2625) and a reassigned one (#2631): detect the ambiguity, then fail safe outright, rather than resolve one layer deeper.

Suggested fix shape (not binding — decide at execute/fix time)

Track the LAST qualifying declaration (rather than the first) as the candidate result, and only fail safe when a later declaration exists that is NOT trivially a no-initializer var name; restatement of the same binding. Add a WU-10 escape-fallback case proving a var name; var name = () => {};-shaped (declare-then-initialize, never reassigned again) handler still correlates once implemented, without weakening the round-14 fail-safe default for a genuine multi-value redeclaration.

Where

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions