Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add logical assignment operator#37727

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

Kingwl
Copy link
Contributor

@KingwlKingwl commentedApr 1, 2020•
edited
Loading

Fixes#37255

ExE-Boss, acrazing, stefnotch, FFKL, wongmjane, ChayimFriedman2, g-patel, chocolateboy, and apalanki reacted with thumbs up emojiExE-Boss reacted with hooray emojileonardodino, dragomirtitian, IllusionMH, dashed, MattiasBuelens, kitos, tomholford, codler, and maykon-oliveira reacted with heart emojiExE-Boss reacted with rocket emojidashed reacted with eyes emoji
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commentedApr 1, 2020•
edited
Loading

The GitHub app is terrible so I can't review properly, but test 5 uses a function type returningvoid | undefined. That's probably a mistake.

Resolved

}

function foo2 (f?: (a: number) => void) {
f ||= (a => a)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Great test!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

😉

if (thing &&= thing.original) {
console.log(thing.name);
~~~~~
!!! error TS2532: Object is possibly 'undefined'.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Glad the test caught this - that was part of my intent when I wrote it. As you probably know, this needs to be fixed becausething.original should be defined. Let@ahejlsberg or@rbuckton know if you need help with that.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I spent all afternoon And evening in this one😂

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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@weswigham about that.

If you want to play around to see if you can get it working, I'll provide a few tips and@weswigham and@ahejlsberg can weigh in case I'm totally off.

First take a look at some of the logic at

constpreRightLabel=createBranchLabel();

and

if(operator===SyntaxKind.AmpersandAmpersandToken||operator===SyntaxKind.BarBarToken||operator===SyntaxKind.QuestionQuestionToken){

The binder needs to create branch labels for these new compound assignment operators (similar to inbindLogicalExpression) while creating at least one flow node that's considered an assignment after the expression (as inbindAssignmentTargetFlow). At a glance, Ithink that a lot of the logic can be reused and the changes can be added aroundhere

Eventually ingetTypeAtFlowNode in the type-checker, you need to try to find a way to combine the logic whenflags & FlowFlags.Assignment andflags & 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.

@@ -4355,6 +4365,12 @@ namespace ts {
|| token === SyntaxKind.ExclamationToken;
}

export function isLogicalAssignmentOperator(token: SyntaxKind): boolean {
Copy link
Member

@DanielRosenwasserDanielRosenwasserApr 1, 2020•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

ConsiderisShortCircuitingAssignmentOperator orisLogicalOrCoalescingAssignmentOperator

@@ -3334,6 +3340,10 @@ namespace ts {
return 14;
case SyntaxKind.AsteriskAsteriskToken:
return 15;
case SyntaxKind.BarBarEqualsToken:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah, it seems to be caught bygetOperatorPrecedence above.

@Kingwl
Copy link
ContributorAuthor

It seems works, but i don't know why.🤷🏻‍♂️

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 2, 2020•
edited
Loading

Heya@DanielRosenwasser, I've started to run the tarball bundle task on this PR at1c92781. You can monitor the buildhere.

@Kingwl
Copy link
ContributorAuthor

Current CFG is

"Call (results.push(100)) ─ ArrayMutation (results.push(100)) ─ Branch ┬ True (results) ─────────────────────────────────────────────────┬ Start                                                                        ├ True (results ||= []) ─┬ Assignment (results) ─ False (results) ╯                                                                         ╰ False (results ||= []) ╯                                            "

the same as downlevel result

"Call (results.push(100)) ─ ArrayMutation (results.push(100)) ─ Branch ┬ True (results) ─────────────────────────────────────────────────┬ Start                                                                        ├ True ((results = [])) ─┬ Assignment (results) ─ False (results) ╯                                                                         ╰ False ((results = [])) ╯                                            "
DanielRosenwasser reacted with thumbs up emojiDanielRosenwasser reacted with hooray emojiDanielRosenwasser reacted with heart emoji

@Kingwl
Copy link
ContributorAuthor

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 2, 2020•
edited
Loading

Heya@Kingwl, I've started to run the tarball bundle task on this PR ata6e0086. You can monitor the buildhere.

@typescript-bot
Copy link
Collaborator

Hey@Kingwl, I've packed this intoan installable tgz. You can install it for testing by referencing it in yourpackage.json like so:

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

and then runningnpm install.

@KingwlKingwl marked this pull request as ready for reviewApril 4, 2020 14:32
@Kingwl
Copy link
ContributorAuthor

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 4, 2020•
edited
Loading

Heya@Kingwl, I've started to run the tarball bundle task on this PR at8a9e234. You can monitor the buildhere.

@typescript-bot
Copy link
Collaborator

Hey@Kingwl, I've packed this intoan installable tgz. You can install it for testing by referencing it in yourpackage.json like so:

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

and then runningnpm install.

@Kingwl
Copy link
ContributorAuthor

Does the playground not supported pull request build now?🤷🏻‍♂️

@DanielRosenwasser
Copy link
Member

Does the playground not supported pull request build now?

@weswigham or@orta might know

@orta
Copy link
Contributor

orta commentedApr 7, 2020•
edited
Loading

Thanks for the ping, looks like I need to dig into the playground builder - been red for a week
https://github.com/orta/make-monaco-builds/actions - looks like a master build of TS gives a new error in the monaco-typescript codebase

@orta
Copy link
Contributor

orta commentedApr 7, 2020

Made a PR:microsoft/monaco-typescript#59

DanielRosenwasser reacted with thumbs up emojiDanielRosenwasser reacted with heart emoji

@orta
Copy link
Contributor

orta commentedApr 7, 2020

and withorta/make-monaco-builds@f17ca4a in

I can request a playground again -@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 7, 2020•
edited
Loading

Heya@orta, I've started to run the tarball bundle task on this PR at8a9e234. You can monitor the buildhere.

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 7, 2020•
edited
Loading

Hey@orta, I've packed this intoan installable tgz. You can install it for testing by referencing it in yourpackage.json like so:

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

and then runningnpm install.

There is also a playgroundfor this build.

@Kingwl
Copy link
ContributorAuthor

Only one pipeline failed with ECONNRESET.

@DanielRosenwasser
Copy link
Member

Looks like this needs to be rebased onto the latestmaster.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commentedMay 20, 2020•
edited
Loading

One more review?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commentedMay 27, 2020•
edited
Loading

One more peek@rbuckton?

@Kingwl
Copy link
ContributorAuthor

up👆

@DanielRosenwasserDanielRosenwasser merged commite832e04 intomicrosoft:masterJun 9, 2020
@KingwlKingwl deleted the logical_assignment branchJune 10, 2020 00:35
@aminpaks
Copy link
Contributor

@Kingwl I mentioned a case inhere. Would you confirm that this is not a bug?

@Xample
Copy link

Glad to see things moved since#20378 😀

DanielRosenwasser reacted with heart emoji

@g-patel
Copy link

What version of typescript will this be available in?

@ExE-Boss
Copy link
Contributor

ExE-Boss commentedSep 3, 2020•
edited
Loading

@g-patel This shippedin TypeScript 4.0.

LinusU reacted with thumbs up emoji

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

@DanielRosenwasserDanielRosenwasserDanielRosenwasser approved these changes

@rbucktonrbucktonrbuckton approved these changes

@ahejlsbergahejlsbergAwaiting requested review from ahejlsberg

Assignees

@elibarzilayelibarzilay

Labels
For Milestone BugPRs that fix a bug with a specific milestone
Projects
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nullish Coalescing and Logical Compound Assignments (??=,||=,&&=)
11 participants
@Kingwl@DanielRosenwasser@typescript-bot@orta@sandersn@elibarzilay@aminpaks@Xample@g-patel@ExE-Boss@rbuckton
[8]ページ先頭
©2009-2025 Movatter.jp