-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Fix emit for optional chain with non-null assertion #36539
Conversation
Why did we decide not to change the types here? Seems like we really shouldn't have one without the other. |
There are some odd quirks to consider if we change the precedence of // note: `undefined*` indicates an `undefined` that was added to the type by `?.`
declare const a: undefined | { b: number | null };
// current behavior
a!.b // number | null
a!.b! // number
a?.b // number | null | undefined*
a?.b! // number
// proposed behavior (differences)
a?.b! // number | undefined* If we give |
With the latest changes, a postfix- declare const a: undefined | { b: { c: number } | null };
a?.b.c; // error
a?.b!.c; // number | undefined
a?.b!; // { c: number } This best matches user expectations. |
# Conflicts: # src/compiler/debug.ts
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 4214548. You can monitor the build here. |
Do we have tests for let x = a?.b!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(To be taken with plenty of salt.)
Hey @DanielRosenwasser, 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. |
@rbuckton is this ready to merge? |
@rbuckton @RyanCavanaugh this should have landed in the beta |
@typescript-bot user test this |
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 4214548. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 4214548. You can monitor the build here. |
@typescript-bot cherry-pick this to release-3.9 |
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
Hey @DanielRosenwasser, I've opened #37599 for you. |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
Component commits: 0f8c1f7 Fix emit for optional chain with non-null assertion d187ca7 Treat '!' differently inside chain vs end of chain 4214548 Merge branch 'master' into fix36031 # Conflicts: # src/compiler/debug.ts Co-authored-by: Ron Buckton <[email protected]>
@DanielRosenwasser I manually cherry-picked the last commit over, which just addresses @elibarzilay's comments about a dead-code branch and fixes a comment. |
Fixes emit for optional chaining when a non-null assertion is present in the chain:
Fixes #36031