Skip to content

fix for 45006 #45020

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
merged 6 commits into from
Aug 21, 2021
Merged
Show file tree
Hide file tree
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
16 changes: 8 additions & 8 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ namespace ts {
IsFunctionExpression = 1 << 4,
HasLocals = 1 << 5,
IsInterface = 1 << 6,
IsObjectLiteralOrClassExpressionMethod = 1 << 7,
IsObjectLiteralOrClassExpressionMethodOrAccessor = 1 << 7,
}

function initFlowNode<T extends FlowNode>(node: T) {
Expand Down Expand Up @@ -663,8 +663,8 @@ namespace ts {
// similarly to break statements that exit to a label just past the statement body.
if (!isIIFE) {
currentFlow = initFlowNode({ flags: FlowFlags.Start });
if (containerFlags & (ContainerFlags.IsFunctionExpression | ContainerFlags.IsObjectLiteralOrClassExpressionMethod)) {
currentFlow.node = node as FunctionExpression | ArrowFunction | MethodDeclaration;
if (containerFlags & (ContainerFlags.IsFunctionExpression | ContainerFlags.IsObjectLiteralOrClassExpressionMethodOrAccessor)) {
currentFlow.node = node as FunctionExpression | ArrowFunction | MethodDeclaration | GetAccessorDeclaration | SetAccessorDeclaration;
}
}
// We create a return control flow graph for IIFEs and constructors. For constructors
Expand Down Expand Up @@ -1815,16 +1815,16 @@ namespace ts {
case SyntaxKind.SourceFile:
return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals;

case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.MethodDeclaration:
if (isObjectLiteralOrClassExpressionMethod(node)) {
return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike | ContainerFlags.IsObjectLiteralOrClassExpressionMethod;
if (isObjectLiteralOrClassExpressionMethodOrAccessor(node)) {
return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike | ContainerFlags.IsObjectLiteralOrClassExpressionMethodOrAccessor;
}
// falls through
case SyntaxKind.Constructor:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.CallSignature:
case SyntaxKind.JSDocSignature:
case SyntaxKind.JSDocFunctionType:
Expand Down Expand Up @@ -3372,7 +3372,7 @@ namespace ts {
emitFlags |= NodeFlags.HasAsyncFunctions;
}

if (currentFlow && isObjectLiteralOrClassExpressionMethod(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

There are only 3 callers of isObjectLiteralOrClassExpressionMethod. Two of them need this addition, and the third is guarded by case SyntaxKind.MethodDeclaration. So I'd like to see the new check moved into that function, and the name should be ... uhh ... isObjectLiteralOrClassExpressionMethodOrAccessor ???

Maybe there's a better name if there's a term that means "method or accessor".

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 just went with choice 1, isObjectLiteralOrClassExpressionMethodOrAccessor; there was also a ContainerFlags by the same name that looked like it could be part of the same renaming effort.

if (currentFlow && isObjectLiteralOrClassExpressionMethodOrAccessor(node)) {
node.flowNode = currentFlow;
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24479,7 +24479,7 @@ namespace ts {
// a const variable or parameter from an outer function, we extend the origin of the control flow
// analysis to include the immediately enclosing function.
while (flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethod(flowContainer)) &&
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethodOrAccessor(flowContainer)) &&
(isConstVariable(localOrExportSymbol) && type !== autoArrayType || isParameter && !isParameterAssigned(localOrExportSymbol))) {
flowContainer = getControlFlowContainer(flowContainer);
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3410,7 +3410,7 @@ namespace ts {
// function, the node property references the function (which in turn has a flowNode
// property for the containing control flow).
export interface FlowStart extends FlowNodeBase {
node?: FunctionExpression | ArrowFunction | MethodDeclaration;
node?: FunctionExpression | ArrowFunction | MethodDeclaration | GetAccessorDeclaration | SetAccessorDeclaration;
}

// FlowLabel represents a junction with multiple possible preceding control flows.
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1526,8 +1526,8 @@ namespace ts {
return node && node.kind === SyntaxKind.MethodDeclaration && node.parent.kind === SyntaxKind.ObjectLiteralExpression;
}

export function isObjectLiteralOrClassExpressionMethod(node: Node): node is MethodDeclaration {
return node.kind === SyntaxKind.MethodDeclaration &&
export function isObjectLiteralOrClassExpressionMethodOrAccessor(node: Node): node is MethodDeclaration {
return (node.kind === SyntaxKind.MethodDeclaration || node.kind === SyntaxKind.GetAccessor || node.kind === SyntaxKind.SetAccessor) &&
(node.parent.kind === SyntaxKind.ObjectLiteralExpression ||
node.parent.kind === SyntaxKind.ClassExpression);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1925,7 +1925,7 @@ declare namespace ts {
id?: number;
}
export interface FlowStart extends FlowNodeBase {
node?: FunctionExpression | ArrowFunction | MethodDeclaration;
node?: FunctionExpression | ArrowFunction | MethodDeclaration | GetAccessorDeclaration | SetAccessorDeclaration;
}
export interface FlowLabel extends FlowNodeBase {
antecedents: FlowNode[] | undefined;
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1925,7 +1925,7 @@ declare namespace ts {
id?: number;
}
export interface FlowStart extends FlowNodeBase {
node?: FunctionExpression | ArrowFunction | MethodDeclaration;
node?: FunctionExpression | ArrowFunction | MethodDeclaration | GetAccessorDeclaration | SetAccessorDeclaration;
}
export interface FlowLabel extends FlowNodeBase {
antecedents: FlowNode[] | undefined;
Expand Down
30 changes: 11 additions & 19 deletions tests/baselines/reference/gettersAndSetters.errors.txt
Original file line number Diff line number Diff line change
@@ -1,33 +1,19 @@
tests/cases/compiler/gettersAndSetters.ts(7,16): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
tests/cases/compiler/gettersAndSetters.ts(8,16): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
tests/cases/compiler/gettersAndSetters.ts(10,16): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
tests/cases/compiler/gettersAndSetters.ts(11,16): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
tests/cases/compiler/gettersAndSetters.ts(25,13): error TS2339: Property 'Baz' does not exist on type 'C'.
tests/cases/compiler/gettersAndSetters.ts(26,3): error TS2339: Property 'Baz' does not exist on type 'C'.
tests/cases/compiler/gettersAndSetters.ts(29,30): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
tests/cases/compiler/gettersAndSetters.ts(29,53): error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.


==== tests/cases/compiler/gettersAndSetters.ts (8 errors) ====
==== tests/cases/compiler/gettersAndSetters.ts (2 errors) ====
// classes
class C {
public fooBack = "";
static barBack:string = "";
public bazBack = "";

public get Foo() { return this.fooBack;} // ok
~~~
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
public set Foo(foo:string) {this.fooBack = foo;} // ok
~~~
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.

static get Bar() {return C.barBack;} // ok
~~~
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
static set Bar(bar:string) {C.barBack = bar;} // ok
~~~
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.

public get = function() {} // ok
public set = function() {} // ok
Expand All @@ -50,10 +36,6 @@ tests/cases/compiler/gettersAndSetters.ts(29,53): error TS1056: Accessors are on

// The Foo accessors' return and param types should be contextually typed to the Foo field
var o : {Foo:number;} = {get Foo() {return 0;}, set Foo(val:number){val}}; // o
~~~
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
~~~
!!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.

var ofg = o.Foo;
o.Foo = 0;
Expand All @@ -64,4 +46,14 @@ tests/cases/compiler/gettersAndSetters.ts(29,53): error TS1056: Accessors are on
}

var i:I1 = function (n) {return n;}

// Repro from #45006
const x: string | number = Math.random() < 0.5 ? "str" : 123;
if (typeof x === "string") {
let obj = {
set prop(_: any) { x.toUpperCase(); },
get prop() { return x.toUpperCase() },
method() { return x.toUpperCase() }
}
}

48 changes: 27 additions & 21 deletions tests/baselines/reference/gettersAndSetters.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,36 +38,33 @@ interface I1 {
}

var i:I1 = function (n) {return n;}

// Repro from #45006
const x: string | number = Math.random() < 0.5 ? "str" : 123;
if (typeof x === "string") {
let obj = {
set prop(_: any) { x.toUpperCase(); },
get prop() { return x.toUpperCase() },
method() { return x.toUpperCase() }
}
}


//// [gettersAndSetters.js]
// classes
var C = /** @class */ (function () {
function C() {
class C {
constructor() {
this.fooBack = "";
this.bazBack = "";
this.get = function () { }; // ok
this.set = function () { }; // ok
}
Object.defineProperty(C.prototype, "Foo", {
get: function () { return this.fooBack; } // ok
,
set: function (foo) { this.fooBack = foo; } // ok
,
enumerable: false,
configurable: true
});
Object.defineProperty(C, "Bar", {
get: function () { return C.barBack; } // ok
,
set: function (bar) { C.barBack = bar; } // ok
,
enumerable: false,
configurable: true
});
C.barBack = "";
return C;
}());
get Foo() { return this.fooBack; } // ok
set Foo(foo) { this.fooBack = foo; } // ok
static get Bar() { return C.barBack; } // ok
static set Bar(bar) { C.barBack = bar; } // ok
}
C.barBack = "";
var c = new C();
var foo = c.Foo;
c.Foo = "foov";
Expand All @@ -80,3 +77,12 @@ var o = { get Foo() { return 0; }, set Foo(val) { val; } }; // o
var ofg = o.Foo;
o.Foo = 0;
var i = function (n) { return n; };
// Repro from #45006
const x = Math.random() < 0.5 ? "str" : 123;
if (typeof x === "string") {
let obj = {
set prop(_) { x.toUpperCase(); },
get prop() { return x.toUpperCase(); },
method() { return x.toUpperCase(); }
};
}
34 changes: 34 additions & 0 deletions tests/baselines/reference/gettersAndSetters.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,37 @@ var i:I1 = function (n) {return n;}
>n : Symbol(n, Decl(gettersAndSetters.ts, 38, 21))
>n : Symbol(n, Decl(gettersAndSetters.ts, 38, 21))

// Repro from #45006
const x: string | number = Math.random() < 0.5 ? "str" : 123;
>x : Symbol(x, Decl(gettersAndSetters.ts, 41, 5))
>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.symbol.wellknown.d.ts, --, --))
>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --))

if (typeof x === "string") {
>x : Symbol(x, Decl(gettersAndSetters.ts, 41, 5))

let obj = {
>obj : Symbol(obj, Decl(gettersAndSetters.ts, 43, 5))

set prop(_: any) { x.toUpperCase(); },
>prop : Symbol(prop, Decl(gettersAndSetters.ts, 43, 13), Decl(gettersAndSetters.ts, 44, 42))
>_ : Symbol(_, Decl(gettersAndSetters.ts, 44, 13))
>x.toUpperCase : Symbol(String.toUpperCase, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(gettersAndSetters.ts, 41, 5))
>toUpperCase : Symbol(String.toUpperCase, Decl(lib.es5.d.ts, --, --))

get prop() { return x.toUpperCase() },
>prop : Symbol(prop, Decl(gettersAndSetters.ts, 43, 13), Decl(gettersAndSetters.ts, 44, 42))
>x.toUpperCase : Symbol(String.toUpperCase, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(gettersAndSetters.ts, 41, 5))
>toUpperCase : Symbol(String.toUpperCase, Decl(lib.es5.d.ts, --, --))

method() { return x.toUpperCase() }
>method : Symbol(method, Decl(gettersAndSetters.ts, 45, 42))
>x.toUpperCase : Symbol(String.toUpperCase, Decl(lib.es5.d.ts, --, --))
>x : Symbol(x, Decl(gettersAndSetters.ts, 41, 5))
>toUpperCase : Symbol(String.toUpperCase, Decl(lib.es5.d.ts, --, --))
}
}

47 changes: 47 additions & 0 deletions tests/baselines/reference/gettersAndSetters.types
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,50 @@ var i:I1 = function (n) {return n;}
>n : number
>n : number

// Repro from #45006
const x: string | number = Math.random() < 0.5 ? "str" : 123;
>x : string | number
>Math.random() < 0.5 ? "str" : 123 : "str" | 123
>Math.random() < 0.5 : boolean
>Math.random() : number
>Math.random : () => number
>Math : Math
>random : () => number
>0.5 : 0.5
>"str" : "str"
>123 : 123

if (typeof x === "string") {
>typeof x === "string" : boolean
>typeof x : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>x : string | number
>"string" : "string"

let obj = {
>obj : { prop: any; method(): string; }
>{ set prop(_: any) { x.toUpperCase(); }, get prop() { return x.toUpperCase() }, method() { return x.toUpperCase() } } : { prop: any; method(): string; }

set prop(_: any) { x.toUpperCase(); },
>prop : any
>_ : any
>x.toUpperCase() : string
>x.toUpperCase : () => string
>x : string
>toUpperCase : () => string

get prop() { return x.toUpperCase() },
>prop : any
>x.toUpperCase() : string
>x.toUpperCase : () => string
>x : string
>toUpperCase : () => string

method() { return x.toUpperCase() }
>method : () => string
>x.toUpperCase() : string
>x.toUpperCase : () => string
>x : string
>toUpperCase : () => string
}
}

11 changes: 11 additions & 0 deletions tests/cases/compiler/gettersAndSetters.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// @target: es2020
// classes
class C {
public fooBack = "";
Expand Down Expand Up @@ -37,3 +38,13 @@ interface I1 {
}

var i:I1 = function (n) {return n;}

Copy link
Member

Choose a reason for hiding this comment

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

Please add a // @target: es2020 (or any target newer than es3) at the top of this file. I wish our tests didn't target ES3 by default.

Copy link
Contributor Author

@softwareCobbler softwareCobbler Aug 20, 2021

Choose a reason for hiding this comment

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

yes, no problem

// Repro from #45006
const x: string | number = Math.random() < 0.5 ? "str" : 123;
if (typeof x === "string") {
let obj = {
set prop(_: any) { x.toUpperCase(); },
get prop() { return x.toUpperCase() },
method() { return x.toUpperCase() }
}
}