Refactor codepaths relying on uncomparable === undefined
#59585
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
| @@ -11,7 +11,7 @@ function verifyMissingFilePaths(missing: ReturnType<ts.Program["getMissingFilePa | |||
| const map = new Set(expected); | |||
| for (const missing of missingPaths) { | |||
| const value = map.has(missing); | |||
| assert.isTrue(value, `${missing} to be ${value === undefined ? "not present" : "present only once"}, in actual: ${missingPaths} expected: ${expected}`); | |||
| assert.isTrue(value, `${missing} to be ${!value ? "not present" : "present only once"}, in actual: ${missingPaths} expected: ${expected}`); | |||
There was a problem hiding this comment.
this was an actual bug but a very small one (just in the assert's message), and just in the test runner
|
I think the overall problem with this sort of thing is that a lot of the checks have just changed from a newly-detectable form to a different form we don't check, e.g. |
|
I agree, the changes to checks that I had to touch bring no value on their own. It's just that those checks are consistently not checked/validated by TS whereas I don't exactly believe that this alone leads to bugs. People can't strongly rely on those checks. But then the current behavior doesn't help them to catch those bugs that could be prevented with this change in place. From my POV, it's a like a post-factum headscratcher - "why this hasn't been caught by TS? it would yell at me if I would have this seemingly similar line of code!". And, of course, the checks here had to be only adjusted because the types were imprecise. If this wouldn't be the case then no changes would be required, this PR would only drop redundant code that was discovered in the process. That said, it's unclear to me how to best deal with unchecked indexed accesses here without... |
This could be pushed as part of #59559 and it would fix the
self-checkthere (when combined with #59584 ). I think there it might be easier to review this in isolation though and that changes here (at least most of them) are good regardless of what will happen to #59559I suspect many of those checks are artifacts of the fact that the codebase is old and originally it was written without
strictNullChecks. Some of the changes are more risky than others as there could be some incorrect!passed to those call sites. I have inspected all of the paths I've touched to derisk this but I'm just a human 😉 The good thing is that the existing tests pass :)