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

when merging a generic object type with new optional properties, these properties are not type checked #36637

Closed
aquark opened this issue Feb 5, 2020 · 6 comments · Fixed by #37537
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@aquark
Copy link

aquark commented Feb 5, 2020

TypeScript Version: Nightly

Search Terms: generic spread optional property

Code

interface Test {
  readonly hi?: string[]
}

function test<T extends object>(value: T): Test {
  return { ...value, hi: true }
}

Expected behavior:
This should fail to type check.

Actual behavior:
It passes. Interestingly, changing any of the following causes it to fail:

  • Make the hi property non-optional
  • Replace T with object.

Related Issues: Not sure

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Mar 2, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.9.1 milestone Mar 2, 2020
@weswigham
Copy link
Member

weswigham commented Mar 2, 2020

While that's particularly egregious, the root cause is this:

interface Test {
    readonly hi?: string[]
}

function test<T extends object>(value: T): Test {
    return value; // `T` possibly doesn't satisfy `Test`
}

test({ hi: "potato" }); // ... clearly not a `Test`

T being assignable to Test seems.... dubious. Highly dubious.

@RyanCavanaugh
Copy link
Member

Need 😲 reaction

@weswigham
Copy link
Member

T is assignable to {hi?: string[]} is apparently allowable because T is constrained to object, which is assignable to {hi?: string[]}.

const x: { hi?: string[] } = { hi: "no" } as object;

This, too, seems wrong.

@RyanCavanaugh RyanCavanaugh assigned ahejlsberg and unassigned weswigham Mar 2, 2020
@weswigham
Copy link
Member

And yet @ahejlsberg rates this as "Design Limitation", with "optional properties are very weakly checked; caveat empor"; so this is yours to triage now~

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Mar 2, 2020

Optional properties were a mistake. /Half-kidding

As a rule of thumb, optional properties should only really be used in function/constructor parameter positions. And never as generic type constraints.

They shouldn't really be used in return types or other kinds of functions. Otherwise, you will find yourself in all kinds of soundness issues.

Not a TS team stance, just something I've personally observed.


Even when used to just type options objects for init, you can still run into soundness issues but they are far less likely to occur by accident

@ahejlsberg
Copy link
Member

Moving fix back into 3.9.1 as I think it is very safe and has caught several legit issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants