Skip to content
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

Keep comments on an 'else' keyword when converting to conditional expressions. #76789

Merged
merged 5 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,7 @@ protected override ExpressionSyntax ConvertToExpression(IThrowOperation throwOpe

protected override ISyntaxFormatting SyntaxFormatting
=> CSharpSyntaxFormatting.Instance;

protected override (ConditionalExpressionSyntax conditional, bool makeMultiLine) UpdateConditionalExpression(IConditionalOperation originalIfStatement, ConditionalExpressionSyntax conditional)
=> CSharpUseConditionalExpressionHelpers.UpdateConditionalExpression(originalIfStatement, conditional);
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected override StatementSyntax WrapWithBlockIfAppropriate(
return statement;
}

protected override SyntaxNode WrapIfStatementIfNecessary(IConditionalOperation operation)
protected override ExpressionSyntax WrapIfStatementIfNecessary(IConditionalOperation operation)
{
if (operation.Syntax is IfStatementSyntax { Condition: CheckedExpressionSyntax exp })
return exp;
Expand All @@ -62,4 +62,7 @@ protected override ExpressionSyntax ConvertToExpression(IThrowOperation throwOpe

protected override ISyntaxFormatting SyntaxFormatting
=> CSharpSyntaxFormatting.Instance;

protected override (ConditionalExpressionSyntax conditional, bool makeMultiLine) UpdateConditionalExpression(IConditionalOperation originalIfStatement, ConditionalExpressionSyntax conditional)
=> CSharpUseConditionalExpressionHelpers.UpdateConditionalExpression(originalIfStatement, conditional);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,19 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.CodeGeneration;
using Microsoft.CodeAnalysis.CSharp.Extensions;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.UseConditionalExpression;

using static CSharpSyntaxTokens;

internal static class CSharpUseConditionalExpressionHelpers
{
public static ExpressionSyntax ConvertToExpression(IThrowOperation throwOperation)
Expand All @@ -16,4 +23,21 @@ public static ExpressionSyntax ConvertToExpression(IThrowOperation throwOperatio
RoslynDebug.Assert(throwStatement.Expression != null);
return SyntaxFactory.ThrowExpression(throwStatement.ThrowKeyword, throwStatement.Expression);
}

public static (ConditionalExpressionSyntax conditional, bool makeMultiLine) UpdateConditionalExpression(
IConditionalOperation originalIfStatement,
ConditionalExpressionSyntax conditional)
{
var ifStatement = (IfStatementSyntax)originalIfStatement.Syntax;

// Move any comments on the `else` keyword to then be on the `:` keyword. In that case, we definitely want to
// make this multiline.
if (ifStatement.Else is null || !ifStatement.Else.ElseKeyword.LeadingTrivia.Any(t => t.IsSingleOrMultiLineComment()))
return (conditional, makeMultiLine: false);

var finalConditional = conditional
.WithColonToken(ColonToken.WithPrependedLeadingTrivia(ifStatement.Else.ElseKeyword.LeadingTrivia))
.WithWhenTrue(conditional.WhenTrue.WithAppendedTrailingTrivia(SyntaxFactory.ElasticCarriageReturnLineFeed));
return (finalConditional, makeMultiLine: true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp;
Expand All @@ -22,7 +23,7 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.UseConditionalExpressio
public sealed partial class UseConditionalExpressionForAssignmentTests
{
private static async Task TestMissingAsync(
string testCode,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string testCode,
LanguageVersion languageVersion = LanguageVersion.CSharp8,
OptionsCollection? options = null)
{
Expand All @@ -37,8 +38,8 @@ private static async Task TestMissingAsync(
}

private static async Task TestInRegularAndScript1Async(
string testCode,
string fixedCode,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string testCode,
[StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string fixedCode,
LanguageVersion languageVersion = LanguageVersion.CSharp8,
OptionsCollection? options = null,
string? equivalenceKey = null)
Expand Down Expand Up @@ -2144,4 +2145,45 @@ public async Task TestGlobalStatements()
}
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/58897")]
public async Task TestCommentsOnElse()
{
await TestInRegularAndScript1Async(
"""
using System;

class C
{
void M(bool containsHighBits)
{
int write;

[|if|] (containsHighBits)
{
write = 0;
}
// Comment on else
else
{
write = 1;
}
}
}
""",
"""
using System;

class C
{
void M(bool containsHighBits)
{
int write = containsHighBits
? 0
// Comment on else
: 1;
}
}
""", LanguageVersion.CSharp9);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -96,21 +96,22 @@ protected async Task<TExpressionSyntax> CreateConditionalExpressionAsync(
{
return negate
? (TExpressionSyntax)generator.Negate(generatorInternal, condition, semanticModel, cancellationToken).WithoutTrivia()
: (TExpressionSyntax)condition.WithoutTrivia();
: condition.WithoutTrivia();
}

var trueExpression = MakeRef(generatorInternal, isRef, CastValueIfNecessary(generator, trueStatement, trueValue));
var falseExpression = MakeRef(generatorInternal, isRef, CastValueIfNecessary(generator, falseStatement, falseValue));
trueExpression = WrapReturnExpressionIfNecessary(trueExpression, trueStatement);
falseExpression = WrapReturnExpressionIfNecessary(falseExpression, falseStatement);

var conditionalExpression = (TConditionalExpressionSyntax)generator.ConditionalExpression(
var initialExpression = (TConditionalExpressionSyntax)generator.ConditionalExpression(
condition.WithoutTrivia(),
trueExpression,
falseExpression);
var (conditionalExpression, makeMultiLine) = UpdateConditionalExpression(ifOperation, initialExpression);

conditionalExpression = conditionalExpression.WithAdditionalAnnotations(Simplifier.Annotation);
var makeMultiLine = await MakeMultiLineAsync(
makeMultiLine = makeMultiLine || await MakeMultiLineAsync(
document, condition,
trueValue.Syntax, falseValue.Syntax, formattingOptions, cancellationToken).ConfigureAwait(false);
if (makeMultiLine)
Expand All @@ -122,8 +123,14 @@ protected async Task<TExpressionSyntax> CreateConditionalExpressionAsync(
return MakeRef(generatorInternal, isRef, conditionalExpression);
}

protected virtual SyntaxNode WrapIfStatementIfNecessary(IConditionalOperation operation)
=> operation.Condition.Syntax;
protected virtual (TConditionalExpressionSyntax conditional, bool makeMultiLine) UpdateConditionalExpression(
IConditionalOperation originalIfStatement, TConditionalExpressionSyntax conditionalExpression)
{
return (conditionalExpression, makeMultiLine: false);
}

protected virtual TExpressionSyntax WrapIfStatementIfNecessary(IConditionalOperation operation)
=> (TExpressionSyntax)operation.Condition.Syntax;

protected virtual TExpressionSyntax WrapReturnExpressionIfNecessary(TExpressionSyntax returnExpression, IOperation returnOperation)
=> returnExpression;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ Imports Microsoft.CodeAnalysis.LanguageService
Imports Microsoft.CodeAnalysis.Operations
Imports Microsoft.CodeAnalysis.Simplification
Imports Microsoft.CodeAnalysis.UseConditionalExpression
Imports Microsoft.CodeAnalysis.VisualBasic.CodeGeneration
Copy link
Contributor

Choose a reason for hiding this comment

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

is the new import needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will see if I can remove

Imports Microsoft.CodeAnalysis.VisualBasic.Formatting
Imports Microsoft.CodeAnalysis.VisualBasic.LanguageService
Imports Microsoft.CodeAnalysis.VisualBasic.Syntax

Namespace Microsoft.CodeAnalysis.VisualBasic.UseConditionalExpression

<ExportCodeFixProvider(LanguageNames.VisualBasic, Name:=PredefinedCodeFixProviderNames.UseConditionalExpressionForAssignment), [Shared]>
Friend Class VisualBasicUseConditionalExpressionForAssignmentCodeFixProvider
Friend NotInheritable Class VisualBasicUseConditionalExpressionForAssignmentCodeFixProvider
Inherits AbstractUseConditionalExpressionForAssignmentCodeFixProvider(Of
StatementSyntax, MultiLineIfBlockSyntax, LocalDeclarationStatementSyntax, VariableDeclaratorSyntax, ExpressionSyntax, TernaryConditionalExpressionSyntax)

Expand Down
Loading