Skip to content

Normalize reused string-literal property names to identifiers in declaration emit#3965

Open
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-property-names-quoting
Open

Normalize reused string-literal property names to identifiers in declaration emit#3965
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-property-names-quoting

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 18, 2026

Fixes #2400

Analysis

In tsgo, when emitting a declaration for a type whose nested object type was reused from a source-position node (via the pseudo-checker / pseudoTypeToNode path), property names that the user wrote as string literals were preserved verbatim — even when their text was a valid identifier. Meanwhile, the same property name produced by the freshly-constructed typeToTypeNode path (which uses createPropertyNameNodeForIdentifierOrLiteral) would emit as a plain identifier.

This caused inconsistent output like:

export declare const platformSlice: {
    query: {              // rebuilt: identifier
        "search": string; // reused: kept as string literal
        "user": string;   // reused: kept as string literal
    };
};

Fix

reuseName in internal/checker/nodecopy.go (only used for property names by the pseudo type node builder) now normalizes a reused StringLiteral whose text is a valid identifier into an Identifier, matching the behavior of createPropertyNameNodeForIdentifierOrLiteral. The name "new" is intentionally excluded so that "new"(): void on a method signature isn't rewritten into a construct signature — mirroring the existing isMethodNamedNew guard.

The result is consistent output, aligned with the TypeScript reference baselines (two existing submodule baselines now match strada exactly, so their .diff files were removed).

Copilot Checklist

  • Add compiler test reproducing inconsistent quoting of string-named properties
  • Fix reuseName to normalize string-literal property names with identifier-valid text
  • Guard the normalization against "new" to preserve method signatures
  • Update / remove affected reference baselines and submoduleAccepted.txt
  • npx hereby build
  • npx hereby test
  • npx hereby lint
  • npx hereby format

Copilot AI changed the title [WIP] Fix property names quoting in inferred type Normalize reused string-literal property names to identifiers in declaration emit May 18, 2026
Copilot AI requested a review from jakebailey May 18, 2026 15:56
@jakebailey jakebailey marked this pull request as ready for review May 19, 2026 21:00
@jakebailey jakebailey requested review from Copilot and weswigham May 19, 2026 21:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Normalizes declaration emit output so that reused string-literal property names that are valid identifiers are emitted as identifiers, eliminating inconsistent quoting between rebuilt vs reused nested object types.

Changes:

  • Normalize reused StringLiteral property names to Identifier (except "new") in reuseName.
  • Update/clean up submodule baselines to match upstream exactly (remove accepted .diff artifacts).
  • Add a new compiler test case and baselines covering nested reused object type property-name consistency.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/checker/nodecopy.go Normalizes reused string-literal property names to identifiers during node reuse, with a special-case for "new".
testdata/tests/cases/compiler/declarationEmitStringNamedPropertyConsistency.ts Adds regression test reproducing inconsistent quoting and asserting consistent identifier emit.
testdata/submoduleAccepted.txt Removes now-unneeded accepted baseline diff entry.
testdata/baselines/reference/submoduleAccepted/compiler/escapedReservedCompilerNamedIdentifier.js.diff Deletes accepted diff file now matching upstream.
testdata/baselines/reference/submodule/conformance/jsDeclarationsPackageJson(target=es2015).types.diff Deletes accepted diff file now matching upstream.
testdata/baselines/reference/submodule/conformance/jsDeclarationsPackageJson(target=es2015).types Updates baseline output to reflect normalized property-name emit.
testdata/baselines/reference/submodule/compiler/escapedReservedCompilerNamedIdentifier.js Updates baseline output to reflect normalized property-name emit.
testdata/baselines/reference/compiler/declarationEmitStringNamedPropertyConsistency.* Adds baselines for the new regression test.

Comment thread internal/checker/nodecopy.go
Comment thread internal/checker/nodecopy.go
Comment thread internal/checker/nodecopy.go
Comment thread testdata/submoduleAccepted.txt Outdated
@@ -1249,7 +1249,6 @@ compiler/es5ExportEqualsDts(target=es2015).js.diff
compiler/declarationEmitNameConflicts.js.diff

## Properties with __proto__-like names now quoted in declaration emit (it's quoted in the originating sourcefile)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanna remove this comment, too?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address this specific comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the now-orphaned comment in f798665.

Copy link
Copy Markdown
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, if the user uses a string we should probably respect that (which may have some minor significant for stringy numeric names w.r.t keyof), but this matches strada so 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Property names are quoted one level deep in inferred type

5 participants