Skip to content

Implement constructor type guard #32774

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

Merged
merged 7 commits into from
Mar 11, 2020
Merged

Implement constructor type guard #32774

merged 7 commits into from
Mar 11, 2020

Conversation

austincummings
Copy link
Contributor

Fixes #23274
tl;dr: Implements constructor check type guards in the form of x.constructor === T.


This PR adds a new constructor equality check type guard. It specifically works on a set of binary expressions in the following syntactic forms:

  • x.constructor === T
  • x.constructor == T
  • x["constructor"] === T
  • x["constructor"] == T

Narrowing is done via the added narrowTypeByConstructor function. This type guard only operates on == and === so if it finds the expression is using a not-equals operator then it returns the type without any narrowing done.

After that it uses isMatchingReference to presumably check that we are still referring to the x part of the x.constructor === T expression. I'm not certain on how this works exactly, but the use case is to ensure only x has the narrowed type in the if body as opposed to x.y having the narrowed type.

Once that check is complete it moves on to getting the type referred to by T. This type eventually turns into our candidate type if it is not a direct reference to the globalObjectType or globalFunctionType. This does introduce a shortcoming of the narrowing as it prevents narrowing using a T of Object or Function.

If the type that is being narrowed is any, then it can assume the candidate type is a subtype of any and so we just narrow directly to candidate. I think this is a safe assumption but maybe not? Also there is no need to check for unknown as you would receive an error attempting to access x.constructor if x's type is unknown.

Finally it returns the result of the filterType function which uses a function isConstructedBy to do the filtering. If none of the types match we end up with never as the flow type.

isConstructedBy

This function checks if either the source or target type are Object types with the Class ObjectFlag. If this is the case then we return if the symbols of source and target type are ===. This is needed because you may have two classes with the same structure and TypeScript will see them as the same type. There was no way I could find that would check this. I think this also introduces the side effect of having pseudo nominal typing in the language.

For all other cases it just checks that the source type is a subtype of target type.

Caveats and questions

  • Does not narrow when using a T of Object or Function.
  • Is comparing the symbols of source and target types a good way to check that they are referring to the exact same class?

@RyanCavanaugh
Copy link
Member

@weswigham this LGTM - please merge if you agree

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.7.0 milestone Aug 15, 2019
@austincummings
Copy link
Contributor Author

Addressed @weswigham's comments.

@weswigham
Copy link
Member

Looks like some tests are failing - you may need to run --skipPercent=0 to prevent the test from being skipped in a test run locally.

@austincummings
Copy link
Contributor Author

Ah ok, I forgot to rebase onto master. Looks like some symbol positions changed.

- Do not limit constructor expression to only identifiers
- Fix `assumeTrue` and operator no-narrow check
- Use better way to check that identifier type is a function
- Loosen restriction on what expr is left of ".constructor"
- Update typeGuardConstructorClassAndNumber test to include else cases
@weswigham
Copy link
Member

So, showerthought here, but.... What do we do when

const x = { constructor: String };
if (x.constructor === String) {
    x.trim(); // runtime error
}

Or any other instance where the constructor property isn't inherited from Object or Function? I think we need to validate that the constructor property accessed comes from one of the built-in apparent types for this narrowing to be safe to do.

@austincummings
Copy link
Contributor Author

austincummings commented Aug 19, 2019 via email

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RyanCavanaugh you can give this another look when you're ready, but as far as the code goes, I think this is pretty good.

@matthew-dean
Copy link

matthew-dean commented Oct 13, 2019

I came here to request this feature, as I'm doing .constructor type checks extensively in my project (but TypeScript still needs type assertions), so this is great! Can't wait for this to make it in!

@KingDarBoja
Copy link

I came here to request this feature, as I'm doing .constructor type checks extensively in my project (but TypeScript still needs type assertions), so this is great! Can't wait for this to make it in!

Same here, this is really an awesome feature and glad the author and the Typescript team going to support it 🍰

@hayd
Copy link

hayd commented Jan 27, 2020

Bump!

@darsain
Copy link

darsain commented Jan 31, 2020

I'd like to suggest adding an alternative x.constructor.name === "T" typeguard.

Why / where am I coming from:

I worked once in a weird environment (I think an nw.js app) where my element objects were defined in a different context than globally available constructors, so stuff like

actualInputElement instanceof HTMLInputElement

would always return false. Since than I consider instanceof or comparing raw constructor objects on stuff like HTML elements unsafe and do constructor.name checks there instead, as that was the only alternative.

Is it too unsound to even be considered? :) Because it's working perfectly on html elements and I don't have to be afraid of weird environments with colliding contexts.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020
@sandersn sandersn merged commit ae3d28b into microsoft:master Mar 11, 2020
@HolgerJeromin
Copy link
Contributor

@darsain

I worked once in a weird environment (I think an nw.js app) where my element objects were defined in a different context than globally available constructors, so stuff like
actualInputElement instanceof HTMLInputElement
would always return false.

I had a similar situation inside an HTMLObjectElement. I think
actualInputElement instanceof actualInputElement.ownerDocument.defaultView.HTMLInputElement
is a saver runtime check, but no TS check either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Feature Request / Proposal: constructor type guard
9 participants