Skip to content
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

Control flow analysis for array construction #11432

Merged
merged 24 commits into from
Oct 14, 2016
Merged
Show file tree
Hide file tree
Changes from 20 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
21 changes: 21 additions & 0 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,15 @@ namespace ts {
};
}

function createFlowArrayMutation(antecedent: FlowNode, node: CallExpression | BinaryExpression): FlowNode {
setFlowNodeReferenced(antecedent);
return <FlowArrayMutation>{
flags: FlowFlags.ArrayMutation,
antecedent,
node
};
}

function finishFlowLabel(flow: FlowLabel): FlowNode {
const antecedents = flow.antecedents;
if (!antecedents) {
Expand Down Expand Up @@ -1140,6 +1149,12 @@ namespace ts {
forEachChild(node, bind);
if (operator === SyntaxKind.EqualsToken && !isAssignmentTarget(node)) {
bindAssignmentTargetFlow(node.left);
if (node.left.kind === SyntaxKind.ElementAccessExpression) {
const elementAccess = <ElementAccessExpression>node.left;
if (isNarrowableOperand(elementAccess.expression)) {
currentFlow = createFlowArrayMutation(currentFlow, node);
}
}
}
}
}
Expand Down Expand Up @@ -1200,6 +1215,12 @@ namespace ts {
else {
forEachChild(node, bind);
}
if (node.expression.kind === SyntaxKind.PropertyAccessExpression) {
const propertyAccess = <PropertyAccessExpression>node.expression;
if (isNarrowableOperand(propertyAccess.expression) && isPushOrUnshiftIdentifier(propertyAccess.name)) {
currentFlow = createFlowArrayMutation(currentFlow, node);
}
}
}

function getContainerFlags(node: Node): ContainerFlags {
Expand Down
286 changes: 232 additions & 54 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

24 changes: 22 additions & 2 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,13 +265,33 @@ namespace ts {
if (array) {
result = [];
for (let i = 0; i < array.length; i++) {
const v = array[i];
result.push(f(v, i));
result.push(f(array[i], i));
}
}
return result;
}

// Maps from T to T and avoids allocation if all elements map to themselves
export function sameMap<T>(array: T[], f: (x: T, i: number) => T): T[] {
let result: T[];
if (array) {
for (let i = 0; i < array.length; i++) {
if (result) {
result.push(f(array[i], i));
}
else {
const item = array[i];
const mapped = f(item, i);
if (item !== mapped) {
result = array.slice(0, i);
result.push(mapped);
}
}
}
}
return result || array;
}

/**
* Flattens an array containing a mix of array or non-array elements.
*
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -2957,7 +2957,7 @@
"category": "Error",
"code": 7033
},
"Variable '{0}' implicitly has type 'any' in some locations where its type cannot be determined.": {
"Variable '{0}' implicitly has type '{1}' in some locations where its type cannot be determined.": {
"category": "Error",
"code": 7034
},
Expand Down
14 changes: 12 additions & 2 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1907,8 +1907,9 @@ namespace ts {
TrueCondition = 1 << 5, // Condition known to be true
FalseCondition = 1 << 6, // Condition known to be false
SwitchClause = 1 << 7, // Switch statement clause
Referenced = 1 << 8, // Referenced as antecedent once
Shared = 1 << 9, // Referenced as antecedent more than once
ArrayMutation = 1 << 8, // Potential array mutation
Referenced = 1 << 9, // Referenced as antecedent once
Shared = 1 << 10, // Referenced as antecedent more than once
Label = BranchLabel | LoopLabel,
Condition = TrueCondition | FalseCondition
}
Expand Down Expand Up @@ -1951,6 +1952,13 @@ namespace ts {
antecedent: FlowNode;
}

// FlowArrayMutation represents a node potentially mutates an array, i.e. an
// operation of the form 'x.push(value)', 'x.unshift(value)' or 'x[n] = value'.
export interface FlowArrayMutation extends FlowNode {
node: CallExpression | BinaryExpression;
antecedent: FlowNode;
}

export type FlowType = Type | IncompleteType;

// Incomplete types occur during control flow analysis of loops. An IncompleteType
Expand Down Expand Up @@ -2748,6 +2756,8 @@ namespace ts {
export interface AnonymousType extends ObjectType {
target?: AnonymousType; // Instantiation target
mapper?: TypeMapper; // Instantiation mapper
elementType?: Type; // Element expressions of evolving array type
Copy link
Member

@sandersn sandersn Oct 12, 2016

Choose a reason for hiding this comment

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

why isn't AutoArrayType a new subtype of AnonymousType? I think it would make it easier to track the property that auto array types don't escape the dynamic scope of getFlowTypeOfReference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow. autoArrayType is the declared type of an auto-inferred any[] and it's already used outside of getFlowTypeOfReference.

Copy link
Member

Choose a reason for hiding this comment

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

It's separate from autoArrayType, whose type is actually TypeReference.

I'm talking about the comment at the top of the new code that says

Evolving array types are ultimately converted into manifest array types
and never escape the getFlowTypeOfReference function."

I was suggesting something like:

export interface AutoArrayType extends AnonymousType {
  elementType?: Type;
  finalArrayType?: Type;
}

And then having the new code that returns AnonymousType today return AutoArrayType instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. The issue with doing it that way is that we don't have a TypeFlags.EvolvingArrayType that would indicate an evolving array type (because we're out of flag bits). Instead, we distinguish by looking for the elementType property on AnonymousType, so it has to be part of AnonymousType.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that makes sense.

finalArrayType?: Type; // Final array type of evolving array type
}

/* @internal */
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1895,6 +1895,10 @@ namespace ts {
return node.kind === SyntaxKind.Identifier && (<Identifier>node).text === "Symbol";
}

export function isPushOrUnshiftIdentifier(node: Identifier) {
return node.text === "push" || node.text === "unshift";
}

export function isModifierKind(token: SyntaxKind): boolean {
switch (token) {
case SyntaxKind.AbstractKeyword:
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/arrayLiterals2ES5.types
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,9 @@ var d5 = [...temp3];

var d6 = [...temp4];
>d6 : any[]
>[...temp4] : any[]
>...temp4 : any
>temp4 : any[]
>[...temp4] : undefined[]
>...temp4 : undefined
>temp4 : undefined[]

var d7 = [...[...temp1]];
>d7 : number[]
Expand Down
99 changes: 99 additions & 0 deletions tests/baselines/reference/controlFlowArrayErrors.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
tests/cases/compiler/controlFlowArrayErrors.ts(6,9): error TS7005: Variable 'y' implicitly has an 'any[]' type.
tests/cases/compiler/controlFlowArrayErrors.ts(14,9): error TS7005: Variable 'y' implicitly has an 'any[]' type.
tests/cases/compiler/controlFlowArrayErrors.ts(20,9): error TS7034: Variable 'x' implicitly has type 'any[]' in some locations where its type cannot be determined.
tests/cases/compiler/controlFlowArrayErrors.ts(23,9): error TS7005: Variable 'x' implicitly has an 'any[]' type.
tests/cases/compiler/controlFlowArrayErrors.ts(30,12): error TS2345: Argument of type 'true' is not assignable to parameter of type 'string | number'.
tests/cases/compiler/controlFlowArrayErrors.ts(35,12): error TS2345: Argument of type 'true' is not assignable to parameter of type 'string | number'.
tests/cases/compiler/controlFlowArrayErrors.ts(49,5): error TS2349: Cannot invoke an expression whose type lacks a call signature. Type '((...items: (string | number)[]) => number) | ((...items: boolean[]) => number)' has no compatible call signatures.
tests/cases/compiler/controlFlowArrayErrors.ts(57,12): error TS2345: Argument of type '"hello"' is not assignable to parameter of type 'number'.
tests/cases/compiler/controlFlowArrayErrors.ts(61,11): error TS7034: Variable 'x' implicitly has type 'any[]' in some locations where its type cannot be determined.
tests/cases/compiler/controlFlowArrayErrors.ts(64,9): error TS7005: Variable 'x' implicitly has an 'any[]' type.


==== tests/cases/compiler/controlFlowArrayErrors.ts (10 errors) ====

declare function cond(): boolean;

function f1() {
let x = [];
let y = x; // Implicit any[] error
~
!!! error TS7005: Variable 'y' implicitly has an 'any[]' type.
x.push(5);
let z = x;
}

function f2() {
let x;
x = [];
let y = x; // Implicit any[] error
~
!!! error TS7005: Variable 'y' implicitly has an 'any[]' type.
x.push(5);
let z = x;
}

function f3() {
let x = []; // Implicit any[] error in some locations
~
!!! error TS7034: Variable 'x' implicitly has type 'any[]' in some locations where its type cannot be determined.
x.push(5);
function g() {
x; // Implicit any[] error
~
!!! error TS7005: Variable 'x' implicitly has an 'any[]' type.
}
}

function f4() {
let x;
x = [5, "hello"]; // Non-evolving array
x.push(true); // Error
~~~~
!!! error TS2345: Argument of type 'true' is not assignable to parameter of type 'string | number'.
}

function f5() {
let x = [5, "hello"]; // Non-evolving array
x.push(true); // Error
~~~~
!!! error TS2345: Argument of type 'true' is not assignable to parameter of type 'string | number'.
}

function f6() {
let x;
if (cond()) {
x = [];
x.push(5);
x.push("hello");
}
else {
x = [true]; // Non-evolving array
}
x; // boolean[] | (string | number)[]
x.push(99); // Error
~~~~~~~~~~
!!! error TS2349: Cannot invoke an expression whose type lacks a call signature. Type '((...items: (string | number)[]) => number) | ((...items: boolean[]) => number)' has no compatible call signatures.
}

function f7() {
let x = []; // x has evolving array value
x.push(5);
let y = x; // y has non-evolving array value
x.push("hello"); // Ok
y.push("hello"); // Error
~~~~~~~
!!! error TS2345: Argument of type '"hello"' is not assignable to parameter of type 'number'.
}

function f8() {
const x = []; // Implicit any[] error in some locations
~
!!! error TS7034: Variable 'x' implicitly has type 'any[]' in some locations where its type cannot be determined.
x.push(5);
function g() {
x; // Implicit any[] error
~
!!! error TS7005: Variable 'x' implicitly has an 'any[]' type.
}
}
125 changes: 125 additions & 0 deletions tests/baselines/reference/controlFlowArrayErrors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
//// [controlFlowArrayErrors.ts]

declare function cond(): boolean;

function f1() {
let x = [];
let y = x; // Implicit any[] error
x.push(5);
let z = x;
}

function f2() {
let x;
x = [];
let y = x; // Implicit any[] error
x.push(5);
let z = x;
}

function f3() {
let x = []; // Implicit any[] error in some locations
x.push(5);
function g() {
x; // Implicit any[] error
}
}

function f4() {
let x;
x = [5, "hello"]; // Non-evolving array
x.push(true); // Error
}

function f5() {
let x = [5, "hello"]; // Non-evolving array
x.push(true); // Error
}

function f6() {
let x;
if (cond()) {
x = [];
x.push(5);
x.push("hello");
}
else {
x = [true]; // Non-evolving array
}
x; // boolean[] | (string | number)[]
x.push(99); // Error
}

function f7() {
let x = []; // x has evolving array value
x.push(5);
let y = x; // y has non-evolving array value
x.push("hello"); // Ok
y.push("hello"); // Error
}

function f8() {
const x = []; // Implicit any[] error in some locations
x.push(5);
function g() {
x; // Implicit any[] error
}
}

//// [controlFlowArrayErrors.js]
function f1() {
var x = [];
var y = x; // Implicit any[] error
x.push(5);
var z = x;
}
function f2() {
var x;
x = [];
var y = x; // Implicit any[] error
x.push(5);
var z = x;
}
function f3() {
var x = []; // Implicit any[] error in some locations
x.push(5);
function g() {
x; // Implicit any[] error
}
}
function f4() {
var x;
x = [5, "hello"]; // Non-evolving array
x.push(true); // Error
}
function f5() {
var x = [5, "hello"]; // Non-evolving array
x.push(true); // Error
}
function f6() {
var x;
if (cond()) {
x = [];
x.push(5);
x.push("hello");
}
else {
x = [true]; // Non-evolving array
}
x; // boolean[] | (string | number)[]
x.push(99); // Error
}
function f7() {
var x = []; // x has evolving array value
x.push(5);
var y = x; // y has non-evolving array value
x.push("hello"); // Ok
y.push("hello"); // Error
}
function f8() {
var x = []; // Implicit any[] error in some locations
x.push(5);
function g() {
x; // Implicit any[] error
}
}
Loading