Forbid function assignment to index signatures:any

[sandersn]

I'll open a bug for this shortly, but in the meantime:

It doesn't make sense for functions or constructors to be assignable to types with an index signature; it doesn't even make sense for anything to be assignable to a target with an index signature if the source doesn't have one (h/t @weswigham). This PR makes a small change of exempting types with call/construct signatures from the rule that makes any non-primitive source type assignable to string/number index signature of any.

This has the concrete advantage of preventing functions from being assigned to ArrayLike<any>.

[sandersn]

@typescript-bot run dt
@typescript-bot run rwc

[typescript-bot]

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at 455c8e2. You can monitor the build here. It should now contribute to this PR's status checks.

[fatcerberus]

it doesn't even make sense for anything to be assignable to a target with an index signature if the source doesn't have one 

This was my thought at first too, but only if you take the index signature to mean "it has all the properties in the world". But that's logistically impossible so I prefer to think of it as "it can have any possible property, but doesn't have to" (effectively it's a type in which every property is optional). TS just avoids bugging you about the potential undefinedness of properties, but I tend to take that possibility as a given when working with types having index signatures. 😉

#31661 (comment)

[Jessidhia]

I'm not sure of any examples in the wild, but I can see how a function might be used as a property bag as well. Maybe the assignment needs to be allowed if the target type also has a call signature?

It's also possible that that is allowed in the code and I didn't notice, but could be good to add a test for it.

[russelldavis]

+1 to this change. I'm curious if we could take it even further. It's confusing for [string: any] indexers get special treatment (in particular compared to [string: unknown], but in other cases as well). I suspect it is also indirectly responsible for another case of special treatment -- ignoring indexers entirely in generic constraints (see #31661 (comment)).

I'm curious what the original context and motivation was for adding the special behvaior, and whether it's too late to revisit it.

[fatcerberus]

@russelldavis Wait - I’m confused: you want the code here: #31808 to be made to work again despite the free-for-all it allows, but are okay with the change proposed in this PR which is about further restricting the semantics of index signatures?

I don’t mean to sound combative but - if the case here is made to be an error, then why shouldn’t the one in #31808 continue to be an error as well?

[russelldavis]

@fatcerberus the change here only applies to index signatures of the form [key: string]: any, which get special treatment in the language. #31808 is using [key: string]: string, so it wouldn't be affected at all by this change.

[russelldavis]

To be a bit more explicit, you're right that I think typescript should be more restrictive here and less restrictive in #31808 (and somewhere in the middle where the two overlap).

Indexers in general are safe, and using them in generic constraints doesn't change anything about their safety (which is why I'm questioning the breaking change that led to #31808).

What is actually unsafe is the special rule that allows everything (except for primitives) to be assigned to [key: string]: any, which is what this issue is about.

If you look at the "crazy stuff" example given at #31661 (comment), the craziness has everything to do with the special rule being discussed here, and nothing to do with generics (it applies equally to an equivalent example without generics).

[fatcerberus]

Right, but any is just a free-for-all anyway and inherently unsafe - and the behavior for any for indexers in this regard doesn’t actually seem that special since similar safety issues arise for e.g. [key: string]: number - just in a more restricted fashion, as I showed here: #31661 (comment)

In that example an assignment was allowed through an alias that otherwise would have been prevented. The only difference with []: any is that the type of the properties involved isn’t restricted - which would equally be the case for something like []: unknown I think.

So for example:

function foo<T extends Record<string, unknown>>(obj: T) {
    obj["s"] = 123;
    obj["n"] = "hello";
}

let z = { n: 1, s: "abc" };
foo(z);
foo([1, 2, 3]);
foo(new Error("wat"));

unknown, unlike any, is typesafe, yet the bad stuff still happens. The "special rule" you cite has nothing to do with any and just boils down to, if all properties' types match the index signature, then it's assignable, even if the source object doesn't have an indexer itself. That's a huge loophole. #30769 was an attempt to restrict (but not fully close) that exact loophole, but you're railing against it...

[russelldavis]

and the behavior for any for indexers in this regard doesn’t actually seem that special

It really is special. More below.

The "special rule" you cite has nothing to do with any and just boils down to, if all properties' types match the index signature, then it's assignable

This is not correct. The assignment let dict: { [s: string]: unknown } = new Error(); is not allowed, even though all properties of new Error() match [s: string]: unknown equally well as [s: string]: any.

Also, just look at this PR. The code literally has a special case for targetInfo.type.flags & TypeFlags.Any.

unknown, unlike any, is typesafe, yet the bad stuff still happens

Your example above that statement actually shows the opposite. The bad stuff does not happen with unknown. It doesn't compile as given, due to #30769, but if you change it to function foo(obj: Record<string, unknown>), you'll still get compile errors, while if you change it to function foo(obj: Record<string, any>), it will allow the bad stuff.

I think you may have gotten confused because, prior to 3.5, there was also a special rule for [s: string]: unknown indexers. That rule was removed in 3.5, which was a great change that increased type safety.

(Note that https://www.typescriptlang.org/play/ isn't updated to 3.5 yet, so you have to be careful when using it to test things -- I've been bitten by that as well.)

[fatcerberus]

let dict: { [s: string]: unknown } = new Error();

Thanks, so you're right - that is indeed an error in 3.5. However, this isn't (I tested in VSCode with 3.5.1 selected):

type X = { [key: string]: unknown };
type Y = { pig: number, cow: number, ape: number };

let y: Y = { pig: 812, cow: 1208, ape: 128 };
let x: X = y;    // shouldn't be allowed but is
x.whale = "it's fat";
x.pig = "type safety is now well and truly broken";

So the new Error() treatment seems to be a special case of some kind. I personally would rather the behavior of index signatures be consistent rather than having a bunch of patches that make certain cases safer while leaving other related loopholes open. The patchwork of special cases makes it more difficult to explain :-/

[russelldavis]

I'm not sure I agree that that example should be disallowed, but it's nuanced and I'm currently out of time. Either way, it's completely unaffected by this PR and by my proposal at #31661 (comment). Thanks for the discussion @fatcerberus.

[fatcerberus]

Mostly I’m just trying to understand what mechanism disallows the assignment from new Error() but allows the assignment above. Under structural typing, the cases don’t seem that different.

[weswigham]

We should sync this, rerun the tests, and go over the DT failures.

[weswigham]

indexTypesRelatedTo just takes Types as arguments, so this should just be getSignaturesOfType:

Suggested change

	getSignaturesOfStructuredType(source, SignatureKind.Call).length === 0 &&
	getSignaturesOfType(source, SignatureKind.Call).length === 0 &&

[weswigham]

Likewise:

Suggested change

	getSignaturesOfStructuredType(source, SignatureKind.Construct).length === 0) {
	getSignaturesOfType(source, SignatureKind.Construct).length === 0) {

[sandersn]

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.