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

Two types should only be compatible if their members are compatible - even if all these members are optional #12904

Closed
DmitryEfimenko opened this issue Dec 14, 2016 · 10 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@DmitryEfimenko
Copy link

I wanted to see if I can update definitions a MongoDB filter object, which utilizes these expressions. Below is just a start, but I already see an issue when the code compiles when it should not. This issue possibly has similar underlying issue as #12769

TypeScript Version: 2.1.4

Code

type MongoFilter<T> = $and<T> | $or<T> | propertyRule<T>;

type logical<T> = $and<T> | $or<T>;

type $and<T> = { $and: Array<logical<T> | propertyRule<T>> }
type $or<T> = { $or: Array<logical<T> | propertyRule<T>> }

type propertyRule<T> = {
  [P in keyof T]?: T[P] | comparison<T[P]>
}

type comparison<V> = $eq<V> | $gt | $lt;

type $eq<V> = { $eq: V };
type $gt = { $gt: number };
type $lt = { $lt: number };

// See if MongoFilter works:
interface Person {
  id: number;
  name: string;
  age: number;
}

// should compile
let f1: MongoFilter<Person> = { $or: [{ id: 1 }, { name: 'dima' }] };
let f2: MongoFilter<Person> = { $and: [{ id: 1 }] };
let f3: MongoFilter<Person> = { id: 1 };

// should not compile
let w1: MongoFilter<Person> = { wrong: 1 }; // does not compile - good
let w2: MongoFilter<Person> = { $or: [{ wrong: 1 }] }; // compiles

Expected behavior:
Code below should not compile

let w1: MongoFilter<Person> = { $or: [{ wrong: 1 }] };

Actual behavior:
But it does

@PyroVortex
Copy link

My understanding of trying to do this myself is that since all properties in type propertyRule<T> are optional, the compiler is resolving the type as {}, which is assignable to { wrong: 1}.

@DmitryEfimenko
Copy link
Author

that would make sense, but I don't think is the case since the following code does not compile, which is expected behaviour:

let w1: MongoFilter<Person> = { wrong: 1 };

In this case the propertyRule<T> is used to evaluate { wrong: 1}, and it properly tells that there is no property wrong on type T

@PyroVortex
Copy link

Note that this, on the other hand, compiles just fine:

const x = { wrong: 1 };
let w1: MongoFilter<Person> = x;

@DmitryEfimenko
Copy link
Author

wow... so clearly there is a disconnect somewhere. My conclusion is that either type propertyRule<T> should not resolve to { } or the code below should not compile:

type s = { };
let t: s = { wrong: 1 }

How does it make sense that it compiles given the example above? Is it by design?

@normalser
Copy link

@DmitryEfimenko DmitryEfimenko changed the title Recursive props of Mapped types don't work as expected two types should only be compatible if their members are compatible - even if all these members are optional Dec 15, 2016
@DmitryEfimenko DmitryEfimenko changed the title two types should only be compatible if their members are compatible - even if all these members are optional Two types should only be compatible if their members are compatible - even if all these members are optional Dec 15, 2016
@DmitryEfimenko
Copy link
Author

Thanks for this link. Fantastic FAQ, I've read through the whole thing.

Types with no members can be substituted by any type.

However, FAQ does not really provide the reason WHY it was decided to do it this way. I also didn't find any discussion about this. In my opinion { } != any. I saw a mention of some mysterious side effects which can be caused by creating an object of type { }, but didn't find what the side effects are.


The bottom line is that an object with the wrong property should not be assignable to an object, optional properties of which do not contain property wrong:

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

let wrongObject = { wrong: 1 };
let p: Person = wrongObject; // should not compile

This supports the statement of what is structural typing, which is used by TypeScript (also from FAQ):

two types are compatible if their members are compatible.

Changing the title now that I phrased the issue better.

@DmitryEfimenko
Copy link
Author

Glad to see I'm not the first one to see an issue with this!

So to summarize: #12501 closes #1809, where @sccolbert said:

This proposal will also solve the problem of typing "object bags" where each property is optional, but other primitive types should be disallowed.

Correct me if I'm wrong - that's not the same as my issue. I could not understand, will this PR address this issue?

#9841 and #12914 indeed discusses this issue, but is labeled with "Working as intended"? However, it mentions #3842, which I believe was intended to solve this issue and had a PR, but was never merged.

Side note: the relevance of this issue will only increase now, that Partial<T> type is introduced.

@mhegazy mhegazy added the In Discussion Not yet reached consensus label Dec 15, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Dec 15, 2016

Excess property checks would have caught that if it operated on unions. tracked by #12745

@inner-peace
Copy link

inner-peace commented Apr 27, 2017

Side note: the relevance of this issue will only increase now, that Partial type is introduced

Yep, i think this behaviour is very strange:

const itFails: Partial<{ foo: number }> = { bar: 42 }; // TS2322:Type '{ bar: number; }' is not assignable

const ext = {bar: 42};
const butItWorks: Partial<{ foo: number }> = ext; // OK

@mhegazy
Copy link
Contributor

mhegazy commented Jun 13, 2017

Fixed by #16047

@mhegazy mhegazy closed this as completed Jun 13, 2017
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Jun 13, 2017
@mhegazy mhegazy added this to the TypeScript 2.4 milestone Jun 13, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants