Skip to content

Regression in 2.8 vs 2.7 for function accepting a union of "Guid" and T, and having, but not always utilising, a keyof T in second param #23131

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 Apr 3, 2018 · 5 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@IanYates
Copy link

IanYates commented Apr 3, 2018

TypeScript Version: 2.8.1 (MS Build NuGet package)
Using "latest" Visual Studio 2017 plugin - v15.6.20202.3

Was working fine all the way through to 2.7.x until trying to update to released 2.8.x this morning

Search Terms: keyof union

Code

//function to conveniently allow passing in a guid, or an object that contains a guid at a known property
declare function extractGuid<T>(guidOrObject: Guid | T, idProperty: keyof T): Guid;

//GUIDs are really just strings, so treat them as such.  Adding toLowerCase() is a good way of enforcing this
interface Guid {
    toLowerCase(): string;
    __GuidBrand?: any;
}

interface Person {
  id: Guid,
  name: string,
  address: string
}

var c1: Guid = '12341234-1234-1234-1234-123412341234';
var g1 = extractGuid(c1, 'id');  //'id' was previously ignored
var c2: Person = {
  id: '12341234-1234-1234-1234-123412341234',
  name: 'my name',
  address: '1 way street'
};
var g2 = extractGuid(c2, 'id');

Expected behavior:
I don't get a compilation error on the line var g1 = ....
The 'id' property is effectively ignored because the T isn't being used

Actual behavior:
I get an error (also shown in playground below)
Argument of type '"id"' is not assignable to parameter of type '"toLowerCase" | "__GuidBrand?"'

Playground Link:
http://www.typescriptlang.org/play/index.html#src=%2F%2Ffunction%20to%20conveniently%20allow%20passing%20in%20a%20guid%2C%20or%20an%20object%20that%20contains%20a%20guid%20at%20a%20known%20property%0D%0Adeclare%20function%20extractGuid%3CT%3E(guidOrObject%3A%20Guid%20%7C%20T%2C%20idProperty%3A%20keyof%20T)%3A%20Guid%3B%0D%0A%0D%0A%2F%2FGUIDs%20are%20really%20just%20strings%2C%20so%20treat%20them%20as%20such.%20%20Adding%20toLowerCase()%20is%20a%20good%20way%20of%20enforcing%20this%0D%0Ainterface%20Guid%20%7B%0D%0A%20%20%20%20toLowerCase()%3A%20string%3B%0D%0A%20%20%20%20__GuidBrand%3F%3A%20any%3B%0D%0A%7D%0D%0A%0D%0Ainterface%20Person%20%7B%0D%0A%20%20id%3A%20Guid%2C%0D%0A%20%20name%3A%20string%2C%0D%0A%20%20address%3A%20string%0D%0A%7D%0D%0A%0D%0Avar%20c1%3A%20Guid%20%3D%20'12341234-1234-1234-1234-123412341234'%3B%0D%0Avar%20g1%20%3D%20extractGuid(c1%2C%20'id')%3B%20%20%2F%2F'id'%20was%20previously%20ignored%0D%0Avar%20c2%3A%20Person%20%3D%20%7B%0D%0A%20%20id%3A%20'12341234-1234-1234-1234-123412341234'%2C%0D%0A%20%20name%3A%20'my%20name'%2C%0D%0A%20%20address%3A%20'1%20way%20street'%0D%0A%7D%3B%0D%0Avar%20g2%20%3D%20extractGuid(c2%2C%20'id')%3B%0D%0A%0D%0A

Related Issues:
Maybe this one
#22300 (comment)
It talks about keyof and intersection (not union) types and branding similar to what I'm doing with the branded Guid. Not entirely sure it's related but it at least fits the timeline of being in 2.8 and not in 2.7.

@IanYates
Copy link
Author

IanYates commented Apr 3, 2018

I've tried making two separate function declarations instead

declare function extractGuid(guidOrObject: Guid, idProperty: string): Guid;
declare function extractGuid<T>(guidOrObject: T, idProperty: keyof T): Guid;

In my own code that gives errors
Argument of type '"id"' is not assignable to parameter of type 'never'

I don't have any sort of strict checks enabled.

But in playground that works... Hmmm...

http://www.typescriptlang.org/play/index.html#src=%2F%2Ffunction%20to%20conveniently%20allow%20passing%20in%20a%20guid%2C%20or%20an%20object%20that%20contains%20a%20guid%20at%20a%20known%20property%0D%0A%2F%2Fdeclare%20function%20extractGuid%3CT%3E(guidOrObject%3A%20Guid%20%7C%20T%2C%20idProperty%3A%20keyof%20T)%3A%20Guid%3B%0D%0Adeclare%20function%20extractGuid(guidOrObject%3A%20Guid%2C%20idProperty%3A%20string)%3A%20Guid%3B%0D%0Adeclare%20function%20extractGuid%3CT%3E(guidOrObject%3A%20T%2C%20idProperty%3A%20keyof%20T)%3A%20Guid%3B%0D%0A%0D%0A%2F%2FGUIDs%20are%20really%20just%20strings%2C%20so%20treat%20them%20as%20such.%20%20Adding%20toLowerCase()%20is%20a%20good%20way%20of%20enforcing%20this%0D%0Ainterface%20Guid%20%7B%0D%0A%20%20%20%20toLowerCase()%3A%20string%3B%0D%0A%20%20%20%20__GuidBrand%3F%3A%20any%3B%0D%0A%7D%0D%0A%0D%0Ainterface%20Person%20%7B%0D%0A%20%20id%3A%20Guid%2C%0D%0A%20%20name%3A%20string%2C%0D%0A%20%20address%3A%20string%0D%0A%7D%0D%0A%0D%0Avar%20c1%3A%20Guid%20%3D%20'12341234-1234-1234-1234-123412341234'%3B%0D%0Avar%20g1%20%3D%20extractGuid(c1%2C%20'id')%3B%20%20%2F%2F'id'%20was%20previously%20ignored%0D%0Avar%20c2%3A%20Person%20%3D%20%7B%0D%0A%20%20id%3A%20'12341234-1234-1234-1234-123412341234'%2C%0D%0A%20%20name%3A%20'my%20name'%2C%0D%0A%20%20address%3A%20'1%20way%20street'%0D%0A%7D%3B%0D%0Avar%20g2%20%3D%20extractGuid(c2%2C%20'id')%3B%0D%0A%0D%0A

@IanYates
Copy link
Author

IanYates commented Apr 3, 2018

Workaround. Use new conditional types

declare function extractGuid<T>(guidOrObject: Guid | T, idProperty: T extends Guid ? any : keyof T): Guid;

That seems to do the trick

http://www.typescriptlang.org/play/index.html#src=%2F%2Ffunction%20to%20conveniently%20allow%20passing%20in%20a%20guid%2C%20or%20an%20object%20that%20contains%20a%20guid%20at%20a%20known%20property%0D%0Adeclare%20function%20extractGuid%3CT%3E(guidOrObject%3A%20Guid%20%7C%20T%2C%20idProperty%3A%20T%20extends%20Guid%20%3F%20any%20%3A%20keyof%20T)%3A%20Guid%3B%0D%0A%0D%0A%2F%2FGUIDs%20are%20really%20just%20strings%2C%20so%20treat%20them%20as%20such.%20%20Adding%20toLowerCase()%20is%20a%20good%20way%20of%20enforcing%20this%0D%0Ainterface%20Guid%20%7B%0D%0A%20%20%20%20toLowerCase()%3A%20string%3B%0D%0A%20%20%20%20__GuidBrand%3F%3A%20any%3B%0D%0A%7D%0D%0A%0D%0Ainterface%20Person%20%7B%0D%0A%20%20id%3A%20Guid%2C%0D%0A%20%20name%3A%20string%2C%0D%0A%20%20address%3A%20string%0D%0A%7D%0D%0A%0D%0Avar%20c1%3A%20Guid%20%3D%20'12341234-1234-1234-1234-123412341234'%3B%0D%0Avar%20g1%20%3D%20extractGuid(c1%2C%20'id')%3B%20%20%2F%2F'id'%20was%20previously%20ignored%0D%0Avar%20c2%3A%20Person%20%3D%20%7B%0D%0A%20%20id%3A%20'12341234-1234-1234-1234-123412341234'%2C%0D%0A%20%20name%3A%20'my%20name'%2C%0D%0A%20%20address%3A%20'1%20way%20street'%0D%0A%7D%3B%0D%0Avar%20g2%20%3D%20extractGuid(c2%2C%20'id')%3B%0D%0A%0D%0A%2F%2FBELOW%20SHOULD%20FAIL%20TO%20COMPILE%20%20(THE%20WHOLE%20POINT%20OF%20THIS)%0D%0Avar%20g3%20%3D%20extractGuid(c2%2C%20%22notValidProperty%22)%3B

@mhegazy
Copy link
Contributor

mhegazy commented Apr 3, 2018

The error in the OP seems right to me. your overloads should work, though i would make the string one come last, since it is more general. the conditional type approach should work as well.

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Apr 3, 2018
@IanYates
Copy link
Author

IanYates commented Apr 3, 2018

So I was relying on "broken" behaviour in v2.7 and earlier?

No problem. I wanted to enquire & flag it since it was a breaking change (for me at least). The conditional type seems to have done the trick though.

Thanks for the quick investigation.

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@microsoft microsoft locked and limited conversation to collaborators Jul 30, 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