noImplicitAny regression in 1.3

[jseanxu]

Pre 1.3, with --noImplicitAny flag on, having the following:

interface Object {
  [key: string]: any;
}

would enable this type of scenario:

var element = document.querySelector("#something");
element["foo"] = { ... };

However, now this results in an implicit-any error. The winjs/winjs team uses this pattern a lot to add and access members on various objects and this is blocking us from moving to 1.3.

[NoelAbrahams]

This was an (unannounced) breaking change. See #835.

[NoelAbrahams]

Incidentally, this was a massive breaking change for us too - especially within the test harness. It seems a bit ironic in the light of new features being turned down, because they _may_ (i.e. potentially) introduce a breaking change (e.g. #16).

We had no problem going through the code base and fixing this particular breaking change (and actually ended up fixing a few holes as a result) thanks to the refactoring magic now possible because of TypeScript.

[jseanxu]

We had no problem going through the code base and fixing this particular breaking change (and actually ended up fixing a few holes as a result) thanks to the refactoring magic now possible because of TypeScript.

What pattern are you using now? Are there any plans in the future to establish a pattern on how to do this as this is a common scenario?

[xirzec]

I feel like we need some kind of solution here, since we already have many thousands of lines of code that rely on it without an easy way to refactor. Also there are plenty of times that it is very ugly to do any casts.

Which do you think is more readable?

obj['foo']

vs

(<any>obj).foo

To me the first is much cleaner, especially when you're chaining or passing properties values as arguments to other functions.

[NoelAbrahams]

The point is when we decide to compile with noimplicitany we should expect an error for obj['foo'].

Also note that it is only an error if the property foo does not exist on obj:

var obj = {
    bar: 10
};

obj['foo'] = 10; // Error: Index signature of object type implicitly has an 'any' type
obj['bar'] = 10; // Okay

Unfortunately, most often what happens is the following:

var bar = 'bar';
obj[bar] = 10; // Error: Index signature of object type implicitly has an 'any' type

There is no quick fix for this. The only solution is to go through the errors and ensure the index signature is modelled correctly:

interface NumbersOnly {
    [index: string]: number;
}

var obj: NumbersOnly = {
    bar: 10
};

var bar = 'bar';
obj[bar] = 10; // Okay

This is what we should expect to do, when deciding to go the noimplicitany route.

Clearly the NumbersOnly interface is pretty limited, but once we have union types in 1.4 we should be able to write:

interface NumbersAndStrings {
       [index: string]: number | string;
}

[NoelAbrahams]

I should also add that the situation with enums is more elaborate and rather unsatisfactory:

interface EnumIndexer<TValue> {
    [index: string]: TValue;
}

enum Foo {
    one,
    two
}

var one = 'one';
var num = (<EnumIndexer<Foo>><any>Foo)[one];

[jseanxu]

The point is when we decide to compile with noimplicitany we should expect an error for obj['foo'].

I agree with the above, but the interface extension on Object defines that a string accessor is always defined, for any object, and its return type is any - this is explicit and should not generate a noImplicitAny error.

[basarat]

Just FYI other Definitely Typed definitions rely on this quite heavily as well. For an example check out angular.d.ts (https://github.com/borisyankov/DefinitelyTyped/blob/master/angularjs/angular.d.ts)

    interface IAttributes {
        [name: string]: any;

    interface IFormController {
        [name: string]: any;

    interface IModelValidators {
        [index: string]: (...args: any[]) => boolean;
    }

    interface IAsyncModelValidators {
        [index: string]: (...args: any[]) => ng.IPromise<boolean>;
    }

and a few more but you get the point.

tagging @vvakame @Bartvds @johnnyreilly just as an FYI.

[NoelAbrahams]

@jseanxu,

the interface extension on Object defines that a string accessor is always defined, for any object, and its return type is any - this is explicit and should not generate a noImplicitAny error

There is some special treatment for index signatures on apparent types: they are not inherited. I believe this was due to a change in the spec:

Old spec (3.8.1):

The augmented form of an object type T adds to T those members of the global interface type Object that aren't hidden by members in T. Furthermore, if T has one or more call or construct signatures, the augmented form of T adds to T the members of the global interface type Function that aren't hidden by members in T. Members in T hide Object or Function interface members in the following manner:

• A property hides an Object or Function property with the same name.
• A call signature hides an Object or Function call signature with the same number of parameters and identical parameter types in the respective positions.
• A construct signature hides an Object or Function construct signature with the same number of parameters and identical parameter types in the respective positions.
• An index signature hides an Object or Function index signature with the same parameter type.

According to the last line, adding an index signature on Object should have worked, but that line (along with the other bullet points) is no longer present in version 1.3 of the spec.

I am not aware of the reason for this change.

[danquirk]

We've been talking about this a bunch the last couple days. Here's an explanation of how we got into this spot and how we're thinking about changing it:

Brief History of Indexers on Object

This string indexer was once defined on Object in lib.d.ts to allow for basic bracket notation indexing as JavaScript programmers are used to. We eventually decided to codify the bracket notation indexing semantics into the language itself, this made the indexer in Object redundant and it was removed. Separately (but intertwined), the compiler had a concept of Apparent Type of a type (now called apparent members of a type per 3.10.1 of the current language spec) which models the prototypical inheritance JavaScript uses and the boxing of primitive values. It is the mechanism by which a value of type string appears to have the members from String and how an arbitrary object type has the members of Object; likewise, types with call or construct signatures appear to have members of the Function type. Essentially, the Apparent Type statically models the types of things you could put on an object's prototype that might be inherited. But indexers do not represent something you could put on a prototype, so they were removed from the checking of Apparent Type which is part of the reason the spec section @NoelAbrahams quoted above now looks meaningfully different. That the only reason anyone appears to be adding indexers to Object is to avoid --noImplicitAny warnings suggests this is the correct conceptual model.

The Break

Upon the introduction of --noImplicitAny some people wanted the ability to use bracket notation indexing without error but did want the benefits of the other additional checks. This led to the workaround noted in this thread and others where the indexer was manually added to Object. When the change to Apparent Type to remove consideration of indexers was made nothing broke for those who weren't using --noImplicitAny but this did cause some pain for those who were. There're varying opinions on whether the new or old behavior was best (as noted in this thread). We've generally been of the opinion that some breaks are ok when you've opted into 'stricter' flags under the assumption that those breaks are catching real bugs, but this assumption was not communicated as clearly as it should've been (and we are still judicious in taking any breaks of this sort).

Proposed Solution

It seems clear at this point that --noImplicitAny is a little too coarse grained. There are really two classes of errors this flag catches. One comes primarily from forgetting type annotations, a mistake which never adds value and leaves open holes for bugs to creep in. The other comes from bracket notation indexing which is a perfectly legitimate use of some of the dynamic features of JavaScript (albeit certainly also a source of errors). That these two checks are under the same flag leads to an awkward choice for some without some easy workaround (which the manual addition of the indexer provided).

What we are thinking at the moment is to leave the current behavior as is, but add a generalized way to suppress all errors with a particular error code. This lets people opt in and out of varying checks without creating an impossible to reason about matrix of compiler flags and difficult to manage implementation. Concretely, for those who want the behavior you get today with the Object indexer you would remove the indexer and build with something to the tune of:

tsc --noImplicitAny --suppressErrors:7017 foo.ts

This will also scale nicely with any new stricter mode (ex #274) without requiring a new compiler flag per additional strictness check.

Any objections or other feedback on the proposed solution (or alternative solutions) are appreciated.

[NoelAbrahams]

@danquirk, I like the proposed solution.

A couple of points:

• Assuming that I understand this correctly, this will introduce breaking changes into the "base compilation mode" (i.e. no flags). For instance, once a stricter check, say Suggestion: disallow use before definition #21, is introduced, users will need to compile with --suppressError:whatever in order to compile code that was compiling without errors previously.
• Bug reporting is going to be more complicated (typical conversation: User: "TypeScript is not catching this error" Ryan: "What flags do you have?" User: --suppressErrors:[505,565,8988,154,45,689,586,589,998,869])

I do not have an objection to this, because I have always been an advocate of stricter type checking, and it's unlikely that we'd use the suppressError flag.

BTW, that's a very skilfully constructed climbdown for introducing a breaking change 😃

[johnnyreilly]

@danquirk,

First of all, great news that you're thinking about splitting --noImplicitAny and bracket notation indexing. I've always hated the extra noise that the current situation provides but I've lived with it because I felt that finding out about missing types was worth the pain.

So yes! (Punches the air)

I like the proposed solution's flexibility - that totally makes sense rather than having to continually add new compiler flags. I share @NoelAbrahams reservations about bug reporting. I don't suppose you'd consider having simple string aliases for the error codes? Given how many error codes there are maybe it would only make sense to provide aliases for the "popular" / often opted out of error codes.... So perhaps this might be possible? So for example:

tsc --noImplicitAny --suppressErrors:bracketNotationIndexing,505 foo.ts 

Where bracketNotationIndexing represents error code 7017 and 505 is not a "popular" error code (doesn't have an alias) and is just passed as is?

Alternatively the user could just use this:

tsc --noImplicitAny --suppressErrors:7017,505 foo.ts 

Which means exactly the same but is less human-readable.

[RyanCavanaugh]

We don't get terribly many "I didn't get an error when I thought I should" bugs so I'm not too worried about that case. Optionally we could write on the commandline the number of suppressed errors as a reminder or something. My hope is that the median number of suppressed error codes would be 0 -- that should be what we aim for.

One thing we discussed was to just remove the implicit Object indexer warning from noImplicitAny and add it back as one of the first errors in #274, and communicate loudly that the flag introduced in #274 will not have any back compat promises (we intended to do this with noImplicitAny but clearly missed the mark).

If I could wave a wand and get some data, I'd like to see how many people have added the indexer to Object because they don't like that the check here is built into noImplicitAny.