Skip to content

Fixed element access expression writes for divergent write types #55585

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

Merged
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
@@ -685,6 +685,7 @@ import {
isRequireCall,
isRestParameter,
isRestTypeNode,
isRightSideOfAccessExpression,
isRightSideOfQualifiedNameOrPropertyAccess,
isRightSideOfQualifiedNameOrPropertyAccessOrJSDocMemberName,
isSameEntityName,
@@ -17705,7 +17706,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return autoType;
}
}
const propType = getTypeOfSymbol(prop);
const propType = accessFlags & AccessFlags.Writing ? getWriteTypeOfSymbol(prop) : getTypeOfSymbol(prop);
return accessExpression && getAssignmentTargetKind(accessExpression) !== AssignmentKind.Definite ? getFlowTypeOfReference(accessExpression, propType) :
accessNode && isIndexedAccessTypeNode(accessNode) && containsMissingType(propType) ? getUnionType([propType, undefinedType]) :
propType;
@@ -28178,7 +28179,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// to it at the given location. Since we have no control flow information for the
// hypothetical reference (control flow information is created and attached by the
// binder), we simply return the declared type of the symbol.
return getNonMissingTypeOfSymbol(symbol);
return isRightSideOfAccessExpression(location) && isWriteAccess(location.parent) ? getWriteTypeOfSymbol(symbol) : getNonMissingTypeOfSymbol(symbol);
}

function getControlFlowContainer(node: Node): Node {
4 changes: 2 additions & 2 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
@@ -7239,8 +7239,8 @@ export function isRightSideOfQualifiedNameOrPropertyAccess(node: Node) {

/** @internal */
export function isRightSideOfAccessExpression(node: Node) {
return isPropertyAccessExpression(node.parent) && node.parent.name === node
|| isElementAccessExpression(node.parent) && node.parent.argumentExpression === node;
return !!node.parent && (isPropertyAccessExpression(node.parent) && node.parent.name === node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other utils like this one do not have a guard like this so it looks a little bit off. However, .parent is actually nullable - a SourceFile doesn't have a parent. I could guard this before calling this function but personally, I prefer to have it here.

|| isElementAccessExpression(node.parent) && node.parent.argumentExpression === node);
}

/** @internal */
6 changes: 4 additions & 2 deletions tests/baselines/reference/divergentAccessorsTypes7.js
Original file line number Diff line number Diff line change
@@ -9,12 +9,12 @@ class Test<S> {
get value(): string {
return null!;
}

// -- Replacing the getter such that the getter/setter types match, removes the error:
// get value(): string | ((item: S) => string) {
// return null!;
// }

// -- Or, replacing the setter such that a concrete type is used, removes the error:
// set value(value: string | ((item: { property: string }) => string)) {}
}
@@ -24,6 +24,7 @@ const a = new Test<{
}>();

a.value = (item) => item.property
a['value'] = (item) => item.property


//// [divergentAccessorsTypes7.js]
@@ -42,3 +43,4 @@ var Test = /** @class */ (function () {
}());
var a = new Test();
a.value = function (item) { return item.property; };
a['value'] = function (item) { return item.property; };
12 changes: 10 additions & 2 deletions tests/baselines/reference/divergentAccessorsTypes7.symbols
Original file line number Diff line number Diff line change
@@ -18,12 +18,12 @@ class Test<S> {

return null!;
}

// -- Replacing the getter such that the getter/setter types match, removes the error:
// get value(): string | ((item: S) => string) {
// return null!;
// }

// -- Or, replacing the setter such that a concrete type is used, removes the error:
// set value(value: string | ((item: { property: string }) => string)) {}
}
@@ -46,3 +46,11 @@ a.value = (item) => item.property
>item : Symbol(item, Decl(divergentAccessorsTypes7.ts, 22, 11))
>property : Symbol(property, Decl(divergentAccessorsTypes7.ts, 18, 20))

a['value'] = (item) => item.property
>a : Symbol(a, Decl(divergentAccessorsTypes7.ts, 18, 5))
>'value' : Symbol(Test.value, Decl(divergentAccessorsTypes7.ts, 1, 20), Decl(divergentAccessorsTypes7.ts, 3, 55))
>item : Symbol(item, Decl(divergentAccessorsTypes7.ts, 23, 14))
>item.property : Symbol(property, Decl(divergentAccessorsTypes7.ts, 18, 20))
>item : Symbol(item, Decl(divergentAccessorsTypes7.ts, 23, 14))
>property : Symbol(property, Decl(divergentAccessorsTypes7.ts, 18, 20))

15 changes: 13 additions & 2 deletions tests/baselines/reference/divergentAccessorsTypes7.types
Original file line number Diff line number Diff line change
@@ -17,12 +17,12 @@ class Test<S> {
return null!;
>null! : null
}

// -- Replacing the getter such that the getter/setter types match, removes the error:
// get value(): string | ((item: S) => string) {
// return null!;
// }

// -- Or, replacing the setter such that a concrete type is used, removes the error:
// set value(value: string | ((item: { property: string }) => string)) {}
}
@@ -48,3 +48,14 @@ a.value = (item) => item.property
>item : { property: string; }
>property : string

a['value'] = (item) => item.property
>a['value'] = (item) => item.property : (item: { property: string; }) => string
>a['value'] : string | ((item: { property: string; }) => string)
>a : Test<{ property: string; }>
>'value' : "value"
>(item) => item.property : (item: { property: string; }) => string
>item : { property: string; }
>item.property : string
>item : { property: string; }
>property : string

174 changes: 174 additions & 0 deletions tests/baselines/reference/divergentAccessorsTypes8.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
divergentAccessorsTypes8.ts(20,1): error TS2322: Type 'CSSStyleDeclaration' is not assignable to type 'string'.
divergentAccessorsTypes8.ts(86,1): error TS2322: Type 'number' is not assignable to type 'string'.
divergentAccessorsTypes8.ts(124,1): error TS2322: Type '42' is not assignable to type '"hello"'.
divergentAccessorsTypes8.ts(128,1): error TS2322: Type '"hello"' is not assignable to type '42'.
divergentAccessorsTypes8.ts(146,1): error TS2322: Type 'number' is not assignable to type 'boolean'.
divergentAccessorsTypes8.ts(148,1): error TS2322: Type 'string' is not assignable to type 'boolean'.
divergentAccessorsTypes8.ts(149,1): error TS2322: Type 'null' is not assignable to type 'boolean'.


==== divergentAccessorsTypes8.ts (7 errors) ====
export {}

interface Serializer {
set value(v: string | number | boolean);
get value(): string;
}
declare let box: Serializer;
const v = box['value']
box['value'] = true;
box['value'] = 42;
box['value'] = "hello";

interface Element {
get style(): CSSStyleDeclaration;
set style(cssText: string);
}

declare const element: Element;
element['style'] = "color: red";
element['style'] = element.style;
~~~~~~~~~~~~~~~~
!!! error TS2322: Type 'CSSStyleDeclaration' is not assignable to type 'string'.

class One {
get prop1(): string {
return "";
}
set prop1(s: string | number) {}

get prop2(): string {
return "";
}
set prop2(s: string | number) {}

prop3: number = 42;

get prop4(): string {
return "";
}
set prop4(s: string | number) {}
}

class Two {
get prop1(): string {
return "";
}
set prop1(s: string | number) {}

get prop2(): string {
return "";
}
set prop2(s: string) {}

get prop3(): string {
return "";
}
set prop3(s: string | boolean) {}

get prop4(): string {
return "";
}
set prop4(s: string | boolean) {}
}

declare const u1: One | Two;

u1['prop1'] = 42;
u1['prop1'] = "hello";

u1['prop2'] = 42;
u1['prop2'] = "hello";

u1['prop3'] = 42;
u1['prop3'] = "hello";
u1['prop3'] = true;

u1['prop4'] = 42;
u1['prop4'] = "hello";
u1['prop4'] = true;

declare const i: One & Two;

const iv1 = i['prop1'];
i['prop1'] = 42;
i['prop1'] = "hello";

const iv2 = i['prop2'];
i['prop2'] = 42;
~~~~~~~~~~
!!! error TS2322: Type 'number' is not assignable to type 'string'.
i['prop2'] = "hello";

class Three {
get prop1(): string {
return "";
}
set prop1(s: string | number) {}

prop2: number = 42;
}

class Four {
get prop1(): "hello" {
return "hello";
}
set prop1(s: "hello" | number) {}

get prop2(): string {
return "";
}
set prop2(s: string | 42) {}
}

class Five {
get prop1(): "hello" {
return "hello";
}
set prop1(s: "hello" | boolean) {}

get prop2(): string {
return "";
}
set prop2(s: string | number | boolean) {}
}

declare const i2: Three & Four & Five;

i2['prop1'] = 42;
~~~~~~~~~~~
!!! error TS2322: Type '42' is not assignable to type '"hello"'.
i2['prop1'] = "hello";

i2['prop2'] = 42;
i2['prop2'] = "hello";
~~~~~~~~~~~
!!! error TS2322: Type '"hello"' is not assignable to type '42'.

class Six {
get prop1(): boolean | number {
return 42;
}
set prop1(s: boolean | string) {}

get prop2(): bigint | number {
return 10;
}
set prop2(s: boolean | null) {}
}

declare const s1: Six
declare const k1: 'prop1' | 'prop2'

const sv1 = s1[k1]
s1[k1] = 42
~~~~~~
!!! error TS2322: Type 'number' is not assignable to type 'boolean'.
s1[k1] = true
s1[k1] = ''
~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'boolean'.
s1[k1] = null
~~~~~~
!!! error TS2322: Type 'null' is not assignable to type 'boolean'.

345 changes: 345 additions & 0 deletions tests/baselines/reference/divergentAccessorsTypes8.symbols

Large diffs are not rendered by default.

429 changes: 429 additions & 0 deletions tests/baselines/reference/divergentAccessorsTypes8.types

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions tests/cases/compiler/divergentAccessorsTypes7.ts
Original file line number Diff line number Diff line change
@@ -6,12 +6,12 @@ class Test<S> {
get value(): string {
return null!;
}

// -- Replacing the getter such that the getter/setter types match, removes the error:
// get value(): string | ((item: S) => string) {
// return null!;
// }

// -- Or, replacing the setter such that a concrete type is used, removes the error:
// set value(value: string | ((item: { property: string }) => string)) {}
}
@@ -21,3 +21,4 @@ const a = new Test<{
}>();

a.value = (item) => item.property
a['value'] = (item) => item.property
153 changes: 153 additions & 0 deletions tests/cases/compiler/divergentAccessorsTypes8.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

most of the test cases here (but not all) are borrowed from tests/cases/compiler/divergentAccessorsTypes1-6.ts

Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
// @strict: true
// @lib: esnext, dom
// @noEmit: true

export {}

interface Serializer {
set value(v: string | number | boolean);
get value(): string;
}
declare let box: Serializer;
const v = box['value']
box['value'] = true;
box['value'] = 42;
box['value'] = "hello";

interface Element {
get style(): CSSStyleDeclaration;
set style(cssText: string);
}

declare const element: Element;
element['style'] = "color: red";
element['style'] = element.style;

class One {
get prop1(): string {
return "";
}
set prop1(s: string | number) {}

get prop2(): string {
return "";
}
set prop2(s: string | number) {}

prop3: number = 42;

get prop4(): string {
return "";
}
set prop4(s: string | number) {}
}

class Two {
get prop1(): string {
return "";
}
set prop1(s: string | number) {}

get prop2(): string {
return "";
}
set prop2(s: string) {}

get prop3(): string {
return "";
}
set prop3(s: string | boolean) {}

get prop4(): string {
return "";
}
set prop4(s: string | boolean) {}
}

declare const u1: One | Two;

u1['prop1'] = 42;
u1['prop1'] = "hello";

u1['prop2'] = 42;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this one should error. See the playground here

I re-verified all of the tests here and everything here behaves in the same way as it behaves for regular property access expressions. So I think that this issue is a separate one and I will try to prepare a follow up PR to fix it.

u1['prop2'] = "hello";

u1['prop3'] = 42;
u1['prop3'] = "hello";
u1['prop3'] = true;

u1['prop4'] = 42;
u1['prop4'] = "hello";
u1['prop4'] = true;

declare const i: One & Two;

const iv1 = i['prop1'];
i['prop1'] = 42;
i['prop1'] = "hello";

const iv2 = i['prop2'];
i['prop2'] = 42;
i['prop2'] = "hello";

class Three {
get prop1(): string {
return "";
}
set prop1(s: string | number) {}

prop2: number = 42;
}

class Four {
get prop1(): "hello" {
return "hello";
}
set prop1(s: "hello" | number) {}

get prop2(): string {
return "";
}
set prop2(s: string | 42) {}
}

class Five {
get prop1(): "hello" {
return "hello";
}
set prop1(s: "hello" | boolean) {}

get prop2(): string {
return "";
}
set prop2(s: string | number | boolean) {}
}

declare const i2: Three & Four & Five;

i2['prop1'] = 42;
i2['prop1'] = "hello";

i2['prop2'] = 42;
i2['prop2'] = "hello";

class Six {
get prop1(): boolean | number {
return 42;
}
set prop1(s: boolean | string) {}

get prop2(): bigint | number {
return 10;
}
set prop2(s: boolean | null) {}
}

declare const s1: Six
declare const k1: 'prop1' | 'prop2'

const sv1 = s1[k1]
s1[k1] = 42
s1[k1] = true
s1[k1] = ''
s1[k1] = null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

those tests are borrowed from #54777 , just adjusted to use the element access expressions

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/// <reference path='fourslash.ts'/>

// @strict: true
// @exactOptionalPropertyTypes: true
//// declare const xx: { prop?: number };
//// xx['prop'/*1*/] = 1;

verify.quickInfoAt('1', '(property) prop?: number');
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/// <reference path='fourslash.ts'/>

// @strict: true
// @exactOptionalPropertyTypes: true
//// declare const xx: { prop?: number };
//// xx['prop'/*1*/] += 1;

verify.quickInfoAt('1', '(property) prop?: number');
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/// <reference path='fourslash.ts'/>

// @strict: true
// @exactOptionalPropertyTypes: true
//// declare const xx: { prop?: number };
//// xx['prop'/*1*/] ??= 1;

verify.quickInfoAt('1', '(property) prop?: number');
11 changes: 11 additions & 0 deletions tests/cases/fourslash/quickInfoOnElementAccessInWriteLocation4.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path='fourslash.ts'/>

// @strict: true
//// interface Serializer {
//// set value(v: string | number | boolean);
//// get value(): string;
//// }
//// declare let box: Serializer;
//// box['value'/*1*/] = true;

verify.quickInfoAt('1', '(property) Serializer.value: string | number | boolean');
11 changes: 11 additions & 0 deletions tests/cases/fourslash/quickInfoOnElementAccessInWriteLocation5.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path='fourslash.ts'/>

// @strict: true
//// interface Serializer {
//// set value(v: string | number);
//// get value(): string;
//// }
//// declare let box: Serializer;
//// box['value'/*1*/] += 10;

verify.quickInfoAt('1', '(property) Serializer.value: string | number');