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.

Sign up for GitHub

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

Better support for mixed formatting #2526

Merged
5 commits merged into from
Sep 24, 2020
Merged

Better support for mixed formatting #2526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b296c4f
Better support for mixed formatting
ajaybhargavb Sep 18, 2020
a61047f
Feedback and bug fixes
ajaybhargavb Sep 24, 2020
e059ff1
Feedback and fix tests
ajaybhargavb Sep 24, 2020
b788d62
fix
ajaybhargavb Sep 24, 2020
0220d47
fix
ajaybhargavb Sep 24, 2020
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
42 changes: 42 additions & 0 deletions ...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer.Common/RazorCodeDocumentExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ internal static class RazorCodeDocumentExtensions
{
private static readonly object UnsupportedKey = new object();
private static readonly object SourceTextKey = new object();
private static readonly object CSharpSourceTextKey = new object();
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird that this should be a "key" when it's just an empty object. Makes sense, it's just not what I would have expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. This is what we use for storing arbitrary things in the CodeDocument. The other option is to use a string but using an object makes it so that no one else can retrieve it directly without using the methods defined here.

private static readonly object HtmlSourceTextKey = new object();

public static bool IsUnsupported(this RazorCodeDocument document)
{
Expand Down Expand Up @@ -59,5 +61,45 @@ public static SourceText GetSourceText(this RazorCodeDocument document)

return (SourceText)sourceTextObj;
}

public static SourceText GetCSharpSourceText(this RazorCodeDocument document)
{
if (document == null)
{
throw new ArgumentNullException(nameof(document));
}

var sourceTextObj = document.Items[CSharpSourceTextKey];
if (sourceTextObj == null)
{
var csharpDocument = document.GetCSharpDocument();
var sourceText = SourceText.From(csharpDocument.GeneratedCode);
document.Items[CSharpSourceTextKey] = sourceText;

return sourceText;
}

return (SourceText)sourceTextObj;
}

public static SourceText GetHtmlSourceText(this RazorCodeDocument document)
{
if (document == null)
{
throw new ArgumentNullException(nameof(document));
}

var sourceTextObj = document.Items[HtmlSourceTextKey];
if (sourceTextObj == null)
{
var htmlDocument = document.GetHtmlDocument();
var sourceText = SourceText.From(htmlDocument.GeneratedHtml);
document.Items[HtmlSourceTextKey] = sourceText;

return sourceText;
}

return (SourceText)sourceTextObj;
}
}
}
131 changes: 92 additions & 39 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.LanguageServer.Common;
using OmniSharp.Extensions.LanguageServer.Protocol;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Formatting;
using Microsoft.CodeAnalysis.Text;
using OmniSharp.Extensions.LanguageServer.Protocol.Models;
using OmniSharp.Extensions.LanguageServer.Protocol.Server;
Expand All @@ -23,12 +22,12 @@ internal class CSharpFormatter
private readonly RazorDocumentMappingService _documentMappingService;
private readonly FilePathNormalizer _filePathNormalizer;
private readonly IClientLanguageServer _server;
private readonly ProjectSnapshotManagerAccessor _projectSnapshotManagerAccessor;
private readonly object _indentationService;
private readonly MethodInfo _getIndentationMethod;

public CSharpFormatter(
RazorDocumentMappingService documentMappingService,
IClientLanguageServer languageServer,
ProjectSnapshotManagerAccessor projectSnapshotManagerAccessor,
FilePathNormalizer filePathNormalizer)
{
if (documentMappingService is null)
Expand All @@ -41,50 +40,113 @@ public CSharpFormatter(
throw new ArgumentNullException(nameof(languageServer));
}

if (projectSnapshotManagerAccessor is null)
{
throw new ArgumentNullException(nameof(projectSnapshotManagerAccessor));
}

if (filePathNormalizer is null)
{
throw new ArgumentNullException(nameof(filePathNormalizer));
}

_documentMappingService = documentMappingService;
_server = languageServer;
_projectSnapshotManagerAccessor = projectSnapshotManagerAccessor;
_filePathNormalizer = filePathNormalizer;

try
{
var type = typeof(CSharpFormattingOptions).Assembly.GetType("Microsoft.CodeAnalysis.CSharp.Indentation.CSharpIndentationService", throwOnError: true);
_indentationService = Activator.CreateInstance(type);
var indentationService = type.GetInterface("IIndentationService");
_getIndentationMethod = indentationService.GetMethod("GetIndentation");
}
catch (Exception ex)
{
throw new InvalidOperationException(
"Error occured when creating an instance of Roslyn's IIndentationService. Roslyn may have changed in an unexpected way.",
ex);
}
}

public async Task<TextEdit[]> FormatAsync(
RazorCodeDocument codeDocument,
Range range,
DocumentUri uri,
FormattingOptions options,
FormattingContext context,
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 love me some context.

Range rangeToFormat,
CancellationToken cancellationToken,
bool formatOnClient = false)
{
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}

if (rangeToFormat is null)
{
throw new ArgumentNullException(nameof(rangeToFormat));
}

Range projectedRange = null;
if (range != null && !_documentMappingService.TryMapToProjectedDocumentRange(codeDocument, range, out projectedRange))
if (rangeToFormat != null && !_documentMappingService.TryMapToProjectedDocumentRange(context.CodeDocument, rangeToFormat, out projectedRange))
{
return Array.Empty<TextEdit>();
}

TextEdit[] edits;
if (formatOnClient)
{
edits = await FormatOnClientAsync(codeDocument, projectedRange, uri, options, cancellationToken);
edits = await FormatOnClientAsync(context, projectedRange, cancellationToken);
}
else
{
edits = await FormatOnServerAsync(codeDocument, projectedRange, uri, options, cancellationToken);
edits = await FormatOnServerAsync(context, projectedRange, cancellationToken);
}

var mappedEdits = MapEditsToHostDocument(codeDocument, edits);
var mappedEdits = MapEditsToHostDocument(context.CodeDocument, edits);
return mappedEdits;
}

public int GetCSharpIndentation(FormattingContext context, int projectedDocumentIndex, CancellationToken cancellationToken)
{
if (context is null)
{
throw new ArgumentNullException(nameof(context));
}

// Add a marker at the position where we need the indentation.
var changedText = context.CSharpSourceText;
var marker = $"{context.NewLineString}#line default{context.NewLineString}#line hidden{context.NewLineString}";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a bunch of different things to use as markers. But this is the only one I found to work in all cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you go a little bit into why the other ones didn't work? Do they just appear in other scenarios? How did you determine that this whole string wouldn't appear otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a hell of a marker lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you go a little bit into why the other ones didn't work?

Not entirely sure. Roslyn's IIndentationService doesn't like it when I provide markers like /**/ or empty lines etc. The API returns wacky results that don't make any sense. I plan to follow up with them to understand this further.

changedText = changedText.WithChanges(new TextChange(TextSpan.FromBounds(projectedDocumentIndex, projectedDocumentIndex), marker));
var changedDocument = context.CSharpWorkspaceDocument.WithText(changedText);

// Get the line number at the position after the marker
var line = changedText.Lines.GetLinePosition(projectedDocumentIndex + marker.Length).Line;

try
{
var result = _getIndentationMethod.Invoke(
_indentationService,
new object[] { changedDocument, line, CodeAnalysis.Formatting.FormattingOptions.IndentStyle.Smart, cancellationToken });

var baseProperty = result.GetType().GetProperty("BasePosition");
var basePosition = (int)baseProperty.GetValue(result);
var offsetProperty = result.GetType().GetProperty("Offset");
var offset = (int)offsetProperty.GetValue(result);

var resultLine = changedText.Lines.GetLinePosition(basePosition);
var indentation = resultLine.Character + offset;

// IIndentationService always returns offset as the number of spaces.
// So if the client uses tabs instead of spaces, we need to convert accordingly.
if (!context.Options.InsertSpaces)
{
indentation /= (int)context.Options.TabSize;
}

return indentation;
}
catch (Exception ex)
{
throw new InvalidOperationException(
"Error occured when reflection invoking Roslyn's IIndentationService. Roslyn may have changed in an unexpected way.",
ex);
}
}

private TextEdit[] MapEditsToHostDocument(RazorCodeDocument codeDocument, TextEdit[] csharpEdits)
{
var actualEdits = new List<TextEdit>();
Expand All @@ -104,18 +166,16 @@ private TextEdit[] MapEditsToHostDocument(RazorCodeDocument codeDocument, TextEd
}

private async Task<TextEdit[]> FormatOnClientAsync(
RazorCodeDocument codeDocument,
FormattingContext context,
Range projectedRange,
DocumentUri uri,
FormattingOptions options,
CancellationToken cancellationToken)
{
var @params = new RazorDocumentRangeFormattingParams()
{
Kind = RazorLanguageKind.CSharp,
ProjectedRange = projectedRange,
HostDocumentFilePath = _filePathNormalizer.Normalize(uri.GetAbsoluteOrUNCPath()),
Options = options
HostDocumentFilePath = _filePathNormalizer.Normalize(context.Uri.GetAbsoluteOrUNCPath()),
Options = context.Options
};

var response = _server.SendRequest(LanguageServerConstants.RazorRangeFormattingEndpoint, @params);
Expand All @@ -125,26 +185,19 @@ private async Task<TextEdit[]> FormatOnClientAsync(
}

private async Task<TextEdit[]> FormatOnServerAsync(
RazorCodeDocument codeDocument,
FormattingContext context,
Range projectedRange,
DocumentUri uri,
FormattingOptions options,
CancellationToken cancellationToken)
{
var workspace = _projectSnapshotManagerAccessor.Instance.Workspace;
var csharpOptions = workspace.Options
.WithChangedOption(CodeAnalysis.Formatting.FormattingOptions.TabSize, LanguageNames.CSharp, (int)options.TabSize)
.WithChangedOption(CodeAnalysis.Formatting.FormattingOptions.UseTabs, LanguageNames.CSharp, !options.InsertSpaces);

var csharpDocument = codeDocument.GetCSharpDocument();
var syntaxTree = CSharpSyntaxTree.ParseText(csharpDocument.GeneratedCode);
var sourceText = SourceText.From(csharpDocument.GeneratedCode);
var root = await syntaxTree.GetRootAsync();
var spanToFormat = projectedRange.AsTextSpan(sourceText);
var csharpSourceText = context.CodeDocument.GetCSharpSourceText();
var spanToFormat = projectedRange.AsTextSpan(csharpSourceText);
var root = await context.CSharpWorkspaceDocument.GetSyntaxRootAsync(cancellationToken);
var workspace = context.CSharpWorkspace;
Copy link
Contributor

Choose a reason for hiding this comment

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

Much cleaner now, good refactor.


var changes = CodeAnalysis.Formatting.Formatter.GetFormattedTextChanges(root, spanToFormat, workspace, options: csharpOptions);
// Formatting options will already be set in the workspace.
var changes = CodeAnalysis.Formatting.Formatter.GetFormattedTextChanges(root, spanToFormat, workspace);

var edits = changes.Select(c => c.AsTextEdit(sourceText)).ToArray();
var edits = changes.Select(c => c.AsTextEdit(csharpSourceText)).ToArray();
return edits;
}
}
Expand Down
Loading