Skip to content

Conversation

@weswigham
Copy link
Member

Fixes #21360

getApparentTypeForLocation (which is how we enabled this, similarly to how we've made guards on this.foo.bar work when this.foo is an indexed access type) uses getAparrentType. That replaces primitives with their corresponding interfaces; I don't think we (ever) actually want do do that here (it just didn't matter when it was only ever used for the left hand side of a property access), so it's probably appropriate to factor out the constraint-mapping behaviors and perform only that here.

I've also renamed the associated functions as appropriate, since they no longer deal with apparent types.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

The fix looks good, but I’d like more work put in on clarifying the constraint concept to prevent future bugs.

return !!(typeParameter.symbol && forEach(typeParameter.symbol.declarations, decl => isTypeParameterDeclaration(decl) && decl.default));
}

function getConstraintType(type: Type): Type {
Copy link
Member

Choose a reason for hiding this comment

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

There is a thicket of related concepts here now that need better names: base constraint, syntactic constraint and this concept — constraint? — that I think is the correct one to use most of the time. But it doesn’t support index access types, for example.

Let’s talk in person and look over the current set of constraint-related functions. I don’t think we should be using the syntactic constraint much anymore, but I’m not sure of the best way to make that happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

TypeFlags.TypeVariable includes indexed accesses.

Copy link
Member Author

Choose a reason for hiding this comment

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

After talking in person, I've since swapped just reusing mostof getBaseConstraintOfType, since that handles most of the behaviors we wanted anyway (including handling intersections correctly).

return type;
}

function getBaseConstraintOrType(type: Type) {
Copy link
Member

Choose a reason for hiding this comment

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

I would inline this to avoid introducing a new name that differs by one (1) character from getBaseConstraintOfType.

Or put both of those functions right next to each other.

@weswigham weswigham changed the title Use a limited version of getApparentType that doesnt map primitives Just map type variables to constraints at certain positions for narrowing so that we do not map primitives Jan 24, 2018
@weswigham weswigham force-pushed the use-trimmed-apparent-logic branch from a0cb0a3 to e3a3b9a Compare February 8, 2018 00:56
@weswigham weswigham force-pushed the use-trimmed-apparent-logic branch from e3a3b9a to 68e594a Compare February 8, 2018 01:33
@weswigham weswigham merged commit 66fa9f6 into microsoft:master Feb 21, 2018
@weswigham weswigham deleted the use-trimmed-apparent-logic branch February 21, 2018 20:51
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants