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.

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

Always discriminate contextual types by existing discriminant property #61097

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Always discriminate contextual types by existing discriminant property #61097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 4 additions & 11 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32029,16 +32029,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
concatenate(
map(
filter(node.properties, (p): p is PropertyAssignment | ShorthandPropertyAssignment => {
if (!p.symbol) {
return false;
}
if (p.kind === SyntaxKind.PropertyAssignment) {
return isPossiblyDiscriminantValue(p.initializer) && isDiscriminantProperty(contextualType, p.symbol.escapedName);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it's at least moderately concerning that this leaves the code with only one use of isPossiblyDiscriminantValue; shouldn't it be the case that if isPossiblyDiscriminantValue returns false, isDiscriminantProperty should have also returned false?

The documentation for isPossiblyDiscriminantValue implies that it's there to avoid circularities, so I'm surprised that it is a problem, and that we aren't somehow introducing circularities by not calling it anymore...

But even more surprisingly, if you just straight up delete isPossiblyDiscriminantValue and its one use (so, make it so that it always returns true), no test fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation for isPossiblyDiscriminantValue implies that it's there to avoid circularities, so I'm surprised that it is a problem, and that we aren't somehow introducing circularities by not calling it anymore...

It may not be about circularities in the form of implicit any. One of the failures here is related to a code like this:

interface KnockoutObservable<T> {
  (value: T): void;
}

interface KnockoutObservableStatic {
  <T>(value: T): KnockoutObservable<T>;
}

interface KnockoutObservableArrayStatic {
  <T>(value: T[]): KnockoutObservable<T>;
}

interface KnockoutStatic {
  observable: KnockoutObservableStatic;
  observableArray: KnockoutObservableArrayStatic;
}
declare var ko: KnockoutStatic;

ko.observableArray([
  { someProp: 1 },
  { someProp: 2, _destroy: "evals to true" },
  { someProp: 3 },
  { someProp: 4, _destroy: ko.observable(false) },
]);

At the moment, the inner ko.observable has a resolving signature - that is temporarily set on it during inference. Requesting its type here changes that prematurely or smth like that. I haven't yet had time to investigate this further to see what I could do to mitigate this problem though.

}
if (p.kind === SyntaxKind.ShorthandPropertyAssignment) {
return isDiscriminantProperty(contextualType, p.symbol.escapedName);
}
return false;
return !!p.symbol
&& (p.kind === SyntaxKind.PropertyAssignment || p.kind === SyntaxKind.ShorthandPropertyAssignment)
&& isDiscriminantProperty(contextualType, p.symbol.escapedName);
}),
prop => ([() => getContextFreeTypeOfExpression(prop.kind === SyntaxKind.PropertyAssignment ? prop.initializer : prop.name), prop.symbol.escapedName] as const),
),
Expand All @@ -32063,7 +32056,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
contextualType,
concatenate(
map(
filter(node.properties, p => !!p.symbol && p.kind === SyntaxKind.JsxAttribute && isDiscriminantProperty(contextualType, p.symbol.escapedName) && (!p.initializer || isPossiblyDiscriminantValue(p.initializer))),
filter(node.properties, p => !!p.symbol && p.kind === SyntaxKind.JsxAttribute && isDiscriminantProperty(contextualType, p.symbol.escapedName)),
prop => ([!(prop as JsxAttribute).initializer ? (() => trueType) : (() => getContextFreeTypeOfExpression((prop as JsxAttribute).initializer!)), prop.symbol.escapedName] as const),
),
map(
Expand Down
95 changes: 95 additions & 0 deletions tests/baselines/reference/contextuallyTypedByDiscriminableUnion2.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
//// [tests/cases/compiler/contextuallyTypedByDiscriminableUnion2.ts] ////

=== contextuallyTypedByDiscriminableUnion2.ts ===
type Props =
>Props : Symbol(Props, Decl(contextuallyTypedByDiscriminableUnion2.ts, 0, 0))

| {
parentId: string[];
>parentId : Symbol(parentId, Decl(contextuallyTypedByDiscriminableUnion2.ts, 1, 5))

onChange: (event: { id: string }) => void;
>onChange : Symbol(onChange, Decl(contextuallyTypedByDiscriminableUnion2.ts, 2, 25))
>event : Symbol(event, Decl(contextuallyTypedByDiscriminableUnion2.ts, 3, 17))
>id : Symbol(id, Decl(contextuallyTypedByDiscriminableUnion2.ts, 3, 25))

onChange2: () => void;
>onChange2 : Symbol(onChange2, Decl(contextuallyTypedByDiscriminableUnion2.ts, 3, 48))
}
| {
parentId?: never;
>parentId : Symbol(parentId, Decl(contextuallyTypedByDiscriminableUnion2.ts, 6, 5))

onChange: (event: { id: number }) => void;
>onChange : Symbol(onChange, Decl(contextuallyTypedByDiscriminableUnion2.ts, 7, 23))
>event : Symbol(event, Decl(contextuallyTypedByDiscriminableUnion2.ts, 8, 17))
>id : Symbol(id, Decl(contextuallyTypedByDiscriminableUnion2.ts, 8, 25))

};

function NonGenericComponent(props: Props) {
>NonGenericComponent : Symbol(NonGenericComponent, Decl(contextuallyTypedByDiscriminableUnion2.ts, 9, 6))
>props : Symbol(props, Decl(contextuallyTypedByDiscriminableUnion2.ts, 11, 29))
>Props : Symbol(Props, Decl(contextuallyTypedByDiscriminableUnion2.ts, 0, 0))

return null;
}

NonGenericComponent({
>NonGenericComponent : Symbol(NonGenericComponent, Decl(contextuallyTypedByDiscriminableUnion2.ts, 9, 6))

onChange: (e) => {},
>onChange : Symbol(onChange, Decl(contextuallyTypedByDiscriminableUnion2.ts, 15, 21))
>e : Symbol(e, Decl(contextuallyTypedByDiscriminableUnion2.ts, 16, 13))

});

const parentId: string[] = [];
>parentId : Symbol(parentId, Decl(contextuallyTypedByDiscriminableUnion2.ts, 19, 5))

NonGenericComponent({
>NonGenericComponent : Symbol(NonGenericComponent, Decl(contextuallyTypedByDiscriminableUnion2.ts, 9, 6))

parentId,
>parentId : Symbol(parentId, Decl(contextuallyTypedByDiscriminableUnion2.ts, 21, 21))

onChange: (e) => {},
>onChange : Symbol(onChange, Decl(contextuallyTypedByDiscriminableUnion2.ts, 22, 11))
>e : Symbol(e, Decl(contextuallyTypedByDiscriminableUnion2.ts, 23, 13))

onChange2: () => {},
>onChange2 : Symbol(onChange2, Decl(contextuallyTypedByDiscriminableUnion2.ts, 23, 22))

});

NonGenericComponent({
>NonGenericComponent : Symbol(NonGenericComponent, Decl(contextuallyTypedByDiscriminableUnion2.ts, 9, 6))

parentId: parentId,
>parentId : Symbol(parentId, Decl(contextuallyTypedByDiscriminableUnion2.ts, 27, 21))
>parentId : Symbol(parentId, Decl(contextuallyTypedByDiscriminableUnion2.ts, 19, 5))

onChange: (e) => {},
>onChange : Symbol(onChange, Decl(contextuallyTypedByDiscriminableUnion2.ts, 28, 21))
>e : Symbol(e, Decl(contextuallyTypedByDiscriminableUnion2.ts, 29, 13))

onChange2: () => {},
>onChange2 : Symbol(onChange2, Decl(contextuallyTypedByDiscriminableUnion2.ts, 29, 22))

});

NonGenericComponent({
>NonGenericComponent : Symbol(NonGenericComponent, Decl(contextuallyTypedByDiscriminableUnion2.ts, 9, 6))

parentId: [],
>parentId : Symbol(parentId, Decl(contextuallyTypedByDiscriminableUnion2.ts, 33, 21))

onChange: (e) => {},
>onChange : Symbol(onChange, Decl(contextuallyTypedByDiscriminableUnion2.ts, 34, 15))
>e : Symbol(e, Decl(contextuallyTypedByDiscriminableUnion2.ts, 35, 13))

onChange2: () => {},
>onChange2 : Symbol(onChange2, Decl(contextuallyTypedByDiscriminableUnion2.ts, 35, 22))

});

160 changes: 160 additions & 0 deletions tests/baselines/reference/contextuallyTypedByDiscriminableUnion2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
//// [tests/cases/compiler/contextuallyTypedByDiscriminableUnion2.ts] ////

=== contextuallyTypedByDiscriminableUnion2.ts ===
type Props =
>Props : Props
> : ^^^^^

| {
parentId: string[];
>parentId : string[]
> : ^^^^^^^^

onChange: (event: { id: string }) => void;
>onChange : (event: { id: string; }) => void
> : ^ ^^ ^^^^^
>event : { id: string; }
> : ^^^^^^ ^^^
>id : string
> : ^^^^^^

onChange2: () => void;
>onChange2 : () => void
> : ^^^^^^
}
| {
parentId?: never;
>parentId : undefined
> : ^^^^^^^^^

onChange: (event: { id: number }) => void;
>onChange : (event: { id: number; }) => void
> : ^ ^^ ^^^^^
>event : { id: number; }
> : ^^^^^^ ^^^
>id : number
> : ^^^^^^

};

function NonGenericComponent(props: Props) {
>NonGenericComponent : (props: Props) => null
> : ^ ^^ ^^^^^^^^^
>props : Props
> : ^^^^^

return null;
}

NonGenericComponent({
>NonGenericComponent({ onChange: (e) => {},}) : null
> : ^^^^
>NonGenericComponent : (props: Props) => null
> : ^ ^^ ^^^^^^^^^
>{ onChange: (e) => {},} : { onChange: (e: { id: number; }) => void; }
> : ^^^^^^^^^^^^^ ^^^^^^^^ ^^^^^^^^^^^^^^^

onChange: (e) => {},
>onChange : (e: { id: number; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>(e) => {} : (e: { id: number; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>e : { id: number; }
> : ^^^^^^ ^^^

});

const parentId: string[] = [];
>parentId : string[]
> : ^^^^^^^^
>[] : never[]
> : ^^^^^^^

NonGenericComponent({
>NonGenericComponent({ parentId, onChange: (e) => {}, onChange2: () => {},}) : null
> : ^^^^
>NonGenericComponent : (props: Props) => null
> : ^ ^^ ^^^^^^^^^
>{ parentId, onChange: (e) => {}, onChange2: () => {},} : { parentId: string[]; onChange: (e: { id: string; }) => void; onChange2: () => void; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

parentId,
>parentId : string[]
> : ^^^^^^^^

onChange: (e) => {},
>onChange : (e: { id: string; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>(e) => {} : (e: { id: string; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>e : { id: string; }
> : ^^^^^^ ^^^

onChange2: () => {},
>onChange2 : () => void
> : ^^^^^^^^^^
>() => {} : () => void
> : ^^^^^^^^^^

});

NonGenericComponent({
>NonGenericComponent({ parentId: parentId, onChange: (e) => {}, onChange2: () => {},}) : null
> : ^^^^
>NonGenericComponent : (props: Props) => null
> : ^ ^^ ^^^^^^^^^
>{ parentId: parentId, onChange: (e) => {}, onChange2: () => {},} : { parentId: string[]; onChange: (e: { id: string; }) => void; onChange2: () => void; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

parentId: parentId,
>parentId : string[]
> : ^^^^^^^^
>parentId : string[]
> : ^^^^^^^^

onChange: (e) => {},
>onChange : (e: { id: string; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>(e) => {} : (e: { id: string; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>e : { id: string; }
> : ^^^^^^ ^^^

onChange2: () => {},
>onChange2 : () => void
> : ^^^^^^^^^^
>() => {} : () => void
> : ^^^^^^^^^^

});

NonGenericComponent({
>NonGenericComponent({ parentId: [], onChange: (e) => {}, onChange2: () => {},}) : null
> : ^^^^
>NonGenericComponent : (props: Props) => null
> : ^ ^^ ^^^^^^^^^
>{ parentId: [], onChange: (e) => {}, onChange2: () => {},} : { parentId: never[]; onChange: (e: { id: string; }) => void; onChange2: () => void; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

parentId: [],
>parentId : never[]
> : ^^^^^^^
>[] : never[]
> : ^^^^^^^

onChange: (e) => {},
>onChange : (e: { id: string; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>(e) => {} : (e: { id: string; }) => void
> : ^ ^^^^^^^^ ^^^^^^^^^^^^
>e : { id: string; }
> : ^^^^^^ ^^^

onChange2: () => {},
>onChange2 : () => void
> : ^^^^^^^^^^
>() => {} : () => void
> : ^^^^^^^^^^

});

96 changes: 96 additions & 0 deletions tests/baselines/reference/contextuallyTypedByDiscriminableUnion3.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
//// [tests/cases/compiler/contextuallyTypedByDiscriminableUnion3.ts] ////

=== contextuallyTypedByDiscriminableUnion3.ts ===
// https://github.com/microsoft/TypeScript/issues/58508

type PathSegment = object[];
>PathSegment : Symbol(PathSegment, Decl(contextuallyTypedByDiscriminableUnion3.ts, 0, 0))

type Handle<TData> = {
>Handle : Symbol(Handle, Decl(contextuallyTypedByDiscriminableUnion3.ts, 2, 28))
>TData : Symbol(TData, Decl(contextuallyTypedByDiscriminableUnion3.ts, 4, 12))

crumbBuilder: (data: TData) => PathSegment[];
>crumbBuilder : Symbol(crumbBuilder, Decl(contextuallyTypedByDiscriminableUnion3.ts, 4, 22))
>data : Symbol(data, Decl(contextuallyTypedByDiscriminableUnion3.ts, 5, 17))
>TData : Symbol(TData, Decl(contextuallyTypedByDiscriminableUnion3.ts, 4, 12))
>PathSegment : Symbol(PathSegment, Decl(contextuallyTypedByDiscriminableUnion3.ts, 0, 0))

};

type Loader<TData> = (args: {
>Loader : Symbol(Loader, Decl(contextuallyTypedByDiscriminableUnion3.ts, 6, 2))
>TData : Symbol(TData, Decl(contextuallyTypedByDiscriminableUnion3.ts, 8, 12))
>args : Symbol(args, Decl(contextuallyTypedByDiscriminableUnion3.ts, 8, 22))

params: Record<string, string>;
>params : Symbol(params, Decl(contextuallyTypedByDiscriminableUnion3.ts, 8, 29))
>Record : Symbol(Record, Decl(lib.es5.d.ts, --, --))

}) => Promise<TData>;
>Promise : Symbol(Promise, Decl(lib.es5.d.ts, --, --))
>TData : Symbol(TData, Decl(contextuallyTypedByDiscriminableUnion3.ts, 8, 12))

type RouteHandler<TData = any> =
>RouteHandler : Symbol(RouteHandler, Decl(contextuallyTypedByDiscriminableUnion3.ts, 10, 21))
>TData : Symbol(TData, Decl(contextuallyTypedByDiscriminableUnion3.ts, 12, 18))

| {
handle: Handle<never>;
>handle : Symbol(handle, Decl(contextuallyTypedByDiscriminableUnion3.ts, 13, 5))
>Handle : Symbol(Handle, Decl(contextuallyTypedByDiscriminableUnion3.ts, 2, 28))

loader?: never;
>loader : Symbol(loader, Decl(contextuallyTypedByDiscriminableUnion3.ts, 14, 28))
}
| {
handle: Handle<TData>;
>handle : Symbol(handle, Decl(contextuallyTypedByDiscriminableUnion3.ts, 17, 5))
>Handle : Symbol(Handle, Decl(contextuallyTypedByDiscriminableUnion3.ts, 2, 28))
>TData : Symbol(TData, Decl(contextuallyTypedByDiscriminableUnion3.ts, 12, 18))

loader: Loader<TData>;
>loader : Symbol(loader, Decl(contextuallyTypedByDiscriminableUnion3.ts, 18, 28))
>Loader : Symbol(Loader, Decl(contextuallyTypedByDiscriminableUnion3.ts, 6, 2))
>TData : Symbol(TData, Decl(contextuallyTypedByDiscriminableUnion3.ts, 12, 18))

};

const routeHandlerWithoutLoader = {
>routeHandlerWithoutLoader : Symbol(routeHandlerWithoutLoader, Decl(contextuallyTypedByDiscriminableUnion3.ts, 22, 5))

handle: {
>handle : Symbol(handle, Decl(contextuallyTypedByDiscriminableUnion3.ts, 22, 35))

crumbBuilder: (data) => [],
>crumbBuilder : Symbol(crumbBuilder, Decl(contextuallyTypedByDiscriminableUnion3.ts, 23, 11))
>data : Symbol(data, Decl(contextuallyTypedByDiscriminableUnion3.ts, 24, 19))

},
} satisfies RouteHandler;
>RouteHandler : Symbol(RouteHandler, Decl(contextuallyTypedByDiscriminableUnion3.ts, 10, 21))

const routeHandler = {
>routeHandler : Symbol(routeHandler, Decl(contextuallyTypedByDiscriminableUnion3.ts, 28, 5))

loader: async (args) => {
>loader : Symbol(loader, Decl(contextuallyTypedByDiscriminableUnion3.ts, 28, 22))
>args : Symbol(args, Decl(contextuallyTypedByDiscriminableUnion3.ts, 29, 17))

return args.params.userId;
>args.params : Symbol(params, Decl(contextuallyTypedByDiscriminableUnion3.ts, 8, 29))
>args : Symbol(args, Decl(contextuallyTypedByDiscriminableUnion3.ts, 29, 17))
>params : Symbol(params, Decl(contextuallyTypedByDiscriminableUnion3.ts, 8, 29))

},
handle: {
>handle : Symbol(handle, Decl(contextuallyTypedByDiscriminableUnion3.ts, 31, 4))

crumbBuilder: (data) => [],
>crumbBuilder : Symbol(crumbBuilder, Decl(contextuallyTypedByDiscriminableUnion3.ts, 32, 11))
>data : Symbol(data, Decl(contextuallyTypedByDiscriminableUnion3.ts, 33, 19))

},
} satisfies RouteHandler<string>;
>RouteHandler : Symbol(RouteHandler, Decl(contextuallyTypedByDiscriminableUnion3.ts, 10, 21))

Loading