GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Improved checking of for-in statements by ahejlsberg · Pull Request #6379 · microsoft/TypeScript

Conversation

@ahejlsberg

@ahejlsberg Anders Hejlsberg (ahejlsberg) commented Jan 6, 2016

Copy link
Copy Markdown
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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Anders Hejlsberg (ahejlsberg) added 4 commits January 6, 2016 10:52
@ahejlsberg
Change for-in iteration variable type from any to string
a05b1de
@ahejlsberg
Updating existing tests
75b4c50
@ahejlsberg
Adding new tests
dbb4544
@ahejlsberg
Accepting new baselines
9ce5e64
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jan 6, 2016
Anders Hejlsberg (ahejlsberg) added 3 commits January 6, 2016 16:12
@ahejlsberg
Support for-in both with and without variable declaration
120569a
@ahejlsberg
Updating tests
1798024
@ahejlsberg
Merge branch 'master' into forInChecking
537fb4a
DanielRosenwasser
DanielRosenwasser reviewed Jan 7, 2016
View reviewed changes
Comment thread src/compiler/checker.ts

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Jan 7, 2016

Copy link
Copy Markdown
Member

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 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Jan 7, 2016

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Jan 7, 2016

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Jan 7, 2016

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Anders Hejlsberg (ahejlsberg) added 2 commits January 7, 2016 10:34
@ahejlsberg
Check for numeric index signature instead of array-like type
3344a42
Better error message when object with numeric index signature is indexed with a string
@ahejlsberg
Accepting new baselines
a0fcc0f
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Jan 7, 2016

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Jan 7, 2016
For var in object: Var is implicitly any without error #4518
Closed
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Jan 7, 2016
use string as type of variable in ForInStatement #6302
Closed
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Jan 8, 2016
Solve the for-in / indexer mess #6346
Closed
@ahejlsberg Anders Hejlsberg (ahejlsberg) added the Breaking Change Would introduce errors in existing code label Jan 8, 2016
Anders Hejlsberg (ahejlsberg) added a commit that referenced this pull request Jan 8, 2016
@ahejlsberg
Merge pull request #6379 from Microsoft/forInChecking
0bfc83b
Improved checking of for-in statements
@ahejlsberg
Anders Hejlsberg (ahejlsberg) merged commit 0bfc83b into master Jan 8, 2016
@ahejlsberg
Anders Hejlsberg (ahejlsberg) deleted the forInChecking branch January 8, 2016 21:49
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Feb 7, 2016
More specific JSON parse reviver argument in lib.d.ts #6955
Closed
@microsoft 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.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Breaking Change Would introduce errors in existing code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Solve the for-in / indexer mess

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@ahejlsberg @sandersn @DanielRosenwasser @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.