-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Let properties having primitive types discriminate object unions #60718
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
base: main
Are you sure you want to change the base?
Changes from all commits
7f3d3fb
1bc0a55
48e86a7
dc0866f
8542a99
2b69230
6959a73
a4c22c2
fd99a91
6f6d928
3c9b24b
086bbe4
f46d0dd
8e0cf94
e8741f2
443320a
2e87e81
81b5d4a
6c0d085
5566306
602ca72
ae873d4
21feb28
d401d2e
868d5dc
addeec3
c846958
981e7ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15050,6 +15050,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
if (isLiteralType(type) || isPatternLiteralType(type)) { | ||
checkFlags |= CheckFlags.HasLiteralType; | ||
} | ||
if (type.flags & TypeFlags.Primitive) { | ||
checkFlags |= CheckFlags.HasPrimitiveType; | ||
} | ||
if (type.flags & TypeFlags.Never && type !== uniqueLiteralType) { | ||
checkFlags |= CheckFlags.HasNeverType; | ||
} | ||
|
@@ -27281,16 +27284,29 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
return false; | ||
} | ||
|
||
function isDiscriminantProperty(type: Type | undefined, name: __String) { | ||
// The flag `considerNonUniformPrimitivePropDiscriminant`, when enabled, introduces a new criterion for a property to be considered discriminant: | ||
// 1 The property must be non-uniform | ||
// 2 The property must include at least one primitive type as a possible type | ||
function isDiscriminantProperty(type: Type | undefined, name: __String, considerNonUniformPrimitivePropDiscriminant: boolean = false) { | ||
if (type && type.flags & TypeFlags.Union) { | ||
const prop = getUnionOrIntersectionProperty(type as UnionType, name); | ||
if (prop && getCheckFlags(prop) & CheckFlags.SyntheticProperty) { | ||
const propType = getTypeOfSymbol(prop); | ||
// NOTE: cast to TransientSymbol should be safe because only TransientSymbols can have CheckFlags.SyntheticProperty | ||
if ((prop as TransientSymbol).links.isDiscriminantProperty === undefined) { | ||
(prop as TransientSymbol).links.isDiscriminantProperty = ((prop as TransientSymbol).links.checkFlags & CheckFlags.Discriminant) === CheckFlags.Discriminant && | ||
!isGenericType(getTypeOfSymbol(prop)); | ||
const transientSymbol = prop as TransientSymbol; | ||
transientSymbol.links.isDiscriminantProperty ??= new Map(); | ||
|
||
jfet97 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let isDiscriminant = transientSymbol.links.isDiscriminantProperty.get(considerNonUniformPrimitivePropDiscriminant); | ||
|
||
if (isDiscriminant === undefined) { | ||
isDiscriminant = (((transientSymbol.links.checkFlags & CheckFlags.Discriminant) === CheckFlags.Discriminant) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check can probably be done in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if it doesn't matter for the perf benchmarks we have, I think yes, to be consistent and keep similar code in a single place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gabritto some feedback. The basic change is quite easy, but there are some problems because Just setting to I pushed the current changes so you can have a look and maybe spot some stupid error I'm doing, or provide some insights for the above conditions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gabritto plot twist, I disabled the new I still have to check why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed a fix that you may dislike, so I'm open to suggestions. The problem: node.body // has type body?: ModuleBody | undefined The condition Then you check node.body.parent // has type ModuleDeclaration | Node | ModuleBody | SourceFile It cannot be non-defined! So the check Now, this is happening when building that structure so it's perfectly possible that the parent is not set yet. And I understand why you may not want to reflect this into types. Probably for all other intents and purposes that field will be always set. But TS just see the types, and with more refinement capabilities it now understands that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR is turning out to be more and more overzealous; I can already see a ton of projects in the top 400 breaking even worse. |
||
|| !!(considerNonUniformPrimitivePropDiscriminant && (transientSymbol.links.checkFlags & (CheckFlags.HasNonUniformType | CheckFlags.HasPrimitiveType)))) | ||
&& !isGenericType(propType); | ||
|
||
transientSymbol.links.isDiscriminantProperty.set(considerNonUniformPrimitivePropDiscriminant, isDiscriminant); | ||
} | ||
return !!(prop as TransientSymbol).links.isDiscriminantProperty; | ||
|
||
return isDiscriminant; | ||
} | ||
} | ||
return false; | ||
|
@@ -28858,7 +28874,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |
const name = getAccessedPropertyName(access); | ||
if (name) { | ||
const type = declaredType.flags & TypeFlags.Union && isTypeSubsetOf(computedType, declaredType) ? declaredType : computedType; | ||
if (isDiscriminantProperty(type, name)) { | ||
if (isDiscriminantProperty(type, name, /*considerNonUniformPrimitivePropDiscriminant*/ true)) { | ||
return access; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
narrowUnionOfObjectsByPrimitiveProperty.ts(62,1): error TS2322: Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: string; other: string; } | { prop: number; other: number; }'. | ||
Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: number; other: number; }'. | ||
Types of property 'prop' are incompatible. | ||
Type 'string | number' is not assignable to type 'number'. | ||
Type 'string' is not assignable to type 'number'. | ||
narrowUnionOfObjectsByPrimitiveProperty.ts(70,1): error TS2322: Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: string; other: string; } | { prop: number; other: number; }'. | ||
Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: number; other: number; }'. | ||
Types of property 'prop' are incompatible. | ||
Type 'string | number' is not assignable to type 'number'. | ||
Type 'string' is not assignable to type 'number'. | ||
narrowUnionOfObjectsByPrimitiveProperty.ts(96,32): error TS2339: Property 'label' does not exist on type 'never'. | ||
|
||
|
||
==== narrowUnionOfObjectsByPrimitiveProperty.ts (3 errors) ==== | ||
export {} | ||
|
||
interface State<Type> { | ||
state: Type; | ||
} | ||
|
||
interface UserName { | ||
first: string; | ||
last?: string; | ||
} | ||
|
||
const nameState = {} as { | ||
value: string; | ||
state: State<string>; | ||
} | { | ||
value: UserName; | ||
state: State<UserName>; | ||
} | ||
|
||
if (typeof nameState.value === "string") { | ||
nameState.state satisfies State<string>; | ||
} else { | ||
nameState.state satisfies State<UserName>; | ||
} | ||
|
||
|
||
declare const arr: [string, number] | [number, string]; | ||
if (typeof arr[0] === "string") { | ||
arr[1] satisfies number; | ||
} else { | ||
arr[1] satisfies string; | ||
} | ||
|
||
|
||
function aStringOrANumber<T extends { a: string } | { a: number }>(param: T): T extends { a: string } ? string : T extends { a: number } ? number : never { | ||
if (typeof param.a === "string") { | ||
return param.a.repeat(3); | ||
} | ||
if (typeof param.a === "number") { | ||
return Math.exp(param.a); | ||
} | ||
throw new Error() | ||
} | ||
|
||
aStringOrANumber({ a: "string" }) | ||
aStringOrANumber({ a: 42 }) | ||
|
||
|
||
// The following two tests ensure that the discriminativeness of property 'prop' | ||
// is treated differently in assignability and narrowing, and that the discriminativeness is properly cached. | ||
declare let obj: { prop: string, other: string } | { prop: number, other: number } | ||
|
||
// Here, we first perform narrowing, but the subsequent assignability should not be affected. | ||
// We expect an error there because of an incorrect value assigned to 'prop'. | ||
// See contextualTypeWithUnionTypeObjectLiteral.ts | ||
if(typeof obj.prop === "string") { | ||
obj.other.repeat(3); | ||
} else { | ||
Math.exp(obj.other); | ||
} | ||
|
||
obj = { prop: Math.random() > 0.5 ? "whatever" : 42, other: "irrelevant" as never } | ||
~~~ | ||
!!! error TS2322: Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: string; other: string; } | { prop: number; other: number; }'. | ||
!!! error TS2322: Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: number; other: number; }'. | ||
!!! error TS2322: Types of property 'prop' are incompatible. | ||
!!! error TS2322: Type 'string | number' is not assignable to type 'number'. | ||
!!! error TS2322: Type 'string' is not assignable to type 'number'. | ||
|
||
|
||
declare let obj2: { prop: string, other: string } | { prop: number, other: number } | ||
|
||
// Here, we first assign a value to 'obj2' and then perform narrowing. | ||
// We expect an error here because of an incorrect value assigned to 'prop', like above, | ||
// but the subsequent narrowing should not be affected by the assignability. | ||
obj2 = { prop: Math.random() > 0.5 ? "whatever" : 42, other: "irrelevant" as never } | ||
~~~~ | ||
!!! error TS2322: Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: string; other: string; } | { prop: number; other: number; }'. | ||
!!! error TS2322: Type '{ prop: string | number; other: never; }' is not assignable to type '{ prop: number; other: number; }'. | ||
!!! error TS2322: Types of property 'prop' are incompatible. | ||
!!! error TS2322: Type 'string | number' is not assignable to type 'number'. | ||
!!! error TS2322: Type 'string' is not assignable to type 'number'. | ||
|
||
if(typeof obj2.prop === "string") { | ||
obj2.other.repeat(3); | ||
} else { | ||
Math.exp(obj2.other); | ||
} | ||
|
||
|
||
interface ILocalizedString { | ||
original: string; | ||
value: string; | ||
} | ||
|
||
type Opt = ({ | ||
label: ILocalizedString; | ||
alias?: string; | ||
} | { | ||
label: string; | ||
alias: string; | ||
}) | ||
|
||
declare const opt: Opt | ||
|
||
if (typeof opt.label === 'string') { | ||
const l = opt.label; | ||
const a = opt.alias ?? opt.label; | ||
~~~~~ | ||
!!! error TS2339: Property 'label' does not exist on type 'never'. | ||
} else { | ||
const l = opt.label; | ||
const a = opt.alias ?? opt.label.original; | ||
} |
Uh oh!
There was an error while loading. Please reload this page.