Skip to content

Conversation

@rbuckton
Copy link
Contributor

Fixes emit for optional chaining when a non-null assertion is present in the chain:

// tsobj?.a!.b// js (old)(obj===null||obj===void0 ? void0 : obj.a).b// js (new)obj===null||obj===void0 ? void0 : obj.a.b

Fixes#36031

@DanielRosenwasser
Copy link
Member

Why did we decide not to change the types here? Seems like we really shouldn't have one without the other.

@rbuckton
Copy link
ContributorAuthor

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 `?.`declareconsta: undefined|{b: number|null};// current behavior a!.b// number | nulla!.b!// numbera?.b// number | null | undefined*a?.b!// number// proposed behavior (differences) a?.b!// number | undefined*

If we give ! a lower precedence than ?. and OptionalChain, then it must always have a lower precedence than OptionalChain. With the proposed changes, to get the current behavior for a?.b! you would instead need to write (a?.b)!.

@sandersnsandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 5, 2020
@rbuckton
Copy link
ContributorAuthor

With the latest changes, a postfix-!inside of an optional chain will remove optionality only in the non-optional control flow branch, while a postfix-!at the end of an optional chain will remove optionality for the whole expression:

declareconsta: undefined|{b: {c: number}|null};a?.b.c;// errora?.b!.c;// number | undefineda?.b!;//{c: number }

This best matches user expectations.

# Conflicts: # src/compiler/debug.ts
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 28, 2020

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 4214548. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

Do we have tests for

letx=a?.b!();

Copy link
Contributor

@elibarzilayelibarzilay left a 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.)

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 28, 2020

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{"devDependencies":{"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/66655/artifacts?artifactName=tgz&fileId=96C836092C4432923665DCDF014700A31A0F45018B869320D4A73487C34B8B9E02&fileName=/typescript-3.9.0-insiders.20200228.tgz" } } 

and then running npm install.


There is also a playground for this build.

@sandersn
Copy link
Member

@rbuckton is this ready to merge?

@DanielRosenwasser
Copy link
Member

@rbuckton@RyanCavanaugh this should have landed in the beta

@DanielRosenwasser
Copy link
Member

@typescript-bot user test this
@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 25, 2020

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 4214548. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 25, 2020

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 4214548. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-3.9

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 25, 2020

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-3.9 on this PR at 4214548. You can monitor the build here.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Mar 25, 2020
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
@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #37599 for you.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

DanielRosenwasser pushed a commit that referenced this pull request Mar 25, 2020
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 <rbuckton@microsoft.com>
@rbucktonrbuckton merged commit b58a29b into masterMar 25, 2020
@rbuckton
Copy link
ContributorAuthor

@DanielRosenwasser I manually cherry-picked the last commit over, which just addresses @elibarzilay's comments about a dead-code branch and fixes a comment.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Optional chaining with non null operator is unsafe, because it could throw an exception

8 participants

@rbuckton@DanielRosenwasser@typescript-bot@sandersn@elibarzilay@weswigham@ajafff