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

Destructuring issue after removing a keyed property #57614

Closed
michaeljota opened this issue Mar 2, 2024 · 17 comments
Closed

Destructuring issue after removing a keyed property #57614

michaeljota opened this issue Mar 2, 2024 · 17 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@michaeljota
Copy link

michaeljota commented Mar 2, 2024

πŸ”Ž Search Terms

destructure omit

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about
    • 3.x
    • 4.x
    • 5.x

⏯ Playground Link

πŸ’» Code

Updated example:

declare function getId<Model, Id>(model: Model): Id;

function removeRecord<
  Model extends object,
  Id extends 'foo' | 'bar'
>(stateModels: Record<Id, Model>, model: Model): Record<Id, Model> {
  const id = getId<Model, Id>(model);

  const { [id]: deletedEntity, ...models } = stateModels;

  return models;
};

Original Example:

type ModelKey = string | number;
type ModelsWithId<Model, Key extends ModelKey> = Record<Key, Model>;

declare function getIdWithKey<Model, Key>(model: Model): Key;
function removeRecordWithKey<
  Model extends object,
  Key extends ModelKey
>(stateModels: ModelsWithId<Model, Key>, model: Model): ModelsWithId<Model, Key> {
  const id = getIdWithKey<Model, Key>(model);
  const { [id]: deletedEntity, ...models } = stateModels;

  return models;
};

πŸ™ Actual behavior

And error is thrown in the return statement:

Type 'Omit<ModelsWithId<Model, Key>, Key>' is not assignable to type 'ModelsWithId<Model, Key>'.
  Type 'Key' is not assignable to type 'Exclude<Key, Key>'.
    Type 'ModelKey' is not assignable to type 'never'.
      Type 'string' is not assignable to type 'never'.(2322)

The result type is an empty object

interface EmptyRecord {}

function removeRecordWorking<
  Model extends object,
  Id extends 'foo' | 'bar'
>(stateModels: Record<Id, Model>, model: Model): EmptyRecord {
  const id = getId<Model, Id>(model);

  const { [id]: deletedEntity, ...models } = stateModels;

  return models; // No errors
};

πŸ™‚ Expected behavior

No error should be thrown, because an object with a removed keyed property, should be equal to the same object.

Additional information about the issue

When you don't use a keyed property, the code is valid:

interface RemoveModel {
  id: string;
}

type ModelWithStringKey = Record<string, RemoveModel>;

function remove(state: ModelWithStringKey, model: RemoveModel): ModelWithStringKey {
  const id = model.id;

  const { [id]: deletedEntity, ...models } = state;

  return models;

};

The Playground link for "Original example" has different examples for more complex and simpler scenarios.

@jcalz
Copy link
Contributor

jcalz commented Mar 2, 2024

I'm trying to reduce the cognitive load here by replacing indirection with direct versions. I see

function removeRecordWithKey<
  T extends object,
  K extends string | number
>(stateModels: Record<K, T>, model: T): Record<K, T> {
  const id = '' as K
  const { [id]: deletedEntity, ...models } = stateModels;
  return models;
};

and I wonder... why would you expcect models to have the key K when you've explicitly stripped it off? Could you demonstrate with actual working code (so, not '', but a real key) where the output is actually Record<K, T> at runtime? Right now it looks like the error is completely valid. Presumably you're encountering a problem, but I'm not seeing it.

@fatcerberus
Copy link

@jcalz Based on the β€œworking” example in the OP (which involves a Record<string, T>), looks like they might not realize that Record<"foo" | "bar", T> has all required properties?

@michaeljota
Copy link
Author

michaeljota commented Mar 3, 2024

@jcalz @fatcerberus Thanks for your feedback. I think have created an isolated version of the issue using the input from both.

Could you demonstrate with actual working code?

I'm updating the example with declare, because in real code, we don't know what the getId function will return. It's a function that will be pass as an argument. You are only checking that it returns what it should returns, because that's why we use TS at the end, am I right? hehe.

looks like they might not realize that Record<"foo" | "bar", T> has all required properties?

I've realice that, and I considered it a feature for my use case, asserting that the id has the required type. But I'm aware of the complexity this could introduce now.

I updated the Issue Description with the updated version of the example, but I'll leave here in case you are checking this from somewhere else:

declare function getId<Model, Id>(model: Model): Id;

function removeRecord<
  Model extends object,
  Id extends 'foo' | 'bar'
>(stateModels: Record<Id, Model>, model: Model): Record<Id, Model> {
  const id = getId<Model, Id>(model);

  const { [id]: deletedEntity, ...models } = stateModels;

  return models;
};

@fatcerberus
Copy link

If you call removeRecord with a Record<"foo" | "bar", Model> and delete the property at e.g. bar, then what you end up is a Record<"foo", Model> - which is not assignable to the original type because it's missing a property. That's working as intended and not a bug.

@michaeljota
Copy link
Author

Yes, that's true. But I think that with this information the resulting type of models should be `Record<"foo" | "bar", Model> because TS doesn't know what property was taken.

Maybe I am mistaken, but, mathematically speaking, if I have a set of M + N, and I take one of them, I can be sure that M + N != M and that M + N != N, but if I don't know which one was taken, then I can't say that (M + N) - Y !== M + N with confidence. And that's the simpler example, but if you go to the original example, we don't have a set of M + N, instead we have a set of M that tends to infinity, and we are substracting one from, leaving a set of M - 1 that tends to infinity. Consdering this, even when M tends to infinity is diferent from M - 1 tends to infinity, because both tend to infinity, we can use M - 1 to be assigned to M.

I consider that Key extends string | number tends to infinity because we can use both string and number allows for an infinity number of options

@fatcerberus
Copy link

That's not how generics work. When you write e.g.

function foo<T>(x: T): T {
    /* ... */
}

This can only legally be an identity function, because there's an implicit universal quantifier on T: the compiler tries to verify that the function is valid for all types that you could possibly substitute for T. In your case, that's not true - if you manually substitute Id with "foo" | "bar", then the function doesn't type check.

@jcalz
Copy link
Contributor

jcalz commented Mar 3, 2024

In any case this isn’t a bug in TypeScript, so you might want to close the issue to save the TS team the effort of triaging it. After that if you still want to discuss it here, that’s fine, although questions are better asked elsewhere (like Stack Overflow or Discord)

@michaeljota
Copy link
Author

I still consider this a bug, because the resulting type is an empty object, and as I mentioned, if you remove just one value, you won't be removing the whole set. I updated the description to consider that behavior is also a bug. Thanks.

@jcalz
Copy link
Contributor

jcalz commented Mar 4, 2024

? I understand the supposed bug less now than I did before. It’s always valid to assign any non-nullish value to the empty object type. I’d explain why but I think I’d better leave this issue to the TS team to handle.

@snarbies
Copy link

snarbies commented Mar 4, 2024

Yes, that's true. But I think that with this information the resulting type of models should be `Record<"foo" | "bar", Model> because TS doesn't know what property was taken.

In other words you would expect either const { [K]: _, ...etc} to type etc as Record<K, ?>, or for Omit<Record<K, ?>, K> to evaluate to Record<K, ?>, but neither of those options make sense in the general case. It's probably wisest for it to take the types it has to work with at face value. This seems to be working as intended.

Not that I don't understand that Id represents a range of possibilities and that the destructure only claims one key. If you'd like to see unions receive special treatment in this context, you might want to re-frame this as a proposal rather than a bug report. I do doubt that the benefit would be seen as enough to justify the breaking change. That is, if there even is a coherent way to make it work in the context of a generic.

@RyanCavanaugh
Copy link
Member

I still consider this a bug, because the resulting type is an empty object, and as I mentioned, if you remove just one value, you won't be removing the whole set

You seem to believe that { foo: "" } is not a valid { }, but it is.

I don't see any bug demonstrated here.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 4, 2024
@michaeljota
Copy link
Author

Yes, I think using an empty interface doesn't provide information about what I would expect, its not about { [key: string]: any } being assignable to {}, but that {} is not assignable to { [any: string]: any }, and the resulting type of the operation is being reduced to an empty object.

In my original example, I created an alternative version that worked as I would expect, something like:

declare function getId<Model, Id>(model: Model): Id;

type ModelWithStringKey<Model> = Record<string, Model>;

function removeRecordString<
  Model extends object,
>(stateModels: ModelWithStringKey<Model>, model: Model): ModelWithStringKey<Model> {
  const id = getId<Model, string>(model);

  const { [id]: deletedEntity, ...models } = stateModels;

  return models;
};

In this example, the models object has the type:

const models: {
    [x: string]: Model;
}

But in the other example:

declare function getId<Model, Id>(model: Model): Id;

function removeRecord<
  Model extends object,
  Id extends string
>(stateModels: Record<Id, Model>, model: Model): Record<Id, Model> {
  const id = getId<Model, Id>(model);

  const { [id]: deletedEntity, ...models } = stateModels;

  return models;
};

The models object has the type:

Omit<Record<Id, Model>, Id>

So, this is now an empty object {}, and because of this is not assignable not Record<Id, Model>.

Is this expected?

I have this issue with a project I'm creating, it's a minor issue, but I thought it was a problem with TS. I don't think the behavior should change like that only because I'm using a generic param for the key instead of a literal. Maybe the issue is related to not being able to restrict the generic between literal type, or maybe it's not an issue. I still think it should be an issue.

@RyanCavanaugh
Copy link
Member

Let's say that this function removeRecord was allowed to pass, which I think is what you're proposing (if not, please clarify). Then you could write

const obj = { foo: {}, bar: { }};
const p = removeRecord(obj, {});
p.bar; // ok
p.foo; // ok

It's hard to reason about this because getId can't really exist, but the way it's being called, it seems to imply that "foo" and "bar" are legal return values, possibly the only legal return values. And in this world, p can't have both foo and bar properties. Or at least it possibly might not have those properties.

@michaeljota
Copy link
Author

michaeljota commented Mar 5, 2024

First, thank you all. Know that I value your time, and I appreciate your input.

Maybe I tried to simplify it too much. Here is a playground of the issue that I'm having:

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgApQPYAcDOAeAWQwBMIAbZCAD0hGJ2QwCMArCBMAGmQEljKaEOgyKkyfAHzIA3gChkyAOYQwfdNmhgAngC5exAOIrIUQiXLdJAblkBfWbO1YUfI2BNmxl-tVr1kouSSyAC8yAAUALbmZHqBZACUoVJ8NrKgJvBIyACCHMAYIPjxUnIKUBDRAG4QAPIgEOFCYMDacTEJelUYwMQ29o5azgExfKHIOGBQoIrIAD7IIACukUzQaU4oACLA+YVwUFqeFvoCfiKjxFJhqActcGR4AErsGFDEeHzcJRJpGdBZFAAUUEdAgxAAymA4JBjmRvGchP54sEyshejg9HwANoAXRsCmarWAEExyB2exAByO8W8vzsDlICDIBxQMCWIEpExUUJhEDwEIk4UmfL0EM6yAhaXZnJahWQiDlRQAYoq3jSYojhIxWOwuKdfEiLmJJOE0cpVMR1M4oG10ZDyHq3pw7HprcUYnSJXklR6xKV5MhNpLoZBxiC-ODebDafp6YGEIVJsgKtU6g1xsLQxAxdnuES7fEJdGUCEAwoFImimB7eNehDHRw3k1wK0tAk0hXkFXk9JkNjeri9GIVOCga3tNwAHQzgskhi2cYiyBTuekzsVns1jFL7NTjFTmDAMgmcLhKpJMvIKrIACEITCvQ7Di7FTASygIBkga7a5wLq7BQMQAitbAJZAwJfFMVA-L80XKSoMBqepGgLdtv0A7kwBLM9lxzEM+UvKRUyQ9NGjw-MJ3bBIfwgwN7HsIA

I tried to extract the important parts and reduce the issue to the minimal possible scenario. I don't know if it has noise around that can be removed, but I'll let everything around to see if this can be considered a bug as it is.

This is the error that I'm having:

Type 'Omit<Partial<Record<Id, Model>>, Id>' is not assignable to type 'Partial<Record<Id, Model>>'.
  Type 'Id' is not assignable to type 'Exclude<Id, Id>'.
    Type 'ModelId' is not assignable to type 'never'.
      Type 'string' is not assignable to type 'never'.(2322)

And I consider that an issue for the reasons above. Suppose I have a partial object with an indeterminate number of properties, and I take from it one property away from a set of indeterminate possibilities. In that case, the resulting object should be assigned to the same partial object.

Maybe this scenario explains better what I'm trying to do. If this is still insufficient to be considered a bug, I understand and close this, even when I still consider this an unexpected behavior πŸ˜…. But this is the specific scenario I'm having the issue with, and for me, the workaround is just casting to the type I would expect.

@jcalz
Copy link
Contributor

jcalz commented Mar 5, 2024

Now this feels like #54508 and the issues linked within. Omit<Partial<Record<Id, Model>>, Id> is a complicated way of saying {}, but one that the compiler can't "see" for generic Id. {} is always assignable to Partial<XXX>, so if you widen entities to {} the error will go away. If there is a bug here it isn't a new one.

@fatcerberus
Copy link

Ah, I see now, the Partial was the piece of the puzzle that was missing. A very important piece it is: Record<"foo", T> is not assignable to Record<"foo" | "bar", T> but that changes when we're talking about Partials of the same types because we can just consider the bar to be missing in that case. However like @jcalz says the compiler isn't (usually) able to reason about such things in higher-order (i.e. generic) contexts.

Unlike humans, who can reason abstractly and understand the meanings of words like "Exclude" and "Omit," the compiler must rely on a heuristic algorithm that is designed to meet performance constraints.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants