Skip to content

Allow JSXAttributes types to be shortcut-spread into the spread type like normal objects #19047

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 14 commits into from
Dec 16, 2017

Conversation

weswigham
Copy link
Member

JSXAttributes object type creation was different that normal object type creation in a number of ways (and still is) - one difference was that, thanks to its filter parameter, it always collapsed a type (even when it would have been shortcut to retain its structure had it been a normal object type, for example like when spreading a union). Now, the filter parameter is no more - the filtering of ignored dashed names is handled during relationship checking instead of during type construction time. (This improves error messages a bunch.) This also enables us to cache the intermediate results between invocations of createJsxAttributesTypeFromAttributesProperty (with a few other modifications to ensure consistent caching where possible). As a side effect of removing the eager-type evaluation semantics of the jsx attributes type, this reorders the properties we witness in jsx attribute types - namely children normally get listed first instead of last now.

Fixes #18670.

Side node: Upon investigation, CheckMode was still Normal when there is a contextual type so long as there is no contextual mapper - this seemed like a bit of an oversight, as it is not a perfect cache control indicator because of that... checkApplicableSignature, for example, can call checkExpressionWithContextualType with a contextualType but no contextualMapper, resulting in a CheckMode of Normal, even though the results are potentially modified by the presence of the contextual type (and by extension get cached that way! - for example, I think this might cause a return type contextually typed by an early overload to get locked in for later checks, when it should have been considered speculative). Simply introducing a new Conextual checkmode to use in checkExpressionWithContextualType to use instead of Normal is sufficient to prevent this. This could be also have fixed without a new checkmode in the case of checkApplicableSignature simply by passing createTypeMapper([], []) instead of undefined; but then you get Inferential behaviors where previously there were none (in some cases this actually looks like it gives much nicer error elaborations, but in others this triggers the instantiation of a type parameter too early).

@mhegazy mhegazy requested a review from ahejlsberg October 12, 2017 21:27
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.

Looks good except I'm not sure about the change to checkExpressionCached. @ahejlsberg can you look at that?

@@ -9580,6 +9580,10 @@ namespace ts {
return Ternary.False;
}

function isIgnoredJsxProperty(source: Type, sourceProp: Symbol, targetMemberType: Type | undefined) {
return source.flags & TypeFlags.JsxAttributes && !(isUnhyphenatedJsxName(sourceProp.escapedName) || !!targetMemberType);
Copy link
Member

Choose a reason for hiding this comment

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

the inner !! just seems to add insult to injury (where insult=unreadability and injury=a language with truthiness). Anyway, it's not needed to get this function to return a boolean.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, !isUn is a five character string that you don't hope to not unsee in readable code too. Could it just be called isLegalIdentifierName or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

3 usages of isUnhyphenatedJsxName are not negated, while 2 usages of isUnhyphenatedJsxName are. While the negation of the negation is linguistically unfortunate at the use site, I think the name is the most straightforward and self-documenting it can be as-is.

const attributeType = createJsxAttributesType(attributes.symbol, attributesTable);
return typeToIntersect && attributesTable.size ? getIntersectionType([typeToIntersect, attributeType]) :
typeToIntersect ? typeToIntersect : attributeType;
return typeToIntersect ? getIntersectionType([typeToIntersect, ...spread === emptyObjectType ? [] : [spread]]) : spread;
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be easier to read

typeToIntersect && spread !== emptyObjectType ? getIntersectionType([typeToIntersect, spread]) : (typeToIntersect || spread);

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -18258,6 +18251,9 @@ namespace ts {
function checkExpressionCached(node: Expression, checkMode?: CheckMode): Type {
const links = getNodeLinks(node);
if (!links.resolvedType) {
if (checkMode) {
Copy link
Member

Choose a reason for hiding this comment

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

so this is just a blanket "never cache expression checking when in contextual mode?" That's probably correct, but it makes checkExpression seem redundant. Why not rename it to checkExpressionUncached and have the only caller be right here? Everybody else could cache! (If not contextually typed)

What in this PR motivated this change?

Copy link
Member

Choose a reason for hiding this comment

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

@weswigham I'd like to understand the motivation for the change as well.

@sandersn Caching isn't cost free (i.e. it will often cause allocation of a NodeLinks object) so we don't want to always cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to cache results in createJsxAttributesTypeFromAttributesProperty (as it's called at least twice for each overload resolved through JSX), but it could be called in a contextual context (and often is), in which case caching indeterminate results would be bad. I'm not in for putting ad-hoc checks on cache safety at every use-site - I'd rather checkExpressionCached make it impossible to do the wrong thing.

Copy link
Member Author

@weswigham weswigham Oct 12, 2017

Choose a reason for hiding this comment

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

Oh, @ahejlsberg if we want to reduce the cost of allocation of our NodeLinks structure (do we? we totally could), we can replace the "A cache of structures which link nodes to other things" structure with "a collection of caches which maps nodes directly to things". So instead of allocating a NodeLinks for each node we cache something on, we initially allocate a Map for each of the current fields of NodeLinks, and use the nodeid as the key on those directly. No extra object allocations (instead you grow an existing object), just extra references. Probably also update usages to getNodeLink(node, "propName") and setNodeLink(node, "propName"), where getNodeLink is implemented like

function getNodeLink<K extends keyof NodeLinks>(node: Node, prop: K): NodeLinks[K] {
  switch (prop) {
    case "resolvedType":
      return resolvedTypes.get(getNodeId(node));
    // ... and so on
  }
}

with our modern features this is completely typesafe, too. 🐱

@@ -14820,7 +14813,7 @@ namespace ts {
// This will allow excess properties in spread type as it is very common pattern to spread outter attributes into React component in its render method.
if (isSourceAttributeTypeAssignableToTarget && !isTypeAny(sourceAttributesType) && !isTypeAny(targetAttributesType)) {
for (const attribute of openingLikeElement.attributes.properties) {
if (isJsxAttribute(attribute) && !isKnownProperty(targetAttributesType, attribute.name.escapedText, /*isComparingJsxAttributes*/ true)) {
if (isJsxAttribute(attribute) && (isUnhyphenatedJsxName(idText(attribute.name)) || !!(getPropertyOfType(targetAttributesType, attribute.name.escapedText))) && !isKnownProperty(targetAttributesType, attribute.name.escapedText, /*isComparingJsxAttributes*/ true)) {
Copy link
Member

Choose a reason for hiding this comment

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

this line is too long

Copy link
Member

Choose a reason for hiding this comment

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

also, I thought we had a standalone predicate for this. isIgnoredJsxProperty?

Copy link
Member Author

Choose a reason for hiding this comment

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

isIgnoredJsxProperty works on the source type, source prop symbol, and target type (optional) - this uses the attribute name and symbol presence instead. So, I can have this

 const isNotIgnoredJsxProperty = (isUnhyphenatedJsxName(idText(attrName)) || !!(getPropertyOfType(targetAttributesType, attrName.escapedText)));

or this:

 const isNotIgnoredJsxProperty = !isIgnoredJsxProperty(sourceAttributesType, getPropertyOfObjectType(sourceAttributesType, attrName.escapedText), getTypeOfPropertyOfType(targetAttributesType, attrName.escapedText));

@@ -14820,7 +14813,7 @@ namespace ts {
// This will allow excess properties in spread type as it is very common pattern to spread outter attributes into React component in its render method.
Copy link
Member

Choose a reason for hiding this comment

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

typo:outer

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -15727,7 +15720,7 @@ namespace ts {
// However "context" and "updater" are implicit and can't be specify by users. Only the first parameter, props,
// can be specified by users through attributes property.
const paramType = getTypeAtPosition(signature, 0);
const attributesType = checkExpressionWithContextualType(node.attributes, paramType, /*contextualMapper*/ undefined);
const attributesType = checkExpressionWithContextualType(node.attributes, paramType, /* contextualMapper */ undefined);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these spaces belong here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@weswigham weswigham merged commit ae73a91 into master Dec 16, 2017
@weswigham weswigham deleted the unify-type-for-jsx branch December 16, 2017 00:13
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

JSX spread doesn't work with union props
3 participants