Skip to content
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 @@ -174,6 +174,11 @@ private async Task<SynchronizationResult> PublishHtmlDocumentAsync(TextDocument

return result;
}
catch (OperationCanceledException)
{
_logger.LogDebug($"Not publishing Html text for {document.FilePath} as the request was cancelled.");
return default;
}
catch (Exception ex)
{
_logger.LogError(ex, $"Error publishing Html text for {document.FilePath}. Html document contents will be stale");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.PooledObjects;
using Microsoft.AspNetCore.Razor.Threading;
using Microsoft.CodeAnalysis.Razor.Logging;
using Microsoft.CodeAnalysis.Text;
Expand All @@ -29,28 +30,37 @@ public Task<bool> IsValidAsync(FormattingContext context, ImmutableArray<TextCha
// Looks like we removed some non-whitespace content as part of formatting. Oops.
// Discard this formatting result.

_logger.LogWarning($"{SR.Format_operation_changed_nonwhitespace}");

foreach (var change in changes)
{
if (change.NewText?.Any(c => !char.IsWhiteSpace(c)) ?? false)
{
_logger.LogWarning($"{SR.FormatEdit_at_adds(text.GetLinePositionSpan(change.Span), change.NewText)}");
}
else if (text.TryGetFirstNonWhitespaceOffset(change.Span, out _))
{
_logger.LogWarning($"{SR.FormatEdit_at_deletes(text.GetLinePositionSpan(change.Span), text.ToString(change.Span))}");
}
}
var message = GetLogMessage(changes, text);
Copy link
Contributor

Choose a reason for hiding this comment

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

var message = GetLogMessage(changes, text);

Not related to this PR, so no changes requested, but I'm a little surprised to see how this determines if there were problematic changes.

Instead of constructing a new sourcetext and walking it completely, couldn't it instead just look for problems within the changes and the old text?

Copy link
Member Author

Choose a reason for hiding this comment

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

Roslyn will happily produce changes like replacing "\tpublic" with " public", so just checking NewText on the change is not sufficient.

Having said that, I think I made a change recently and we might always do our own diffing before getting to this point, which I think won't produce such changes, so it might be possible to change this now.

Will follow up if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, this commit outlines the change I was thinking. It seems like it would be ok with the kind of change you mentioned, but I could easily be missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry, I completely misunderstood what you were saying. I can certainly come up with a scenario in my head where that approach would fail, but whether that scenario exists in real life is an entirely different (and likely unanswerable) question. If the formatting tests all pass with that change, I have nothing against taking it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like roslyn does push non-whitespace characters between edits, so the commit linked above doesn't work. But, the code can still be quite a bit smarter and just do the validation walk over the affected range.

PR: #12384

_logger.LogError(message);

if (DebugAssertsEnabled)
{
Debug.Fail("A formatting result was rejected because it was going to change non-whitespace content in the document.");
Debug.Fail(message);
}

return SpecializedTasks.False;
}

return SpecializedTasks.True;
}

private static string GetLogMessage(ImmutableArray<TextChange> changes, SourceText text)
{
using var _1 = StringBuilderPool.GetPooledObject(out var builder);
builder.AppendLine(SR.Format_operation_changed_nonwhitespace);

foreach (var change in changes)
{
if (change.NewText?.Any(c => !char.IsWhiteSpace(c)) ?? false)
{
builder.AppendLine(SR.FormatEdit_at_adds(text.GetLinePositionSpan(change.Span), change.NewText));
}
else if (text.TryGetFirstNonWhitespaceOffset(change.Span, out _))
{
builder.AppendLine(SR.FormatEdit_at_deletes(text.GetLinePositionSpan(change.Span), text.ToString(change.Span)));
}
}

return builder.ToString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.PooledObjects;
using Microsoft.CodeAnalysis.Razor.Logging;
using Microsoft.CodeAnalysis.Text;

Expand Down Expand Up @@ -37,22 +38,12 @@ public async Task<bool> IsValidAsync(FormattingContext context, ImmutableArray<T
// at all possible). Also worth noting the order has to be maintained in that case.
if (!originalDiagnostics.SequenceEqual(changedDiagnostics, LocationIgnoringDiagnosticComparer.Instance))
{
_logger.LogWarning($"{SR.Format_operation_changed_diagnostics}");
_logger.LogWarning($"{SR.Diagnostics_before}");
foreach (var diagnostic in originalDiagnostics)
{
_logger.LogWarning($"{diagnostic}");
}

_logger.LogWarning($"{SR.Diagnostics_after}");
foreach (var diagnostic in changedDiagnostics)
{
_logger.LogWarning($"{diagnostic}");
}
var message = GetLogMessage(originalDiagnostics, changedDiagnostics);
_logger.LogError(message);

if (DebugAssertsEnabled)
{
Debug.Fail("A formatting result was rejected because the formatted text produced different diagnostics compared to the original text.");
Debug.Fail(message);
}

return false;
Expand All @@ -61,6 +52,26 @@ public async Task<bool> IsValidAsync(FormattingContext context, ImmutableArray<T
return true;
}

private static string GetLogMessage(ImmutableArray<RazorDiagnostic> originalDiagnostics, ImmutableArray<RazorDiagnostic> changedDiagnostics)
{
using var _ = StringBuilderPool.GetPooledObject(out var builder);

builder.AppendLine(SR.Format_operation_changed_diagnostics);
builder.AppendLine(SR.Diagnostics_before);
foreach (var diagnostic in originalDiagnostics)
{
builder.AppendLine(diagnostic.ToString());
}

builder.AppendLine(SR.Diagnostics_after);
foreach (var diagnostic in changedDiagnostics)
{
builder.AppendLine(diagnostic.ToString());
}

return builder.ToString();
}

private class LocationIgnoringDiagnosticComparer : IEqualityComparer<RazorDiagnostic>
{
public static IEqualityComparer<RazorDiagnostic> Instance = new LocationIgnoringDiagnosticComparer();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,20 @@ internal sealed class RemoteLoggerFactory : ILoggerFactory
{
private ILoggerFactory _targetLoggerFactory = EmptyLoggerFactory.Instance;

internal void SetTargetLoggerFactory(ILoggerFactory loggerFactory)
/// <summary>
/// Sets the logger factory to use in OOP
/// </summary>
/// <returns>True if the factory was set</returns>
internal bool SetTargetLoggerFactory(ILoggerFactory loggerFactory)
{
// We only set the target logger factory if the current factory is empty.
if (_targetLoggerFactory is EmptyLoggerFactory)
{
_targetLoggerFactory = loggerFactory;
return true;
}

return false;
}

public void AddLoggerProvider(ILoggerProvider provider)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ protected virtual async Task<object> CreateInternalAsync(
// Note that this means that the first non-empty ILoggerFactory that we use
// will be used for MEF component logging for the lifetime of all services.
var remoteLoggerFactory = exportProvider.GetExportedValue<RemoteLoggerFactory>();
remoteLoggerFactory.SetTargetLoggerFactory(targetLoggerFactory);
var didSetLoggerFactory = remoteLoggerFactory.SetTargetLoggerFactory(targetLoggerFactory);

// In proc services don't use any service hub infra
if (stream is null)
Expand All @@ -103,6 +103,13 @@ protected virtual async Task<object> CreateInternalAsync(
return CreateService(in inProcArgs);
}

// At this point, we know we're in a remote scenario where we're on the end of a service hub connection, so we want
// logged errors to be thrown, so they're bubbled up to the client.
if (didSetLoggerFactory)
{
remoteLoggerFactory.AddLoggerProvider(new ThrowingErrorLoggerProvider());
}

var serverConnection = CreateServerConnection(stream, traceSource);

var args = new ServiceArgs(serviceBroker.AssumeNotNull(), exportProvider, targetLoggerFactory, workspaceProvider, serverConnection, brokeredServiceData?.Interceptor);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using Microsoft.CodeAnalysis.Razor.Logging;

namespace Microsoft.CodeAnalysis.Remote.Razor;

/// <summary>
/// An error logger provider that provides a logger that simple throws an exception for any LogError call.
/// </summary>
internal class ThrowingErrorLoggerProvider : ILoggerProvider
{
public ILogger CreateLogger(string categoryName)
{
return Logger.Instance;
}

private class Logger : ILogger
{
public static readonly Logger Instance = new();

public bool IsEnabled(LogLevel logLevel)
{
return logLevel == LogLevel.Error;
}

public void Log(LogLevel logLevel, string message, Exception? exception)
{
if (logLevel != LogLevel.Error)
{
return;
}

if (exception is not null)
{
throw exception;
}

throw new InvalidOperationException(message);
}
}
}