Interface Index Signatures + Strict Null Checking

[blakeembrey]

TypeScript Version:

Version 1.9.0-dev.20160617-1.0

Code

interface Foo {
  prop?: string
  [key: string]: string
}

{
  "compilerOptions": {
    "strictNullChecks": true
  }
}

Expected behavior: No error. Wouldn't the fact that it's an index signature implicitly signal that it could also be undefined?

Actual behavior:

index.ts(2,3): error TS2411: Property 'prop' of type 'string | undefined' is not assignable to string index type 'string'.

Edit: On the other side of the coin with this usage:

interface Foo {
  [key: string]: string
}

const foo: Foo = {}
const bar = foo['test']

Should bar be type of string | undefined?

[ahejlsberg]

Yes, you could argue that every index signature should have undefined added to its type, but it would break the very common case of arrays and you can always add a ? yourself. See conversation here.

[blakeembrey]

@ahejlsberg I didn't think you could add ? to an index signature. The things that tricked me up here is that if you take a definition generated when using strictNullChecks: false and try to use on a system that strictNullChecks: true, it'll error. This seems like a point where there'd be a lot of back and forth between a module author and consumers of the module.

I do see the argument about the array case, but I feel like you'd catch a lot more user issues with this check. Eventually, flow control could even detect that i is between 0 and arr.length and know you aren't accessing something out of range, right? (Assuming the main array index case you mention is a loop).

However, isn't this a similar case as Object.keys() now? You could do the same loop on Object.keys(). Seems like there's not much benefit to the bugs it could be catching and errors it could avoid. New users can be pointed to use for..of.

And because I do feel it's going to be painful, anyone that has written code with an index signature today will break on 2.0 immediately with strictNullChecks: true. That could be a decent chunk of TypeScript on NPM.

Edit: Also, strictNullChecks is a toggle that's disabled by default - if someone enables it, I'm sure they expect to catch things like this and won't find it that irritating.

[rkirov]

This issue has an "interesting" consequence - a compilation with tsc test.ts --declarations (no strictNullChecks) can produce a .d.ts file that is internally invalid when used in a following compilation. When calling tsc test.d.ts more.ts --strictNullChecks one sees the error above, but it has nothing to do with the code at hand.

So far this is the only way that I know of that happening, i.e. the validity of the .d.ts being dependent on the strictNullChecks flag. It would be nice to have the property that independent of the compilation flags, produced .d.ts files are valid in any compilation downstream (even if they don't describe the API completely as described in #9655).

[ewinslow]

If the concern is just breaking folks who have --strictNullChecks on, could this be considered for something like a 3.0 (intended breaking change)?

Reposting the rest of my response from #11122 since that was closed as dupe and I am bummed about the current decision:

+1 for fixing this. I've been bitten by this compromise a couple times now so count me in for more safety not less!

the only practical result is that xs[i]! gets written everywhere instead

If folks are using an unsafe pattern everywhere, maybe that's exactly what should happen. I don't find it particularly onerous. I guess I'd like to see some data on exactly how much impact this would have on existing TypeScript programs before we write it off as making the language unusable.

• how many real bugs it would find
• how many false positives
• how much it costs to annotate the false positives w/ ! (this could be automated for existing code)

Right now the sentiment I've seen from TS team is that 1 is negligible, 2 is huge, and 3 is prohibitive. Are there numbers to back up these hunches?

Some alternatives to ! proliferating:

• Do undefined-checks on the result. This is the right thing to do in many cases!
  Rewrite loops with for (let item of array) or array.forEach instead of for(let i = 0; i < array.length; i++).
• Looping over a string[] array with for-of or forEach (or map, etc.) would never emit undefined, since each result is guaranteed to be in-bounds.

The same could be argued for Map<string,string>. map.get('foo') returns string | undefined, but map.set('foo', undefined) would be banned so that when looping over values with forEach or somesuch, you are guaranteed only to get string values, not string | undefined. And indeed, that is the current behavior of Map.

[OliverJAsh]

If authors enable strictNullChecking then they should expect some boilerplate overhead in return for increased safety. I'm surprised TypeScript isn't consistent in its approach here—as far as I know, this is the only scenario where the team has decided to go against type safety in favour of reduced boilerplate with regards to strictNullChecking?

I'm not too familiar with many other type systems, but Scala does have List.headOption which will return Option<T>, which would be the same thing as T | undefined in TypeScript.

Indeed there would be slightly more boilerplate if we did return T | undefined, as discussed above, but surely the job of the type system is to return the correct type, so the author can handle that correctly.

Without this there will be a whole class of bugs that TypeScript will never catch. 😒 😢

[Flarna]

Maybe there is the need to distinguish somehow between properties which are optional and properties which are present but may have undefined as value?
Currently it seems to me like this is the same for TypeScript but not not for JavaScript.

[gcnew]

@Flarna It actually does. Both for functions and objects.

type Bag = {
    optional?: number;
    mustBePresent: number|undefined;
}

const bag: Bag = {
    mustBePresent: 3 // type error if missing
    // we dont need to specify `optional`
};

function f(mustBePresent: number|undefined, optional?: number) {}

f(undefined); // OK
f(15, undefined); // OK
f() // not OK, `mustBePresent` is missing

[Flarna]

Sorry, seems I was not clear with my comment. If I have a type like that

type A = {
    a?: number;
    b?: number;
}
const a: A = {
    a: undefined,
    b: 15
};

I would expect that I can iterate over all properties and get only numbers. But actually I may get also undefined.
So the ? tells that the property may be missing and may have the value undefined.

If an index signature is used this is different:

type B = {
    [key: string]: number
}
const b: B = {
    optional: undefined   // property optional is incompatible....
}

Here the properties are optional but if they are present they are numbers.

[Strate]

Maybe it could be handled with extra compiler option?
In runtime calling array[index] or object[propName] definetely could return undefined if index is out of bounds or propName does not exist on object. It is really good to force to check returned that value before using it.

[MartinJohns]

This really seems like a half-hearted solution. I expected the strictNullChecking flag to warn me about potential undefined in my code - but in the case of indexers it let me down. This is extremely irritating.

[ewinslow]

😭

[blakeembrey]

@RyanCavanaugh Sadly I'm already starting to see this leak into projects that now are forced to enable strictNullChecks just so their consumers don't break. See moment/moment#3591, which is the issue I brought up - a module configuration can be incompatible with the consumers configuration resulting in unexpected type errors.

[felixfbecker]

The current behavior is just wrong, and I don't understand how this can be labeled as "working as intended". It is a fact that when accessing an index signature, it can always be undefined. So as a user, when is enable strictNullChecks, I expect TypeScript to catch that bug for me and prevent me from a null pointer exception, forcing me to wrap it in an if check.

If a user does not want this, nobody forces him to enable strict null checks. It's opt-in. But the current situation causes huge problems with declaration files like in moment, where we do not have the possibility to fix this without a breaking change. To make them compatible with TS2 & strict null checks we would have to add | undefined to the signature, which makes the typings incompatible with TS1.8.

[OliverJAsh]

For those interested I created a proposal that this behaviour (index signatures returning T | undefined) should be support behind an option of some kind. Please chime in! #13778

[ccorcos]

How about a --strictArrayIndexing and a --strictObjectIndexing abstraction? I understand why you might not want to use these, but not having the option feels like a gaping hole.

We can create abstractions like Array.forEach that alleviate the pain of null checking for iteration...

[thw0rted]

Hasn't Douglas Crockford spent the last few years telling everybody how he stopped using for loops, in favor of Array.prototype.forEach? I have too, and in my hundred-ish-file codebase I don't think I have a single array index operation.

This is just a degenerate case of a bigger problem:

Object.keys(myObject).forEach(key => {
  myObject[key].doAThing();  // myObject[key] "may be undefined"
});

// or

myMap.keys().forEach(key => {
  myMap.get(key).doAThing(); // same
});

We "know" that in each case, the accessor should return a value, exactly the same way that we "know" that an array index will return a value when we're looping from 0 to .length. But do we really know, in all these scenarios?

Object.keys(myObject).forEach(key => {
  delete myObject[key];
  myObject[key].doAThing();  // oh look it actually was undefined!
});

That's why index signatures should in fact always have an implied undefined return type -- make me check, or make me assert that I know it definitely can't be undefined.

[ccorcos]

I'm in the same boat. Furthermore, sometimes you have an object like: {[key: string]: number} and sometimes you have an object like {a: number, b: string}. Iterating over each of these can be must stricter. For example, in objectForEach(obj, (key, value) => {}) key can value can be typed accordingly.

[felixfbecker]

@thw0rted @ccorcos that was before iterators and for of existed. Now we can use that, which is way better because it has break, you can step through it with a debugger and TS can maintain narrowed types (which it can't for callbacks). For object iteration, there's Object.entries()

[ccorcos]

I've noticed that about narrowed types...

It looks like typescript doesn't have Object.entries yet...

https://www.typescriptlang.org/play/index.html#src=const%20x%20%3D%20%7Ba%3A%201%2C%20b%3A%20%222%22%7D%0D%0AObject.entries(x)

[felixfbecker]

@ccorcos it has, you just need to set the right lib/target options

[ccorcos]

Ah thanks