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

typeof union.membername errors. #22598

Closed
Griffork opened this issue Mar 15, 2018 · 20 comments
Closed

typeof union.membername errors. #22598

Griffork opened this issue Mar 15, 2018 · 20 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@Griffork
Copy link

Griffork commented Mar 15, 2018

TypeScript Version: 2.8.0-dev.20180308

Search Terms:
typeof union parameter undefined.

Code

declare var value: string | Date;
if (typeof value.now /*error!*/ !== "undefined") {}

Expected behavior:
No error and appropriate type narrowing.

Actual behavior:

[ts]
Property 'now' does not exist on type 'string | Date'.
  Property 'now' does not exist on type 'string'.

Playground Link

NOTE
I don't like having to make a new function to properly narrow types for every union I create, is there another way around this (like manually overriding the variable's type within the scope of a code block without declaring a new variable?).

@DanielRosenwasser DanielRosenwasser added the Bug A bug in TypeScript label Mar 15, 2018
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 15, 2018

I think this is a bug. I guess the rule is that for typeof expr.id, if expr is a union type and id exists on at least one member, permit the check and narrow based on the property.

@nomcopter, in #22094 you did something similar.

@weswigham
Copy link
Member

I definitely don't think this is bug (it's a feature request) because this is definitely how union type property accesses are intended to work (properties only exist if they're in all union members, and that's how they've worked forever)... however allowing accesses of nonexistant union member properties for the purposes of checking for their existence seems reasonableish? Only -ish, though... I think union excess property checking will catch most of the obvious problems that could have cropped up from allowing it nowadays, so... it may be fine? I can't convince myself it definitely is... Like, here's the deal, if I say I have type Union = {kind: "a", Foo: number} | {kind: "b", Foo: string} | {kind: "c"}, and a variable x of type Union, I cannot confidently say that x.Foo is string | number | undefined, because someone could have used {kind: "c", Foo: true} as a perfectly valid value for that variable; all I can say is any, really, which doesn't help. Unions aren't actually closed to subtypes, which muddles this - discrimination is only possible for discriminable unions precisely because a field exists on all union members and whose types give that field a finite domain of possible values into which each possible union member can be bucketed (which is also why we only discriminate on unit types).

@falsandtru
Copy link
Contributor

I agree with @weswigham . Additionally, this example is originally wrong. typeof is missing:

declare var value: string | typeof Date;
if (typeof value.now /*error!*/ !== "undefined") {}

@Griffork
Copy link
Author

@weswigham I don't understand why type narrowing by property existance has to function differently to type narrowing by property string literal?

@falsandtru thanks! Ironically that's a bug that would have been caught by this "proposal".

By the logic that union types are meant to be extended, my argument is that so are interface types. I don't know why you'd cater for invalid assignment during member access on a union type and not also allow it on an interface type (by that logic all interfaces should implicitly have [param: string]:any). That seems extremely counterintuitive.

Actually, to further summarise, you don't think you should do this because you want to implicitly treat all unions as though they have [param: string]:any?

As someone who chooses to code with strict mode on I really dislike that line of logic.

@weswigham
Copy link
Member

weswigham commented Mar 15, 2018

@Griffork No? The point is that when you ask for the type of value.now when value is type string | typeof Date, since now only exists on one union member, nothing is constraining its' type on the other member, meaning you could have potentially assigned a value of a conflicting type to it in the past. For example,

type Union = {kind: "a", Foo: number} | {kind: "b", Foo: string} | {kind: "c"};
let a: Union;
function getVal() {
  return { kind: "c" as "c", Foo: true }
}
a = Math.random() > 0.5 ? getVal() : { kind: "a", Foo: 12 }
if (typeof a.Foo !== "undefined") {
  // a.Foo exists, yes. What is a.Foo's type here? No idea. Can't say it's `number | string`  -
  // we've clearly just constructed an expression above where it might not be! What's
  // `a`'s type? Don't know. Can't draw any conclusions on the type of `a` based on the
  // presence of `Foo`, since not all union members have a known value for it.
}

So while property access won't immediately throw, it is not safe to use (and using it would likely be a code smell), since its type is indeterminate!

You should probably just cast inside your typeof. Alternatively, augment the String interface with an explicit property stating the type of the now member (undefined), to make the fields mutually exclusive (and prevent you from ever assigning something with a defined now type to a string):

interface String {
  now: undefined;
}
declare var value: string | typeof Date;
if (typeof value.now !== "undefined") {
  value.now();
}

@RyanCavanaugh
Copy link
Member

FWIW Flow just changed their behavior from "Any property is OK in a conditional" to match the proposed behavior (a property is OK in a conditional if it exists in any union constituent): facebook/flow@c39aa44

Still not a huge fan of expression typechecking behaving differently depending on the surround context, e.g. this

@weswigham
Copy link
Member

weswigham commented Mar 16, 2018

Yeah, it's probably a good thing that they tightened their checking a bit, however I'm not a fan of loosening ours, given there's very little information we can safely derive from the check, and you can always just use a cast to perform the check anyway.

@Griffork
Copy link
Author

Griffork commented Mar 16, 2018

@RyanCavanaugh I agree (thus the comment about type narrowing).

Related issue:
In javascript you can typeof check a value that hasn't been declared. In typescript this is an error. (I don't think it should be)

This issue:
In javascript you can typeof check a property of an object that hasn't been declared. In typescript this is an error. (I don't think it should be)

I see both of these issues as two sides of the same coin, and it bugs me.
For the first one, I like to do a test (within a shared library) for global properties that only exist on the server, but on the client this test errors if I don't declare the properties (making code sharing really hard).

@weswigham by your argument the following should also be valid, and return an any type.

interface Union { kind: "c" };
let a: Union;
function getVal() {
  return { kind: "c" as "c", Foo: true }
}
a = getVal();

if (typeof a.Foo /*error*/ !== "undefined") 

Which I understand, and am OK with. We can do additional typeof checks to further narrow the scope.

I do think that typeof should still be possible on non-existing members of an object (although I don't know how to resolve this properly with spelling errors, maybe make it only available with string literals in the [] notation?).

Alternatively combining it with some syntax to retype the variable in the following codeblock, such as (example, not proposed syntax):

if (typeof a.Foo !== "undefined") a is {kind: "a", Foo: number} | {kind: "b", Foo: string} 
{

}

Which also checks whether or not a is actually assignable to the resulting type would be good.

Alternatively (again, and this one is a crazy idea) maybe make invalid/undefined properties a different colour in the syntax highlighting? That way it would be obvious why typeof works and nothing else does.

@Griffork
Copy link
Author

At the moment, all over our code we've had to do things like this:

var matchingoptions = <{value: WriteQuery<any>, queryname: string} | {value: WriteQuery<any>, query: ReadQuery<any>}> write[operator];
if (typeof (<any>matchingoptions).queryname !== "undefined") {
    arraypath = path + ".$[" + (<any>matchingoptions).queryname + "]";
}

I do not like having to introduce <any> everywhere I use a union because I can't type narrow on property existance.
This happens everywhere.
I want type safety within my if statements and I can't have it. This is why I thought of this as a bug, and why I'm asking for it to be fixed.

@RyanCavanaugh
Copy link
Member

You can use if ("queryname" in matchingoptions) { which narrows and doesn't error. One advantage of using it in a union position is that if you misspelll the left operand, the right operand isn't narrowed (which hopefully clues you in).

@Griffork
Copy link
Author

Griffork commented Mar 16, 2018

@RyanCavanaugh I like that answer and will use that.

I do have a question though; why does the in operator behave differently to the typeof operator in this context? Won't it have the same problems as @weswigham's example with the getVal scenario?

In javascript aren't they essentially performing the same job?

@RyanCavanaugh
Copy link
Member

We could have at most one consistency.

The in operator is used for all kinds of dynamic tests and we never had a rule in place to disallow any left operand of it, so we blessed it with the narrowing behavior because why not. There's currently no special casing for property access anywhere.

@weswigham
Copy link
Member

IIRC, the slight unsoundness of it's narrowing behavior when it was implemented and just kinda shrugged off, since it provides an option for people like @Griffork who want an unsound way to make a membership check and narrow, and we didn't think people really used in too much.

@Griffork
Copy link
Author

Griffork commented Mar 16, 2018

Actually, I think what I suggested would work better with this proposal:
#14094

@mhegazy mhegazy removed this from the TypeScript 2.9 milestone Mar 29, 2018
@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Bug A bug in TypeScript labels Mar 29, 2018
@Griffork
Copy link
Author

Griffork commented Apr 4, 2018

@RyanCavanaugh I'm trying to show this functionality to my team-mates but don't seem to be able to find in's behaviour anywhere in the docs. Am I missing something?

@mhegazy
Copy link
Contributor

mhegazy commented Apr 4, 2018

https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#type-guards-inferred-from--in-operator

@RyanCavanaugh
Copy link
Member

@Griffork do you consider this use case adequately solved by in, or should we keep this on the backlog of things to review?

@niieani
Copy link

niieani commented Aug 13, 2018

Support for exact (sealed/frozen) types in TypeScript, in case they're in the union being tested, would solve the issues of not being sure of the narrowed type after doing the typeof check.

@Griffork
Copy link
Author

@RyanCavanaugh it's enough for me.

I'd like it to be in the language docs somewhere.
It's a pain getting new people up to speed on typescript because a lot of useful features are not covered at all in the language docs but are halfway down a random blog post (and you can't find it unless you know the exact name for it).

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed In Discussion Not yet reached consensus labels Aug 14, 2018
@RyanCavanaugh
Copy link
Member

Thanks! I agree the docs need some love.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants