Skip to content

Add features to offer using conditional expressions ?: over explicit if-statement flows. (squashed)#26572

Closed
CyrusNajmabadi wants to merge 3 commits intodotnet:masterfrom
CyrusNajmabadi:useConditionalSquashed
Closed

Add features to offer using conditional expressions ?: over explicit if-statement flows. (squashed)#26572
CyrusNajmabadi wants to merge 3 commits intodotnet:masterfrom
CyrusNajmabadi:useConditionalSquashed

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented May 2, 2018 •
edited by jcouv
Loading

[jcouv edited] This PR is a squashed version of #26236

For example, with:

image

We offer:

image

Todo:

  • VB
  • Tests
  • Better formatting for large or multi-line clauses in the ?: expression.
  • Support this for if-statements with return statements in their bodies.

For the last one, this means recognizing:

if (expr) {
    return a;
} else {
    return b;
}

// as well as

if (expr) {
    return a;
}

return b;

and offering to convert to:

return expr ? a : b;

[jcouv edited] Fixes #26539

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 2, 2018 20:43
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 2, 2018

Tagging @ivanbasov you can review this. I'll incorporate your suggested changes into both branches.

@ivanbasov
Copy link
Copy Markdown
Contributor

ivanbasov commented May 2, 2018

Thank you very much, @CyrusNajmabadi! Will review.

@etbyrd etbyrd added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 2, 2018
ivanbasov
ivanbasov reviewed May 2, 2018
View reviewed changes
Comment thread ...onalExpression/ForAssignment/AbstractUseConditionalExpressionForAssignmentCodeFixProvider.cs Outdated

// We found a valid local declaration right above the if-statement.
var localDeclaration = localDeclarationOperation.Syntax;
var declarator = localDeclarationOperation.Declarations[0].Declarators[0];
Copy link
Copy Markdown
Contributor

@ivanbasov ivanbasov May 2, 2018

Choose a reason for hiding this comment

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

var declarator = localDeclarationOperation.Declarations[0].Declarators[0]; [](start = 12, length = 74)

what if there are multiple declarations just above the if?
e.g.

int x = 5, y = M();
if (condition)
{
x = 4;
}
else
{
x = 6;
}

Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi CyrusNajmabadi May 2, 2018

Choose a reason for hiding this comment

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

then we will convert that to:

int x = 5, y = M();
x = condition ? 4 : 6;

Copy link
Copy Markdown
Contributor

@ivanbasov ivanbasov May 2, 2018

Choose a reason for hiding this comment

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

Likely, you have a check for the count of local declarations in the method below. Then, a comment in the current method should be enough.

In reply to: 185655094 [](ancestors = 185655094)

Copy link
Copy Markdown
Contributor

@ivanbasov ivanbasov May 2, 2018

Choose a reason for hiding this comment

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

Thank you! I see.
I just want to mention that this can be found when reviewing the method below but not the current one.

I'd add a comment saying something like:
// The call of TryFindMatchingLocalDeclarationImmediatelyAbove above guarantees a single Declaration and a single Declarator.

In reply to: 185656240 [](ancestors = 185656240)

Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi CyrusNajmabadi May 2, 2018

Choose a reason for hiding this comment

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

I updated the helper method to pass back the declarator as well. that just keeps things nice and simple.

ivanbasov
ivanbasov reviewed May 2, 2018
View reviewed changes
Comment thread ...nditionalExpression/ForReturn/AbstractUseConditionalExpressionForReturnDiagnosticAnalyzer.cs Outdated
protected override void InitializeWorker(AnalysisContext context)
=> context.RegisterOperationAction(AnalyzeOperation, OperationKind.Conditional);

private void AnalyzeOperation(OperationAnalysisContext context)
Copy link
Copy Markdown
Contributor

@ivanbasov ivanbasov May 2, 2018

Choose a reason for hiding this comment

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

AnalyzeOperation [](start = 21, length = 16)

If I'm not mistaken, this method completely coincides with the same method in AbstractUseConditionalExpressionForAssignmentDiagnosticAnalyzer. Should not we create a base class for both them sharing the method?

Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi CyrusNajmabadi May 2, 2018

Choose a reason for hiding this comment

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

i'll take a looksie.

Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi CyrusNajmabadi May 2, 2018

Choose a reason for hiding this comment

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

Done. Great suggestion!

ivanbasov
ivanbasov approved these changes May 2, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ivanbasov ivanbasov left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi mentioned this pull request May 3, 2018
Merged
4 tasks
@jcouv jcouv added the Area-IDE label May 3, 2018
@jcouv jcouv added this to the 15.8 milestone May 3, 2018
@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 3, 2018

@jinujoseph for ask-mode approval for 15.8. Thanks

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 3, 2018

@jinujoseph Sorry, the present PR is actually a squashed version of #26236. Please approve that one (#26236).

@jcouv jcouv changed the title Add features to offer using conditional expressions ?: over explicit if-statement flows. Add features to offer using conditional expressions ?: over explicit if-statement flows. (squashed) May 3, 2018
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented May 3, 2018

Closing in favor of #26236

@CyrusNajmabadi CyrusNajmabadi deleted the useConditionalSquashed branch May 3, 2018 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ivanbasov ivanbasov ivanbasov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

15.8

Development

Successfully merging this pull request may close these issues.

5 participants

@CyrusNajmabadi @ivanbasov @jcouv @jinujoseph @etbyrd