- Notifications
You must be signed in to change notification settings - Fork13.2k
Control flow analysis for implicit any variables#11263
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
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.
A couple of questions each about the change and the tests, plus a couple of ideas for the tests.
| function isAutoVariableInitializer(initializer: Expression) { | ||
| const expr = initializer && skipParentheses(initializer); | ||
| return !expr || expr.kind === SyntaxKind.NullKeyword || expr.kind === SyntaxKind.Identifier && getResolvedSymbol(<Identifier>expr) === undefinedSymbol; |
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 this work for[] or{} as well?
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, but they each need specialized strategies and I'll cover them in separate PRs.
| location = location.parent; | ||
| } | ||
| if (isExpression(location) && !isAssignmentTarget(location)) { | ||
| if (isPartOfExpression(location) && !isAssignmentTarget(location)) { |
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 does this change and why? I'm guessing that now we can get the type of a symbol given a child of the symbol's declaration, but I can't guess why.
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.
This is a bug that somehow got introduced during some refactoring for the new emitter. The oldisExpression was renamed toisPartOfExpression and a newisExpression was introduced that doesn't look at the context (i.e. doesn't access theparent property).
| for(varxinnewDate()){} | ||
| varc,d,e; | ||
| varc:any,d:any,e:any; |
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 addany here? Is it to preserve the intent of the tests — does control flow pick a union type for these variables now?
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.
The test just needed some values of typeany but they aren't actually meaningful once we start analyzing control flow.
| //@noEmitHelpers: true | ||
| //@target: ES5 | ||
| declarevarx,y,z,a,b,c; | ||
| declarevarx:any,y:any,z:any,a:any,b:any,c:any; |
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 thought that auto typing didn't apply to ambient contexts. Why did you have to addany here?
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.
Yeah, you're right about that. But having them doesn't hurt and is a better indication of intent (i.e. we expresslydon't want CFA here).
| //@target: ES5 | ||
| moduleM{ | ||
| varSymbol; | ||
| varSymbol:any; |
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 understand why auto typing would apply here either
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.
Same here.
| declareletcond: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 if these functions had names that showed what they were testing.
| if(cond){ | ||
| x="hello"; | ||
| } | ||
| consty=x; |
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.
if you haveif (cond) .. else theny: string | number, right? Might be nice to have a case for that.
ahejlsberg commentedOct 3, 2016
@mhegazy Want to take a look? |
* Fix fialing tests aftermicrosoft/TypeScript#11263* Fix more failures
jods4 commentedNov 8, 2016
@ahejlsberg functionf(x:number[]):void{ ...}letx=[];// x: any[]x.push(3);// x: number[]f(x);// x is ok according to control flow, so no --noImplicitAny error? |
mhegazy commentedNov 8, 2016
You are correct. this is not flagged as an implicit any error. |
jods4 commentedNov 8, 2016
@mhegazy this is not safe, though. functionf(x:number[]):void{setImmediate(function(){console.log(x.reduce((a,b)=>a*b,1));});}letx=[];x.push(3);f(x);x.push("I am not a number");// ok, now x: (string|number)[] |
sandersn commentedNov 9, 2016
Typescript is already unsafe in this way. You can also write |
sandersn commentedNov 9, 2016
Never mind, I was completely wrong. It doesn't work. |
jods4 commentedNov 9, 2016
@sandersn But I think many people like TS for being (for the most part) statically type-safe and use Thus, I would suggest that you limit what is allowed by this feature or (harder) only allow cases that are provably correct (e.g. determine that the dynamic type never changes to an incompatible type). I can at least imagine the following operations to be potentially unsafe: saving the value in an object, an array or a variable in an outer scope; assigning the value to a setter; passing the value to a function; capturing the value in a closure; calling a member of the value;new-ing the value. |
mhegazy commentedNov 10, 2016
@jods4, the compiler today does not guard against side effects in multiple places. it is not specific to noImplicitAny checks. for instance: functionf(x:{a:number}){setTimeout(()=>{x.a.toFixed();},10);}varx:{kind:"number",a:number}|{kind:"string",a:string};if(x.kind==="number"){f(x);}x.a="string";// potentially unsafe, as x has already been captured. Arrays are just like objects with properties when it comes to narrowing. I do understand your concern, but really this issue is a general design limitation and not specific to this change. |
jods4 commentedNov 10, 2016
@mhegazy That said, having safety holes doesn't necessarily mean we should add new ones. I guess the question is: do benefits outweight the drawbacks? Your example is a lot more convoluted than the simple example I gave, which was just: letx=[];x.push(3);f(x);x.push("NaN"); I guess what annoys me most here is that this is unsafe because I use My opinion:
To maximize both safety and value,I would suggest that under letx;// this is not an error yet.x=4;// so x: number at this pointconsole.log(x.toFixed());// surex+=5;// and so on...lety;// same thingy=4;// so y: numbery="Four";// still an error when --noImplicitAny. Can't use dynamic types without explicit declaration. |
Uh oh!
There was an error while loading.Please reload this page.
This PR introduces control flow analysis for
letandvarvariables that have no type annotation and either no initial value or an initial value ofnullorundefined.In the example above,
xandyimplicitly have declared types ofanybut control flow analysis can determine theiractual types at every reference. Therefore, no errors are reported even when the example is compiled with--noImplicitAny.Control flow analysis is unable to determine the actual types of implicit
anyvariables when they are referenced in nested functions. In such cases, the variables will appear to have typeanyin the nested functions and errors will be reported for the references if--noImplicitAnyis enabled.In time it is possible that control flow analysis will be able to more accurately determine types of references to outer variables from nested functions insome cases, but given that nested functions are first class objects that can be arbitrarily passed around and called, it is effectively impossible to analyze all such scenarios.