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

Fix StackOverflowException in RoutePatternAnalyzer #48105

Merged
merged 3 commits into from
May 10, 2023
Merged

Fix StackOverflowException in RoutePatternAnalyzer #48105

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cdf8247
Fix StackOverflow in RoutePatternAnalyzer
JamesNK May 6, 2023
3eeea1d
PR feedback
JamesNK May 10, 2023
e1ef757
Update src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedL…
JamesNK May 10, 2023
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
146 changes: 74 additions & 72 deletions ...Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/RoutePatternAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,94 +31,96 @@ private void AnalyzeSemanticModel(SemanticModelAnalysisContext context)
var cancellationToken = context.CancellationToken;

var root = syntaxTree.GetRoot(cancellationToken);
var wellKnownTypes = WellKnownTypes.GetOrCreate(context.SemanticModel.Compilation);
var routeUsageCache = RouteUsageCache.GetOrCreate(context.SemanticModel.Compilation);
Analyze(context, root, wellKnownTypes, routeUsageCache, cancellationToken);
}

private void Analyze(
SemanticModelAnalysisContext context,
SyntaxNode node,
WellKnownTypes wellKnownTypes,
RouteUsageCache routeUsageCache,
CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
// Use a stack collection so that we don't blow the actual stack through recursion.
var stack = new Stack<SyntaxNode>();
stack.Push(root);

foreach (var child in node.ChildNodesAndTokens())
while (stack.Count != 0)
{
if (child.IsNode)
{
Analyze(context, child.AsNode()!, wellKnownTypes, routeUsageCache, cancellationToken);
}
else
cancellationToken.ThrowIfCancellationRequested();
var current = stack.Pop();

foreach (var child in current.ChildNodesAndTokens())
Copy link
Member

@cston cston May 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

foreach (var child in current.ChildNodesAndTokens())

It looks like the analysis is only interested in tokens, so this could be replaced with:

foreach (var token in root.DescendantTokens()) { ... }

Copy link
Member Author

@JamesNK JamesNK May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The code I added follows what the Regex analyzer does. I investigated why the regex analyzer doesn't use DecendantTokens. I think it could have, but the recently added FilteredSpans check wouldn't work with the helper method:

https://github.com/dotnet/roslyn/blob/4dcbb0bdb9f7630fec6ce81b311525ef974fd4b0/src/Features/Core/Portable/EmbeddedLanguages/RegularExpressions/LanguageServices/AbstractRegexDiagnosticAnalyzer.cs#L63-L64

It looks like FilteredSpans has only just been made public. aspnetcore probably won't update to bits with it until after .NET 8. I'd like to leave the method as is so using FilteredSpans in the future is possible.

Btw, what are filtered spans? I'm guessing it's an optimization, but I'd be interested if you could point me at more detail.

Copy link
Member

Choose a reason for hiding this comment

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

Adding @mavasani for the question on filtered spans.

Copy link

Choose a reason for hiding this comment

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

FilterSpan is the new API added on analysis context types when computing diagnostics for lightbulb. This allows expensive analyzers to optimize their performance based on the span within the tree for which diagnostics are being computed. If the analyzer ignores this span, the analyzer driver will filter the reported diagnostics outside this span, there should be no functional impact of ignoring this filter span.

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 analyzer is expensive. Do you recommend eventually using filter span like other analyzers are doing?

Copy link

Choose a reason for hiding this comment

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

Definitely - all SemanticModelAnalysisContext callbacks should respect the filter span. I plan to write a meta-analyzer in Microsoft.CodeAnalysis.Analyzers package to recommend the same. These analyzers are known to be the biggest beneficiaries of this API on lightbulb path.

Copy link
Member

Choose a reason for hiding this comment

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

I do not understand why DescendantTokens is in compatible with FilteredSpans. It can be implemented by passing down the predicate correct? Having every analyzer / generator out there build their own stack for navigating the tree to scan tokens is not the right solution.

@333fred

Copy link
Member Author

@JamesNK JamesNK May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see there is a DescendantTokens overload that takes a span: https://github.com/dotnet/roslyn/blob/252def9efa028af3f0562929b4b0e8a74dcd3124/src/Compilers/Core/Portable/Syntax/SyntaxNode.cs#L1049

It looks like the overload checks span overlaps.

I think the below would work (assuming FilterSpan was available):

foreach (var item in root.DescendantTokens(context.FilterSpan))
{
    cancellationToken.ThrowIfCancellationRequested();

    AnalyzeToken(context, routeUsageCache, item, cancellationToken);
}

DescendantTokens allocates an enumerator, but this method was allocating a collection before anyway. I'll switch the PR to use it and we can add FilterSpan when it's available.

{
var token = child.AsToken();
if (!RouteStringSyntaxDetector.IsRouteStringSyntaxToken(token, context.SemanticModel, cancellationToken, out var options))
if (child.IsNode)
{
continue;
stack.Push(child.AsNode()!);
}

var routeUsage = routeUsageCache.Get(token, cancellationToken);
if (routeUsage is null)
else
{
continue;
AnalyzeToken(context, routeUsageCache, child.AsToken(), cancellationToken);
}
}
}
}

foreach (var diag in routeUsage.RoutePattern.Diagnostics)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.RoutePatternIssue,
Location.Create(context.SemanticModel.SyntaxTree, diag.Span),
DiagnosticDescriptors.RoutePatternIssue.DefaultSeverity,
additionalLocations: null,
properties: null,
diag.Message));
}
private static void AnalyzeToken(SemanticModelAnalysisContext context, RouteUsageCache routeUsageCache, SyntaxToken token, CancellationToken cancellationToken)
{
if (!RouteStringSyntaxDetector.IsRouteStringSyntaxToken(token, context.SemanticModel, cancellationToken, out var options))
{
return;
}

if (routeUsage.UsageContext.MethodSymbol != null)
{
var routeParameterNames = new HashSet<string>(routeUsage.RoutePattern.RouteParameters.Select(p => p.Name), StringComparer.OrdinalIgnoreCase);
var routeUsage = routeUsageCache.Get(token, cancellationToken);
if (routeUsage is null)
{
return;
}

foreach (var parameter in routeUsage.UsageContext.ResolvedParameters)
{
routeParameterNames.Remove(parameter.RouteParameterName);
}
foreach (var diag in routeUsage.RoutePattern.Diagnostics)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.RoutePatternIssue,
Location.Create(context.SemanticModel.SyntaxTree, diag.Span),
DiagnosticDescriptors.RoutePatternIssue.DefaultSeverity,
additionalLocations: null,
properties: null,
diag.Message));
}

foreach (var unusedParameterName in routeParameterNames)
if (routeUsage.UsageContext.MethodSymbol != null)
{
var routeParameterNames = new HashSet<string>(routeUsage.RoutePattern.RouteParameters.Select(p => p.Name), StringComparer.OrdinalIgnoreCase);

foreach (var parameter in routeUsage.UsageContext.ResolvedParameters)
{
routeParameterNames.Remove(parameter.RouteParameterName);
}

foreach (var unusedParameterName in routeParameterNames)
{
var unusedParameter = routeUsage.RoutePattern.GetRouteParameter(unusedParameterName);

var parameterInsertIndex = -1;
var insertPoint = CalculateInsertPoint(
unusedParameter.Name,
routeUsage.RoutePattern.RouteParameters,
routeUsage.UsageContext.ResolvedParameters);
if (insertPoint is { } ip)
{
parameterInsertIndex = routeUsage.UsageContext.Parameters.IndexOf(ip.ExistingParameter);
if (!ip.Before)
{
var unusedParameter = routeUsage.RoutePattern.GetRouteParameter(unusedParameterName);

var parameterInsertIndex = -1;
var insertPoint = CalculateInsertPoint(
unusedParameter.Name,
routeUsage.RoutePattern.RouteParameters,
routeUsage.UsageContext.ResolvedParameters);
if (insertPoint is { } ip)
{
parameterInsertIndex = routeUsage.UsageContext.Parameters.IndexOf(ip.ExistingParameter);
if (!ip.Before)
{
parameterInsertIndex++;
}
}

// These properties are used by the fixer.
var propertiesBuilder = ImmutableDictionary.CreateBuilder<string, string?>();
propertiesBuilder.Add("RouteParameterName", unusedParameter.Name);
propertiesBuilder.Add("RouteParameterPolicy", string.Join(string.Empty, unusedParameter.Policies));
propertiesBuilder.Add("RouteParameterIsOptional", unusedParameter.IsOptional.ToString(CultureInfo.InvariantCulture));
propertiesBuilder.Add("RouteParameterInsertIndex", parameterInsertIndex.ToString(CultureInfo.InvariantCulture));

context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.RoutePatternUnusedParameter,
Location.Create(context.SemanticModel.SyntaxTree, unusedParameter.Span),
DiagnosticDescriptors.RoutePatternUnusedParameter.DefaultSeverity,
additionalLocations: null,
properties: propertiesBuilder.ToImmutableDictionary(),
unusedParameterName));
parameterInsertIndex++;
}
}

// These properties are used by the fixer.
var propertiesBuilder = ImmutableDictionary.CreateBuilder<string, string?>();
propertiesBuilder.Add("RouteParameterName", unusedParameter.Name);
propertiesBuilder.Add("RouteParameterPolicy", string.Join(string.Empty, unusedParameter.Policies));
propertiesBuilder.Add("RouteParameterIsOptional", unusedParameter.IsOptional.ToString(CultureInfo.InvariantCulture));
propertiesBuilder.Add("RouteParameterInsertIndex", parameterInsertIndex.ToString(CultureInfo.InvariantCulture));

context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.RoutePatternUnusedParameter,
Location.Create(context.SemanticModel.SyntaxTree, unusedParameter.Span),
DiagnosticDescriptors.RoutePatternUnusedParameter.DefaultSeverity,
additionalLocations: null,
properties: propertiesBuilder.ToImmutableDictionary(),
unusedParameterName));
}
}
}
Expand Down