GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Control flow analysis for implicit any variables by ahejlsberg · Pull Request #11263 · microsoft/TypeScript Conversation
|
|
|
||
| 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.
The reason will be displayed to describe this comment to others. Learn more.
Can this work for [] or {} as well?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| location = location.parent; | ||
| } | ||
| if (isExpression(location) && !isAssignmentTarget(location)) { | ||
| if (isPartOfExpression(location) && !isAssignmentTarget(location)) { |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 old isExpression was renamed to isPartOfExpression and a new isExpression was introduced that doesn't look at the context (i.e. doesn't access the parent property).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| for (var x in new Date()) { } | ||
|
|
||
| var c, d, e; | ||
| var c: any, d: any, e: any; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
why add any here? Is it to preserve the intent of the tests — does control flow pick a union type for these variables now?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The test just needed some values of type any but they aren't actually meaningful once we start analyzing control flow.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // @noEmitHelpers: true | ||
| // @target: ES5 | ||
| declare var x, y, z, a, b, c; | ||
| declare var x: any, y: any, z: any, a: any, b: any, c: any; |
There was a problem 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 add any here?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 expressly don't want CFA here).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| //@target: ES5 | ||
| module M { | ||
| var Symbol; | ||
| var Symbol: any; |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| declare let cond: boolean; | ||
|
|
||
| function f1() { |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if (cond) { | ||
| x = "hello"; | ||
| } | ||
| const y = x; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
if you have if (cond) .. else then y: string | number, right? Might be nice to have a case for that.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Mohamed Hegazy (@mhegazy) Want to take a look? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
* Fix fialing tests after microsoft/TypeScript#11263 * Fix more failures
|
Anders Hejlsberg (@ahejlsberg) function f(x: number[]): void { ... } let x = []; // x: any[] x.push(3); // x: number[] f(x); // x is ok according to control flow, so no --noImplicitAny error? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
You are correct. this is not flagged as an implicit any error. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Mohamed Hegazy (@mhegazy) this is not safe, though. function f(x: number[]): void { setImmediate(function () { console.log(x.reduce((a, b) => a * b, 1)); }); } let x = []; x.push(3); f(x); x.push("I am not a number"); // ok, now x: (string|number)[] |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Typescript is already unsafe in this way. You can also write |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Never mind, I was completely wrong. It doesn't work. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Nathan Shively-Sanders (@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
jods (@jods4), the compiler today does not guard against side effects in multiple places. it is not specific to noImplicitAny checks. for instance: function f(x: { a: number }) { setTimeout(() => { x.a.toFixed(); }, 10); } var x: { 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Mohamed Hegazy (@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: let x = []; 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 let x; // this is not an error yet. x = 4; // so x: number at this point console.log(x.toFixed()); // sure x += 5; // and so on... let y; // same thing y = 4; // so y: number y = "Four"; // still an error when --noImplicitAny. Can't use dynamic types without explicit declaration. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.