Computed names in declarations files are resolved even when non-literal, preserve computed names when expressions are entity names by weswigham · Pull Request #60052 · microsoft/TypeScript (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation44 Commits11 Checks32 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This PR basically removes the grammar error requiring computed names in declaration files/other contexts resolve to literal types, and allows them to resolve to whatever so long as they're an entity name expression (a.b.c), and endeavors in declaration emit to preserve those input computed names that imply index signatures wherever possible. This clears the way for isolatedDeclarations to allow always emitting a computed property name on the output for a computed property name on the input, so long as the expression is an entity name expression.

@weswigham Here are the results of running the user tests with tsc comparing main and refs/pull/60052/merge:

Something interesting changed - please have a look.

Details

effect

tsconfig.json

• [MISSING] error TS1166: A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type.
  • packages/effect/test/Sink/constructors.test.ts#L92 in packages/effect/tsconfig.test.json
  • packages/effect/test/Sink/constructors.test.ts#L93 in packages/effect/tsconfig.test.json

tsconfig.base.json

• [MISSING] error TS1166: A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type.
  • packages/effect/test/Sink/constructors.test.ts#L92 in tsconfig.base.json
  • packages/effect/test/Sink/constructors.test.ts#L93 in tsconfig.base.json
• [MISSING] error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
  • packages/schema/dtslint/Class.ts#L9 in tsconfig.base.json

packages/schema/dtslint/tsconfig.json

• [MISSING] error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
  • packages/schema/dtslint/Class.ts#L9 in packages/schema/dtslint/tsconfig.json

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

@weswigham Here are the results of running the top 400 repos with tsc comparing main and refs/pull/60052/merge:

Something interesting changed - please have a look.

Details

alibaba/lowcode-engine

40 of 42 projects failed to build with the old tsc and were ignored

modules/code-generator/tsconfig.json

• error TS4082: Default export of the module has or is using private name 'CLASS_DEFINE_CHUNK_NAME'.
  • modules/code-generator/src/index.ts#L40
  • modules/code-generator/src/standalone.ts#L40
• error TS4082: Default export of the module has or is using private name 'COMMON_CHUNK_NAME'.
  • modules/code-generator/src/index.ts#L40
  • modules/code-generator/src/standalone.ts#L40

FormidableLabs/victory

9 of 63 projects failed to build with the old tsc and were ignored

packages/victory-axis/tsconfig.json

• error TS4025: Exported variable 'getBaseProps' has or is using private name 'axis'.
  • packages/victory-axis/src/helper-methods.tsx#L549
• error TS4050: Return type of public static method from exported class has or is using name 'axis' from external module "/mnt/ts_downloads/_/m/victory/packages/victory-axis/src/helper-methods" but cannot be named.
  • packages/victory-axis/src/victory-axis.tsx#L108

packages/victory-axis/tsconfig.build.json

• error TS4025: Exported variable 'getBaseProps' has or is using private name 'axis'.
  • packages/victory-axis/src/helper-methods.tsx#L549
• error TS4050: Return type of public static method from exported class has or is using name 'axis' from external module "/mnt/ts_downloads/_/m/victory/packages/victory-axis/src/helper-methods" but cannot be named.
  • packages/victory-axis/src/victory-axis.tsx#L108

tinacms/tinacms

13 of 25 projects failed to build with the old tsc and were ignored

packages/@tinacms/schema-tools/tsconfig.json

• error TS4031: Public property 'transformPayload' of exported class has or is using private name 'collectionName'.
  • packages/@tinacms/schema-tools/src/schema/TinaSchema.ts#L253

packages/@tinacms/graphql/tsconfig.json

• error TS2305: Module '"@tinacms/schema-tools"' has no exported member 'TinaSchema'.
  • packages/@tinacms/graphql/src/build.ts#L6
  • packages/@tinacms/graphql/src/builder/index.ts#L29
  • packages/@tinacms/graphql/src/database/index.ts#L26
  • packages/@tinacms/graphql/src/database/util.ts#L13
  • packages/@tinacms/graphql/src/resolve.ts#L6
  • packages/@tinacms/graphql/src/resolver/index.ts#L18
  • packages/@tinacms/graphql/src/schema/createSchema.ts#L4
  • packages/@tinacms/graphql/src/sdkBuilder/index.ts#L5
• error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Collection<true>'.
  • packages/@tinacms/graphql/src/build.ts#L60
  • packages/@tinacms/graphql/src/build.ts#L180
  • packages/@tinacms/graphql/src/build.ts#L184
  • packages/@tinacms/graphql/src/build.ts#L187
  • packages/@tinacms/graphql/src/build.ts#L190
  • packages/@tinacms/graphql/src/build.ts#L195
  • packages/@tinacms/graphql/src/build.ts#L198
  • packages/@tinacms/graphql/src/build.ts#L201
  • packages/@tinacms/graphql/src/database/util.ts#L191
• error TS2339: Property 'namespace' does not exist on type 'unknown'.
  • packages/@tinacms/graphql/src/build.ts#L84
  • packages/@tinacms/graphql/src/build.ts#L85
  • packages/@tinacms/graphql/src/build.ts#L87
  • packages/@tinacms/graphql/src/build.ts#L89
  • packages/@tinacms/graphql/src/builder/index.ts#L975
  • packages/@tinacms/graphql/src/builder/index.ts#L975
  • packages/@tinacms/graphql/src/builder/index.ts#L1009
  • packages/@tinacms/graphql/src/builder/index.ts#L1009
  • packages/@tinacms/graphql/src/resolver/index.ts#L584
  • packages/@tinacms/graphql/src/resolver/index.ts#L599
• error TS2339: Property 'isAuthCollection' does not exist on type 'unknown'.
  • packages/@tinacms/graphql/src/build.ts#L182
• error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'TinaField<true>'.
  • packages/@tinacms/graphql/src/builder/index.ts#L900
  • packages/@tinacms/graphql/src/builder/index.ts#L953
  • packages/@tinacms/graphql/src/resolver/index.ts#L128
  • packages/@tinacms/graphql/src/resolver/index.ts#L159
  • packages/@tinacms/graphql/src/resolver/index.ts#L217
• error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Template<true>'.
  • packages/@tinacms/graphql/src/builder/index.ts#L976
  • packages/@tinacms/graphql/src/builder/index.ts#L1010
• error TS2339: Property 'name' does not exist on type 'unknown'.
  • packages/@tinacms/graphql/src/builder/index.ts#L1320
  • packages/@tinacms/graphql/src/builder/index.ts#L1321
  • packages/@tinacms/graphql/src/builder/index.ts#L1362
  • packages/@tinacms/graphql/src/builder/index.ts#L1363
  • packages/@tinacms/graphql/src/database/util.ts#L174
  • packages/@tinacms/graphql/src/database/util.ts#L177
• error TS2339: Property 'path' does not exist on type 'unknown'.
  • packages/@tinacms/graphql/src/database/util.ts#L162
• error TS2339: Property 'format' does not exist on type 'unknown'.
  • packages/@tinacms/graphql/src/database/util.ts#L163

packages/@tinacms/cli/tsconfig.json

• error TS2305: Module '"@tinacms/schema-tools"' has no exported member 'TinaSchema'.
  • packages/@tinacms/cli/src/next/codegen/index.ts#L8
  • packages/@tinacms/cli/src/next/commands/baseCommands.ts#L8

tusen-ai/naive-ui

3 of 6 projects failed to build with the old tsc and were ignored

src/tsconfig.demo.json

• error TS4082: Default export of the module has or is using private name 'styleDirection'.
  • src/slider/src/Slider.tsx#L107 in src/tsconfig.demo.json

vuetifyjs/vuetify

3 of 7 projects failed to build with the old tsc and were ignored

packages/vuetify/tsconfig.json

• error TS4060: Return type of exported function has or is using private name 'scopeId'.
  • packages/vuetify/src/composables/scopeId.ts#L4

packages/vuetify/tsconfig.dist.json

• error TS4060: Return type of exported function has or is using private name 'scopeId'.
  • packages/vuetify/src/composables/scopeId.ts#L4

packages/vuetify/tsconfig.checks.json

• error TS4060: Return type of exported function has or is using private name 'scopeId'.
  • packages/vuetify/src/composables/scopeId.ts#L4

You can't ship this before we do because it produces declaration files that can't be correctly read by current versions of TS 😉

…re entity name expressions

@weswigham Here are the results of running the user tests with tsc comparing main and refs/pull/60052/merge:

Something interesting changed - please have a look.

Details

effect

tsconfig.json

• [MISSING] error TS1166: A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type.
  • packages/effect/test/Sink/constructors.test.ts#L92 in packages/effect/tsconfig.test.json
  • packages/effect/test/Sink/constructors.test.ts#L93 in packages/effect/tsconfig.test.json

tsconfig.base.json

• [MISSING] error TS1166: A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type.
  • packages/effect/test/Sink/constructors.test.ts#L92 in tsconfig.base.json
  • packages/effect/test/Sink/constructors.test.ts#L93 in tsconfig.base.json
• [MISSING] error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
  • packages/schema/dtslint/Class.ts#L9 in tsconfig.base.json

packages/schema/dtslint/tsconfig.json

• [MISSING] error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
  • packages/schema/dtslint/Class.ts#L9 in packages/schema/dtslint/tsconfig.json

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

@weswigham Here are the results of running the user tests with tsc comparing main and refs/pull/60052/merge:

Something interesting changed - please have a look.

Details

effect

tsconfig.json

• [MISSING] error TS1166: A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type.
  • packages/effect/test/Sink/constructors.test.ts#L92 in packages/effect/tsconfig.test.json
  • packages/effect/test/Sink/constructors.test.ts#L93 in packages/effect/tsconfig.test.json

tsconfig.base.json

• [MISSING] error TS1166: A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type.
  • packages/effect/test/Sink/constructors.test.ts#L92 in tsconfig.base.json
  • packages/effect/test/Sink/constructors.test.ts#L93 in tsconfig.base.json
• [MISSING] error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
  • packages/schema/dtslint/Class.ts#L9 in tsconfig.base.json

packages/schema/dtslint/tsconfig.json

• [MISSING] error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
  • packages/schema/dtslint/Class.ts#L9 in packages/schema/dtslint/tsconfig.json

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

1 similar comment

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

@weswigham Here are the results of running the top 400 repos with tsc comparing main and refs/pull/60052/merge:

Everything looks good!

Computed properties that have a computed name of a literal type seem to be duplicated when another computed property that is not a literal type is present:

// index.ts declare const x: string; declare const y: "y";

export class Test { [x] = 10; [y] = 10; }

// index.d.ts declare const x: string; declare const y: "y"; export declare class Test { [x]: number; [y]: number; [y]: number; // ❌ } export {};

Does this definitely fix #60818?

Not intentionally, since it's only a.b.c style names this PR special-cases to copy through to declaration emit, but the late bound index signature logic may be generic enough to at least remove the index error and make an indexer on the resulting type, rather than not and issuing an error. Though it'd obviously be better if the element access behaved more like the equivalent dotted name and produced a concrete property, which is what I'd say the "real fix" aughta be.

@typescript-bot pack this

Starting jobs; this comment will be updated as builds start and complete.

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/164590/artifacts?artifactName=tgz&fileId=F6ABB72746FFE8FA00220E3BDB1DD1A1944C2317AEEF5C0C73BAC8C6962134A102&fileName=/typescript-5.8.0-insiders.20250121.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.8.0-pr-60052-36".;

I think this does what it should, but what's our strategy for dealing with the fact that this PR will cause index expressions to be emitted in such a way that TS<5.8 will be unable to read them?

Is this break acceptable? I think it has to be, because we really want to do it, but it seems like we're going to have to be really clear in the blog post to people that the types may have to be downleveled somehow...

@DanielRosenwasser

Concretely, before:

declare class Base1 { [x: string]: () => void; }

After:

declare let prop: string; declare class Base1 { [prop]: () => void; }

Is this break acceptable? I think it has to be, because we really want to do it, but it seems like we're going to have to be really clear in the blog post to people that the types may have to be downleveled somehow

Yeah. Wouldn't be the first time. It's also notable that while you do have to downlevel the declaration to inline the computed name's type to remove the A computed property name in a class property declaration must have a simple literal type or a 'unique symbol' type. error in old versions of TS, it actually does still typecheck as expected despite the error in TS 5.7, thanks to the existing index signature work. So there's a tiny bit of versioning grace there, at least.

What's the minimal input file that would cause a freshly-generated .d.ts file to not be readable by 5.7?

In testing this, I think I found a bug:

let prop!: string;

export class Base1 { [x: string]: () => void; [prop]!: () => void; }

export class Base2 { [prop]!: () => void; [x: string]: () => void; }

Playground Link

The d.ts emit for this after the PR is:

export declare class Base1 { [x: string]: () => void; } export declare class Base2 { [x: string]: () => void; }

But, we can't know without type analysis that the computed property is safe to drop, and the emitter can't make that decision based on whether or not isolatedDeclarations is enabled (and those kinds of emitters would preserve both).

What's the minimal input file that would cause a freshly-generated .d.ts file to not be readable by 5.7?

let prop!: string;

export class Base1 { [prop]!: () => void; }

which will produce

declare prop: string;

declare export class Base1 { [prop]: () => void; }

except it is readable in 5.7, where I introduced the new computed name index signature behavior for classes (but not earlier), it just has an error in the .d.ts (though will typecheck correctly).

In testing this, I think I found a bug:

A full ID emitter will be forced to retain all possibly-conflicting names, yeah. Which is fine, duplicate declarations basically merge. Where you can, removing dupes from the output is our expected behavior, though.

A full ID emitter will be forced to retain all possibly-conflicting names, yeah. Which is fine, duplicate declarations basically merge. Where you can, removing dupes from the output is our expected behavior, though.

Don't we want to keep declaration emit in sync to what an ID emitter could produce though? Because what I understand form this is that TS will remove the dupes while an ID emitter will not (since it needs type info to do so).

Yes, that is why I am concerned; I do not think we should be removing them as we have previously said that we should have one emit that does the same as ID.