Skip to content

Commit

Permalink
On branch tsmainb/main/44856
Browse files Browse the repository at this point in the history
 Changes to be committed:
        modified:   src/compiler/checker.ts
        new file:   tests/baselines/reference/_44856.js
        new file:   tests/baselines/reference/_44856.symbols
        new file:   tests/baselines/reference/_44856.types
        new file:   tests/cases/compiler/_44856.ts

- fix for issue microsoft#44856

- minor modifications to checker.ts

  - Preexisting algorithm:
    - In the case of checking for excess properties being assigned to a union, the logic forks:
      - If there is an index in the union whose type qualifies it as discriminant index, then don't allow excess properties (roughly)
      - otherwise, allow excess properites which satisfy some member of the union

  - Change:
    - Don't consider an index with a boolean type to qualify for being a discriminant index.

- Before:

  - In *src/compiler/checker.ts*, in the function `createUnionOrIntersectionProperty` the preexisting code is

```
if ((isLiteralType(type) || isPatternLiteralType(type) || type === uniqueLiteralType) {
        checkFlags |= CheckFlags.HasLiteralType;
    }
```
  - `isLiteralType(type)` returns true for a boolean type, and setting `checkFlags |= CheckFlags.HasLiteralType` results in a higher up decision to qualify the corresponding index as a discriminated index.

- After:
```
if ((isLiteralType(type) &&
        (!fromIsDiscriminantProperty || !(type.flags & TypeFlags.Boolean && type.flags & TypeFlags.Union)))
    || isPatternLiteralType(type) || type === uniqueLiteralType) {
    checkFlags |= CheckFlags.HasLiteralType;
}
```

- the `fromIsDiscriminantProperty` is to provide the context in which `createUnionOrIntersectionProperty`.
- That is necessary because `createUnionOrIntersectionProperty` is called in other contexts that require the existing behavior.
  - Here is one such scenario from `objectSpread.ts`
```
function f<T, U>(t: T, u: U) {
    return { ...t, ...u, id: 'id' }
}
let overwriteId: { id: string, a: number, c: number, d: string } =
    f({ a: 1, id: true }, { c: 1, d: 'no' })
```
  - Without introducing the condition `fromIsDiscriminantProperty`, overwriteId would have type never.
  • Loading branch information
craigphicks committed May 6, 2022
1 parent 4765355 commit 78b9245
Show file tree
Hide file tree
Showing 5 changed files with 354 additions and 5 deletions.
10 changes: 5 additions & 5 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12313,7 +12313,7 @@ namespace ts {
return getReducedType(getApparentType(getReducedType(type)));
}

function createUnionOrIntersectionProperty(containingType: UnionOrIntersectionType, name: __String, skipObjectFunctionPropertyAugment?: boolean): Symbol | undefined {
function createUnionOrIntersectionProperty(containingType: UnionOrIntersectionType, name: __String, skipObjectFunctionPropertyAugment?: boolean, fromIsDiscriminantProperty?: boolean | undefined): Symbol | undefined {
let singleProp: Symbol | undefined;
let propSet: ESMap<SymbolId, Symbol> | undefined;
let indexTypes: Type[] | undefined;
Expand Down Expand Up @@ -12438,7 +12438,7 @@ namespace ts {
else if (type !== firstType) {
checkFlags |= CheckFlags.HasNonUniformType;
}
if (isLiteralType(type) || isPatternLiteralType(type) || type === uniqueLiteralType) {
if ((isLiteralType(type) && (!fromIsDiscriminantProperty || !(type.flags & TypeFlags.Boolean && type.flags & TypeFlags.Union))) || isPatternLiteralType(type) || type === uniqueLiteralType) {
checkFlags |= CheckFlags.HasLiteralType;
}
if (type.flags & TypeFlags.Never && type !== uniqueLiteralType) {
Expand Down Expand Up @@ -12481,11 +12481,11 @@ namespace ts {
// constituents, in which case the isPartial flag is set when the containing type is union type. We need
// these partial properties when identifying discriminant properties, but otherwise they are filtered out
// and do not appear to be present in the union type.
function getUnionOrIntersectionProperty(type: UnionOrIntersectionType, name: __String, skipObjectFunctionPropertyAugment?: boolean): Symbol | undefined {
function getUnionOrIntersectionProperty(type: UnionOrIntersectionType, name: __String, skipObjectFunctionPropertyAugment?: boolean, fromIsDiscriminantProperty?: boolean | undefined): Symbol | undefined {
let property = type.propertyCacheWithoutObjectFunctionPropertyAugment?.get(name) ||
!skipObjectFunctionPropertyAugment ? type.propertyCache?.get(name) : undefined;
if (!property) {
property = createUnionOrIntersectionProperty(type, name, skipObjectFunctionPropertyAugment);
property = createUnionOrIntersectionProperty(type, name, skipObjectFunctionPropertyAugment, fromIsDiscriminantProperty);
if (property) {
const properties = skipObjectFunctionPropertyAugment ?
type.propertyCacheWithoutObjectFunctionPropertyAugment ||= createSymbolTable() :
Expand Down Expand Up @@ -23219,7 +23219,7 @@ namespace ts {

function isDiscriminantProperty(type: Type | undefined, name: __String) {
if (type && type.flags & TypeFlags.Union) {
const prop = getUnionOrIntersectionProperty(type as UnionType, name);
const prop = getUnionOrIntersectionProperty(type as UnionType, name, /* skipObjectFunctionPropertyAugment */ undefined, /* fromIsDiscriminantProperty */ true);
if (prop && getCheckFlags(prop) & CheckFlags.SyntheticProperty) {
if ((prop as TransientSymbol).isDiscriminantProperty === undefined) {
(prop as TransientSymbol).isDiscriminantProperty =
Expand Down
76 changes: 76 additions & 0 deletions tests/baselines/reference/_44856.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//// [_44856.ts]
// The expected rule for properties of union type (|) is
// (1) must contain all required properties of at least one union member
// (2) may contain additional properties that belong to any union member

{
type A2 = {a: string; b: string};
type A3 = {a: string; b: boolean; c: number};

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: 1}; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
}

{
type A2 = {a: string; b: string};
type A3 = {a: string; b: boolean; c: boolean};
type A4 = {a: string; b: number; c: number};

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
}
}

{
type A2 = {a: string; b: string};
type A3 = {a: string; b: boolean; c: boolean};
type A4 = {a: string; b: boolean; c: number};

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
}
}


//// [_44856.js]
// The expected rule for properties of union type (|) is
// (1) must contain all required properties of at least one union member
// (2) may contain additional properties that belong to any union member
{
{
// THIS IS THE UNEXPECTED OUTLIER
var b = { a: '', b: '', c: 1 }; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
}
{
{
// THIS IS THE UNEXPECTED OUTLIER
var b = { a: '', b: '', c: true }; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
{
// BEHAVING AS EXPECTED
var b = { a: '', b: '', c: true }; // ✔ assignments allow extra props of other union types
}
}
{
{
// THIS IS THE UNEXPECTED OUTLIER
var b = { a: '', b: '', c: true }; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
{
// BEHAVING AS EXPECTED
var b = { a: '', b: '', c: true }; // ✔ assignments allow extra props of other union types
}
}
111 changes: 111 additions & 0 deletions tests/baselines/reference/_44856.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
=== tests/cases/compiler/_44856.ts ===
// The expected rule for properties of union type (|) is
// (1) must contain all required properties of at least one union member
// (2) may contain additional properties that belong to any union member

{
type A2 = {a: string; b: string};
>A2 : Symbol(A2, Decl(_44856.ts, 4, 1))
>a : Symbol(a, Decl(_44856.ts, 5, 15))
>b : Symbol(b, Decl(_44856.ts, 5, 25))

type A3 = {a: string; b: boolean; c: number};
>A3 : Symbol(A3, Decl(_44856.ts, 5, 37))
>a : Symbol(a, Decl(_44856.ts, 6, 15))
>b : Symbol(b, Decl(_44856.ts, 6, 25))
>c : Symbol(c, Decl(_44856.ts, 6, 37))

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: 1}; // ❌ assignment strictly checked, extra prop from A3 not allowed
>b : Symbol(b, Decl(_44856.ts, 10, 11))
>A2 : Symbol(A2, Decl(_44856.ts, 4, 1))
>A3 : Symbol(A3, Decl(_44856.ts, 5, 37))
>a : Symbol(a, Decl(_44856.ts, 10, 24))
>b : Symbol(b, Decl(_44856.ts, 10, 30))
>c : Symbol(c, Decl(_44856.ts, 10, 37))
}
}

{
type A2 = {a: string; b: string};
>A2 : Symbol(A2, Decl(_44856.ts, 14, 1))
>a : Symbol(a, Decl(_44856.ts, 15, 15))
>b : Symbol(b, Decl(_44856.ts, 15, 25))

type A3 = {a: string; b: boolean; c: boolean};
>A3 : Symbol(A3, Decl(_44856.ts, 15, 37))
>a : Symbol(a, Decl(_44856.ts, 16, 15))
>b : Symbol(b, Decl(_44856.ts, 16, 25))
>c : Symbol(c, Decl(_44856.ts, 16, 37))

type A4 = {a: string; b: number; c: number};
>A4 : Symbol(A4, Decl(_44856.ts, 16, 50))
>a : Symbol(a, Decl(_44856.ts, 17, 15))
>b : Symbol(b, Decl(_44856.ts, 17, 25))
>c : Symbol(c, Decl(_44856.ts, 17, 36))

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
>b : Symbol(b, Decl(_44856.ts, 21, 11))
>A2 : Symbol(A2, Decl(_44856.ts, 14, 1))
>A3 : Symbol(A3, Decl(_44856.ts, 15, 37))
>a : Symbol(a, Decl(_44856.ts, 21, 24))
>b : Symbol(b, Decl(_44856.ts, 21, 30))
>c : Symbol(c, Decl(_44856.ts, 21, 37))
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
>b : Symbol(b, Decl(_44856.ts, 25, 11))
>A2 : Symbol(A2, Decl(_44856.ts, 14, 1))
>A3 : Symbol(A3, Decl(_44856.ts, 15, 37))
>A4 : Symbol(A4, Decl(_44856.ts, 16, 50))
>a : Symbol(a, Decl(_44856.ts, 25, 27))
>b : Symbol(b, Decl(_44856.ts, 25, 33))
>c : Symbol(c, Decl(_44856.ts, 25, 40))
}
}

{
type A2 = {a: string; b: string};
>A2 : Symbol(A2, Decl(_44856.ts, 29, 3))
>a : Symbol(a, Decl(_44856.ts, 30, 15))
>b : Symbol(b, Decl(_44856.ts, 30, 25))

type A3 = {a: string; b: boolean; c: boolean};
>A3 : Symbol(A3, Decl(_44856.ts, 30, 37))
>a : Symbol(a, Decl(_44856.ts, 31, 15))
>b : Symbol(b, Decl(_44856.ts, 31, 25))
>c : Symbol(c, Decl(_44856.ts, 31, 37))

type A4 = {a: string; b: boolean; c: number};
>A4 : Symbol(A4, Decl(_44856.ts, 31, 50))
>a : Symbol(a, Decl(_44856.ts, 32, 15))
>b : Symbol(b, Decl(_44856.ts, 32, 25))
>c : Symbol(c, Decl(_44856.ts, 32, 37))

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
>b : Symbol(b, Decl(_44856.ts, 36, 11))
>A2 : Symbol(A2, Decl(_44856.ts, 29, 3))
>A3 : Symbol(A3, Decl(_44856.ts, 30, 37))
>a : Symbol(a, Decl(_44856.ts, 36, 24))
>b : Symbol(b, Decl(_44856.ts, 36, 30))
>c : Symbol(c, Decl(_44856.ts, 36, 37))
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
>b : Symbol(b, Decl(_44856.ts, 40, 11))
>A2 : Symbol(A2, Decl(_44856.ts, 29, 3))
>A3 : Symbol(A3, Decl(_44856.ts, 30, 37))
>A4 : Symbol(A4, Decl(_44856.ts, 31, 50))
>a : Symbol(a, Decl(_44856.ts, 40, 27))
>b : Symbol(b, Decl(_44856.ts, 40, 33))
>c : Symbol(c, Decl(_44856.ts, 40, 40))
}
}

119 changes: 119 additions & 0 deletions tests/baselines/reference/_44856.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
=== tests/cases/compiler/_44856.ts ===
// The expected rule for properties of union type (|) is
// (1) must contain all required properties of at least one union member
// (2) may contain additional properties that belong to any union member

{
type A2 = {a: string; b: string};
>A2 : { a: string; b: string; }
>a : string
>b : string

type A3 = {a: string; b: boolean; c: number};
>A3 : { a: string; b: boolean; c: number; }
>a : string
>b : boolean
>c : number

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: 1}; // ❌ assignment strictly checked, extra prop from A3 not allowed
>b : { a: string; b: string; } | { a: string; b: boolean; c: number; }
>{a: '', b: '', c: 1} : { a: string; b: string; c: number; }
>a : string
>'' : ""
>b : string
>'' : ""
>c : number
>1 : 1
}
}

{
type A2 = {a: string; b: string};
>A2 : { a: string; b: string; }
>a : string
>b : string

type A3 = {a: string; b: boolean; c: boolean};
>A3 : { a: string; b: boolean; c: boolean; }
>a : string
>b : boolean
>c : boolean

type A4 = {a: string; b: number; c: number};
>A4 : { a: string; b: number; c: number; }
>a : string
>b : number
>c : number

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
>b : { a: string; b: string; } | { a: string; b: boolean; c: boolean; }
>{a: '', b: '', c: true} : { a: string; b: string; c: true; }
>a : string
>'' : ""
>b : string
>'' : ""
>c : true
>true : true
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
>b : { a: string; b: string; } | { a: string; b: boolean; c: boolean; } | { a: string; b: number; c: number; }
>{a: '', b: '', c: true} : { a: string; b: string; c: true; }
>a : string
>'' : ""
>b : string
>'' : ""
>c : true
>true : true
}
}

{
type A2 = {a: string; b: string};
>A2 : { a: string; b: string; }
>a : string
>b : string

type A3 = {a: string; b: boolean; c: boolean};
>A3 : { a: string; b: boolean; c: boolean; }
>a : string
>b : boolean
>c : boolean

type A4 = {a: string; b: boolean; c: number};
>A4 : { a: string; b: boolean; c: number; }
>a : string
>b : boolean
>c : number

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
>b : { a: string; b: string; } | { a: string; b: boolean; c: boolean; }
>{a: '', b: '', c: true} : { a: string; b: string; c: true; }
>a : string
>'' : ""
>b : string
>'' : ""
>c : true
>true : true
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
>b : { a: string; b: string; } | { a: string; b: boolean; c: boolean; } | { a: string; b: boolean; c: number; }
>{a: '', b: '', c: true} : { a: string; b: string; c: true; }
>a : string
>'' : ""
>b : string
>'' : ""
>c : true
>true : true
}
}

43 changes: 43 additions & 0 deletions tests/cases/compiler/_44856.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// The expected rule for properties of union type (|) is
// (1) must contain all required properties of at least one union member
// (2) may contain additional properties that belong to any union member

{
type A2 = {a: string; b: string};
type A3 = {a: string; b: boolean; c: number};

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: 1}; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
}

{
type A2 = {a: string; b: string};
type A3 = {a: string; b: boolean; c: boolean};
type A4 = {a: string; b: number; c: number};

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
}
}

{
type A2 = {a: string; b: string};
type A3 = {a: string; b: boolean; c: boolean};
type A4 = {a: string; b: boolean; c: number};

{
// THIS IS THE UNEXPECTED OUTLIER
const b: A2|A3 = {a: '', b: '', c: true}; // ❌ assignment strictly checked, extra prop from A3 not allowed
}
{
// BEHAVING AS EXPECTED
const b: A2|A3|A4 = {a: '', b: '', c: true}; // ✔ assignments allow extra props of other union types
}
}

0 comments on commit 78b9245

Please sign in to comment.