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

Jump to bottom

Don't pass RazorCodeDocument and SourceText around together #10719

Merged
merged 3 commits into from
Aug 8, 2024
Merged

Don't pass RazorCodeDocument and SourceText around together #10719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c0e252
Remove unnecessary parameter, because it can be trivially retrieved
davidwengier Aug 8, 2024
00e584a
Don't pass code document and source text around in diagnostics transl…
davidwengier Aug 8, 2024
79b2d26
I spent 10 minutes looking up cool Mr Freeze catch phrases for this c…
davidwengier Aug 8, 2024
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
69 changes: 32 additions & 37 deletions ...Microsoft.AspNetCore.Razor.LanguageServer/Diagnostics/RazorTranslateDiagnosticsService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Frozen;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
Expand Down Expand Up @@ -36,12 +37,11 @@ internal class RazorTranslateDiagnosticsService(IDocumentMappingService document
private readonly ILogger _logger = loggerFactory.GetOrCreateLogger<RazorTranslateDiagnosticsService>();
private readonly IDocumentMappingService _documentMappingService = documentMappingService;

// Internal for testing
internal static readonly IReadOnlyCollection<string> CSharpDiagnosticsToIgnore = new HashSet<string>()
{
private static readonly FrozenSet<string> s_cSharpDiagnosticsToIgnore = new HashSet<string>(
[
"RemoveUnnecessaryImportsFixable",
"IDE0005_gen", // Using directive is unnecessary
};
]).ToFrozenSet();

/// <summary>
/// Translates code diagnostics from one representation into another.
Expand All @@ -64,11 +64,9 @@ internal async Task<Diagnostic[]> TranslateAsync(
return [];
}

var sourceText = await documentContext.GetSourceTextAsync(cancellationToken).ConfigureAwait(false);

var filteredDiagnostics = diagnosticKind == RazorLanguageKind.CSharp
? FilterCSharpDiagnostics(diagnostics, codeDocument, sourceText)
: FilterHTMLDiagnostics(diagnostics, codeDocument, sourceText);
? FilterCSharpDiagnostics(diagnostics, codeDocument)
: FilterHTMLDiagnostics(diagnostics, codeDocument);
if (filteredDiagnostics.Length == 0)
{
_logger.LogDebug($"No diagnostics remaining after filtering.");
Expand All @@ -81,24 +79,23 @@ internal async Task<Diagnostic[]> TranslateAsync(
diagnosticKind,
filteredDiagnostics,
documentContext.Snapshot,
codeDocument,
sourceText);
codeDocument);

return mappedDiagnostics;
}

private Diagnostic[] FilterCSharpDiagnostics(Diagnostic[] unmappedDiagnostics, RazorCodeDocument codeDocument, SourceText sourceText)
private Diagnostic[] FilterCSharpDiagnostics(Diagnostic[] unmappedDiagnostics, RazorCodeDocument codeDocument)
{
return unmappedDiagnostics.Where(d =>
!ShouldFilterCSharpDiagnosticBasedOnErrorCode(d, codeDocument, sourceText)).ToArray();
!ShouldFilterCSharpDiagnosticBasedOnErrorCode(d, codeDocument)).ToArray();
}

private static Diagnostic[] FilterHTMLDiagnostics(
Diagnostic[] unmappedDiagnostics,
RazorCodeDocument codeDocument,
SourceText sourceText)
RazorCodeDocument codeDocument)
{
var syntaxTree = codeDocument.GetSyntaxTree();
var sourceText = codeDocument.Source.Text;

var processedAttributes = new Dictionary<TextSpan, bool>();

Expand All @@ -115,22 +112,19 @@ private static Diagnostic[] FilterHTMLDiagnostics(

private Diagnostic[] MapDiagnostics(
RazorLanguageKind languageKind,
IReadOnlyList<Diagnostic> diagnostics,
Diagnostic[] diagnostics,
IDocumentSnapshot documentSnapshot,
RazorCodeDocument codeDocument,
SourceText sourceText)
RazorCodeDocument codeDocument)
{
var projects = RazorDiagnosticConverter.GetProjectInformation(documentSnapshot);
using var mappedDiagnostics = new PooledArrayBuilder<Diagnostic>();

for (var i = 0; i < diagnostics.Count; i++)
foreach (var diagnostic in diagnostics)
{
var diagnostic = diagnostics[i];

// C# requests don't map directly to where they are in the document.
if (languageKind == RazorLanguageKind.CSharp)
{
if (!TryGetOriginalDiagnosticRange(diagnostic, codeDocument, sourceText, out var originalRange))
if (!TryGetOriginalDiagnosticRange(diagnostic, codeDocument, out var originalRange))
{
continue;
}
Expand Down Expand Up @@ -253,7 +247,8 @@ static bool IsCSharpInStyleBlock(Diagnostic diagnostic, SourceText sourceText, R
var element = owner.FirstAncestorOrSelf<MarkupElementSyntax>(static n => n.StartTag?.Name.Content == "style");
var csharp = owner.FirstAncestorOrSelf<CSharpCodeBlockSyntax>();

return element?.Body.Any(static c => c is CSharpCodeBlockSyntax) ?? false || csharp is not null;
return csharp is not null ||
(element?.Body.Any(static c => c is CSharpCodeBlockSyntax) ?? false);
}

// Ideally this would be solved instead by not emitting the "!" at the HTML backing file,
Expand Down Expand Up @@ -405,26 +400,26 @@ n is GenericBlockSyntax ||
}
}

private bool ShouldFilterCSharpDiagnosticBasedOnErrorCode(Diagnostic diagnostic, RazorCodeDocument codeDocument, SourceText sourceText)
private bool ShouldFilterCSharpDiagnosticBasedOnErrorCode(Diagnostic diagnostic, RazorCodeDocument codeDocument)
{
if (!diagnostic.Code.HasValue)
if (diagnostic.Code is not { } code ||
!code.TryGetSecond(out var str) ||
str is null)
{
return false;
}

diagnostic.Code.Value.TryGetSecond(out var str);

return str switch
{
"CS1525" => ShouldIgnoreCS1525(diagnostic, codeDocument, sourceText),
_ => CSharpDiagnosticsToIgnore.Contains(str) &&
diagnostic.Severity != DiagnosticSeverity.Error,
"CS1525" => ShouldIgnoreCS1525(diagnostic, codeDocument),
_ => s_cSharpDiagnosticsToIgnore.Contains(str) &&
diagnostic.Severity != DiagnosticSeverity.Error
};

bool ShouldIgnoreCS1525(Diagnostic diagnostic, RazorCodeDocument codeDocument, SourceText sourceText)
bool ShouldIgnoreCS1525(Diagnostic diagnostic, RazorCodeDocument codeDocument)
{
if (CheckIfDocumentHasRazorDiagnostic(codeDocument, RazorDiagnosticFactory.TagHelper_EmptyBoundAttribute.Id) &&
TryGetOriginalDiagnosticRange(diagnostic, codeDocument, sourceText, out var originalRange) &&
TryGetOriginalDiagnosticRange(diagnostic, codeDocument, out var originalRange) &&
originalRange.IsUndefined())
{
// Empty attribute values will take the following form in the generated C# document:
Expand All @@ -440,17 +435,16 @@ bool ShouldIgnoreCS1525(Diagnostic diagnostic, RazorCodeDocument codeDocument, S
}
}

// Internal & virtual for testing
internal virtual bool CheckIfDocumentHasRazorDiagnostic(RazorCodeDocument codeDocument, string razorDiagnosticCode)
private static bool CheckIfDocumentHasRazorDiagnostic(RazorCodeDocument codeDocument, string razorDiagnosticCode)
{
return codeDocument.GetSyntaxTree().Diagnostics.Any(d => d.Id.Equals(razorDiagnosticCode, StringComparison.Ordinal));
}

private bool TryGetOriginalDiagnosticRange(Diagnostic diagnostic, RazorCodeDocument codeDocument, SourceText sourceText, [NotNullWhen(true)] out Range? originalRange)
private bool TryGetOriginalDiagnosticRange(Diagnostic diagnostic, RazorCodeDocument codeDocument, [NotNullWhen(true)] out Range? originalRange)
{
if (IsRudeEditDiagnostic(diagnostic))
{
if (TryRemapRudeEditRange(diagnostic.Range, codeDocument, sourceText, out originalRange))
if (TryRemapRudeEditRange(diagnostic.Range, codeDocument, out originalRange))
{
return true;
}
Expand Down Expand Up @@ -487,14 +481,15 @@ private static bool IsRudeEditDiagnostic(Diagnostic diagnostic)
str.StartsWith("ENC");
}

private bool TryRemapRudeEditRange(Range diagnosticRange, RazorCodeDocument codeDocument, SourceText sourceText, [NotNullWhen(true)] out Range? remappedRange)
private bool TryRemapRudeEditRange(Range diagnosticRange, RazorCodeDocument codeDocument, [NotNullWhen(true)] out Range? remappedRange)
{
// This is a rude edit diagnostic that has already been mapped to the Razor document. The mapping isn't absolutely correct though,
// it's based on the runtime code generation of the Razor document therefore we need to re-map the already mapped diagnostic in a
// semi-intelligent way.

var syntaxTree = codeDocument.GetSyntaxTree();
var span = codeDocument.Source.Text.GetTextSpan(diagnosticRange);
var sourceText = codeDocument.Source.Text;
var span = sourceText.GetTextSpan(diagnosticRange);
var owner = syntaxTree.Root.FindNode(span, getInnermostNodeForTie: true);

switch (owner?.Kind)
Expand Down
5 changes: 2 additions & 3 deletions ...rosoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/IDocumentMappingServiceExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,16 @@ public static bool TryMapToHostDocumentRange(this IDocumentMappingService servic
public static async Task<DocumentPositionInfo> GetPositionInfoAsync(this IDocumentMappingService service, DocumentContext documentContext, int hostDocumentIndex, CancellationToken cancellationToken)
{
var codeDocument = await documentContext.GetCodeDocumentAsync(cancellationToken).ConfigureAwait(false);
var sourceText = await documentContext.GetSourceTextAsync(cancellationToken).ConfigureAwait(false);

return service.GetPositionInfo(codeDocument, sourceText, hostDocumentIndex);
return service.GetPositionInfo(codeDocument, hostDocumentIndex);
}

public static DocumentPositionInfo GetPositionInfo(
this IDocumentMappingService service,
RazorCodeDocument codeDocument,
SourceText sourceText,
int hostDocumentIndex)
{
var sourceText = codeDocument.Source.Text;
var position = sourceText.GetPosition(hostDocumentIndex);

var languageKind = service.GetLanguageKind(codeDocument, hostDocumentIndex, rightAssociative: false);
Expand Down