Skip to content

Disallow primitive assignability to indexer of type any #5267

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

Merged
merged 3 commits into from
Oct 19, 2015

Conversation

sandersn
Copy link
Member

Fixes #4588.

string/numberIndexTypesRelatedTo needs to prevent primitives from being
assignable to an indexer of type 'any'. However, these two functions take
an apparent type, which no longer has the primitive flag set. I thought of
three ways to provide this information:

  1. Pass the original type into string/numberIndexTypesRelatedTo and
    check its flag.
  2. Record a boolean isPrimitive before converting to the apparent type,
    and pass it to string/numberIndexTypesRelatedTo.
  3. Create a helper function isPrimitive that takes the apparent type and
    compares it to globalString/Number/Boolean/ESSymbolType.

I decided on (1) because it seems like the simplest and safest. But none
of the options are elegant. Please suggest improvements.

`string/numberIndexTypesRelatedTo` needs to prevent primitives from being
assignable to an indexer of type 'any'. However, these two functions take
an apparent type, which no longer has the primitive flag set. I thought of
three ways to provide this information:

1. Pass the original type into `string/numberIndexTypesRelatedTo` and
check its flag.
2. Record a boolean `isPrimitive` before converting to the apparent type,
and pass it to `string/numberIndexTypesRelatedTo`.
3. Create a helper function `isPrimitive` that takes the apparent type and
compares it to globalString/Number/Boolean/ESSymbolType.

I decided on (1) because it seems like the simplest and safest. But none
of the options are elegant. Please suggest improvements.
@RyanCavanaugh
Copy link
Member

Is there a bug tracking this change? I'd like to have more context on what we're saying is desirable/non-desirable

@sandersn
Copy link
Member Author

Yes, I'll remember someday to add a link to the bug number: #4588. And it's an improvement to #4074 which was a little too permissive.

@RyanCavanaugh
Copy link
Member

FWIW I was interested in the status of this code

interface X {
    [n: number]: any;
}

var x: X;
x = 'hello';

which seems to still compile under this branch, which is what I'd expect.

@sandersn
Copy link
Member Author

The test I added in assignmentCompat1.ts has this case, but I didn't think about it enough and expected it to fail (I forgot that strings do support numeric indexing in typescript). I'll add another test case like

var x: { [n: number]: any } = false

which should (and does) fail.

Existing: String assignment to a numeric indexer should succeed, not fail.
  (The baseline was already correct but the inline comment was wrong.)
New: Boolean assignment to a numeric indexer should fail.
@sandersn
Copy link
Member Author

Ping @RyanCavanaugh -- can you take another look now that I added more test cases?

@RyanCavanaugh
Copy link
Member

👍

sandersn added a commit that referenced this pull request Oct 19, 2015
…ity-to-index-of-any

Disallow primitive assignability to indexer of type any
@sandersn sandersn merged commit f9c40d1 into master Oct 19, 2015
@sandersn sandersn deleted the disallow-primitive-assignability-to-index-of-any branch October 19, 2015 22:48
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants