-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Add type relationship functions to checker public api #9943
Conversation
src/compiler/types.ts
Outdated
@@ -1861,6 +1861,53 @@ namespace ts { | |||
getJsxIntrinsicTagNames(): Symbol[]; | |||
isOptionalParameter(node: ParameterDeclaration): boolean; | |||
|
|||
/** | |||
* Two types are identical iff they are references to the exact same fully qualified, resolved type (usually an alias to ===) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by "usually an alias to ==="?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most often, the identity relation will only be true when a === b
. There's only a few other things (identical generic instantiations, I think) not covered by that.
38e991c
to
f210368
Compare
@ahejlsberg You probably want to weigh in on this. |
f210368
to
a7ba54d
Compare
I've added getter methods for numeric literals, true, and false. (Enum members should be lookup-able) |
806ffe1
to
acf75c3
Compare
Could this be rebased and reviewed please? |
I rebased this branch and all tests pass. You can access my changes at: Sheyne#1. |
+1 @DanielRosenwasser can this one be revived? |
acf75c3
to
a18fe4d
Compare
This would be great, would love to see it make it in! |
ping @DanielRosenwasser this came up again |
908a6ae
to
3a9686e
Compare
const symbol = getSymbol(globals, escapeLeadingUnderscores(name), SymbolFlags.Value); | ||
return symbol ? getTypeOfSymbol(symbol) : unknownType; | ||
}, | ||
lookupTypeAt: (name, node) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wounder if we should just expose resolveName instead..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing it directly would make public way too many of the internal details, like usage checking, that are built into it. It's very overloaded, and the public API edition of its functionality should be simplified, I think. Looking at this again, I think Type/Value
may be better qualifiers on the function names than Type/ValueType
, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use resolveName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. @ahejlsberg can you also take a look please.
The thing I find hard to reason about is where to stop. Makes sense to expose the type relationship functions, Just trying to understand what the guiding principle is here? |
I (way back when this PR was first made) had a separate PR exposing safe APIs to programmatically build types (since the way we build types internally is very lazy and requires an associated node tree, etc) separate from this one. That's the main distinction here - this PR is for comparing and accessing existing types, while I have other work for public APIs for building types. However many things could operate only using existing types (like many lint rules), so the two need not be coupled (this is both easier to expose directly and arguably more immediately useful). |
src/compiler/checker.ts
Outdated
}, | ||
getTypeOfSymbol, | ||
|
||
getAnyType: () => anyType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have a new function getBuiltinType instead of all of these..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just getBuiltinTypes()
that returns an object literal with all of them, so that it is future-proof.
We should also add the factories that are needed, e.g. union, intersection, type reference instantiation, anonymous types, createSymbol. createSignature. and have a full proposal for the change. |
isSubtypeOf: (a, b) => checkTypeRelatedTo(a, b, subtypeRelation, /*errorNode*/ undefined), | ||
isAssignableTo: (a, b) => checkTypeRelatedTo(a, b, assignableRelation, /*errorNode*/ undefined), | ||
isComparableTo: areTypesComparable, | ||
isInstantiationOf: (a, b) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this one i would say.
}, | ||
getTypeOfSymbol, | ||
getUnknownType: () => unknownType, | ||
getStringLiteralType: (text: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider merging these two into one getLiteralType
Any update on this? |
@kevinphelps The added wish of
Is rather complicated, since what we use internally is super easy to misuse, and I, at least, would rather not directly expose those bits as is (esp. given how often we breakingly change them). A comprehensive proposal for a stable public view on type creation/comparison is big. A long time ago I started drafting an API here, but it's hopelessly outmoded now (even moreso than this PR), and I'm not even sure a fluent API is correct. 🙁 |
Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it. |
Why didn't we actually go for type relationship APIs in the first place and add the factories later? People (like me) have to use a 3rd party library (like I understand that ideally in the end, everything like the factories is exposed as well. But to be honest, if the type relationship API, which @weswigham did in this PR, was exposed, it would make things much easier for people working with the TypeScript type checker directly. |
If you can tell us a little bit about the use cases, that would be one step in the right direction. We assumed that the relationship APIs alone wouldn't be enough to solve most use cases. I still am a bit skeptical given that there's other information I've heard people asking for (e.g. tracking the declared type vs. the flow-analyzed type at read positions) which requires more information to be exposed than originally expected. |
Sure. So I'm working on tsd which helps in testing type definitions written by hand. @sindresorhus uses it in a ton of his packages already. It's a little like One thing that I'm now solving is exact type assertions. This means that I want to check that the expected type is exactly the type returned and isn't too wide. For instance, How do I check this? I use the AST to extract the type information of the generic and of the argument. If the argument type ( So I looked into I added one line to the type checker which exposed the So in my use case, only the assignability check would be necessary and would help me an awefull lot. Given the fact that there is a library like |
I'm not sure what the original use cases are. But I think that for everyone working with the AST, based upon source files, will have all the types available and have enough with the relationships API. I see them as separates scopes. The scope of this PR is "small" and easy to review. It offers functionality that I can use directly and helps me when working with the AST. The factory methods on the other hand, as much as I understand them, allow you to create types dynamically and thus not based upon source files, or an already existing AST. This feels to me like a different scope and thus a different PR which can be specced out differently. This is just my opinion though. You guys have much more knowledge and insights on use cases. I just think that it's a pitty that this PR became stall and people have to use 3rd party packages which try to mimick the TypeScript internals while the core already have all this information. |
My use case in Sinap (https://github.com/2graphic/sinap-typescript-loader) was to use Typescript as a DSL for writing simple graph interpreters. The UI would allow you to build a graph (as in network, not as in plot) and the valid edges and nodes on the graph depended on the assignability of various types in .TS files in the interpreter. I've not followed the factories described above, but if they don't allow easily checking assignability from types loaded from source files, then this is another example use case. |
I also need |
Fixes #9879
This PR adds the following API surface to the checker:
Along with appropriate unit tests.
This differs from the original proposal in that it adds
lookupGlobalValueType
,lookupValueTypeAt
, andgetTypeOfSymbol
(along withgetNumberLiteralType
,getFalseType
, andgetTrueType
, which were only enabled after the original issue was opened). Without these, it was difficult (impossible?) to do things like lookup the type of a functionfoo
within a given scope (since this is a type in value space rather than type space) and get the type associated with the symbol returned for a type's property.