Revert PR #52589 #53280
Revert PR #52589 #53280
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| // simplified repro from 52589#issuecomment-1416180638 | ||
| declare class MyCompiler { | ||
| compile(): void; | ||
| } | ||
| interface WebpackPluginInstance { | ||
| apply: (compiler: MyCompiler) => void; | ||
| } | ||
| type WebpackPluginFunction = (this: MyCompiler, compiler: MyCompiler) => void; | ||
| interface Optimization { | ||
| minimizer?: (WebpackPluginInstance | WebpackPluginFunction)[]; | ||
| } | ||
| declare const A: <T, P extends keyof T>( | ||
| obj: T, | ||
| prop: P, | ||
| factory: () => T[P] | ||
| ) => void; | ||
| export const applyOptimizationDefaults = (optimization: Optimization) => { | ||
| A(optimization, "minimizer", () => [ | ||
| { | ||
| apply: (compiler) => {}, | ||
| }, | ||
| ]); | ||
| }; |
There was a problem hiding this comment.
This was somewhat unrelated to my change, it was a weird intersection of my change and a temporary lack of #52611 . So I decided to include it in my PR as an additional regression test. For that reason, at the very least, I wouldn't remove this.
| @@ -1,5 +1,7 @@ | |||
| // @lib: es2015 | |||
|
|
|||
| function f<T extends { "0": (p1: number) => number }>(p: T): T { | |||
There was a problem hiding this comment.
While I have no idea why anybody would like to write this code... ok 😅 this is definitely valid 😉
There was a problem hiding this comment.
This pattern definitely occurs in real world code, though most of it probably pre-dates our support for tuples.
| declare function test( | ||
| arg: Record<string, (arg: string) => void> | Array<(arg: number) => void> | ||
| ): void; | ||
|
|
||
| test([ | ||
| (arg) => { | ||
| arg; // number | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
I see @ahejlsberg's point about string index signatures contributing to the contextual type of arrays, it's kinda how it works. At the same time, I find it somewhat hard to believe that the user's intention would ever be for this to happen in such a mixed union.
This doesn't happen to objects (as the Array type doesn't contribute to their contextual type here) and it feels wrong that the opposite can't be expressed. We can mix a string and a number signature to achieve this:
declare function test(
arg:
| (Record<string, (arg: string) => void> & { [index: number]: never })
| Array<(arg: number) => void>
): void;
test([
(arg) => {
arg; // number, it works!
},
]);But that's not quite intuitive and a lot of users wouldn't figure this out on their own as a solution.
It also has a drawback... those become errors:
test({
1: (arg) => {}, // error
});
test({
"2": (arg) => {}, // error
});I feel like if this test case doesn't represent the desired behavior then there is a clear need for something in the language to represent non-arrays. I understand that almost everything is an object... but that's not exactly how people think about those things. Arrays are special enough to not be thought of as plain objects 99% of the time.
Either way... if this is how things should work and if no changes are planned to the current behavior, then I feel like those expectations should be codified in the test suite. So instead of reverting the whole change, I would just revert the checker.ts and adjust the tests.
There was a problem hiding this comment.
We generally only discriminate contextual types based on discriminant properties with explicit literal values in the source type. In an ideal world we'd discriminate a contextual union type based on assignability of the source type to each individial constituent, but given that the contextual type influences the source type, that quickly becomes circular. So, as a rule, we don't do that.
While an object-but-not-array type might be nice to have (object-but-not-function is another one that comes to mind), it is not something we're equipped to consistently reason about. The notion that an array is just an object with numerically named properties is quite deeply engrained in JS/TS.
Anyway, I'm fine with just reverting the changes in checker.ts and keeping the tests.
…eral elements (microsoft#52589)" This reverts commit e775383.
jakebailey
force-pushed
the
revert-52589
branch
from
b50c3f4 to
c7386ea
Compare
|
I've reverted the checker change but kept the tests. Will merge once green and reopen the original issue. |
...aselines/reference/contextualSignatureInArrayElementPrefersArrayUnionMemberLibEs5.errors.txt
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| ==== tests/cases/compiler/contextualSignatureInArrayElementPrefersArrayUnionMemberLibEs5.ts (1 errors) ==== | ||
| // repro from #52588 |
There was a problem hiding this comment.
I'm also not sure what's your policy for those comments - but a future reader might find this reference confusing (as the issue is about making this work but this new test case will test that it doesn't work). I would personally either remove this line (from the test case file) or adjust it to point to Ander's comment: 53280#discussion_r1138744885
There was a problem hiding this comment.
I'm swapping them out to be real links, so this will be updated in a moment.
I don't know if there's any policy; my personal preference is to leave real actual links that I can click on, but most don't do that. Even more, I wish we just named tests like issue53280.ts rather than having to come up with some readable name that nobody's ever going to change again even if the behavior changes.
From what I understand there is no need to reopen the original issue, it should just be labeled with "Working as intended"/"Design limitation". |
Per: #52589 (comment)
@Andarist