- Notifications
You must be signed in to change notification settings - Fork13.2k
Control flow analysis for array construction#11432
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
zenmumbler commentedOct 11, 2016
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! |
src/compiler/core.ts Outdated
| returnresult; | ||
| } | ||
| // Maps from T to T and avoids allocation of all elements map to themselves |
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
ahejlsberg commentedOct 12, 2016
@zenmumbler No, this PR doesn't affect the issue in#11082. |
ahejlsberg commentedOct 12, 2016
@mhegazy Want to take a look before I merge this? |
sandersn left a comment
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.
functionf(){letx=[];x.push(12);x.push('oops');returnx;}functiong(){leta=g();a.push('oops 2');// shouldn't be allowedreturna;// g now has type (string | number)[]}letsum=0;for(constnofg()){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.
| var widenArray = [null, undefined]; // error at "widenArray" | ||
| ~~~~~~~~~~ | ||
| !!! error TS7005: Variable 'widenArray' implicitly has an 'any[]' type. | ||
| var emptyArray = []; // error at "emptyArray" |
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.
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.
| declarefunctioncond():boolean; | ||
| functionf1(){ |
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
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.
| x[1]="hello"; | ||
| x[2]=true; | ||
| returnx;// (string | number | boolean)[] | ||
| } |
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 doesx[0] = 5; x[0] = "hello"; x[0] = true? It would still return(string | number | boolean)[], right?
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.
src/compiler/binder.ts Outdated
| } | ||
| if(node.expression.kind===SyntaxKind.PropertyAccessExpression){ | ||
| constpropertyAccess=<PropertyAccessExpression>node.expression; | ||
| if(isNarrowableOperand(propertyAccess.expression)&&propertyAccess.name.text==="push"){ |
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 aboutunshift? I used unshift briefly when writing spread types. :)
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 supportunshift, uncommon as it may be.
src/compiler/checker.ts Outdated
| } | ||
| function isEmptyArrayAssignment(node: VariableDeclaration | BindingElement | Expression) { | ||
| return node.kind === SyntaxKind.VariableDeclaration && (<VariableDeclaration>node).initializer && isEmptyArrayLiteral((<VariableDeclaration>node).initializer) || |
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.
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.
| return incomplete ? { flags: 0, type } : type; | ||
| } | ||
| // An evolving array type tracks the element types that have so far been seen in an |
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
| exportinterfaceAnonymousTypeextendsObjectType{ | ||
| target?:AnonymousType;// Instantiation target | ||
| mapper?:TypeMapper;// Instantiation mapper | ||
| elementType?:Type;// Element expressions of evolving array type |
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 ofgetFlowTypeOfReference.
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 thedeclared type of an auto-inferredany[] and it's already used outside ofgetFlowTypeOfReference.
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 fromautoArrayType, 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:
exportinterfaceAutoArrayTypeextendsAnonymousType{elementType?:Type;finalArrayType?:Type;}
And then having the new code that returns AnonymousType today return AutoArrayType instead.
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 aTypeFlags.EvolvingArrayType that would indicate an evolving array type (because we're out of flag bits). Instead, we distinguish by looking for theelementType property onAnonymousType, so it has to be part ofAnonymousType.
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.
src/compiler/checker.ts Outdated
| getUnionType(sameMap(types, finalizeEvolvingArrayType), subtypeReduction); | ||
| } | ||
| // Return true if the given node is 'x' in an 'x.push(value)' operation. |
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
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 |
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
ahejlsberg commentedOct 13, 2016
With the latest commits, when an evolving array has no functionf1(){leta=[];if(cond()){a.push("hello");}a;// string[]}functionf2(){leta=[];a;// any[], error with --noImplicitAny} Note that references to the functionf3(){leta=[];while(a.length<10){a.push("test");}returna;// string[]} |
ahejlsberg commentedOct 13, 2016
@mhegazy Latest commits should solve the issue we discussed yesterday. |
# Conflicts:#src/compiler/checker.ts
Uh oh!
There was an error while loading.Please reload this page.
This PR introduces control flow analysis for array construction patterns that originate in an empty array literal (
x = []) followed by some number ofx.push(value),x.unshift(value)orx[n] = valueoperations.A
let,const, orvarvariable declared with no type annotation and an initial value of[]is considered an implicitany[]variable. When an implicitanyvariable (see#11263) or implicitany[]variable is assigned an empty array literal[], each followingx.push(value),x.unshift(value)orx[n] = valueoperationevolves the type of the variable in accordance with what elements are added to it.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.
Similar to implicit
anyvariables, control flow analysis is unable to determine the actual types of implicitany[]variables when they are referenced in nested functions. In such cases, the variables will appear to have typeany[]in the nested functions and errors will be reported for the references if --noImplicitAny is enabled.