Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved checking of for-in statements #6379

Merged
merged 9 commits into from
Jan 8, 2016
Merged

Improved checking of for-in statements #6379

merged 9 commits into from
Jan 8, 2016

Conversation

ahejlsberg
Copy link
Member

This PR fixes #6346 by implementing two changes:

  • The type of a variable declared in a for-in statement is implicitly string (previously it was any).
  • When an object with a numeric index signature of type T (such as an array) is indexed by a for-in variable of a containing for-in statement for an object with a numeric index signature and without a string index signature (again such as an array), the value produced is of type T.

An example:

var a: MyObject[];
for (var x in a) {   // Type of x is implicitly string
    var obj = a[x];  // Type of obj is MyObject
}

With this PR, the type of x is now string and the type of obj is MyObject because the array is indexed with a for-in variable for an array (and thus the for-in variable is known to contain a numeric string). Had the array been indexed with any other string expression, the type of obj would be any because the string could be the name of a property or method (e.g. "length" or "push").

For historical reasons we have used type any for for-in variables even though the iterated values are always strings. The reason we used type any is that when an array is indexed with an expression of type any or number we produce a value of the array element type, and thus the example above produces the correct type for obj. However, with the second new rule above, indexing with an expression of type string can now also produce a value of the array element type if the expression is a for-in variable for an array.

The use of type any was never a great solution, in particular because the any is "silent" and not caught by the -noImplicitAny flag. Furthermore, with the introduction of the new for-of statement in ES2015 it got worse. Consider:

var a: MyObject[];
for (var x in a) {       // Accidentally used for-in instead of for-of
    x.myObjectMethod();  // Reports an error now, but didn't before
}

This PR is a breaking change for certain (suspicious) constructs. For example:

var a: MyObject[];
for (var x in a) {
    if (x == 1) {  // Previously ok, now an error
        // ...
    }
}

Previously, the static type of x was of type any. Thus, even though x contains a string it could be compared to a numeric literal (at run-time this coerces the string operand to a number and then compares that to the numeric operand). This is one of the not so good parts of JavaScript. Mixing of strings and numbers only work for some operators and are a source of many a surprising bug (for example, it doesn't work with the === operator, and + concatenates string values instead of adding the numeric values).

if (symbol.flags & SymbolFlags.Variable) {
let child: Node = expr;
let node = expr.parent;
while (node) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to climb up? Can't you look up the variable declaration from the symbol and just check if the parent is a ForInStatement?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I realized it's partially because you might have multiple variable declarations. But in that case, you'll have gotten an error on inconsistent types between the two variables anyway, so maybe it doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Several reasons we need to climb up. Multiple declarations is one, making sure we're actually within a for-in statement is another, supporting for-in that references a variable instead of declaring one is a third.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha. Maybe you should add a comment in case anyone stumbles along and tries to "optimize" it in the future

Better error message when object with numeric index signature is indexed with a string
@ahejlsberg
Copy link
Member Author

After looking at some real-world code I've changed the test for an "array-like" object to be an object that has a numeric index signature and has no string index signature (as we originally contemplated). I found several instances of the following pattern:

var fooMap: { [x: number]: Foo } = {};
// ...
for (var i in fooMap) {
    var foo = fooMap[i];  // Expect type of foo to be Foo
    // ...
}

With the latest changes we'll assume that a for-in variable for an object with a numeric index signature and without a string index signature will iterate over a sequence of numeric property names, and when such a variable is used to index an object, the numeric index signature applies.

Also, I've added a better error message for the case where an object with a numeric index signature is indexed with a non-numeric value (such as a string). See the latest baseline changes for examples.

@ahejlsberg ahejlsberg added the Breaking Change Would introduce errors in existing code label Jan 8, 2016
ahejlsberg added a commit that referenced this pull request Jan 8, 2016
Improved checking of for-in statements
@ahejlsberg ahejlsberg merged commit 0bfc83b into master Jan 8, 2016
@ahejlsberg ahejlsberg deleted the forInChecking branch January 8, 2016 21:49
@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.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Solve the for-in / indexer mess
4 participants