Skip to content

More structural comparisons during variance computation #45628

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

More structural comparisons during variance computation #45628

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
73fc830
Fix variance computation logic
ahejlsberg Aug 29, 2021
2d5ba1b
Accept new baselines
ahejlsberg Aug 29, 2021
a1f82c1
Add regression test
ahejlsberg Aug 29, 2021
813b04b
Merge branch 'main' into fix44572
ahejlsberg Aug 29, 2021
82c02a7
Merge branch 'main' into fix44572
ahejlsberg Sep 9, 2021
2e859a2
Merge branch 'main' into fix44572
ahejlsberg Feb 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12901,13 +12901,18 @@ namespace ts {
return result & ObjectFlags.PropagatingFlags;
}

function containsMarkerType(typeArguments: readonly Type[] | undefined) {
return some(typeArguments, t => t === markerSuperType || t === markerSubType || t === markerOtherType);
Copy link
Member

Choose a reason for hiding this comment

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

This check happens a few times - maybe just hoist it out to its own function and call it in reportUnmeasurableMarkers/reportUnreliableMarkers.

}

function createTypeReference(target: GenericType, typeArguments: readonly Type[] | undefined): TypeReference {
const id = getTypeListId(typeArguments);
let type = target.instantiations.get(id);
if (!type) {
type = createObjectType(ObjectFlags.Reference, target.symbol) as TypeReference;
target.instantiations.set(id, type);
type.objectFlags |= typeArguments ? getPropagatingFlagsOfTypes(typeArguments, /*excludeKinds*/ 0) : 0;
type.objectFlags |= (typeArguments ? getPropagatingFlagsOfTypes(typeArguments, /*excludeKinds*/ 0) : 0) |
(containsMarkerType(typeArguments) ? ObjectFlags.MarkerType : 0);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just add MarkerType to PropagatingFlags?

Copy link
Member

Choose a reason for hiding this comment

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

I agree - it was a flag on the type/list before so we could avoid doing any kind of deep checks for markers and catch pretty much the exact type we initially make. It was a known shortcoming that we missed markers that flowed into other versions of the reference we're checking the variance of (known to me, at least - I thought it was an intentional performance tradeoff). This now catches if a Whatever<?, T> has a Whatever<T, ?> dependency (and forces structural comparison, rather than assuming covariance), but still misses a Whatever<{x: ?}, T> dependency. Shouldn't we go all the way and catch if the marker appears in the type argument list in any way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we'd identify any occurrence of a marker type, but it would require eager exploration which we know doesn't end well for many classes of types. Our variance measurement is effectively a balance between correctness and performance, and here we're moving a little further towards correctness. I know we've tried doing only structural comparisons when computing variance, but it isn't feasible. It just reveals the same problems with (almost) infinite types that we're trying to solve by doing variance measurement in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add MarkerType to PropagatingFlags, but it wouldn't save us much and would require us to also include TypeFlags.TypeParameter in TypeFlags.ObjectFlagsType, which would mean an extra property on every instance that we otherwise don't need.

type.target = target;
type.resolvedTypeArguments = typeArguments;
}
Expand Down Expand Up @@ -17390,7 +17395,9 @@ namespace ts {
if (source.flags !== target.flags) return false;
if (source.flags & TypeFlags.Singleton) return true;
}
if (source.flags & TypeFlags.Object && target.flags & TypeFlags.Object) {
// We skip the cache lookup shortcut when we're in a variance computation so the outofbandVarianceMarkerHandler
// will get called for cached results that are unreliable or unmeasurable.
if (source.flags & TypeFlags.Object && target.flags & TypeFlags.Object && !outofbandVarianceMarkerHandler) {
const related = relation.get(getRelationKey(source, target, IntersectionState.None, relation));
if (related !== undefined) {
return !!(related & RelationComparisonResult.Succeeded);
Expand Down Expand Up @@ -18478,7 +18485,7 @@ namespace ts {
// the order in which things were checked.
if (source.flags & (TypeFlags.Object | TypeFlags.Conditional) && source.aliasSymbol &&
source.aliasTypeArguments && source.aliasSymbol === target.aliasSymbol &&
!(source.aliasTypeArgumentsContainsMarker || target.aliasTypeArgumentsContainsMarker)) {
!(containsMarkerType(source.aliasTypeArguments) || containsMarkerType(target.aliasTypeArguments))) {
const variances = getAliasVariances(source.aliasSymbol);
if (variances === emptyArray) {
return Ternary.Unknown;
Expand Down Expand Up @@ -19713,18 +19720,14 @@ namespace ts {
// Return a type reference where the source type parameter is replaced with the target marker
// type, and flag the result as a marker type reference.
function getMarkerTypeReference(type: GenericType, source: TypeParameter, target: Type) {
const result = createTypeReference(type, map(type.typeParameters, t => t === source ? target : t));
result.objectFlags |= ObjectFlags.MarkerType;
return result;
return createTypeReference(type, map(type.typeParameters, t => t === source ? target : t));
}

function getAliasVariances(symbol: Symbol) {
const links = getSymbolLinks(symbol);
return getVariancesWorker(links.typeParameters, links, (_links, param, marker) => {
const type = getTypeAliasInstantiation(symbol, instantiateTypes(links.typeParameters!, makeUnaryTypeMapper(param, marker)));
type.aliasTypeArgumentsContainsMarker = true;
return type;
});
return getVariancesWorker(links.typeParameters, links, (_links, param, marker) =>
getTypeAliasInstantiation(symbol, instantiateTypes(links.typeParameters!, makeUnaryTypeMapper(param, marker)))
);
}

// Return an array containing the variance of each type parameter. The variance is effectively
Expand Down
1 change: 0 additions & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5155,7 +5155,6 @@ namespace ts {
pattern?: DestructuringPattern; // Destructuring pattern represented by type (if any)
aliasSymbol?: Symbol; // Alias associated with type
aliasTypeArguments?: readonly Type[]; // Alias type arguments (if any)
/* @internal */ aliasTypeArgumentsContainsMarker?: boolean; // Alias type arguments (if any)
/* @internal */
permissiveInstantiation?: Type; // Instantiation with type parameters mapped to wildcard type
/* @internal */
Expand Down
65 changes: 65 additions & 0 deletions tests/baselines/reference/checkOrderDependenceGenericAssignability.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
tests/cases/compiler/checkOrderDependenceGenericAssignability.ts(24,1): error TS2322: Type 'Parent1<unknown>' is not assignable to type 'Parent1<string>'.
Type 'unknown' is not assignable to type 'string'.
tests/cases/compiler/checkOrderDependenceGenericAssignability.ts(51,1): error TS2322: Type 'Parent2<unknown>' is not assignable to type 'Parent2<string>'.
Type 'unknown' is not assignable to type 'string'.


==== tests/cases/compiler/checkOrderDependenceGenericAssignability.ts (2 errors) ====
// Repro from #44572 with interface types

interface Parent1<A> {
child: Child1<A>;
parent: Parent1<A>;
}

interface Child1<A, B = unknown> extends Parent1<A> {
readonly a: A;
// This field isn't necessary to the repro, but the
// type parameter is, so including it
readonly b: B;
}

function fn1<A>(inp: Child1<A>) {
// This assignability check defeats the later one
const a: Child1<unknown> = inp;
}

declare let pu1: Parent1<unknown>;
declare let ps1: Parent1<string>;

pu1 = ps1; // Ok
ps1 = pu1; // Error expected
~~~
!!! error TS2322: Type 'Parent1<unknown>' is not assignable to type 'Parent1<string>'.
!!! error TS2322: Type 'unknown' is not assignable to type 'string'.

// Repro from #44572 with aliased object types

type Parent2<A> = {
child: Child2<A>;
parent: Parent2<A>;
}

type Child2<A, B = unknown> = {
child: Child2<A>;
parent: Parent2<A>;
readonly a: A;
// This field isn't necessary to the repro, but the
// type parameter is, so including it
readonly b: B;
}

function fn2<A>(inp: Child2<A>) {
// This assignability check defeats the later one
const a: Child2<unknown> = inp;
}

declare let pu2: Parent2<unknown>;
declare let ps2: Parent2<string>;

pu2 = ps2; // Ok
ps2 = pu2; // Error expected
~~~
!!! error TS2322: Type 'Parent2<unknown>' is not assignable to type 'Parent2<string>'.
!!! error TS2322: Type 'unknown' is not assignable to type 'string'.

69 changes: 69 additions & 0 deletions tests/baselines/reference/checkOrderDependenceGenericAssignability.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
//// [checkOrderDependenceGenericAssignability.ts]
// Repro from #44572 with interface types

interface Parent1<A> {
child: Child1<A>;
parent: Parent1<A>;
}

interface Child1<A, B = unknown> extends Parent1<A> {
readonly a: A;
// This field isn't necessary to the repro, but the
// type parameter is, so including it
readonly b: B;
}

function fn1<A>(inp: Child1<A>) {
// This assignability check defeats the later one
const a: Child1<unknown> = inp;
}

declare let pu1: Parent1<unknown>;
declare let ps1: Parent1<string>;

pu1 = ps1; // Ok
ps1 = pu1; // Error expected

// Repro from #44572 with aliased object types

type Parent2<A> = {
child: Child2<A>;
parent: Parent2<A>;
}

type Child2<A, B = unknown> = {
child: Child2<A>;
parent: Parent2<A>;
readonly a: A;
// This field isn't necessary to the repro, but the
// type parameter is, so including it
readonly b: B;
}

function fn2<A>(inp: Child2<A>) {
// This assignability check defeats the later one
const a: Child2<unknown> = inp;
}

declare let pu2: Parent2<unknown>;
declare let ps2: Parent2<string>;

pu2 = ps2; // Ok
ps2 = pu2; // Error expected


//// [checkOrderDependenceGenericAssignability.js]
"use strict";
// Repro from #44572 with interface types
function fn1(inp) {
// This assignability check defeats the later one
var a = inp;
}
pu1 = ps1; // Ok
ps1 = pu1; // Error expected
function fn2(inp) {
// This assignability check defeats the later one
var a = inp;
}
pu2 = ps2; // Ok
ps2 = pu2; // Error expected
139 changes: 139 additions & 0 deletions tests/baselines/reference/checkOrderDependenceGenericAssignability.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
=== tests/cases/compiler/checkOrderDependenceGenericAssignability.ts ===
// Repro from #44572 with interface types

interface Parent1<A> {
>Parent1 : Symbol(Parent1, Decl(checkOrderDependenceGenericAssignability.ts, 0, 0))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 2, 18))

child: Child1<A>;
>child : Symbol(Parent1.child, Decl(checkOrderDependenceGenericAssignability.ts, 2, 22))
>Child1 : Symbol(Child1, Decl(checkOrderDependenceGenericAssignability.ts, 5, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 2, 18))

parent: Parent1<A>;
>parent : Symbol(Parent1.parent, Decl(checkOrderDependenceGenericAssignability.ts, 3, 21))
>Parent1 : Symbol(Parent1, Decl(checkOrderDependenceGenericAssignability.ts, 0, 0))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 2, 18))
}

interface Child1<A, B = unknown> extends Parent1<A> {
>Child1 : Symbol(Child1, Decl(checkOrderDependenceGenericAssignability.ts, 5, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 7, 17))
>B : Symbol(B, Decl(checkOrderDependenceGenericAssignability.ts, 7, 19))
>Parent1 : Symbol(Parent1, Decl(checkOrderDependenceGenericAssignability.ts, 0, 0))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 7, 17))

readonly a: A;
>a : Symbol(Child1.a, Decl(checkOrderDependenceGenericAssignability.ts, 7, 53))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 7, 17))

// This field isn't necessary to the repro, but the
// type parameter is, so including it
readonly b: B;
>b : Symbol(Child1.b, Decl(checkOrderDependenceGenericAssignability.ts, 8, 18))
>B : Symbol(B, Decl(checkOrderDependenceGenericAssignability.ts, 7, 19))
}

function fn1<A>(inp: Child1<A>) {
>fn1 : Symbol(fn1, Decl(checkOrderDependenceGenericAssignability.ts, 12, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 14, 13))
>inp : Symbol(inp, Decl(checkOrderDependenceGenericAssignability.ts, 14, 16))
>Child1 : Symbol(Child1, Decl(checkOrderDependenceGenericAssignability.ts, 5, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 14, 13))

// This assignability check defeats the later one
const a: Child1<unknown> = inp;
>a : Symbol(a, Decl(checkOrderDependenceGenericAssignability.ts, 16, 9))
>Child1 : Symbol(Child1, Decl(checkOrderDependenceGenericAssignability.ts, 5, 1))
>inp : Symbol(inp, Decl(checkOrderDependenceGenericAssignability.ts, 14, 16))
}

declare let pu1: Parent1<unknown>;
>pu1 : Symbol(pu1, Decl(checkOrderDependenceGenericAssignability.ts, 19, 11))
>Parent1 : Symbol(Parent1, Decl(checkOrderDependenceGenericAssignability.ts, 0, 0))

declare let ps1: Parent1<string>;
>ps1 : Symbol(ps1, Decl(checkOrderDependenceGenericAssignability.ts, 20, 11))
>Parent1 : Symbol(Parent1, Decl(checkOrderDependenceGenericAssignability.ts, 0, 0))

pu1 = ps1; // Ok
>pu1 : Symbol(pu1, Decl(checkOrderDependenceGenericAssignability.ts, 19, 11))
>ps1 : Symbol(ps1, Decl(checkOrderDependenceGenericAssignability.ts, 20, 11))

ps1 = pu1; // Error expected
>ps1 : Symbol(ps1, Decl(checkOrderDependenceGenericAssignability.ts, 20, 11))
>pu1 : Symbol(pu1, Decl(checkOrderDependenceGenericAssignability.ts, 19, 11))

// Repro from #44572 with aliased object types

type Parent2<A> = {
>Parent2 : Symbol(Parent2, Decl(checkOrderDependenceGenericAssignability.ts, 23, 10))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 27, 13))

child: Child2<A>;
>child : Symbol(child, Decl(checkOrderDependenceGenericAssignability.ts, 27, 19))
>Child2 : Symbol(Child2, Decl(checkOrderDependenceGenericAssignability.ts, 30, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 27, 13))

parent: Parent2<A>;
>parent : Symbol(parent, Decl(checkOrderDependenceGenericAssignability.ts, 28, 21))
>Parent2 : Symbol(Parent2, Decl(checkOrderDependenceGenericAssignability.ts, 23, 10))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 27, 13))
}

type Child2<A, B = unknown> = {
>Child2 : Symbol(Child2, Decl(checkOrderDependenceGenericAssignability.ts, 30, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 32, 12))
>B : Symbol(B, Decl(checkOrderDependenceGenericAssignability.ts, 32, 14))

child: Child2<A>;
>child : Symbol(child, Decl(checkOrderDependenceGenericAssignability.ts, 32, 31))
>Child2 : Symbol(Child2, Decl(checkOrderDependenceGenericAssignability.ts, 30, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 32, 12))

parent: Parent2<A>;
>parent : Symbol(parent, Decl(checkOrderDependenceGenericAssignability.ts, 33, 21))
>Parent2 : Symbol(Parent2, Decl(checkOrderDependenceGenericAssignability.ts, 23, 10))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 32, 12))

readonly a: A;
>a : Symbol(a, Decl(checkOrderDependenceGenericAssignability.ts, 34, 23))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 32, 12))

// This field isn't necessary to the repro, but the
// type parameter is, so including it
readonly b: B;
>b : Symbol(b, Decl(checkOrderDependenceGenericAssignability.ts, 35, 18))
>B : Symbol(B, Decl(checkOrderDependenceGenericAssignability.ts, 32, 14))
}

function fn2<A>(inp: Child2<A>) {
>fn2 : Symbol(fn2, Decl(checkOrderDependenceGenericAssignability.ts, 39, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 41, 13))
>inp : Symbol(inp, Decl(checkOrderDependenceGenericAssignability.ts, 41, 16))
>Child2 : Symbol(Child2, Decl(checkOrderDependenceGenericAssignability.ts, 30, 1))
>A : Symbol(A, Decl(checkOrderDependenceGenericAssignability.ts, 41, 13))

// This assignability check defeats the later one
const a: Child2<unknown> = inp;
>a : Symbol(a, Decl(checkOrderDependenceGenericAssignability.ts, 43, 9))
>Child2 : Symbol(Child2, Decl(checkOrderDependenceGenericAssignability.ts, 30, 1))
>inp : Symbol(inp, Decl(checkOrderDependenceGenericAssignability.ts, 41, 16))
}

declare let pu2: Parent2<unknown>;
>pu2 : Symbol(pu2, Decl(checkOrderDependenceGenericAssignability.ts, 46, 11))
>Parent2 : Symbol(Parent2, Decl(checkOrderDependenceGenericAssignability.ts, 23, 10))

declare let ps2: Parent2<string>;
>ps2 : Symbol(ps2, Decl(checkOrderDependenceGenericAssignability.ts, 47, 11))
>Parent2 : Symbol(Parent2, Decl(checkOrderDependenceGenericAssignability.ts, 23, 10))

pu2 = ps2; // Ok
>pu2 : Symbol(pu2, Decl(checkOrderDependenceGenericAssignability.ts, 46, 11))
>ps2 : Symbol(ps2, Decl(checkOrderDependenceGenericAssignability.ts, 47, 11))

ps2 = pu2; // Error expected
>ps2 : Symbol(ps2, Decl(checkOrderDependenceGenericAssignability.ts, 47, 11))
>pu2 : Symbol(pu2, Decl(checkOrderDependenceGenericAssignability.ts, 46, 11))

Loading