Skip to content

Commit

Permalink
fix for 45006 (#45020)
Browse files Browse the repository at this point in the history
* fix for 45006

* treat setters like getters in preceding commit; move test accordingly

* fix test baselines

* changes per code review

in `getContainerFlags`, move cases for get/set accessors
to fallthrough into the block that currently handles MethodDeclaration;
so get/set accessors and method declarations all get the same container flags,
such that during `bindContainer`, `startFlow.node` is assigned to
getters/accessors
(this changes a public api in tsserverlibrary.d.ts and typescript.d.ts
by adding `GetAccessorDeclaration` and `SetAccessorDeclaration` to the type
of `FlowStart.node`)

consolidate predicates checking whether a node is either a get or set
accessor, into `isObjectLiteralOrClassExpressionMethodOrAccessor`
(formerly `isObjectLiteralOrClassExpressionMethod`)

annotate updated test with `@target: es2020`

* fix `isObjectLiteralOrClassExpressionMethodOrAccessor`

require that Getter/Setters are parented by an ObjectLiteralExpression or ClassExpression
  • Loading branch information
softwareCobbler committed Aug 21, 2021
1 parent 65ed412 commit d18e82b
Show file tree
Hide file tree
Showing 11 changed files with 144 additions and 54 deletions.
16 changes: 8 additions & 8 deletions src/compiler/binder.ts
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)) {
if (currentFlow && isObjectLiteralOrClassExpressionMethodOrAccessor(node)) {
node.flowNode = currentFlow;
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
Expand Up @@ -24492,7 +24492,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
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
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
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
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
@@ -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
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
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
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
@@ -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;}

// 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() }
}
}

0 comments on commit d18e82b

Please sign in to comment.