Skip to content

User-defined type guards and structural typing - possible failure #5518

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

Closed
IanYates opened this issue Nov 4, 2015 · 5 comments
Closed

User-defined type guards and structural typing - possible failure #5518

IanYates opened this issue Nov 4, 2015 · 5 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@IanYates
Copy link

IanYates commented Nov 4, 2015

Hi,

Write this code in the playground

interface Guid {
    toLowerCase:()=>string;
    // dummyExtra: number;
}

function isGuid(g: any): g is Guid {
    return (g['toLowerCase'] !== undefined) && (g.length === 32);  //skip other checks (just an example)
}


function doSomething(guidOrString : Guid | string) {
    if (isGuid(guidOrString)) {
        iTakeGuids(guidOrString);
    } else {
        iTakeStrings(guidOrString);         
    }
}

function iTakeStrings(s:string) {


}

function iTakeGuids(g: Guid) {


}

You'll see an error in the call iTakeStrings(guidOrString); because the type of guidOrString here is deemed to be {} rather than string. I'm guessing this is because of structural typing. This is evident (in my mind) because if you uncomment the // dummyExtra: number; line then there's no error.

I've got types that are notionally guids in my code (lots of interfaces generated from my C# DTOs using some T4 templates) but really they're strings. I have toLowerCase() specified in my Guid interface as a way of differentiating the guids from the any type and also because I sometimes need to call toLowerCase() on them anyway.

Is it possible to support this? I'm guessing the types look similar but string has a lot more than what my Guid type has (eg length, toUpperCase, etc) so the types aren't the same really.

I thought this was a regression introduced by TypeScript v1.7.2 that I've got with VS 2015 Update 1 RC, but I then checked on VS 2015 RTM with TypeScript v1.6, and in the playground (showing the version on there would be handy!) and the problem exists in all of them.

Thanks for reading. I've tried searching other issues but couldn't see one quite matching this scenario, particularly as the code works when you restore the commented-out line.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 4, 2015

A user-defined type guard function, just like the instance of type will filter out types that are assignable to target type specified by the guard. in your example, string is assingable to Guid, so it is removed from the union. This is a general problem when using a structural type system and needing to express nominal types. there is already a discussion about is issue in #4895.

In the typescript compiler code base we use intersection types to create tags on types we want to treat as nominal. see https://github.com/Microsoft/TypeScript/blob/master/src/compiler/types.ts#L8.

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Nov 4, 2015
@DanielRosenwasser
Copy link
Member

As a minor correction, we use the subtype relation, not assignability.

@IanYates
Copy link
Author

IanYates commented Nov 5, 2015

Thanks @mhegazy for the pointer (and @DanielRosenwasser for the extra bit of understanding) - great to have conversations with the TypeScript devs so easily and quickly!

It's certainly not a bad approach (I had read #4895 a week or so ago which gives the idea some thorough treatment!).

The toPath function referenced in the comment is nice, but I've got Guid instances coming from all over the place - lots of JSON.parse(), etc. They won't have the tag. Or does that not really matter since, from what I can see in the TypeScript compiler code's use of toPath, you never populate the __PathBrand either? As you say, it's just there to make it look not related to string, similar to my dummyExtra in the Guid interface.

I'll give it a try and report back.

@IanYates
Copy link
Author

IanYates commented Nov 5, 2015

OK. Done :)

This code works well in the playground. I originally was trying the branding this way

interface Guid {
    toLowerCase:()=>string;
    __GuidBrand: any;
}

That gave a LOT of mess and I had a huge post written about using functions, inlining, casting, etc.

I then gave it some thought and realised I instead should have had

interface Guid {
    toLowerCase:()=>string;
    __GuidBrand?: any;   //note this is marked optional
}

Full playground code (for others)...

interface Guid {
    toLowerCase:()=>string;
    __GuidBrand?: any;
}

function isGuid(g: any): g is Guid {
    return (g['toLowerCase'] !== undefined) && (g.length === 36);  //skip other checks
}


function doSomething(guidOrString : Guid | string) {
    if (isGuid(guidOrString)) {
        iTakeGuids(guidOrString);
    } else {
        iTakeStrings(guidOrString);         
    }
}

function iTakeStrings(s:string) {
        alert("string: " + s);

}

function iTakeGuids(g: Guid) {
    alert("guid: " + g);

}

var x: Guid = "12341234-1324-1234-1234-123412341234";
doSomething(x);
var s: string = "some string";
doSomething(s);
var implicit = "aaa";
doSomething(implicit);

Thanks very much!! :) I had to change just two spots in my code where I returned something that was typed as a string (neither was a bug, as the string was a guid, but it's nice this will now be flagged for me).

@IanYates
Copy link
Author

IanYates commented Nov 5, 2015

Note that the code wouldn't have been as easily changed if I had

type Guid = string & {_GuidBrand: any};

since that's effectively my first interface above and things like
var x: Guid = "12341234-1324-1234-1234-123412341234";
don't work

type Guid = string & {_GuidBrand?: any};
works for me (same as the second interface definition but brings in more string functions like length, etc - probably best to avoid that in my use case since guids are equatable and not a lot else for me)

@IanYates IanYates closed this as completed Nov 5, 2015
@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
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

3 participants