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

Index signature arguments are not type checked based on the specialized key type they are constrained to #7660

Closed
malibuzios opened this issue Mar 23, 2016 · 10 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@malibuzios
Copy link

TypeScript Version:

1.8.7

Code

type OnlyNumberKeys = {[key: number]: any};
let a: OnlyNumberKeys = {};

a["abcd"] = 1; // No error?
let x = a["abcd"]; // No error?
type OnlyStringKeys = {[key: string]: any};
let b: OnlyStringKeys = {};

b[1234] = 1; // No error?
let y = b[1234]; // No error?

Expected behavior:
Should error

Actual behavior:
Doesn't error

(Sorry if this is a duplicate but a search did not yield anything on this)

@RyanCavanaugh
Copy link
Member

The first example is an error under noImplicitAny.

The second is by design -- indexing by a number is a subset of indexing by a string, so indexing by a number when a string would have been valid is also valid.

@malibuzios malibuzios changed the title Indexer keys are not type checked Indexer keys are not checked based on the specialized key type they are constrained to Mar 23, 2016
@malibuzios malibuzios changed the title Indexer keys are not checked based on the specialized key type they are constrained to Indexer keys are not type checked based on the specialized key type they are constrained to Mar 23, 2016
@malibuzios
Copy link
Author

@RyanCavanaugh

I'm not sure exactly what the logic is on why the first one errors on noImplicitAny? and why not in standard mode?

I renamed the title to a more accurate one, as the keys are of course checked if they receive something like an object or a boolean, but not if the key type is one of the generally valid one (i.e. string, number, Symbol).

I also tested in ES6 mode for Symbol, and it also did not error on both cases:

type OnlyNumberKeys = { [key: number]: any };
let a: OnlyNumberKeys = {};

a[Symbol("abcd")] = 1; // No error?
let x = a[Symbol("abcd")]; // No error?
type OnlyStringKeys = { [key: string]: any };
let b: OnlyStringKeys = {};

b[Symbol("abcd")] = 1; // No error?
let y = b[Symbol("abcd")]; // No error?

@jods4
Copy link

jods4 commented Mar 24, 2016

Indexing is special because it's the built-in JS way to perform "reflection", i.e. to access a field dynamically at runtime.

type X = { length: number; [key: number]: string };
let some: X;
let len = some['length']; // equivalent to some.length;
let dynamic = "length";
len = some[dynamic]; // works at runtime, treated as any at compile time

And because the result of dynamic access has to be any, doing so is an error under --noImplicitAny. But it turned out to be an important enough scenario to be allowed even in no implicit any code, with its sibling option --suppressImplicitAnyIndexErrors.

I think a similar argument can be made for Symbol.

@malibuzios
Copy link
Author

@jods4 @RyanCavanaugh

I'm aware that square bracket notation was designed to become a "back door" where anything can be accessed. I do use it myself occasionally. However, I don't remember a case I needed it when a type had an explicit index signature. The reason is very simple:

Since an index signature was designed to "open up" access to what is usually a large amount of key space, such as "all the strings", "all the numbers" and even "all the symbols" (though that is not supported yet, I opened issue #7667 for that) or a combination of them. One can always write something like:

type OnlyStringOrNumberKeys = { [key: string | number]: any };
type OnlyStringOrSymbolKeys = { [key: string | symbol]: any };
type OnlyNumberOrSymbolKeys = { [key: number | symbol]: any };
type AllPossibleKeys = { [key: string | number | symbol]: any };

I mean what really is the benefit of allowing the programmer to specify a parameter type if it is not actually enforced?

There's a very good reason I'm mentioning this at this particular time. The reason is that in light of a proposal like #5683 (also see the duplicate #7656) which is currently in discussion, where the key would be constrained to a finite set of string literals or possibly numbers (with numeric literals), there's not much point of implementing it if key types are not fully checked. E.g. you had something like:

type OnlySomeStringLiteralKeys = { [key: "A" | "B" | "C"]: any };
x: OnlySomeStringLiteralKeys = {};

x["D"] = 1; // Error, specified key type is not assignable to "A" | "B" | "C"

I can't see how that constraint would be truly realized if you can still, "by design" do things like:

x[4385] = "a";
x[Symbol("abcd")] = 12;

?

@malibuzios malibuzios changed the title Indexer keys are not type checked based on the specialized key type they are constrained to Index signature arguments are not type checked based on the specialized key type they are constrained to Mar 24, 2016
@jods4
Copy link

jods4 commented Mar 24, 2016

I mean what really is the benefit of allowing the programmer to specify a parameter type if it is not actually enforced?

You make a good point.

But I think disallowing dynamic access completely on those objects is too much collateral damage.

In noImplicitAny code, but with suppressImplicitAnyIndexErrors, I have seen this done:

let arr: number[];

let x = arr[4]; // array indexer -> x: number
// dynamically augment the array with metadata or additional infos
arr['priority'] = 'high';  // ok...

This is certainly not the best code, and you probably should use an intersection type to describe your array number[] & Metadata but just sayin' I've seen things like that.

Not sure what the best approach is here.
Some thoughts:

  • If an indexed access is specific (with upcoming string or symbol literals support), then it should be OK and properly typed.
  • Otherwise, if you run without noImplicitAny or with suppressImplicitAnyIndexErrors I don't think you really can forbid a dynamic access?

Perhaps some real-world examples could help better define a solution?

@malibuzios
Copy link
Author

@jods4

I'm not sure if internally Array<T> type is bound by the rules of the { [x: number]: T } interface. I think it may be a type that is handled separately (though I could be wrong). I can understand that in other types like Node's Buffer that may actually be the case. So you're right that for something like:

interface NodeBuffer {
    [index: number]: number;
        ... Lots of properties follow ...
}

It would break the ability to "back door" into, say, undocumented properties. The simplest workaround that one would need to either cast it to any:

let buf: Buffer;
(<any> buf)["undocumentedProperty"] = 12;

Or use some sort of a intersection type (though I don't think that would be something that would appeal or even be understood by beginners).

But I can see this becoming a bit tedious when it is frequently used, and it would break some existing code, so I agree this would be a major problem if key checking was stricter. Unfortunately I don't really have a good alternative solution for that at this point.

Anyway, as for a real-world example (other the ones described in #7656 for the string literal keys). In the case of "symbols keys only" signatures I could see an example where I have a function that only cares about the metadata hidden inside some object so ideally it would be nice to have a type that basically sees anything thrown at it as just a bunch of symbols, and no more:

function iOnlyCareAboutTheSymbolsInWhateverYouGiveMe(x: any) {
  let symbolsOnlyHere: { [key: symbol]: any } = x;
  // ... do something with the hidden metadata ...
}

@jods4
Copy link

jods4 commented Mar 24, 2016

Nitpicking maybe, but it's not just a "backdoor". It's how you can access members dynamically at runtime.
There are good use cases for that. E.g. try to write a serializer, a string format or an ORM without it.
It's like saying Reflection in .net is a "backdoor", although it's the backbone of many frameworks and features.

Regarding examples:

Probably the most compelling example so far is this one of yours:

type RequestMethod = "GET" | "SET" | "PUT" | "UPDATE" | "DELETE";
type HandlerObject = { [key: RequestMethod]: (requestObject: RequestObject) => boolean; };

I think this is desirable but I'm not sure what the best way to proceed here is.

Another thing I haven't seen mentionned yet: how should it work with generics?

function dump<T>(obj: T) {
  for (let prop in obj)
    console.log(obj[prop]);  // <-- trouble spot
}

let hi = { greetings: "hello world" };
dump(hi);  // sure.

interface SymbolsOnly { [key: symbol]: any };
dump<SymbolsOnly>(hi);  // ok or not? there's indexed access in dump, which may not be a symbol.

@malibuzios
Copy link
Author

@jods4

I'm trying to figure out what is the actual effect of having index signature key types at the first place, also considering what would happen if string literals are allowed as keys. First:

let x: { [key: number] : Array<number> };

There seem to be two different interpretations of what this should mean:

  1. You can only index x using a number when using square brackets, which would give you a value of type Array<number> or error otherwise.
  2. If you index x using using a number key using square brackets, you get a value of type Array<number>, but if you index something else you get any (this is how this works today).

Now let's take this to string literals:

let x: { [key: "A" | "B" | "C" ]: Array<number> };
  1. You can only index x using a "A", "B" or "C" using square brackets, which would give you a value of type Array<number> or error otherwise.
  2. If you index x using using "A", "B" or "C" using square brackets, you get a value of type Array<number>, but if you index something else you get any.

The second option here wouldn't seem to be be very useful in this case as I feel it is not strict enough, so I'm afraid that if the semantics of indexer key types were generally intended to be this way then I would personally recommend against implementing #5683 at this point. I'm also considering the fact that { [key: "A" | "B" | "C" ]: Array<number> } seem like it may be assignable from anything if interpreted as an all-optional interface type.

@jods4
Copy link

jods4 commented Mar 24, 2016

@malibuzios
2. seems obvious.

  1. I don't know. On one hand it makes sense to restrict to the type def. On the other, you break all dynamic access on the object (could get it back by explicit casting to any).

Another aspect of indexers is that JS does not allow you to define your own indexer (like say C#). So using an indexer is in fact always going through the JS associative array design.
In that context, defining specific indexer keys and types seems to only make sense for "collections" objects.
Maybe that's an argument for 1. Not sure, though <_<

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Mar 24, 2016
@malibuzios
Copy link
Author

I'm assuming the behavior described for key type checking is by design. Essentially - there is no specialized type checking and any indexer would take string, number or symbol regardless of the key type. This was a very important investigation that lead me to recommend not to implement string literals as key types (tracked in #5683), as the level of strictness of the type checking they would receive would be too weak to catch important errors.

E.g:

let x: { ["A", "B", "C"]: number } = {};

x["A"] = 1; // OK
x["D"] = 1; // No error
x[12] = 1; // No error
x[Symbol("abc")] = 1; // No error;

The alternative - the all optional interface:

let x: { A?: number, B?: number, C?: number} = {};

Would have a sufficient level of checking when using the dot notation (e.g. x.A = 1), but would be limited to names that can be used as property identifiers, and the set of possible property names would not be possible to reference as an external type.

It seems very unlikely (and I'm not sure if desirable at this point of the development of the language) the [] notation would get stricter type checking any time soon so I have decided to close this issue.

@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
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

4 participants