Disallow more flavors of property accesses on never in expression space
#56780
Conversation
| let b: 0 | 1 | 9; | ||
| [{ [(a = 1)]: b } = [9, a] as const] = []; | ||
| ~~~~~~~ | ||
| !!! error TS2339: Property '1' does not exist on type 'never'. |
There was a problem hiding this comment.
I'm not entirely sure if the error here is completely desirable but it feels consistent to me with things like:
const obj = { b: '' }
let a: number
;({ a = 10 } = obj) // Property 'a' does not exist on type '{ b: string; }'.(2339)What I mean is that despite the first element in this array pattern having an initializer it's the "right-side" type of the original source that is used to check for valid property accesses.
So since we have never[] on the right side, its first element is actually never and that has no properties so disallowing those patterns here looks desirable to me.
There was a problem hiding this comment.
This error doesn't look right to me: it says "Property '1' does not exist on type 'never'", but we should be getting property 1 of type [9, typeof a], and that tuple should not have type never.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@typescript-bot pack this |
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at ba7033b. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at ba7033b. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at ba7033b. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at ba7033b. You can monitor the build here. Update: The results are in! |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@gabritto Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @gabritto, the results of running the DT tests are ready. |
|
@gabritto Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
Of the new errors on the top repos test, most seem to be because of "defensive programming", where there is an exhaustive check on some variable followed by a branch for when it has an unexpected type. In this defensive branch, the variable has type Those seem to be actual, desirable errors: |
|
We discussed this at design meeting today, and the conclusion was that this change is not worth the breaks it causes. As evidenced by the extended tests, most new errors are not really desirable and come from an intentional usage of |
|
I unfortunately missed the meeting, but I find it a little weird that we're saying that it's okay to access any prop on never; if that were the case, then I personally feel like all of the errors this PR showed are real and that |
Since we know
|
Ah, that is true; I had it flipped in my mind; obviously if you have a union of two types, you can only access what they have in common. |
|
I'm OK with the judgment itself but this inconsistency is killing me: declare const foo: never
foo.prop // error ?????
foo["prop"] // ok
const { prop } = foo // okIt feels just wildly inconsistent and hard to explain to people. How this difference in behavior would be explained in the docs? I didn't yet have the time to review carefully everything reported by the extended test suite but I'd definitely call most of those failures improvements. I feel like the only use case that was said to be valid here is the defensive programming case. And to quote @jakebailey:
It would at least give some visual indicator that something funky is going on there. Those would also often appear in the codepaths that would be clearly associated with such defensive programming fallbacks (close to thrown error messages, On the other hand, I'm quite worried - as the user - that This isn't exactly a new risk though. |
I'm not even sure we did that on purpose, I'll have to investigate.
I'm open to believing that, given evidence, but it could still be a burden to make this update if it's needed in too many places.
Yeah, that was a genuine error, as I pointed out above.
I'm having this thought that |
fixes #42999
fixes #56778