GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Control flow analysis for array construction by ahejlsberg · Pull Request #11432 · microsoft/TypeScript

Conversation

@ahejlsberg

@ahejlsberg Anders Hejlsberg (ahejlsberg) commented Oct 6, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This PR introduces control flow analysis for array construction patterns that originate in an empty array literal (x = []) followed by some number of x.push(value), x.unshift(value) or x[n] = value operations.

A let, const, or var variable declared with no type annotation and an initial value of [] is considered an implicit any[] variable. When an implicit any variable (see #11263) or implicit any[] variable is assigned an empty array literal [], each following x.push(value), x.unshift(value) or x[n] = value operation evolves the type of the variable in accordance with what elements are added to it.

function f1() {
    let x = [];
    x[0] = 5;
    x[1] = "hello";
    x[2] = true;
    return x;  // (string | number | boolean)[]
}

function f2() {
    let x = [];
    x.push(5);
    x.push("hello");
    x.push(true);
    return x;  // (string | number | boolean)[]
}

function f3() {
    let x = null;
    if (cond()) {
        x = [];
        while (cond()) {
            x.push("hello");
        }
    }
    return x;  // string[] | null
}

An array type is evolved only by operations on the variable in which the evolving array type originated. When an evolving array type variable is referenced in an expression, its type is "fixed" and cannot be further evolved.

function f4() {
    let x = [];       // x has evolving array type
    x.push(5);
    let y = x;        // y has fixed array type
    x.push("hello");  // Ok
    y.push("hello");  // Error
}

Similar to implicit any variables, control flow analysis is unable to determine the actual types of implicit any[] variables when they are referenced in nested functions. In such cases, the variables will appear to have type any[] in the nested functions and errors will be reported for the references if --noImplicitAny is enabled.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
normalser, svieira, sanex3339, weswigham, aluanhaddad, basarat, mpawelski, and elvircrn reacted with thumbs up emoji svieira, tinganho, sanex3339, aluanhaddad, and mpawelski reacted with hooray emoji alitaheri and aluanhaddad reacted with heart emoji
All reactions
  • 👍 8 reactions
  • 🎉 5 reactions
  • ❤️ 2 reactions
Anders Hejlsberg (ahejlsberg) added 6 commits October 6, 2016 11:04
@ahejlsberg
Use control flow analysis for variables initialized with []
9ca2569
@ahejlsberg
Accept new baselines
b5c1055
@ahejlsberg
Introduce sameMap function
9c4190b
@ahejlsberg
Accept new baselines
144f4ff
@ahejlsberg
Add tests
a9c2b23
@ahejlsberg
Accept new baselines
c0c2271
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Oct 6, 2016
Anders Hejlsberg (ahejlsberg) added 6 commits October 6, 2016 14:36
@ahejlsberg
Merge branch 'master' into controlFlowArrays
b8d5226
@ahejlsberg
Fix some minor issues
12906a7
@ahejlsberg
Add additional test
2f5af2e
@ahejlsberg
Fix issue in control flow analysis of nested loops
71b9b33
@ahejlsberg
Add test
d202b1c
@ahejlsberg
Accept new baselines
89826a5
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Oct 10, 2016
Design Meeting Notes, 10/10/2016 #11505
Closed
Anders Hejlsberg (ahejlsberg) added 4 commits October 11, 2016 10:14
@ahejlsberg
Support parentheses and comma operator with evolving arrays
e6b588a
@ahejlsberg
Fix minor issue
612ed1e
@ahejlsberg
Add test
e9858de
@ahejlsberg
Accept new baselines
c9e2f95
@zenmumbler

zenmumbler commented Oct 11, 2016

Copy link
Copy Markdown

Will this affect a problem (#11082) I submitted earlier? This sounds like its different enough from my problem but wanted to check if there is any overlap, thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sandersn
Nathan Shively-Sanders (sandersn) reviewed Oct 11, 2016
View reviewed changes
Comment thread src/compiler/core.ts Outdated
return result;
}

// Maps from T to T and avoids allocation of all elements map to themselves

@sandersn Nathan Shively-Sanders (sandersn) Oct 11, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

typo: of → if

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Oct 12, 2016

Copy link
Copy Markdown
Member Author

zenmumbler No, this PR doesn't affect the issue in #11082.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg
Fix typo
38278ee
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Oct 12, 2016

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) Want to take a look before I merge this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sandersn
Nathan Shively-Sanders (sandersn) requested changes Oct 12, 2016
View reviewed changes

@sandersn Nathan Shively-Sanders (sandersn) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Besides a few small changes, I also want to make sure we're OK with less localised errors in a series of functions with no return type annotations. In the example below, the error moves from the line with 'oops' on it to the last line.

function f() {
  let x = [];
  x.push(12);
  x.push('oops');
  return x;
}
function g() {
  let a = g();
  a.push('oops 2'); // shouldn't be allowed
  return a; // g now has type (string | number)[]
}
let sum = 0;
for (const n of g()) {
  sum += n; // error, can't add (string | number) to number
}

Right now, I think I'm on the side of fewer annotations, but I think we should know about the pitfalls of the new behaviour.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/baselines/reference/implicitAnyWidenToAny.errors.txt Outdated
var widenArray = [null, undefined]; // error at "widenArray"
~~~~~~~~~~
!!! error TS7005: Variable 'widenArray' implicitly has an 'any[]' type.
var emptyArray = []; // error at "emptyArray"

@sandersn Nathan Shively-Sanders (sandersn) Oct 11, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Delete comment and consider deleting the entire line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 13, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, will fix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/cases/compiler/controlFlowArrays.ts

declare function cond(): boolean;

function f1() {

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It would be nice to have meaningful names for these test cases

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 13, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think we're fine with the short names.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/cases/compiler/controlFlowArrays.ts
x[1] = "hello";
x[2] = true;
return x; // (string | number | boolean)[]
}

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

what about a test case that case that does x[0] = 5; x[0] = "hello"; x[0] = true? It would still return (string | number | boolean)[], right?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 13, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, you'd get the same type. We don't look at the value of the index expression, so nothing gained by an additional test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/binder.ts Outdated
}
if (node.expression.kind === SyntaxKind.PropertyAccessExpression) {
const propertyAccess = <PropertyAccessExpression>node.expression;
if (isNarrowableOperand(propertyAccess.expression) && propertyAccess.name.text === "push") {

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

what about unshift? I used unshift briefly when writing spread types. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 13, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yup, we should support unshift, uncommon as it may be.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts Outdated
}

function isEmptyArrayAssignment(node: VariableDeclaration | BindingElement | Expression) {
return node.kind === SyntaxKind.VariableDeclaration && (<VariableDeclaration>node).initializer && isEmptyArrayLiteral((<VariableDeclaration>node).initializer) ||

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can you break these lines up a bit more? They are really hard to read on github.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 13, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts
return incomplete ? { flags: 0, type } : type;
}

// An evolving array type tracks the element types that have so far been seen in an

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

comment should be jsdoc formatted

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/types.ts
export interface AnonymousType extends ObjectType {
target?: AnonymousType; // Instantiation target
mapper?: TypeMapper; // Instantiation mapper
elementType?: Type; // Element expressions of evolving array type

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 13, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Oct 13, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 13, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Oct 13, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ok, that makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts Outdated
getUnionType(sameMap(types, finalizeEvolvingArrayType), subtypeReduction);
}

// Return true if the given node is 'x' in an 'x.push(value)' operation.

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

should support unshift here too

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts Outdated
visitedFlowCount = visitedFlowStart;
if (reference.parent.kind === SyntaxKind.NonNullExpression && getTypeWithFacts(result, TypeFacts.NEUndefinedOrNull).flags & TypeFlags.Never) {
// When the reference is 'x' in an 'x.push(value)' or 'x[n] = value' operation, we give type
// 'any[]' to 'x' instead of using the type determed by control flow analysis such that new

@sandersn Nathan Shively-Sanders (sandersn) Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

typo:determined

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Anders Hejlsberg (ahejlsberg) added 3 commits October 13, 2016 09:43
@ahejlsberg
Support 'unshift' and fix typo
1dedca7
@ahejlsberg
Fix test
620b3f9
@ahejlsberg
Add additional tests
a27a68f
sandersn
Nathan Shively-Sanders (sandersn) approved these changes Oct 13, 2016
View reviewed changes
Anders Hejlsberg (ahejlsberg) added 2 commits October 13, 2016 13:28
@ahejlsberg
Treat reference to empty evolving array as an implicit any[]
bf301e9
@ahejlsberg
Update tests
bfa4197
@ahejlsberg
Accept new baselines
79ed3a7
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Oct 13, 2016

Copy link
Copy Markdown
Member Author

With the latest commits, when an evolving array has no x.push(value), x.unshift(value) or x[n] = value operations in any control flow path leading to the current location, its type is implicitly any[]. Thus, any reference to such an empty evolving array will produce an error with --noImplicitAny:

function f1() {
    let a = [];
    if (cond()) {
        a.push("hello");
    }
    a;  // string[]
}

function f2() {
    let a = [];
    a;  // any[], error with --noImplicitAny
}

Note that references to the length property are always allowed since they don't depend on the element type of the array:

function f3() {
    let a = [];
    while (a.length < 10) {
        a.push("test");
    }
    return a;  // string[]
}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Oct 13, 2016

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) Latest commits should solve the issue we discussed yesterday.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg
Merge branch 'master' into controlFlowArrays
c876d92 
# Conflicts:
#	src/compiler/checker.ts
@ahejlsberg
Anders Hejlsberg (ahejlsberg) merged commit b5d1e4c into master Oct 14, 2016
@ahejlsberg
Anders Hejlsberg (ahejlsberg) deleted the controlFlowArrays branch October 14, 2016 23:28
@zhuravlik Anton (zhuravlik) mentioned this pull request Jan 31, 2017
Control flow analysis for implicit any and array initialization works only when --noImplicitAny is on #13779
Closed
@ghost ghost mentioned this pull request Jun 2, 2017
Infer variable type form return type #16218
Closed
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Aug 16, 2017
Some code written in strict mode is invalid in non-strict mode #17774
Closed
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Sep 5, 2017
Error on excessively large control flow graphs #18246
Merged
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Oct 23, 2017
implicitAny doesn't work for 2.5.2 #19416
Closed
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersn Nathan Shively-Sanders (sandersn) sandersn approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@ahejlsberg @zenmumbler @sandersn @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.