Fix TS 4.0 breaks #10415
Fix TS 4.0 breaks #10415
Conversation
1. Pass `undefined` to now-required parameters of typeToTypeNode.
2. Fix new error "operand of `delete` must be optional".
Typescript 4.0 only allows deletion of properties that are marked as
optional. This allows control flow to narrow the type after deletion to
`undefined`, preventing use-after-delete:
```ts
declare var o: { p?: number }
o.p // p: number | undefined
o.p = 12
o.p // p: number
o.p++ // OK
delete o.p
o.p // p: undefined
o.p++ // ERROR
```
Unfortunately, for this to work, the property must include `undefined` in
its type. Currently, this PR doesn't actually make properties optional.
It just casts the object operand to `any`, which silences the error.
Please use this PR as a starting point for an actual fix, which may end
up pretty complex.
Fixes Azure#10414, although casting to `any` is *not* a good long-term fix
for problem (2).
sandersn
requested review from
bterlson,
chradek,
daviwil,
jeremymeng,
KarishmaGhiya,
mikeharder,
richardpark-msft and
xirzec
as code owners
|
The CI log lists @azure/ai-form-recognizer as the failing test suite, but I can't find the actual failure. |
If you are referring to the Mac-only test failure, it was probably a flaky test. I triggered a re-run of the Mac tests. |
There was a problem hiding this comment.
@sandersn: Thanks for testing our repo with TS 4.0 and submitting this PR.
I defer to @xirzec regarding the code changes.
In terms of process, my preference would be to keep this PR open until we are ready to actually upgrade to TS 4.0, and include the upgrade to TS 4.0 in the PR itself (as opposed to merging this now and upgrading to TS 4.0 later). So we could add the changes to upgrade to TS 4.0.0-beta in this PR right now, then update to 4.0.0-rc and finally 4.0.0, each time ensuring the CI builds pass. And if we find additional changes required for 4.0 we can collect them in this PR.
|
The type checker change seems reasonable to me, though /cc @willmtemple for visibility. The test change I'm okay with as-is, though we could also consider figuring out a better way to write these tests. I'm not very motivated to spend a lot of time on |
undefinedto now-required parameters of typeToTypeNode.deletemust be optional".Typescript 4.0 only allows deletion of properties that are marked as optional. This allows control flow to narrow the type after deletion to
undefined, preventing use-after-delete:Unfortunately, for this to work, the property must include
undefinedin its type. Currently, this PR doesn't actually make properties optional. It just casts the object operand toany, which silences the error. Please use this PR as a starting point for an actual fix, which may end up pretty complex.Fixes #10414, although casting to
anyis not a good long-term fix for problem (2).Edit: I just noticed that all the
deletes happen in tests, to eliminate unique ids before testing equality, so probably casting toanyis good enough.This PR does not upgrade azure-sdk to TS 4.0. We'll have to manually upgrade the repo to 4.0 to test this. I can provide the instructions I used if needed, although they're pretty hacky.