Skip to content
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

Union in a computed property allows any assignment to property value #38663

Open
zcabjro opened this issue May 19, 2020 · 9 comments
Open

Union in a computed property allows any assignment to property value #38663

zcabjro opened this issue May 19, 2020 · 9 comments
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript

Comments

@zcabjro
Copy link

zcabjro commented May 19, 2020

TypeScript Version: 4.0.0-dev.20200518

Search Terms: computed property, union

Code

type Key = 'a' | 'b';

const index1: Record<Key, string> = { a: '', b: 0 }; // Errors as expected, ok
const index2: Record<Key, string> = { a: '', b: '' };

const b: Key = 'b';

const index3: Record<Key, string> = { ...index2, [b]: 0 }; // Errors as expected, ok

declare var k: Key;

// No errors, allowing anything to be assigned to string
const index4: Record<Key, string> = { ...index2, [k]: [] };
const index5: Record<Key, string> = { a: '', b: '', [k]: 0 };

Expected behavior:
Expected error for invalid assignments in index4 and index5.

Actual behavior:
No error, allowing anything to be assigned to string

Playground Link: here

Related Issues:
#36920: perhaps the computed property is deemed an excess property, though I don't think it should be

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jun 9, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.0 milestone Jun 9, 2020
@RyanCavanaugh
Copy link
Member

@weswigham what's the designed behavior here?

@jcalz
Copy link
Contributor

jcalz commented Jun 28, 2020

#13948 + #27144 = 💥 ?

@weswigham
Copy link
Member

So, there's two things, first:

declare var str: string;

const x = { a: "", b: "" };

const y = { [str]: 0, ...x };

const z = { ...x, [str]: 0 };

A computed property name with a non-literal name produces an index signature. getSpreadType in the compiler elides the index signature from the output type if either input elides the index signature. So you'll note in the above, no type has an index signature - the types introduced by the computed names simply evaporate. Now... including it is a little awkward, as it can easily produce a type like

{
    [x: string]: number;
    a: string;
    b: string;
}

where the index signature does not actually cover all the members in the type, but that can be fixed.

Second, Record<Key, string> is {a: string, b: string} - when {[x: string]: whatever, a: string, b: string} is assigned to it, the index signature is not considered to be an excess property. This is done for good reason! If the computed name only actually edits the known names, then there aren't any excess properties (and index signatures being as wishy-washy as they are, that's how we prefer it). Since the index signature can't be excess, the desired error must come from property assignability - which, when you look at it, you realize something's missing - the issue is that the computed property doesn't edit the types of the properties it may overwrite!

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 31, 2020
@nth-commit
Copy link

+1 on this, allowed a bad type to silently survive during a refactor for us.

@nth-commit
Copy link

nth-commit commented Oct 18, 2020

Here's my minimal example, similar to OP's:

type Key = 'foo' | 'bar';

// Ex 1. Attempting to assign `unknown` in place of `string`

declare const unknownValue: unknown;

const dict1: Partial<Record<Key, string>> = {
  foo: unknownValue, // Error ✔️
};

// Ex 2. Attempting to assign `unknown` in place of `string`, except this time using a computed property

declare const keyValuePair: {
  key: Key;
  value: unknown;
};

const dict2: Partial<Record<Key, string>> = {
  [keyValuePair.key]: keyValuePair.value, // No Error ❌
};

@RyanCavanaugh
Copy link
Member

I think we could get away with raising an error for t = { [k]: v } when k is a literal union type "s1" | "s2" | "s3" ... and v isn't assignable to the union t["s1"] | t["s2"] | t["s3"] | ... (intersection would be more sound, but impractical due to producing never a lot of the time). It'd be a breaking change but it's hard to see how that code wouldn't have an actual bug.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone labels Mar 9, 2022
@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 4.6.1 milestone Mar 9, 2022
@RyanCavanaugh RyanCavanaugh added the Experimentation Needed Someone needs to try this out to see what happens label Mar 9, 2022
@ChristianIvicevic
Copy link

The examples in the previous posts show unexpected behavior where the value is not type-safe. I noticed a similar issue with a dynamic key, although I am not sure whether this is strictly related.

My use-case is working with prisma as an ORM to dynamically update a column where a correct call looks like this:

prisma.table.update({
  data: {
    columnToUpdate: newValue,
  }
});

I wanted to dynamically select the column to update and did Pick legal keys from that data property and noticed the dynamic keys not being type-safe. Here is my minimal example:

type T = {
  one?: 1;
};
type Keys = 'one' | 'two';
const dynamicKey = (): Keys => 'two';
const absurd: T = {
  [dynamicKey()]: 2 // TS silently accepts this, both key AND value
};

@fatcerberus
Copy link

intersection would be more sound, but impractical due to producing never a lot of the time

You say that, but #30769 exists. 🚎 (Lots of people did used to trip on that one, but it doesn’t seem like a big stumbling block anymore.)

@LBBO
Copy link

LBBO commented May 9, 2024

Is there any update on this? Could this at least be considered a bug? This has allowed a very preventable bug to exist in our code for ages where a string value is set in an object that may only contain numbers (minimal example below). I realise it would be a breaking change, but maybe it could at least be added behind a compiler option or something.

type OnlyNumberValues = {
    foo: number
    bar: number
}

const whyDoesThisWork = (obj: OnlyNumberValues, key: keyof OnlyNumberValues): OnlyNumberValues => ({
    ...obj,
    [key]: 'definitely not a number'
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants