-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Suggestion: treat in
operator as type guard which asserts property existence
#21732
Comments
related discussion also in #10715 about type guards "evolving" the type of an expression. |
This! Currently, we have the odd situation that when you write:
When the ES6 typings aren't loaded, it will narrow |
@mhegazy Yup, I know. I was giving it as an example for what this suggestion would fix. I know why it's happening. |
Let's add some other ways of asserting property existence from #25720 to this proposal: let x: unknown;
// All these should narrow x to {prop: unknown} (s/any/unknown/ from original proposal by Matt)
"prop" in x;
x.prop != null;
x.prop !== undefined;
typeof x.prop !== "undefined";
// typeof should work on properties of the unknown variable
typeof x.prop === "string"; // should narrow x to {prop: string} |
@mattmccutchen I think you'd want those to narrow to |
@mattmccutchen The problem is that trying to access a property on null/undefined will throw an error at runtime. I would support adding those type guards, but only on the |
TL;DR: #10485 narrows by filtering union constituents. This suggestion narrows by adding properties to the type. In cases where In #10485, the type of the object you're checking with type A = { w: string, x: number };
type B = { y: string, z: number };
function foo(p: A | B): number {
if ('w' in p) {
return p.x; // p is narrowed to A
} else {
return p.z; // p is narrowed to B
}
} Note that this isn't exactly sound, since you can call foo({ y: "oops", z: 100, w: true }); but the point of property checking on unions is usually to use that property as a discriminant, and the sound behavior would probably annoy the heck out of developers. Compare to the following: function bar(p: {}): string {
if ('w' in p) {
return String(p.w); // error?! ☹
} else {
return "nope";
}
} It is surprising that after what feels like an explicit test for What I want to see happen is:
|
@jcalz This guard doesn't work with partial interfaces function foo(x: {foo: "bar"} | {toFixed?: () => any}) {
if (inOperator("toFixed", x)) {
x.toFixed(); // Error, Object is of type unknown
}
} |
@sirian Yeah, strictly speaking, all you know is that foo(Object.assign({ foo: "bar" as "bar" }, { toFixed: "whoops" })); // no error ), but assuming people still want the current behavior in #10485 where we eliminate So in your case, If you want a better idea what the behavior would end up being like, try the following complex user-defined type guard using conditional types: type Discriminate<U, K> = ( U extends any ? K extends keyof Required<U> ? U : never : never )
extends infer D ? [D] extends [never] ? U : D : never
function hybridInOperator<K extends keyof any, T>(
k: K,
o: T
): o is Discriminate<T, K> & Record<K, unknown> {
return k in o;
} producing: function foo(x: { foo: "bar" } | { toFixed?: () => any }) {
if (hybridInOperator("toFixed", x)) {
x.toFixed(); // error, possibly undefined
if (x.toFixed) x.toFixed(); // okay
}
} Does that seem better? |
@jcalz function foo(x: { foo: "bar" } | Number | { toFixed?: () => any }) {
if (hybridInOperator("toFixed", x)) {
x.toFixed(); // no error. since x resolved as Number
if (x.toFixed) x.toFixed(); // okay
}
} I also tried various type guards. But always found a new counterexample( Upd. As a quick fix - if you change Upd2.
Not right... |
I don't know how worthwhile it is to come up with a user-defined type guard which exactly mimics the desired behavior of The "quick fix" has re: Upd2, I think you're missing that the suggestion is to apply the inOperator() behavior after the elimination that happens in #10485 ? Not sure if I wasn't clear about that. |
@jcalz Maybe add smth like |
@jcalz what about this one? type Discriminate<U, K extends PropertyKey> =
U extends any
? K extends keyof U ? U : U & Record<K, unknown>
: never;
function inOperator<K extends PropertyKey, T>(k: K, o: T): o is Discriminate<T, K> {
return k in o;
}
function foo(x: null | string | number | Number | { toFixed?: () => any } | { foo: 1 }) {
if (inOperator("toFixed", x)) {
// x is number | Number | { toFixed?: () => any | undefined }
x.toFixed && x.toFixed();
}
}
function g<K extends string, T>(key: K, genericObj: T, concreteObj: {foo: string}) {
if (inOperator('a', concreteObj)) {
concreteObj.a // unknown
}
if (inOperator('a', genericObj)) {
genericObj.a // any
}
if (inOperator(key, concreteObj)) {
concreteObj[key]; // unknown
}
if (inOperator(key, genericObj)) {
genericObj[key] // any
}
} |
Looks good at first glance! |
@jcalz There is a dilemma. how should work typeguard? function foo(x: {foo?: number} | {bar?: string}) {
if (inOperator("foo", x)) {
// x is {foo: number | undefined}
// or
// x is {foo: number | undefined} | {foo: unknown, bar?: string}
}
} or in this case: function foo(x: {foo?: number} | string) {
if (inOperator("foo", x)) {
// x is {foo: number | undefined}
// or
// x is {foo: number | undefined} | string & {foo: unknown}
}
} |
It should be the first one in both cases, |
@jcalz So if we extract types rather than widen it with additional property - what should be at this examples? function foo(x: {bar?: number}) {
if (inOperator("foo", x)) {
// x is never? so is x.foo not accessible?
}
}
function foo(x: object) {
if (inOperator("foo", x)) {
// x is never? so is x.foo not accessible?
}
} |
It should be
|
I recently came across this problem: const person = {
"name": "Benny"
} as unknown;
if (person && typeof person === "object" && "name" in person) {
console.log(person.name);
} I am narrowing
|
const person = {
"name": "Benny"
} as unknown;
if (typeof person === "object" && person !== null) {
const o = person as Record<string, unknown>
if ('name' in o) {
console.log(o.name);
}
} |
Thanks for you demo, @iugo. Using the type assertion of const person = {
"name": "Benny"
} as unknown as Record<string, unknown>;
console.log(person.name); |
…ard to use ([see this issue](microsoft/TypeScript#21732)).
Because the data is unknown (such as data returned from a JSON string), we use {
const person = JSON.parse(`{"name": "Benny"}`) as unknown;
check(person);
}
{
const person = JSON.parse(`["Benny", 21]`) as unknown;
check(person);
}
function check(person: unknown) {
if (typeof person === "object" && person !== null) {
const o = person as Record<string, unknown>
if ('name' in o) {
console.log(o.name);
}
if (Array.isArray(person)) {
console.log(...person);
}
}
} |
It's happening!!! |
#50666 did not fully fix this issue:
function f<K extends string, T extends object>(key: K, genericObj: T, concreteObj: {foo: string}) {
if ('a' in concreteObj) {
concreteObj.a // no longer an error
}
if ('a' in genericObj) {
genericObj.a // no longer an error
}
if (key in concreteObj) {
concreteObj[key]; // still an error
}
if (key in genericObj) {
genericObj[key] // still an error
}
} can this issue be re-opened? or is there a new issue for this? |
@DetachHead How would you expect this to work? // `concreteObj` is type `{foo: string}`
if (key in concreteObj) {
// `concreteObj` is narrowed to ???
concreteObj[key];
}
function f<K extends string, T extends object>(key1: K, key2: K, genericObj: T, concreteObj: {foo: string}) {
if (key1 in concreteObj) {
concreteObj[key2];
} This is obviously unsafe, and there's no way in the type system to express that |
Yeah, you'd need a construct like |
I think it does not make sense that TypeScript assumes there are additional properties somehow on an object but also does not allow adding new properties to an object at the same time. Just like an
So maybe the actual bug is that
But I assume this would be a pretty big change and there would need to be a solution for letting people know when they accidentally create a new property with a typo, so that's your reason why this will probably never be fixed. |
This is the entire crux of it; a computer can't tell whether this code is intentional or not: const dimensions = { width: 0, height: 0 };
dimensions.widht = 3; Our experience is that detecting retrospectively-obvious-to-human typos is seen as worth the false positives in other cases. It's why we have type annotations. |
Could "as const" be used to detect that case? const dimensions = { width: 0, height: 0 } as const;
dimensions.widht = 3; |
if you want to allow adding unknown keys to an object, you can define it like so in the type: interface Dimensions {
width: number
height: number
[key: string]: number
}
const dimensions: Dimensions = { width: 0, height: 0 };
dimensions.widht = 3; // no error that's an edge case though, and as @RyanCavanaugh said it's far more likely that assigning an unknown key is an error, which is why it wouldn't make sense to not show an error by default, expecting the user to use |
@aaroncowie A subtype could already contain that key with an unrelated type, therefor losing type soundness. function foo(a: { width: number, height: number }) {
a.length = "idk"
}
const a = { width: 0, height: 0, length: 0 }
foo(a)
a.length // STRING????? HUUUHHH????? |
I'm not following what's obvious about this issue/thread/example. It makes sense to me that you can't index by |
The goal of the comment I was replying to is to be able to index |
https://nitro.powerhrg.com/runway/backlog_items/PLAY-1083 **What does this PR do?** A clear and concise description with your runway ticket url. **Task** Run our new linter on kits that start with F - J. **Because** We've implemented a linter config, but most/all of our library files are still failing its ruleset. We need to update all kits in groups to eventually clean up the entire library to confirm with the new linter configs. **Note** I also fixed as many warning as I could. N/A 💻 `yarn run eslint --quiet "./playbook/app/pb_kits/playbook/{pb_f,pb_g,pb_h,pb_i}*/**/*.{js,ts,tsx,jsx}"` ``` yarn run v1.22.15 $ /Users/stephen.marshall/code/playbook/node_modules/.bin/eslint --quiet './playbook/app/pb_kits/playbook/{pb_f,pb_g,pb_h,pb_i}*/**/*.{js,ts,tsx,jsx}' /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_fixed_confirmation_toast/_fixed_confirmation_toast.tsx 44:21 error Unexpected empty arrow function @typescript-eslint/no-empty-function 83:30 error Prop `onClick` must be placed on a new line react/jsx-max-props-per-line 84:46 error Prop `fixedWidth` must be placed on a new line react/jsx-max-props-per-line 90:17 error Expected indentation of 18 space characters but found 16 react/jsx-indent-props 91:17 error Expected indentation of 18 space characters but found 16 react/jsx-indent-props 92:17 error Expected indentation of 18 space characters but found 16 react/jsx-indent-props 98:39 error Prop `cursor` must be placed on a new line react/jsx-max-props-per-line /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_flex/_flex.tsx 10:10 error Don't use `object` as a type. The `object` type is currently hard to use ([see this issue](microsoft/TypeScript#21732)). Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys @typescript-eslint/ban-types /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_form_group/_form_group.tsx 10:10 error Don't use `object` as a type. The `object` type is currently hard to use ([see this issue](microsoft/TypeScript#21732)). Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys @typescript-eslint/ban-types /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_form_pill/_form_pill.tsx 34:21 error Unexpected empty arrow function @typescript-eslint/no-empty-function 51:26 error Prop `id` must be placed on a new line react/jsx-max-props-per-line /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_gauge/_gauge.tsx 20:10 error Don't use `Boolean` as a type. Use boolean instead @typescript-eslint/ban-types 193:7 error Expected indentation of 8 space characters but found 6 react/jsx-indent-props 200:7 error Expected indentation of 8 space characters but found 6 react/jsx-indent-props 201:7 error Expected indentation of 8 space characters but found 6 react/jsx-indent-props /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_home_address_street/_home_address_street.tsx 66:7 error Expected indentation of 8 space characters but found 6 react/jsx-indent-props 67:7 error Expected indentation of 8 space characters but found 6 react/jsx-indent-props 68:7 error Expected indentation of 8 space characters but found 6 react/jsx-indent-props 69:7 error Expected indentation of 8 space characters but found 6 react/jsx-indent-props /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_icon_stat_value/_icon_stat_value.tsx 16:10 error Don't use `object` as a type. The `object` type is currently hard to use ([see this issue](microsoft/TypeScript#21732)). Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys @typescript-eslint/ban-types /Users/stephen.marshall/code/playbook/playbook/app/pb_kits/playbook/pb_icon_value/_icon_value.tsx 15:10 error Don't use `object` as a type. The `object` type is currently hard to use ([see this issue](microsoft/TypeScript#21732)). Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys @typescript-eslint/ban-types ✖ 21 problems (21 errors, 0 warnings) 15 errors and 0 warnings potentially fixable with the `--fix` option. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. ``` #### Checklist: - [x] **LABELS** Add a label: `enhancement`, `bug`, `improvement`, `new kit`, `deprecated`, or `breaking`. See [Changelog & Labels](https://github.com/powerhome/playbook/wiki/Changelog-&-Labels) for details. - [x] **DEPLOY** I have added the `milano` label to show I'm ready for a review. - [x] **TESTS** I have added test coverage to my code.
TypeScript Version: 2.8.0-dev.20180204
Search Terms:
in
operator type guard generic assertCode
Actual behavior:
The compiler does not recognize that the objects have relevant keys even after checking for the existence of the key with the
in
operator. According to a comment by @sandersn, thein
type guard (as implemented in #15256) narrows by eliminating members from a union; it does not assert that a key exists.Desired behavior:
The compiler would assert that each object had a property with the relevant key, after having checked for the existence of the key with the
in
operator. Note that one possible implementation of an asserting type guard would look likebut this does not behave exactly as desired, possibly due to the bug in #18538:(not sure if #18538 was officially fixed, but it's not erroring anymore)If a fix for #18538 appears and makes theg()
function compile without error, great. Otherwise, maybe the property assertion forin
could happen some other way. Not sure.Playground Link: Here
Related Issues:
#10485, Treat
in
operator as type guard#15256, Add type guard for
in
keyword#18538, Error when mixing
keyof
and intersection type and type variable (fixed?)(EDIT: #18538 seems to be fixed)
(EDIT: change
any
tounknown
)The text was updated successfully, but these errors were encountered: