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

Check for any types in strict expectType #43

Closed
danvk opened this issue Oct 8, 2019 · 9 comments
Closed

Check for any types in strict expectType #43

danvk opened this issue Oct 8, 2019 · 9 comments

Comments

@danvk
Copy link

danvk commented Oct 8, 2019

I would expect this to fail, especially with "strict" type assertions:

import {expectType} from 'tsd';

function fnReturnsAny(x: number): any {
  return x;
}

expectType<string>(fnReturnsAny(10));

Everything is assignable to any and any is assignable to everything, so the check passes.

It's not entirely clear to me what the ideal behavior is here for types like any[] or for anys in larger types like {name: string; metadata: any;}:

interface File {
  name: string;
  metadata: any;
}
declare let f: File;
expectType<{name: string; metadata: unknown}>(f);  // should probably fail?

But a check that the argument to expectType isn't an any type if the expected type is non-any would make sense:

expectType<string>(fnReturnsAny(10));  // should fail
expectType<any>(fnReturnsAny(10));  // should succeed

You might find this 2ality post interesting, the comments point out some drawbacks of testing types by looking for assignability both ways. any is one, but there may also be some issues related to distributing conditional types.

@danvk
Copy link
Author

danvk commented Oct 8, 2019

(FWIW the fact that there's no good way to handle any is one of the reasons dtslint uses string comparison. Though there are obviously downsides to that, too, like A|B and B|A being considered different types.)

@SamVerschueren
Copy link
Collaborator

This is a good catch! Didn't think about that one.

is one of the reasons dtslint uses string comparison

I don't believe this is a reason. The reason is because they work comment based, thus a string. Meaning they have a Type (source) and a string (expectation) to compare, which can only be done by string matching.

I will take a look what I can do to prevent this behaviour. Thanks for reporting!

@danvk
Copy link
Author

danvk commented Oct 9, 2019

I wrote typings-checker which eventually became dtslint. String comparison was expedient, but resilience in the face of any was definitely a big motivator :)

Happy to give feedback on anything you come up with.

@SamVerschueren
Copy link
Collaborator

Instead of using the assignableTo method of TypeScript, I used identicalTo and I believe this is what we actually need to use. It fails if you do expectType<any>(numberFn());.

Two types are considered identical when

they are both the Any type,
they are the same primitive type,
they are the same type parameter,
they are union types with identical sets of constituent types, or
they are intersection types with identical sets of constituent types, or
they are object types with identical sets of members.

More info here https://github.com/Microsoft/TypeScript/blob/master/doc/spec.md#3.11.2

@orta Do you have any feedback on this? If we do a strict type assertion, we don't want that any messes up with this. It feels and looks like isIdenticalTo is the function we actually need.

@SamVerschueren
Copy link
Collaborator

I tested it more thorougly and it looks like isIdenticalTo is the one we need.

These are use cases where it fails

expectType<string>(m<any>());

expectType<any>(m<string>());

expectType<Observable<string | number | any> | Observable<Date> | Observable<Symbol>>(
	one<Observable<Date> | Observable<Symbol> | Observable<number | string>>()
);

I will probably change the error message though

Currently we have

Parameter type string | number is declared too wide for argument type number.

But now it could be in both directions. So I think it makes sense to change it to

Parameter type string | number is not identical to argument type number.
Parameter type string is not identical to argument type string | number.

@sindresorhus What do you think regarding the error message?

@danvk
Copy link
Author

danvk commented Oct 15, 2019

@SamVerschueren the error message about "declared too wide" more precisely describes the problem, so it would be sad to lose. You could run the one-way assignability checks only if isIdenticalTo fails, to get better errors.

BTW, I asked Anders about this at tsconf this weekend. isIdenticalTo is private, of course, but he also said that they don't use it very much internally since most checks involve being assignability to, not being identical to. So there may be unexplored corner cases.

@SamVerschueren
Copy link
Collaborator

Good idea regarding the error message. Will try to keep it. I agree that it's pretty descriptive.

I know that it's a private API, but to be honest, I stopped caring after reading this PR microsoft/TypeScript#9943. It got stale for nothing in my opinion, the PR was fine as it was. I'm not part of the team though, and I don't know what direction they want to go to. So from a user point of view, it's easy for me to say that...
To fix that issue, I copied over TypeScript entirely and only exposed isAssignableTo so I can use it in tsd. That's also how I will do it with isIdenticalTo.

Thanks for asking Anders about this API, it's good to know that it might not be entirely battle tested in the real world. But for now, it looks like the only correct solution which works and which takes any into account. If we find a corner case, we can always create a new ticket in the TypeScript repo and make the isIdenticalTo better in the long term :).

@SamVerschueren
Copy link
Collaborator

This is now fixed and will be part of the next release. Thanks for bringing this to my attention :)! Great work!

@SamVerschueren
Copy link
Collaborator

SamVerschueren commented Oct 23, 2019

This is released in v0.10.0 🎉. Thanks for reporting this @danvk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants