Skip to content

Reduce excessive allocations in BannedSymbols analyzer. #6568

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.

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

Merged
merged 9 commits into from
Apr 4, 2023
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 @@ -86,38 +86,39 @@ bool shouldReportNotSpecifiedEnforceAnalyzerBannedApisSetting(AttributeData attr
}

#pragma warning disable RS1012 // 'compilationContext' does not register any analyzer actions. Consider moving actions registered in 'Initialize' that depend on this start action to 'compilationContext'.
protected sealed override Dictionary<ISymbol, BanFileEntry>? ReadBannedApis(CompilationStartAnalysisContext compilationContext)
protected sealed override Dictionary<(string ContainerName, string SymbolName), ImmutableArray<BanFileEntry>>? ReadBannedApis(CompilationStartAnalysisContext compilationContext)
{
var propertyValue = compilationContext.Options.GetMSBuildPropertyValue(MSBuildPropertyOptionNames.EnforceExtendedAnalyzerRules, compilationContext.Compilation);
var compilation = compilationContext.Compilation;
var propertyValue = compilationContext.Options.GetMSBuildPropertyValue(MSBuildPropertyOptionNames.EnforceExtendedAnalyzerRules, compilation);
if (propertyValue != "true")
{
return null;
}

const string fileName = "Microsoft.CodeAnalysis.Analyzers.AnalyzerBannedSymbols.txt";
using var stream = typeof(SymbolIsBannedInAnalyzersAnalyzer<>).Assembly.GetManifestResourceStream(fileName);
var source = SourceText.From(stream);
var result = new Dictionary<ISymbol, BanFileEntry>(SymbolEqualityComparer.Default);

var result = new Dictionary<(string ContainerName, string SymbolName), List<BanFileEntry>>();
foreach (var line in source.Lines)
{
var text = line.ToString();
if (string.IsNullOrWhiteSpace(text))
{
continue;
}

var entry = new BanFileEntry(text, line.Span, source, fileName);
var symbols = DocumentationCommentId.GetSymbolsForDeclarationId(entry.DeclarationId, compilationContext.Compilation);
if (!symbols.IsDefaultOrEmpty)
var entry = new BanFileEntry(compilation, text, line.Span, source, fileName);
var parsed = DocumentationCommentIdParser.ParseDeclaredSymbolId(entry.DeclarationId);
if (parsed != null)
{
foreach (var symbol in symbols)
if (!result.TryGetValue(parsed.Value, out var existing))
{
result.Add(symbol, entry);
existing = new();
result.Add(parsed.Value, existing);
}

existing.Add(entry);
}
}

return result;
return result.ToDictionary(kvp => kvp.Key, kvp => kvp.Value.ToImmutableArray());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<PackageId>*$(MSBuildProjectFile)*</PackageId>
</PropertyGroup>
<ItemGroup>
<Compile Include="..\..\Microsoft.CodeAnalysis.BannedApiAnalyzers\Core\DocumentationCommentIdParser.cs" Link="DocumentationCommentIdParser.cs" />
<Compile Include="..\..\Microsoft.CodeAnalysis.BannedApiAnalyzers\Core\SymbolIsBannedAnalyzerBase.cs" Link="SymbolIsBannedAnalyzerBase.cs" />
<EmbeddedResource Include="AnalyzerBannedSymbols.txt" />

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

namespace Microsoft.CodeAnalysis.BannedApiAnalyzers
{
/// <summary>
/// Stripped down port of the code in roslyn. Responsible only for determining the <see cref="ISymbol.Name"/> and
/// name of the <see cref="ISymbol.ContainingSymbol"/> for a given xml doc comment symbol id.
/// </summary>
internal static class DocumentationCommentIdParser
{
private static readonly char[] s_nameDelimiters = { ':', '.', '(', ')', '{', '}', '[', ']', ',', '\'', '@', '*', '`', '~' };

public static (string ParentName, string SymbolName)? ParseDeclaredSymbolId(string id)
{
if (id == null)
return null;

if (id.Length < 2)
return null;

int index = 0;
return ParseDeclaredId(id, ref index);
}

private static (string ParentName, string SymbolName)? ParseDeclaredId(string id, ref int index)
Copy link
Member Author

Choose a reason for hiding this comment

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

{
var kindChar = PeekNextChar(id, index);

switch (kindChar)
{
case 'E': // Events
case 'F': // Fields
case 'M': // Methods
case 'P': // Properties
case 'T': // Types
break;
case 'N': // Namespaces
default:
// Documentation comment id must start with E, F, M, N, P or T. Note: we don't support banning full
// namespaces, so we bail in that case as well.
return null;
}

index++;
if (PeekNextChar(id, index) == ':')
index++;

string parentName = "";

// process dotted names
while (true)
Copy link
Member Author

Choose a reason for hiding this comment

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

honestly, this could be a lot better. HOwever, i don't care much simple the logic is:

  1. trivial
  2. the same as the version in Roslyn
  3. there are usually only a couple hundred banned API items. So the allocations here are not a concern and not something to worry about. The thing we care about fixing is avoiding hte massively expensive symbol walk.

{
var symbolName = ParseName(id, ref index);

// has type parameters?
if (PeekNextChar(id, index) == '`')
{
index++;

// method type parameters?
if (PeekNextChar(id, index) == '`')
index++;

ReadNextInteger(id, ref index);
}

if (PeekNextChar(id, index) == '.')
{
index++;
parentName = symbolName;
continue;
}
else
{
return (parentName, symbolName);
}
}
}

private static char PeekNextChar(string id, int index)
=> index >= id.Length ? '\0' : id[index];

private static string ParseName(string id, ref int index)
{
string name;

int delimiterOffset = id.IndexOfAny(s_nameDelimiters, index);
if (delimiterOffset >= 0)
{
name = id[index..delimiterOffset];
index = delimiterOffset;
}
else
{
name = id[index..];
index = id.Length;
}

return name.Replace('#', '.');
}

private static void ReadNextInteger(string id, ref int index)
{
while (index < id.Length && char.IsDigit(id[index]))
index++;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,11 @@ public abstract class SymbolIsBannedAnalyzer<TSyntaxKind> : SymbolIsBannedAnalyz
protected sealed override DiagnosticDescriptor SymbolIsBannedRule => SymbolIsBannedAnalyzer.SymbolIsBannedRule;

#pragma warning disable RS1013 // 'compilationContext' does not register any analyzer actions, except for a 'CompilationEndAction'. Consider replacing this start/end action pair with a 'RegisterCompilationAction' or moving actions registered in 'Initialize' that depend on this start action to 'compilationContext'.
protected sealed override Dictionary<ISymbol, BanFileEntry>? ReadBannedApis(CompilationStartAnalysisContext compilationContext)
protected sealed override Dictionary<(string ContainerName, string SymbolName), ImmutableArray<BanFileEntry>>? ReadBannedApis(
CompilationStartAnalysisContext compilationContext)
{
var compilation = compilationContext.Compilation;

var query =
from additionalFile in compilationContext.Options.AdditionalFiles
let fileName = Path.GetFileName(additionalFile.Path)
Expand All @@ -64,35 +67,30 @@ from line in sourceText.Lines
where !string.IsNullOrWhiteSpace(textWithoutComment)
let trimmedTextWithoutComment = textWithoutComment.TrimEnd()
let span = commentIndex == -1 ? line.Span : new Text.TextSpan(line.Span.Start, trimmedTextWithoutComment.Length)
select new BanFileEntry(trimmedTextWithoutComment, span, sourceText, additionalFile.Path);
select new BanFileEntry(compilation, trimmedTextWithoutComment, span, sourceText, additionalFile.Path);

var entries = query.ToList();

if (entries.Count == 0)
{
return null;
}

var errors = new List<Diagnostic>();

var result = new Dictionary<ISymbol, BanFileEntry>(SymbolEqualityComparer.Default);

foreach (var line in entries)
// Report any duplicates.
var groups = entries.GroupBy(e => TrimForErrorReporting(e.DeclarationId));
foreach (var group in groups)
{
var symbols = DocumentationCommentId.GetSymbolsForDeclarationId(line.DeclarationId, compilationContext.Compilation);

if (!symbols.IsDefaultOrEmpty)
if (group.Count() >= 2)
{
foreach (var symbol in symbols)
var groupList = group.ToList();
var firstEntry = groupList[0];
for (int i = 1; i < groupList.Count; i++)
{
if (result.TryGetValue(symbol, out var existingLine))
{
errors.Add(Diagnostic.Create(SymbolIsBannedAnalyzer.DuplicateBannedSymbolRule, line.Location, new[] { existingLine.Location }, symbol.ToDisplayString()));
}
else
{
result.Add(symbol, line);
}
var nextEntry = groupList[i];
errors.Add(Diagnostic.Create(
SymbolIsBannedAnalyzer.DuplicateBannedSymbolRule,
nextEntry.Location, new[] { firstEntry.Location },
firstEntry.Symbols.FirstOrDefault()?.ToDisplayString() ?? ""));
}
}
}
Expand All @@ -103,13 +101,40 @@ from line in sourceText.Lines
endContext =>
{
foreach (var error in errors)
{
endContext.ReportDiagnostic(error);
}
});
}

return result;
var result = new Dictionary<(string ContainerName, string SymbolName), List<BanFileEntry>>();

foreach (var entry in entries)
{
var parsed = DocumentationCommentIdParser.ParseDeclaredSymbolId(entry.DeclarationId);
if (parsed is null)
continue;

if (!result.TryGetValue(parsed.Value, out var existing))
{
existing = new();
result.Add(parsed.Value, existing);
}

existing.Add(entry);
}

return result.ToDictionary(kvp => kvp.Key, kvp => kvp.Value.ToImmutableArray());

static string TrimForErrorReporting(string declarationId)
{
return declarationId switch
{
// Remove the prefix and colon if there.
[_, ':', .. var rest] => rest,
// Colon is technically optional. So remove just the first character if not there.
[_, .. var rest] => rest,
_ => declarationId,
};
}
}
}
}
Loading