Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
ahejlsberg merged 16 commits intomasterfromcontrolFlowLetVar
Oct 6, 2016

Conversation

@ahejlsberg
Copy link
Member

@ahejlsbergahejlsberg commentedSep 29, 2016
edited
Loading

This PR introduces control flow analysis forlet andvar variables that have no type annotation and either no initial value or an initial value ofnull orundefined.

functionf(cond:boolean){letx;if(cond){x="hello";x;// string}else{x=123;x;// number}returnx;// string | number}functiong(cond:boolean){lety=null;if(cond){y="hello";}returny;// string | null}

In the example above,x andy implicitly have declared types ofany but 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 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.

functionf(cond:boolean){letx;// Error: Variable 'x' implicitly has type 'any' in some locations where its type cannot be determined.if(cond){x="hello";}else{x=123;}returnx;// type string | numberfunctionbar(){consty=x;// Error: Variable 'x' implicitly has an 'any' type.}}

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.

tinganho, normalser, weswigham, mpawelski, geoffreak, aciccarello, lazdmx, and unional reacted with thumbs up emojimpawelski reacted with hooray emojibenjamin21st reacted with confused emojis-panferov and mpawelski reacted with heart emoji
Copy link
Member

@sandersnsandersn left a 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;
Copy link
Member

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?

Copy link
MemberAuthor

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.

sandersn reacted with hooray emoji
location = location.parent;
}
if (isExpression(location) && !isAssignmentTarget(location)) {
if (isPartOfExpression(location) && !isAssignmentTarget(location)) {
Copy link
Member

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.

Copy link
MemberAuthor

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;
Copy link
Member

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?

Copy link
MemberAuthor

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;
Copy link
Member

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?

Copy link
MemberAuthor

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;
Copy link
Member

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

Copy link
MemberAuthor

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(){
Copy link
Member

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;
Copy link
Member

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
Copy link
MemberAuthor

@mhegazy Want to take a look?

@ahejlsbergahejlsberg merged commit44c475f intomasterOct 6, 2016
@ahejlsbergahejlsberg deleted the controlFlowLetVar branchOctober 6, 2016 21:23
mhegazy added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull requestOct 7, 2016
mhegazy added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull requestOct 7, 2016
@jods4
Copy link

@ahejlsberg
Reading the descriptions, I have the impression that the following code example won't be a--noImplicitAny error. Am I wrong?

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
Copy link
Contributor

Am I wrong?

You are correct. this is not flagged as an implicit any error.

@jods4
Copy link

@mhegazy this is not safe, though.
Consider thatf() might use thex value later (through capture or saving into a global variable) and the code might arbitrarily changex.

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
Copy link
Member

Typescript is already unsafe in this way. You can also write

let sns: (string | number)[];f(sns);

@sandersn
Copy link
Member

Never mind, I was completely wrong. It doesn't work.

@jods4
Copy link

@sandersn
Yeah, I like that this enables more errors to be caught when migrating (untyped) JS code.
I like that you now can drop more type declarations for local variables as long as you use them correctly.

But I think many people like TS for being (for the most part) statically type-safe and usenoImplicitAny to guarantee that no dynamically typed code slips through, i.e.as a way to enforce type safety.
Currently, this PR weakens the guarantees provided bynoImplicitAny and allows some unsafe code that was not possible before.

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
Copy link
Contributor

@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
Copy link

@mhegazy
Yes, I agree.
That's why I said TS isfor the most part statically type-safe, which is already a lot 😃

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 usex in a dynamic way, without declaring my intent, undernoImplicitAny. I think the essence ofnoImplicitAny is to forbid errors from unwanted dynamic code.

My opinion:

  1. This is great for migrating existing JS code andI wouldn't change anything in this context.
  2. I believe people who usenoImplicitAny have little use for that feature. Typically those people want maximum safety and try to have everything typed. In this context and with all the inference we already have, you seldom have the need for this shortcut. In my company we build web apps exclusively in this model and I can't say I ever had feature envy for that.

To maximize both safety and value,I would suggest that undernoImplicitAny, the dynamic type of locals cannot be changed. I.e.:

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.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@sandersnsandersnsandersn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ahejlsberg@jods4@mhegazy@sandersn@msftclas

[8]ページ先頭

©2009-2025 Movatter.jp