Add logical assignment operator - #37727
Conversation
|
Resolved |
| } | ||
|
|
||
| function foo2 (f?: (a: number) => void) { | ||
| f ||= (a => a) |
There was a problem hiding this comment.
Great test!
| if (thing &&= thing.original) { | ||
| console.log(thing.name); | ||
| ~~~~~ | ||
| !!! error TS2532: Object is possibly 'undefined'. |
There was a problem hiding this comment.
Glad the test caught this - that was part of my intent when I wrote it. As you probably know, this needs to be fixed because thing.original should be defined. Let Anders Hejlsberg (@ahejlsberg) or Ron Buckton (@rbuckton) know if you need help with that.
There was a problem hiding this comment.
I spent all afternoon And evening in this one😂
There was a problem hiding this comment.
Yeah, the code can be hard to get through, and I've honestly never wired up the CFG. Also, binary expressions recently changed to use a "trampolining" approach to avoid stack overflows on deep traversals. You can ask Wesley Wigham (@weswigham) about that.
If you want to play around to see if you can get it working, I'll provide a few tips and Wesley Wigham (@weswigham) and Anders Hejlsberg (@ahejlsberg) can weigh in case I'm totally off.
First take a look at some of the logic at
TypeScript/src/compiler/binder.ts
Line 1418 in b58a29b
and
TypeScript/src/compiler/binder.ts
Line 1513 in b58a29b
The binder needs to create branch labels for these new compound assignment operators (similar to in bindLogicalExpression) while creating at least one flow node that's considered an assignment after the expression (as in bindAssignmentTargetFlow). At a glance, I think that a lot of the logic can be reused and the changes can be added around here
Eventually in getTypeAtFlowNode in the type-checker, you need to try to find a way to combine the logic when flags & FlowFlags.Assignment and flags & FlowFlags.Label both apply. To start, I'd just get something working, then worry about how to share the code.
Feel free to ask some questions if you need help. If you'd prefer, I'm also sure we could send a PR if you'd like, but it's up to you.
| || token === SyntaxKind.ExclamationToken; | ||
| } | ||
|
|
||
| export function isLogicalAssignmentOperator(token: SyntaxKind): boolean { |
There was a problem hiding this comment.
Consider isShortCircuitingAssignmentOperator or isLogicalOrCoalescingAssignmentOperator
| return 14; | ||
| case SyntaxKind.AsteriskAsteriskToken: | ||
| return 15; | ||
| case SyntaxKind.BarBarEqualsToken: |
There was a problem hiding this comment.
This section doesn't seem to cover any of the assignment or compound assignment operators. I'd get clarity from whoever originally wrote this function whether this should be here, because I think these 3 cases don't belong here.
There was a problem hiding this comment.
Yeah, it seems to be caught by getOperatorPrecedence above.
2f6dfd7 to
1c92781
Compare
|
It seems works, but i don't know why.🤷🏻♂️ |
|
TypeScript Bot (@typescript-bot) pack this |
|
Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the tarball bundle task on this PR at 1c92781. You can monitor the build here. |
|
Current CFG is the same as downlevel result |
|
TypeScript Bot (@typescript-bot) pack this. |
|
Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at a6e0086. You can monitor the build here. |
|
Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
17b858c to
1ed6a3e
Compare
|
TypeScript Bot (@typescript-bot) pack this. |
|
Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at 8a9e234. You can monitor the build here. |
|
Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
Does the playground not supported pull request build now?🤷🏻♂️ |
Wesley Wigham (@weswigham) or Orta Therox (@orta) might know |
|
Thanks for the ping, looks like I need to dig into the playground builder - been red for a week |
|
Made a PR: microsoft/monaco-typescript#59 |
|
and with orta/make-monaco-builds@f17ca4a in I can request a playground again - TypeScript Bot (@typescript-bot) pack this. |
|
Heya Orta Therox (@orta), I've started to run the tarball bundle task on this PR at 8a9e234. You can monitor the build here. |
|
Hey Orta Therox (@orta), 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. |
f17eb88 to
39ae072
Compare
|
Only one pipeline failed with ECONNRESET. |
|
Looks like this needs to be rebased onto the latest |
ad70d78 to
b59e4c5
Compare
|
One more review? |
|
One more peek Ron Buckton (@rbuckton)? |
|
up👆 |
|
Wenlu Wang (@Kingwl) I mentioned a case in here. Would you confirm that this is not a bug? |
|
Glad to see things moved since #20378 😀 |
|
What version of typescript will this be available in? |
|
Gaurang Patel (@g-patel) This shipped in TypeScript 4.0. |
Fixes #37255