microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.22k stars 12.39k forks source link

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

Open fmauquie opened 3 years ago

fmauquie commented 3 years ago

Bug Report

šŸ”Ž Search Terms

assignment check type record template literal index spread

šŸ•— Version & Regression Information

āÆ 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:

Still, thank you all for the great language !

MartinJohns commented 3 years ago

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

fmauquie commented 3 years ago

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.

sandersn commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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}`
sandersn commented 2 years ago

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

AllySummers commented 2 years ago

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;
}, {});