GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add type guard for `in` keyword by IdeaHunter · Pull Request #15256 · microsoft/TypeScript Conversation
Contributor
Fixes #10485 Added support for typeguarding for following cases: if ("a" in (x)) if ("a" in x) if ("a" in x.prop) if ("a" in x.outer.prop) if ('a' in this) if ('a' in this.prop)
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two spaces here... we should turn on a lint rule
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't correct to narrow to
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ryan Cavanaugh (@RyanCavanaugh) it isn't clear what should we do here since
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now we have following logic: propertyIsOptional ? assumeTrue && propertyIsInType : propertyIsInType === assumeTrue right?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems like it might do the trick
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
|
Ryan Cavanaugh (@RyanCavanaugh) I have updated code to match requested behavior, can you, please, review it? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Looks good overall to me. Anders Hejlsberg (@ahejlsberg) care to review? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ping Ryan Cavanaugh (@RyanCavanaugh) Anders Hejlsberg (@ahejlsberg) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm really looking forward to this landing! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Is it possible this will land for the Typescript 2.4 release? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Can I ask what block this PR being merged? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Can anyone comment as to why this PR is still languishing 6 months later? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There is two reasons of it:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| function isTypePresencePossible(type: Type, propName: string, shouldHaveProperty: boolean) { | ||
| const prop = getPropertyOfType(type, propName); | ||
| if (!prop) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Two edits in this body - rename shouldHaveProperty to assumeTrue and reduce the if to
if (prop) {
return (prop.flags & SymbolFlags.Optional) ? true : assumeTrue;
}
return !assumeTrue;
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Good to merge after one edit. Sorry for the delay on this 🙁 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if ("a" in x) { | ||
| x.b = "1"; | ||
| ~ | ||
| !!! error TS2339: Property 'b' does not exist on type 'A'. |
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to be able to use in for narrowing, but I'm not sure this is it...
Consider:
const foo = { a: true, b: '' } negativeClassesTest(foo)
foo is only valid as a B, but it would pass the in check, and then be treated as an A; but its a key is not of type string.
This means that, within the branch, the type of x should still be A|B, but you are allowed to access .a; I just don't know what type .a should have. It probably should be any, which should trip --noImplicitAny if it's not used with a type narrowing expression.
Scenarios:
function negativeClassesTest(x: A | B) { if ("a" in x) { x.a // --noImplicitAny error const y: string = x.a // ideally a --noImplicitAny error, if it's possible to have "signalling any"s const z = x.a as string // accepted because it's a cast const u: string = typeof x.a === 'string' ? x.a : '' // accepted because it's narrowed } }
It doesn't seem to be all that useful to work like this, but it at least allows you to access .a at all.
Bonus points, but probably hard to actually do in practice:
function negativeClassesTest(x: A | B) { if ("a" in x && typeof x.a === "string") { // x _should_ be an A! } }
Alternative solution: Have a way to indicate that B has "not this key". Maybe, for example, interface B { a: never; b: string } should mean that B is not allowed to have an a key at all; even if it's set to a value of type never such as undefined!.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We considered the soundness aspect and it's not very different from existing soundness issues around aliased objects with undeclared properties. IOW you can already write this code, error-free (even in flow!), which observably fails:
interface T { x: string, y?: number }; const s = { x: "", y: "uh oh" }; const not_t: { x: string } = s; const unsound: T = not_t; (unsound.y && unsound.y.toFixed());
The reality is that most unions are already correctly disjointed and don't alias enough to manifest the problem. Someone writing an in test is not going to write a "better" check; all that really happens in practice is that people add type assertions or move the code to an equally-unsound user-defined type predicate. On net I don't think this is any worse than the status quo (and is better because it induces fewer user-defined type predicates, which are error-prone if written using in due to a lack of typo-checking).
For automatically disjointed unions, see #14094.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@Kovensky im agree, this is nasty case, but it grows from the fact we have structural type system in place. If we, for example, had nominal type system, the info we provided in function definitions would exactly matching all its usages and we never had this problem in place.
Workaround is to give compiler more complete info about types given, like this:
class A { a: string } class B { b: string } class C { a: number; b: string } function z(x: A|B|C){ if("a" in x){ var num_a: number = x.a; // error TS2322: Type 'string | number' is not assignable to type 'number'. // Type 'string' is not assignable to type 'number'. var str_a: string = x.a; // error TS2322: Type 'string | number' is not assignable to type 'string'. // Type 'number' is not assignable to type 'string'. } }
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Going to to do requested fixes later, after my work day ends :-) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.