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

Measure variance of aliased conditional types using variance markers #27804

Merged

Conversation

weswigham
Copy link
Member

Fixes #26681

We do this for interface references already, and extending it to type aliases wasn't too difficult. Doing this provides an easy fast-path bailout when comparing different instantiations of the same (type-aliased) conditional.

This PR has no impact on either RWC or user suite baselines, I checked 😄

@ahejlsberg
Copy link
Member

Interesting that it has no effect on test and RWC baselines. However, I'm definitely concerned about the fact that the presence or absence of alias symbols now has a semantic effect. As I mentioned, in a worst case scenario, your code might stop compiling just because interned type (such as a unit type) was materialized without an alias symbol instead of with an alias symbol.

@weswigham
Copy link
Member Author

I'm definitely concerned about the fact that the presence or absence of alias symbols now has a semantic effect.

It already affects inference, which in turn affects assignability via infer types, so it's not quite the first time. Tbqh, it's honestly strange that we're OK with it for inference but don't bother with it for relationship checking, since the two usually mirror one another in structure.

@weswigham
Copy link
Member Author

As I mentioned, in a worst case scenario, your code might stop compiling just because interned type (such as a unit type) was materialized without an alias symbol instead of with an alias symbol.

I'm only checking for alias symbols on conditional types for now, so this will not be the case. Unless we start interning conditionals or something.

@weswigham
Copy link
Member Author

weswigham commented Oct 11, 2018

For the record: The reason this fixes the linked issue is that when we're computing variances, we have a circularity guard that gets triggered while computing weather NameFieldConstructor<SO_FAR & {maxMsToWaitBeforePublishing: number}> is assignable to NameFieldConstructor<SO_FAR & {maxMsToWaitBeforePublishing: number}> given a different copy of the SO_FAR type variable. Specifically, the variance of T in NameFieldConstructor<T> requires computing variance of NameFieldConstructor<T> (actually prompted by a comparison of the same pair of types, but that's unimportant), so we assume the type variable is covariant (there's a comment indicating that this is the desired behavior on detection of a circularity). That lets us easily bail out, walk back up, and easily determine that, in fact, SO_FAR (copy) is subtype of SO_FAR (v1) (since v1 is the constraint of the copy, somehow), and so it's assignable. Without this we (apparently) spin out of control, creating copies of copies of copies of the type variables of signatures, never being able to relate them back to the concrete types they originally began as.

@weswigham
Copy link
Member Author

@ahejlsberg like we spoke about in person, I've swapped to just do variance probing for all type aliases - this doesn't seem to adversely impact any RWC tests at all. I'll try it out on DT; but that's going to take a wee bit to run.

@weswigham
Copy link
Member Author

@ahejlsberg This PR causes no change to RWC, user, or DT baselines. By all the things we can test, this optimization has no effect on any code we know of (except, ofc, the new test case in this PR that it fixes!). 😄

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

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

Approved with fixes.


function getAliasVariances(symbol: Symbol) {
const links = getSymbolLinks(symbol);
return getVariancesWorker(links.typeParameters, links, (_links, param, marker) => getTypeAliasInstantiation(symbol, instantiateTypes(links.typeParameters!, makeUnaryTypeMapper(param, marker))));
Copy link
Member

Choose a reason for hiding this comment

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

Just set the marker boolean here.

return type === markerSuperType || type === markerSubType || type === markerOtherType;
}

function getAliasArgumentsContainsMarker(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.

Can get rid of this function if you eagerly set the marker property.

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

Successfully merging this pull request may close these issues.

2 participants