Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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 @@ -3,16 +3,12 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using Microsoft.CodeAnalysis.BraceCompletion;
using Microsoft.CodeAnalysis.CSharp.Formatting;
using Microsoft.CodeAnalysis.Editor.UnitTests.AutomaticCompletion;
using Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions;
using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Test.Utilities;
using Microsoft.VisualStudio.Text.Editor;
using Roslyn.Test.Utilities;
using Xunit;
using static Microsoft.CodeAnalysis.BraceCompletion.AbstractBraceCompletionService;
Expand Down Expand Up @@ -1665,6 +1661,36 @@ public void man()
CheckReturn(session.Session, 12, expected);
}

[WpfFact, WorkItem("https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1758005")]
public void NoFormattingAfterNewlineIfOptionsDisabled()
{
var code = @"namespace NS1
$$";

var expected = @"namespace NS1
{}";

var expectedAfterReturn = @"namespace NS1
{

}";

// Those option ensures no additional formatting would happen around added braces, including indention of added newline
var globalOptions = new OptionsCollection(LanguageNames.CSharp)
{
{ FormattingOptions2.SmartIndent, FormattingOptions2.IndentStyle.None },
{ AutoFormattingOptionsStorage.FormatOnCloseBrace, false },
};

using var session = CreateSession(code, globalOptions);
Assert.NotNull(session);

CheckStart(session.Session);
Assert.Equal(expected, session.Session.SubjectBuffer.CurrentSnapshot.GetText());

CheckReturn(session.Session, 0, expectedAfterReturn);
Copy link
Member

Choose a reason for hiding this comment

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

can you clarify what is was about this prior code that was causing the problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

We previous assume calling formatter on the brace pair (including added newline in-between) would always cause text changes. But it's not true with certain option combination, which would cause us the throw.

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

}

internal static Holder CreateSession(string code, OptionsCollection? globalOptions = null)
{
return CreateSession(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis.BraceCompletion;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.Formatting.Rules;
using Microsoft.CodeAnalysis.Indentation;
Expand All @@ -25,7 +24,8 @@ internal abstract class AbstractCurlyBraceOrBracketCompletionService : AbstractC
/// Annotation used to find the closing brace location after formatting changes are applied.
/// The closing brace location is then used as the caret location.
/// </summary>
private static readonly SyntaxAnnotation s_closingBraceSyntaxAnnotation = new(nameof(s_closingBraceSyntaxAnnotation));
private static readonly SyntaxAnnotation s_closingBraceFormatAnnotation = new(nameof(s_closingBraceFormatAnnotation));
private static readonly SyntaxAnnotation s_closingBraceNewlineAnnotation = new(nameof(s_closingBraceNewlineAnnotation));

protected abstract ImmutableArray<AbstractFormattingRule> GetBraceFormattingIndentationRulesAfterReturn(IndentationOptions options);

Expand Down Expand Up @@ -115,18 +115,18 @@ private static bool ContainsOnlyWhitespace(SourceText text, int openingPosition,
if (closingPointLine - openingPointLine == 1)
{
// Handling syntax tree directly to avoid parsing in potentially UI blocking code-path
var closingToken = document.Root.FindTokenOnLeftOfPosition(context.ClosingPoint);
var newLineString = options.FormattingOptions.NewLine;
newLineEdit = new TextChange(new TextSpan(closingToken.FullSpan.Start, 0), newLineString);
var closingToken = FindClosingBraceToken(document.Root, closingPoint);
var annotatedNewline = SyntaxFactory.EndOfLine(options.FormattingOptions.NewLine)
.WithAdditionalAnnotations(SpecializedCollections.SingletonEnumerable(s_closingBraceNewlineAnnotation));
var newClosingToken = closingToken.WithPrependedLeadingTrivia(SpecializedCollections.SingletonEnumerable(annotatedNewline));

var generator = document.LanguageServices.GetRequiredService<SyntaxGeneratorInternal>();
var endOfLine = generator.EndOfLine(newLineString);

var rootToFormat = document.Root.ReplaceToken(closingToken, closingToken.WithPrependedLeadingTrivia(endOfLine));
var rootToFormat = document.Root.ReplaceToken(closingToken, newClosingToken);
annotatedNewline = rootToFormat.GetAnnotatedTrivia(s_closingBraceNewlineAnnotation).Single();
document = document.WithChangedRoot(rootToFormat, cancellationToken);

// Modify the closing point location to adjust for the newly inserted line.
closingPoint += newLineString.Length;
// Calculate text change for adding a newline and adjust closing point location.
closingPoint = annotatedNewline.Token.Span.End;
newLineEdit = new TextChange(new TextSpan(annotatedNewline.SpanStart, 0), annotatedNewline.ToString());
}

// Format the text that contains the newly inserted line.
Expand All @@ -147,11 +147,23 @@ private static bool ContainsOnlyWhitespace(SourceText text, int openingPosition,
// Set the caret position to the properly indented column in the desired line.
var caretPosition = GetIndentedLinePosition(newDocument, newDocument.Text, desiredCaretLine.LineNumber, options, cancellationToken);

// The new line edit is calculated against the original text, d0, to get text d1.
// The formatting edits are calculated against d1 to get text d2.
// Merge the formatting and new line edits into a set of whitespace only text edits that all apply to d0.
var overallChanges = newLineEdit != null ? GetMergedChanges(newLineEdit.Value, formattingChanges, newDocument.Text) : formattingChanges;
return new BraceCompletionResult(overallChanges, caretPosition);
return new BraceCompletionResult(GetOverallChanges(), caretPosition);

ImmutableArray<TextChange> GetOverallChanges()
{
// The new line edit is calculated against the original text, d0, to get text d1.
// The formatting edits are calculated against d1 to get text d2.
// Merge the formatting and new line edits into a set of whitespace only text edits that all apply to d0.
if (!newLineEdit.HasValue)
return formattingChanges;

// Depending on options, we might not get any formatting change.
// In this case, the newline edit is the only change.
if (formattingChanges.IsEmpty)
return ImmutableArray.Create(newLineEdit.Value);
Copy link
Member

Choose a reason for hiding this comment

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

consdier moving this into GetMergedChanges.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


return GetMergedChanges(newLineEdit.Value, formattingChanges, newDocument.Text);
}

static TextLine GetLineBetweenCurlys(int closingPosition, SourceText text)
{
Expand All @@ -165,8 +177,8 @@ static LinePosition GetIndentedLinePosition(ParsedDocument document, SourceText
var indentation = indentationService.GetIndentation(document, lineNumber, options, cancellationToken);

var baseLinePosition = sourceText.Lines.GetLinePosition(indentation.BasePosition);
var offsetOfBacePosition = baseLinePosition.Character;
var totalOffset = offsetOfBacePosition + indentation.Offset;
var offsetOfBasePosition = baseLinePosition.Character;
var totalOffset = offsetOfBasePosition + indentation.Offset;
var indentedLinePosition = new LinePosition(lineNumber, totalOffset);
return indentedLinePosition;
}
Expand Down Expand Up @@ -247,19 +259,24 @@ static ImmutableArray<TextChange> GetMergedChanges(TextChange newLineEdit, Immut
}

var newRoot = result.GetFormattedRoot(cancellationToken);
var newClosingPoint = newRoot.GetAnnotatedTokens(s_closingBraceSyntaxAnnotation).Single().SpanStart + 1;
var newClosingPoint = newRoot.GetAnnotatedTokens(s_closingBraceFormatAnnotation).Single().SpanStart + 1;

var textChanges = result.GetTextChanges(cancellationToken).ToImmutableArray();
return (newRoot, textChanges, newClosingPoint);

SyntaxNode GetSyntaxRootWithAnnotatedClosingBrace(SyntaxNode originalRoot, int closingBraceEndPoint)
{
var closeBraceToken = originalRoot.FindToken(closingBraceEndPoint - 1);
Debug.Assert(IsValidClosingBraceToken(closeBraceToken));

var newCloseBraceToken = closeBraceToken.WithAdditionalAnnotations(s_closingBraceSyntaxAnnotation);
var closeBraceToken = FindClosingBraceToken(originalRoot, closingBraceEndPoint);
var newCloseBraceToken = closeBraceToken.WithAdditionalAnnotations(s_closingBraceFormatAnnotation);
return originalRoot.ReplaceToken(closeBraceToken, newCloseBraceToken);
}
}

private SyntaxToken FindClosingBraceToken(SyntaxNode root, int closingBraceEndPoint)
{
var closeBraceToken = root.FindToken(closingBraceEndPoint - 1);
Debug.Assert(IsValidClosingBraceToken(closeBraceToken));
return closeBraceToken;
}
}
}