Skip to content

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

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
fmauquie opened this issue Sep 9, 2021 · 9 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@fmauquie
Copy link

fmauquie commented Sep 9, 2021

Bug Report

πŸ”Ž Search Terms

assignment check type record template literal index spread

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about records

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

interface Person {
    id: number;
    name: string;
}

/* This only happens if index type is a template literal type */
let persons: Record<`person-${Person["id"]}`, Person> = {};

persons = {
    ...persons, /* spreading previous value */
    [`person-${1}`]: { nonExistingProperty: "something" /* note the missing required properties */ }
}

πŸ™ Actual behavior

No error on the assignment on last line.

πŸ™‚ Expected behavior

There should be a type error because the object being assigned is not a Person.

Error triggers correctly in any of the following scenarios:

  • Index type is a string
  • Object is assigned to a directly entered key (e.g. { "person-1": {/* error triggers correctly */}), or a "simple" computed property (e.g. { ["person-1"]: {/* error triggers correctly */})
  • You remove the spread assignment on the previous line (putting the spread assignment last does exhibits the buggy behavior)

Still, thank you all for the great language !

@MartinJohns
Copy link
Contributor

As a temporary workaround: Writing [`person-${1}` as const]: { ... } works.

@fmauquie
Copy link
Author

fmauquie commented Sep 9, 2021

As a temporary workaround: Writing [`person-${1}` as const]: { ... } works.

It does !

I actually create the key from a function that returns the constructed string type, and you can't "as const" a function return value. Forcing the type ("as PersonKey") does not work either.

type PersonKey = `person-${Person["id"]}`;
function personKey(id: number): PersonKey {
  return `person-${id}`;
}

I simplified it as much as I could for the report.

The workaround I'm using is to assign the value to an explicitly-typed variable first, then assign this variable to the attribute:

/* When using computed properties as object key, TS does not check value type.
   https://github.com/microsoft/TypeScript/issues/45798
   DO NOT INLINE THAT ASSIGNMENT
*/
const myPerson: Person = { id: 1, name: "" };
persons = {
  ...persons,
 [personKey(1)]: myPerson /* see comment on myPerson */,
}

The type is still not checked on assignment in persons, but it is checked just above. It will work until someone (or myself next month) bypasses the warning in the comment and inlines it.

@andrewbranch andrewbranch added the Bug A bug in TypeScript label Sep 9, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.5.1 milestone Sep 9, 2021
@sandersn sandersn added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Oct 20, 2021
@sandersn
Copy link
Member

sandersn commented Oct 20, 2021

This is a side effect of template literals having type string by default, not the matching template literal type.

A few possibilities come immediately to mind

  1. Do nothing, keep requiring as const here as elsewhere.
  2. If a template literal's template literal type could evaluate down to a string literal type, use that instead. But I'd want to check that this would solve your actual problem -- I suspect not.
  3. Template literals could have their template literal type in computed property names. But this is unlike the rest of the language and doesn't improve anything for template literals whose contained types mean they can't be evaluated down to a string literal. Edit: There is a special rule for ElementAccess argumentExpression like x[template], which is pretty close to equivalent to computed property names.
  4. Revisit the decision to make template literals have type string by default. I don't remember the cases where that made sense, but they were quite different than these.

@andrewbranch
Copy link
Member

I think I imagined that template literals as computed property names should be as narrow as possible, but I probably was overconfident about that when labeling it a bug. In hindsight, I think this should be Suggestion / Awaiting More Feedback or In Discussion. I’m pretty sure calling it a bug is too optimistic and calling it a design limitation is too pessimistic. I think we could change the behavior if we were confident it's worth the tradeoffs.

@sandersn
Copy link
Member

Another option:

  1. The computed property name should be contextually typed by the index signature. At least, I think that contextual typing should apply here. It definitely works for
const x: `person-${Person["id"]}` = `person-${1}`

@andrewbranch andrewbranch added In Discussion Not yet reached consensus Suggestion An idea for TypeScript and removed Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed labels Oct 20, 2021
@andrewbranch andrewbranch removed this from the TypeScript 4.5.1 milestone Oct 20, 2021
@sandersn
Copy link
Member

I experimented with a couple of implementations. I'll write up my notes on a PR.

sandersn added a commit that referenced this issue Oct 20, 2021
By analogy with the existing rule that element access argument
expressions are a const context.

Fixes #45798
@AllySummers
Copy link

I just noticed something similar and am 99% sure it's the same issue, although I think I might have come up with a weird ish edge case? (If this is a separate issue, please let me know and I'll make a new issue).

In the below code/playground link I find it interesting how withExplicitAssign correctly has an issue with a wrong index signature, and withExplicitSpread only has an issue with the "static" no.5, but has no issue with no.${number} - and has a different error compared to withExplicitAssign.

The withImplicit doesn't have an issue with any of them and throws no errors.

Playground link

const users = ['frank', 'charles', 'bob', 'whitney', 'richard'];

interface UsersParams {
  [key: `username.${number}`]: string;
}

const withImplicit = users.reduce<UsersParams>((acc, cur, index) => ({
  ...acc,
  [`username.${index}`]: cur,
  [`no.${index}`]: cur,
  [`no.5`]: cur
}), {});

const withExplicitAssign = users.reduce<UsersParams>((acc, cur, index) => {
  acc[`username.${index}`] = cur;
  acc[`no.${index}`] = cur; // Element implicitly has an 'any' type because expression of type '`no.${number}`' can't be used to index type 'UsersParams'.(7053)
  acc[`no.5`] = cur; // Element implicitly has an 'any' type because expression of type '"no.5"' can't be used to index type 'UsersParams'. Property 'no.5' does not exist on type 'UsersParams'.(7053)

  return acc;
}, {});

const withExplicitSpread = users.reduce<UsersParams>((acc, cur, index) => {
  acc = {
    ...acc,
    [`username.${index}`]: cur,
    [`no.${index}`]: cur,
    [`no.5`]: cur // Type '{ "no.5": string; }' is not assignable to type 'UsersParams'. Object literal may only specify known properties, and '[`no.5`]' does not exist in type 'UsersParams'.(2322)
  }
  return acc;
}, {});

@mkantor
Copy link
Contributor

mkantor commented Nov 15, 2024

This issue appears to be fixed and can probably be closed?

Seems like some change in 5.2.x addressed it, though I don't see it mentioned in the release notes.

@fmauquie
Copy link
Author

fmauquie commented Nov 18, 2024

It looks like it yes. If I stumble upon it again on any project with TS > 5.2.x I'll reopen.
I'm not sure @AllySummers issue is the same (it is still present in the playground), if it is feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants