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

Do syntactic cleanup during the initial pass of producing fixed documents in fix-all. #73383

Merged
merged 15 commits into from
May 8, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ internal class CSharpMakeMethodAsynchronousCodeFixProvider : AbstractMakeMethodA
private const string CS4034 = nameof(CS4034); // The 'await' operator can only be used within an async lambda expression. Consider marking this method with the 'async' modifier.
private const string CS0246 = nameof(CS0246); // The type or namespace name 'await' could not be found

private static readonly SyntaxToken s_asyncKeywordWithSpace = AsyncKeyword.WithoutTrivia().WithTrailingTrivia(Space);
Copy link
Member Author

Choose a reason for hiding this comment

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

fixing up the feature so it consistently emits async without messing with other trivia.


[ImportingConstructor]
[SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
public CSharpMakeMethodAsynchronousCodeFixProvider()
Expand Down Expand Up @@ -92,7 +94,7 @@ private static MethodDeclarationSyntax FixMethod(
CancellationToken cancellationToken)
{
var newReturnType = FixMethodReturnType(keepVoid, methodSymbol, method.ReturnType, knownTypes, cancellationToken);
var newModifiers = AddAsyncModifierWithCorrectedTrivia(method.Modifiers, ref newReturnType);
(var newModifiers, newReturnType) = AddAsyncModifierWithCorrectedTrivia(method.Modifiers, newReturnType);
return method.WithReturnType(newReturnType).WithModifiers(newModifiers);
}

Expand All @@ -104,7 +106,7 @@ private static LocalFunctionStatementSyntax FixLocalFunction(
CancellationToken cancellationToken)
{
var newReturnType = FixMethodReturnType(keepVoid, methodSymbol, localFunction.ReturnType, knownTypes, cancellationToken);
var newModifiers = AddAsyncModifierWithCorrectedTrivia(localFunction.Modifiers, ref newReturnType);
(var newModifiers, newReturnType) = AddAsyncModifierWithCorrectedTrivia(localFunction.Modifiers, newReturnType);
return localFunction.WithReturnType(newReturnType).WithModifiers(newModifiers);
}

Expand Down Expand Up @@ -176,15 +178,15 @@ private static bool IsIEnumerable(ITypeSymbol returnType, KnownTaskTypes knownTy
private static bool IsIEnumerator(ITypeSymbol returnType, KnownTaskTypes knownTypes)
=> returnType.OriginalDefinition.Equals(knownTypes.IEnumeratorOfTType);

private static SyntaxTokenList AddAsyncModifierWithCorrectedTrivia(SyntaxTokenList modifiers, ref TypeSyntax newReturnType)
private static (SyntaxTokenList newModifiers, TypeSyntax newReturnType) AddAsyncModifierWithCorrectedTrivia(SyntaxTokenList modifiers, TypeSyntax returnType)
{
if (modifiers.Any())
return modifiers.Add(AsyncKeyword);
return (modifiers.Add(s_asyncKeywordWithSpace), returnType);

// Move the leading trivia from the return type to the new modifiers list.
var result = TokenList(AsyncKeyword.WithLeadingTrivia(newReturnType.GetLeadingTrivia()));
newReturnType = newReturnType.WithoutLeadingTrivia();
return result;
var newModifiers = TokenList(s_asyncKeywordWithSpace.WithLeadingTrivia(returnType.GetLeadingTrivia()));
var newReturnType = returnType.WithoutLeadingTrivia();
return (newModifiers, newReturnType);
}

private static AnonymousFunctionExpressionSyntax FixAnonymousFunction(AnonymousFunctionExpressionSyntax anonymous)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ class C
void Goo()
{
a?.Invoke();

a?.Invoke();
Copy link
Member Author

Choose a reason for hiding this comment

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

these changed because the code that was generated was (a)?.Invoke(); with elastic trivia on the parens. But when the parens were removed in the simplification pass, we lost that trivia. So we weren't formatting properly. the new formatting follows the expected formatting rules now.

}
}
Expand Down Expand Up @@ -86,7 +85,6 @@ class C
void Goo()
{
a?.Invoke();

a?.Invoke();
}
}
Expand Down Expand Up @@ -126,7 +124,6 @@ class C
void Goo()
{
a?.Invoke();

a?.Invoke();
}
}
Expand Down Expand Up @@ -166,7 +163,6 @@ class C
void Goo()
{
a?.Invoke();

a?.Invoke();
}
}
Expand Down Expand Up @@ -206,7 +202,6 @@ class C
void Goo()
{
a?.Invoke();

a?.Invoke();
}
}
Expand Down Expand Up @@ -246,7 +241,6 @@ class C
void Goo()
{
a?.Invoke();

a?.Invoke();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,8 @@ class C
{
public void M1()
{
Func<int, Task> foo = x => {
Func<int, Task> foo = x =>
{
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the correct formatting ew want. the previous results were undesirable.

if (System.DateTime.Now.Ticks > 0)
{
return Task.CompletedTask;
Expand Down Expand Up @@ -1040,7 +1041,8 @@ class C
{
public void M1()
{
Func<Task> foo = () => {
Func<Task> foo = () =>
{
if (System.DateTime.Now.Ticks > 0)
{
return Task.CompletedTask;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class C
{
void M(int? x, int? y)
{
var z = x ?? y ;
var z = x ?? y;
Copy link
Member Author

Choose a reason for hiding this comment

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

same. the old results were undesirable.

}
}
""");
Expand Down Expand Up @@ -105,7 +105,7 @@ class C
{
void M(int? x, int? y)
{
var z = (x + y) ?? y ;
var z = (x + y) ?? y;
}
}
""");
Expand Down Expand Up @@ -163,7 +163,7 @@ class C
void M(int? x, int? y)
{
var z1 = x ?? y;
var z2 = x ?? y ;
var z2 = x ?? y;
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixed an issue here where we had errant whitespace.

}
}
""");
Expand Down Expand Up @@ -249,7 +249,7 @@ class C
{
void M(int? x, int? y)
{
Expression<Func<int>> e = () => {|Warning:x ?? y|} ;
Expression<Func<int>> e = () => {|Warning:x ?? y|};
}
}
""");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -796,8 +796,8 @@ Imports System
Public Class TestClass
Public Sub Caller(i As Integer, j As Integer)
Dim x = Function()
Return i * j
End Function()
Return i * j
End Function()
End Sub
##
Private Function Callee(i As Integer, j As Integer) as Func(Of Integer)
Expand Down Expand Up @@ -1288,7 +1288,7 @@ Public Class TestClass
End Class", "
Public Class TestClass
Public Sub Caller(i As Integer)
Dim y = If (true, 10, 100)
Dim y = If(true, 10, 100)
End Sub
##
Private Function Callee(a As Boolean) As Integer
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4156,10 +4156,10 @@ List<int> M(IEnumerable<int> nums)
return /*30*/ /* 1 *//* 2 *//* 3 *//* 4 */// 5
/*31*//* 6 */
(from/* 8 *//* 7 *//* 9 */x /* 10 */ in/* 11 */nums/* 12 */// 13
/* 14 */// 15
/* 16 *//* 17 */
let y /* 18 */ = /* 19 */ x + 1/* 20 *///21
select y)/* 24 *//*27*///28
/* 14 */// 15
/* 16 *//* 17 */
let y /* 18 */ = /* 19 */ x + 1/* 20 *///21
select y)/* 24 *//*27*///28
Copy link
Member Author

Choose a reason for hiding this comment

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

these results are expected. the other clauses align with the from now.

Copy link
Member Author

Choose a reason for hiding this comment

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

aside; these tests are insane. they don't reflect any sort of real code, and it's nigh impossible to reason about them.

.ToList()/* 22 *//* 23 *//* 25 *///26
; //32
}
Expand Down Expand Up @@ -4205,7 +4205,7 @@ List<int> M(IEnumerable<int> nums)
/*25*//* 14 */// 15
/* 6 */
(from/* 8 *//* 7 *//* 9 */x /* 10 */ in/* 11 */nums/* 12 */// 13
select x + 1)/* 18 *//*21*///22
select x + 1)/* 18 *//*21*///22
.ToList()/* 16 *//* 17 *//* 19 *///20
; //26
}
Expand All @@ -4223,7 +4223,8 @@ List<int> M(IEnumerable<int> nums)
{
/*23*/
return /*24*/ /* 1 *//* 2 *//* 3 *//* 4 */// 5
/*25*/nums /* 12 */.Select(
/*25*/
nums /* 12 */.Select(
/* 6 *//* 7 *//* 14 */// 15
/* 9 */x /* 10 */ => x + 1/* 18 *//*21*///22
/* 8 *//* 11 */// 13
Expand Down Expand Up @@ -4268,7 +4269,7 @@ int M(IEnumerable<int> nums)
/*23*//* 14 */// 15
/* 6 */
(from/* 8 *//* 7 *//* 9 */x /* 10 */ in/* 11 */nums/* 12 */// 13
select x)/* 10 *//*19*///20
select x)/* 10 *//*19*///20
.Count()/* 16 *//* 17 *///18
; //24
}
Expand All @@ -4286,7 +4287,8 @@ int M(IEnumerable<int> nums)
{
/*21*/
return /*22*/ /* 1 *//* 2 *//* 3 *//* 4 */// 5
/*23*/nums /* 12 *//* 6 *//* 7 *//* 14 */// 15
/*23*/
nums /* 12 *//* 6 *//* 7 *//* 14 */// 15
/* 9 *//* 10 *//* 10 *//*19*///20
/* 8 *//* 11 */// 13
.Count()/* 16 *//* 17 *///18
Expand Down Expand Up @@ -4328,11 +4330,11 @@ void M(IEnumerable<int> nums)
{
foreach (var (a /* 12 */ , b /*16*/ ) in
/* 1 */from/* 2 */int /* 3 */ n1 /* 4 */in/* 5 */nums/* 6 */// 7
/* 8*/// 9
/* 10 *//* 11 */
let a /* 12 */ = /* 13 */ n1 + n1/* 14*//* 15 */
let b /*16*/ = /*17*/ n1 * n1/*18*///19
select (a /* 12 */ , b /*16*/ )/*22*//*23*/)
/* 8*/// 9
/* 10 *//* 11 */
let a /* 12 */ = /* 13 */ n1 + n1/* 14*//* 15 */
let b /*16*/ = /*17*/ n1 * n1/*18*///19
select (a /* 12 */ , b /*16*/ )/*22*//*23*/)
{
/*20*/
Console.WriteLine(a + b);//21
Expand Down Expand Up @@ -4384,7 +4386,7 @@ void M(IEnumerable<int> nums)
/* 10 */
where/* 11 *//* 12 */n1 /* 13 */ > /* 14 */ 0/* 15 */// 16
select n1/* 4 *//* 21 */// 22
/*23*//*24*/
/*23*//*24*/
)
{
/*19*/
Expand Down
24 changes: 15 additions & 9 deletions src/Features/CSharpTest/InlineTemporary/InlineTemporaryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.CodeRefactorings.InlineTemporary;
using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.CodeAnalysis.UnitTests;
Expand All @@ -16,7 +15,7 @@
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.CodeRefactorings.InlineTemporary
{
[Trait(Traits.Feature, Traits.Features.CodeActionsInlineTemporary)]
public class InlineTemporaryTests : AbstractCSharpCodeActionTest_NoEditor
public sealed class InlineTemporaryTests : AbstractCSharpCodeActionTest_NoEditor
{
protected override CodeRefactoringProvider CreateCodeRefactoringProvider(TestWorkspace workspace, TestParameters parameters)
=> new CSharpInlineTemporaryCodeRefactoringProvider();
Expand Down Expand Up @@ -253,16 +252,23 @@ public void M()
[Fact]
public async Task Conversion_NoConversion()
{
await TestFixOneAsync(
await TestAsync(
"""
{ int [||]x = 3;
class C
{
void F(){ int [||]x = 3;

x.ToString(); }
}
""",
"""
class C
{
void F(){
3.ToString(); }
}
""",
"""
{
3.ToString(); }
""");
CSharpParseOptions.Default);
Copy link
Member Author

Choose a reason for hiding this comment

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

this test was impossible to understand in its original form (due to how it concats strings). The updated test shows the full code sample, and shows the desired behavior (we don't touch the { on the method when inlining here.

}

[Fact]
Expand Down Expand Up @@ -690,7 +696,7 @@ class Program
static void Main()
{
int x = 2;
Bar(x < x, x > 1+2);
Bar(x < x, x > 1 + 2);
Copy link
Member Author

Choose a reason for hiding this comment

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

formatting this now because it's parsed as an arithmetic + binexpr.

}

static void Bar(object a, object b)
Expand Down
2 changes: 1 addition & 1 deletion src/Features/CSharpTest/InvertIf/InvertIfTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,7 @@ public async Task TestEmptyIf()
{
await TestInRegularAndScriptAsync(
@"class C { void M(string s){ [||]if (s == ""a""){}else{ s = ""b""}}}",
@"class C { void M(string s){ if (s != ""a""){ s = ""b""}}}");
@"class C { void M(string s){ if (s != ""a"") { s = ""b""}}}");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/43224")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeRefactorings;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.LanguageService;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.CodeAnalysis.Shared.Extensions;
Expand Down Expand Up @@ -552,10 +553,14 @@ private async Task<SyntaxNode> GetChangedCallerAsync(Document document,
// After:
// void Caller() { var x = ((Func<int>)(() => 1))(); }
// Func<int> Callee() { return () => 1; }
//
// Also, ensure that the node is formatted properly at the destination location. This is needed as the
// location of the destination node might be very different (indentation/nesting wise) from the original
// method where the inlined code is coming from.
inlineExpression = (TExpressionSyntax)syntaxGenerator.AddParentheses(
syntaxGenerator.CastExpression(
GenerateTypeSyntax(calleeMethodSymbol.ReturnType, allowVar: false),
syntaxGenerator.AddParentheses(inlineMethodContext.InlineExpression)));
syntaxGenerator.AddParentheses(inlineMethodContext.InlineExpression.WithAdditionalAnnotations(Formatter.Annotation))));

}

Expand Down
36 changes: 26 additions & 10 deletions src/Workspaces/Core/Portable/CodeActions/CodeAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -518,23 +518,39 @@ protected virtual async Task<Document> PostProcessChangesAsync(Document document
return document;
}

internal static async Task<Document> CleanupDocumentAsync(
Document document, CodeCleanupOptions options, CancellationToken cancellationToken)
internal static async Task<Document> CleanupDocumentAsync(Document document, CodeCleanupOptions options, CancellationToken cancellationToken)
{
document = await ImportAdder.AddImportsFromSymbolAnnotationAsync(
document, Simplifier.AddImportsAnnotation, options.AddImportOptions, cancellationToken).ConfigureAwait(false);

document = await Simplifier.ReduceAsync(document, Simplifier.Annotation, options.SimplifierOptions, cancellationToken).ConfigureAwait(false);
// First, do a syntax pass. Ensuring that things are formatted correctly based on the original nodes and
// tokens. We want to do this prior to cleaning semantics as semantic cleanup can change the shape of the tree
// (for example, by removing tokens), which can then cause formatting to not work as expected.
var document1 = await CleanupSyntaxAsync(document, options, cancellationToken).ConfigureAwait(false);

// Now, do the semantic cleaning pass.
var document2 = await CleanupSemanticsAsync(document1, options, cancellationToken).ConfigureAwait(false);
return document2;
}

internal static async Task<Document> CleanupSyntaxAsync(Document document, CodeCleanupOptions options, CancellationToken cancellationToken)
{
// format any node with explicit formatter annotation
document = await Formatter.FormatAsync(document, Formatter.Annotation, options.FormattingOptions, cancellationToken).ConfigureAwait(false);
var document1 = await Formatter.FormatAsync(document, Formatter.Annotation, options.FormattingOptions, cancellationToken).ConfigureAwait(false);

// format any elastic whitespace
document = await Formatter.FormatAsync(document, SyntaxAnnotation.ElasticAnnotation, options.FormattingOptions, cancellationToken).ConfigureAwait(false);
var document2 = await Formatter.FormatAsync(document1, SyntaxAnnotation.ElasticAnnotation, options.FormattingOptions, cancellationToken).ConfigureAwait(false);
return document2;
}

document = await CaseCorrector.CaseCorrectAsync(document, CaseCorrector.Annotation, cancellationToken).ConfigureAwait(false);
internal static async Task<Document> CleanupSemanticsAsync(Document document, CodeCleanupOptions options, CancellationToken cancellationToken)
{
var document1 = await ImportAdder.AddImportsFromSymbolAnnotationAsync(document, Simplifier.AddImportsAnnotation, options.AddImportOptions, cancellationToken).ConfigureAwait(false);
var document2 = await Simplifier.ReduceAsync(document1, Simplifier.Annotation, options.SimplifierOptions, cancellationToken).ConfigureAwait(false);
var document3 = await CaseCorrector.CaseCorrectAsync(document2, CaseCorrector.Annotation, cancellationToken).ConfigureAwait(false);

return document;
// After doing the semantic cleanup, do another syntax cleanup pass to ensure that the tree is in a good state.
// The semantic cleanup passes may have introduced new nodes with elastic trivia that have to be cleaned.
var document4 = await CleanupSyntaxAsync(document3, options, cancellationToken).ConfigureAwait(false);

return document4;
}

#region Factories for standard code actions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixesAndRefactorings;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;
Expand Down
Loading
Loading