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

Intuitive != expected != actual behaviour when combining string and symbol keys #26470

Closed
yortus opened this issue Aug 15, 2018 · 16 comments · Fixed by #44512
Closed

Intuitive != expected != actual behaviour when combining string and symbol keys #26470

yortus opened this issue Aug 15, 2018 · 16 comments · Fixed by #44512
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Milestone

Comments

@yortus
Copy link
Contributor

yortus commented Aug 15, 2018

TypeScript Version: 3.1.0-dev.20180813

Search Terms: string index inference, string key inference

Code

// function foo takes an 'options' object that can have:
// (a) any number of string keys whose values are unary functions
// (b) an optional 'directive' unique symbol key with a unrelated type
const directive = Symbol('directive');
declare function foo<TArg, TRet, TDir>(options:
    {[x in string]: (arg: TArg) => TRet}    // all the string keys are unary functions
    & {[directive]?: TDir}                  // there is an optional 'directive' symbol key
): void;

// CASE 1
// ~~~ERROR~~~ - ... prop 'addOne' incompatible with index signature
//               ... 'x' and 'arg' are incompatible
//               ... 'string' not assignable to 'number'
let case1 = foo({
    [directive]: (x: string) => 'str',
    addOne: (x: number) => x + 1,
    double: (x: number) => x + x,
});

// CASE 2: identical to case 1 except for order of properties
// Compiles OK - infers TArg = number, TRet = number, TDir = (x: string) => string
let case2 = foo({
    addOne: (x: number) => x + 1,
    double: (x: number) => x + x,
    [directive]: (x: string) => 'str',
});

// CASE 3: identical to case 1 except for type of directive
// Compiles OK - infers TArg = number, TRet = number, TDir = string
let case3 = foo({
    [directive]: 'str',
    addOne: (x: number) => x + 1,
    double: (x: number) => x + x,
});

Intuitive expected behaviour:

All three cases compile without errors, with the string and symbol keys typed separately, since the type declaration clearly distinguishes the string keys from the symbol one.

Expected behavior according to #26257 (comment):

That comment suggests that all three cases should fail to compile, since the string index signature covers all symbol keys too, so any symbol key would have to conform to the string index signature, and they don't conform in any of the three cases in the code above.

Actual behavior:

Case 1 fails to compile, but Case 2 and 3 compile fine and in fact do type the string and symbol keys separately as intended by foo's definition.

Playground Link: here

Related Issues:
Taken directly from #26257 (comment). But the issue containing that comment is marked as 'working as intended' so I thought it would be better to open a new issue for this specific case.

@yortus
Copy link
Contributor Author

yortus commented Aug 15, 2018

Another small example from #26257:

If {[x: string]: T} is intended to apply to all keys (i.e., strings, symbols and numbers), then shouldn't the following fail to compile, since the symbol key doesn't conform to the string index signature?

const SYM = Symbol();
declare function foo(obj: {[x: string]: number}): void;
foo({a: 1, [SYM]: 'sym'});  // Compiles fine

@yortus yortus changed the title Type inference problem when combining string and symbol keys Intuitive != expected != actual behaviour when combining string and symbol keys Aug 20, 2018
@yortus
Copy link
Contributor Author

yortus commented Aug 20, 2018

@RyanCavanaugh would you mind triaging this issue please?

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Aug 20, 2018
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.1 milestone Aug 20, 2018
@yortus
Copy link
Contributor Author

yortus commented Aug 20, 2018

Thanks @RyanCavanaugh. Can we clarify what the bug is here exactly? Is it that the string index seems to include symbol keys under certain conditions, or is it that the string index doesn't include symbol keys under certain conditions?

@RyanCavanaugh
Copy link
Member

I'm pretty unclear on which behavior is the desired one, which is why I was procrastinating on this one. In any case the property order really shouldn't matter, though.

@yortus
Copy link
Contributor Author

yortus commented Aug 20, 2018

I see. If it turns out that symbols must conform to string indexes, then I hope some consideration is given to how to declare things like foo where we want to type string and symbol keys separately.

FWIW I can't think of any scenarios where you'd want a strongly-typed symbol property to be constrained by the string index. Symbol-keyed properties usually serve a very specific purpose (think of all the well-known symbols). The type of a symbol-keyed property is usually unrelated to the types of other keys in the same object. Certainly unrelated to string keys, and usually to other symbol-keyed properties too.

@yortus
Copy link
Contributor Author

yortus commented Nov 6, 2018

Hey @weswigham, may I ask if there is something holding up the fix for this? Your PR has sat there for a couple of months now. Just wanting to know if this will be addressed by the time v3.2 comes out.

@weswigham
Copy link
Member

weswigham commented Nov 6, 2018

3.2 is a no at this point, given it's size, I'd guess. I'm still pushing for it, though. It's just hard to convince the relevant parties that it is both the correct fix (easy) and worth the change (hard). This issue and the others linked in the fix don't make the best case for a change of that size (not many upvotes, little obvious impact), so it's slow going.

@yortus
Copy link
Contributor Author

yortus commented Nov 6, 2018

OK. Well the PR has a much broader scope of changes that what this issue raises. Would a more limited change have more chance of making progress - eg just stop string indexes from constraining symbol keys? The other changes could possibly be approached later if there is demand.

@weswigham
Copy link
Member

So that's not really the issue here. The issue is that mapped types have a concept of mapping symbol keys, but no representation to map them into - so when we do inference, we end up discarding information (which is why the order matters). You need symbol index signatures to fix this.

@yortus
Copy link
Contributor Author

yortus commented Nov 6, 2018

I see (and thanks for explaining why this issue manifests order-dependence!).

Your PR has quite a few upvotes. If it fixes known issues (like this one) and otherwise has little obvious impact, I guess there must be some other reason why the PR is facing resistance? Does it break anything or slow down the compiler or preclude other future features under consideration?

Usually issues go unresolved because nobody has had time to create the PR, but in this case you have already done the work. So just curious why it can't be merged.

@weswigham
Copy link
Member

weswigham commented Nov 6, 2018

Your PR has quite a few upvotes. If it fixes known issues (like this one) and otherwise has little obvious impact, I guess there must be some other reason why the PR is facing resistance? Does it break anything or slow down the compiler or preclude other future features under consideration?

Just complexity. Index signatures are currently represented trivially internally, while my change fleshes them out a bit more and makes them more generic.

@yortus
Copy link
Contributor Author

yortus commented Nov 6, 2018

You need symbol index signatures to fix this.

You wouldn't need them to fix this issue though, right? I thought the problem here is not the lack of symbol index info. Rather, it's that the current string index behaviour is so broad that it affects unrelated symbol properties.

@yortus
Copy link
Contributor Author

yortus commented Nov 9, 2018

Note a closely related issue to this was fixed earlier this year - #21217. That PR states:

This fixes errors when declaring a type with a symbol-named property that also specifies a string indexer, as symbol-named properties do not collide with string indexers.

In that light, is this issue actually a regression? I still think this one can be resolved by just modifying string indexer behaviour without adding any other indexer types.

@yortus
Copy link
Contributor Author

yortus commented Nov 9, 2018

Also @weswigham you mention this is a problem with mapped types, but the currently behaviour manifests with no mapped types anywhere, eg tsc acts the same if we change foo in the OP to:

declare function foo<TArg, TRet, TDir>(options:
    {[x: string]: (arg: TArg) => TRet}    // NB string indexer, not mapped type
    & {[directive]?: TDir}         
): void;

I don't know if the compiler treats that as a mapped type internally, but at least on the surface it appears to be an issue with string index signatures.

@yortus
Copy link
Contributor Author

yortus commented Nov 9, 2018

CC/ @rbuckton (since you made #21217).

@MattMcFarland
Copy link

What's the status on this? Is there anything holding it up right now?

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Investigating Is in active investigation and removed Bug A bug in TypeScript labels Mar 13, 2019
@weswigham weswigham added the Fix Available A PR has been opened for this issue label Jul 15, 2019
@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript and removed Fix Available A PR has been opened for this issue Investigating Is in active investigation Suggestion An idea for TypeScript labels Jan 23, 2020
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
6 participants