Skip to content

Computed property names are const contexts #46464

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

sandersn
Copy link
Member

By analogy with the existing rule that element access argument expressions are a const context. This is useful for template literals, which otherwise have type string by default.

For example, from #45798:

type Person = { id: number }
const persons: Record<`person-${Person["id"]}`, { a: any }> = {
    [`person-${1}`]: { b: "something" }, // should error with excess property check on 'b'
}

The problem in #45798 is that person-${1} is intended to be checked against the index signature person-${Person["id"]}. But it gets the type string, which makes it a late-bound property. With a template type literal, it is instantiated as a string literal and is no longer late-bound.

There are a couple of ways that seem best to fix this:

  1. Make computed property names contextually typed by their index signature. But this requires choosing the index signature that matches (if any), which requires getting the template literal type even if it's later not used.
  2. Make computed property names const locations. This is a new rule, but is quite similar to the existing rule for element access argument expressions.

I don't consider (3), making all template literals produce template literal types, because I still believe the common case is that both string and template literals are intended to be widened to string.

Fixes #45798

By analogy with the existing rule that element access argument
expressions are a const context.

Fixes #45798
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 21, 2021
@sandersn
Copy link
Member Author

This likely shouldn't go into 4.5. Let's discuss at a design meeting.

Copy link
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.

This, or something like it, was originally one of the changes we had slated to go out with template literal types as a whole, but held back on it to reduce potential breakage. I still think it's a good idea. Question, though; as-written, does this make an unannotated object with a computed name get a template literal indexer rather than a string indexer? Eg,

export const a = (thing: string) => ({[`some-${thing}`]: "whatever"});

and if so, do we want that?

@sandersn
Copy link
Member Author

@typescript-bot dt test this

@sandersn
Copy link
Member Author

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 22, 2021

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at ea7d4c6. You can monitor the build here.

@sandersn
Copy link
Member Author

does this make an unannotated object with a computed name get a template literal indexer rather than a string indexer?

No, although I don't understand why.

do we want that?

I don't think so, but I don't have a strong opinion either way.

@sandersn
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 22, 2021

Heya @sandersn, I've started to run the tarball bundle task on this PR at ea7d4c6. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 22, 2021

Hey @sandersn, 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/113578/artifacts?artifactName=tgz&fileId=DBE4112E03F645C21F419874787CB3A0A18BDE70185F6BC83DC990983D5CADDF02&fileName=/typescript-4.5.0-insiders.20211022.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/[email protected]".;

@sandersn
Copy link
Member Author

No new failures in DT.

@sandersn
Copy link
Member Author

From the design discussion today, we think this is a fine change to make, but will result in such a small improvement that it's not worth the churn.

In particular, it fixes the precise repro from #45798, but I doubt that it fixes the original code since nobody is likely to write literal numbers in a template literal slot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type of record value assigned to template literal computed property name is not checked if it follows a spread of the record
3 participants