Skip to content

Cache the ApiData values we produce for an AdditionalText file. #6556

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

Merged
merged 20 commits into from
Mar 30, 2023
Merged

Cache the ApiData values we produce for an AdditionalText file. #6556

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
42d760e
In progress
CyrusNajmabadi Mar 29, 2023
1bb8e66
in progrss
CyrusNajmabadi Mar 29, 2023
4412ee7
Finish
CyrusNajmabadi Mar 29, 2023
830e5de
Pooling
CyrusNajmabadi Mar 29, 2023
92d39d2
Simplify
CyrusNajmabadi Mar 29, 2023
6cae209
Share data
CyrusNajmabadi Mar 29, 2023
7a29344
Make into struct
CyrusNajmabadi Mar 30, 2023
254e7f1
Records
CyrusNajmabadi Mar 30, 2023
55f177c
inline
CyrusNajmabadi Mar 30, 2023
7e20263
REstore
CyrusNajmabadi Mar 30, 2023
ff71b5f
REstore
CyrusNajmabadi Mar 30, 2023
87dfd85
REstore
CyrusNajmabadi Mar 30, 2023
914c18b
REstore
CyrusNajmabadi Mar 30, 2023
7704c79
Simplify
CyrusNajmabadi Mar 30, 2023
c3b9928
Simplify
CyrusNajmabadi Mar 30, 2023
7882c3d
Simplufy
CyrusNajmabadi Mar 30, 2023
a86ecec
doc
CyrusNajmabadi Mar 30, 2023
0aa693f
Switch approach
CyrusNajmabadi Mar 30, 2023
b9eaa3f
Make non static
CyrusNajmabadi Mar 30, 2023
ba8f956
Update src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs
CyrusNajmabadi Mar 30, 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
43 changes: 18 additions & 25 deletions src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.Impl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,24 @@ namespace Microsoft.CodeAnalysis.PublicApiAnalyzers
{
public partial class DeclarePublicApiAnalyzer : DiagnosticAnalyzer
{
private sealed class ApiLine
private sealed record AdditionalFileInfo(string Path, SourceText SourceText, bool IsShippedApi);
Copy link
Member Author

Choose a reason for hiding this comment

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

common data shared among all ApiLine instances.


private readonly record struct ApiLine
Copy link
Member Author

Choose a reason for hiding this comment

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

made into a simple struct, which also has a pointer to the common data stored across all lines from teh same file.

{
public string Text { get; }
public TextSpan Span { get; }
public SourceText SourceText { get; }
public string Path { get; }
public bool IsShippedApi { get; }

internal ApiLine(string text, TextSpan span, SourceText sourceText, string path, bool isShippedApi)
private readonly AdditionalFileInfo _fileInfo;

public SourceText SourceText => _fileInfo.SourceText;
public string Path => _fileInfo.Path;
public bool IsShippedApi => _fileInfo != null && _fileInfo.IsShippedApi;

public ApiLine(string text, TextSpan span, AdditionalFileInfo fileInfo)
{
Text = text;
Span = span;
SourceText = sourceText;
Path = path;
IsShippedApi = isShippedApi;
_fileInfo = fileInfo;
}
}

Expand Down Expand Up @@ -66,23 +69,13 @@ public ApiName(string name, string nameWithNullability)
}
}

#pragma warning disable CA1815 // Override equals and operator equals on value types
private readonly struct ApiData
#pragma warning restore CA1815 // Override equals and operator equals on value types
{
public static readonly ApiData Empty = new(ImmutableArray<ApiLine>.Empty, ImmutableArray<RemovedApiLine>.Empty, nullableRank: -1);

public ImmutableArray<ApiLine> ApiList { get; }
public ImmutableArray<RemovedApiLine> RemovedApiList { get; }
private sealed record ApiData(
ImmutableArray<ApiLine> ApiList,
ImmutableArray<RemovedApiLine> RemovedApiList,
// Number for the max line where #nullable enable was found (-1 otherwise)
public int NullableRank { get; }

internal ApiData(ImmutableArray<ApiLine> apiList, ImmutableArray<RemovedApiLine> removedApiList, int nullableRank)
{
ApiList = apiList;
RemovedApiList = removedApiList;
NullableRank = nullableRank;
}
int NullableRank)
{
public static readonly ApiData Empty = new(ImmutableArray<ApiLine>.Empty, ImmutableArray<RemovedApiLine>.Empty, NullableRank: -1);
}

private sealed class Impl
Expand Down Expand Up @@ -293,7 +286,7 @@ private void OnSymbolActionCore(ISymbol symbol, Action<Diagnostic> reportDiagnos
if (symbol.Kind == SymbolKind.Method)
{
var method = (IMethodSymbol)symbol;
var isMethodShippedApi = foundApiLine?.IsShippedApi == true;
var isMethodShippedApi = foundApiLine.IsShippedApi;

// Check if a public API is a constructor that makes this class instantiable, even though the base class
// is not instantiable. That API pattern is not allowed, because it causes protected members of
Expand Down
228 changes: 136 additions & 92 deletions src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Threading;
using Analyzer.Utilities.PooledObjects;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Text;

Expand All @@ -16,6 +18,12 @@ namespace Microsoft.CodeAnalysis.PublicApiAnalyzers
[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
public sealed partial class DeclarePublicApiAnalyzer : DiagnosticAnalyzer
{
/// <summary>
/// Cache from additional text instance to the api data we have read out for that specific file. We only store
/// data for additional texts that explicitly match the public/internal api file names we expect.
/// </summary>
private static readonly ConditionalWeakTable<AdditionalText, ApiData> s_additionalTextToApiData = new();
Copy link

Choose a reason for hiding this comment

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

Please don’t use a CWT here. We already have an API on AnalysisContext to allow caching per-file data across compilations: https://github.com/dotnet/roslyn/blob/3a7a7407ea3c831630ecf2754092c33df3a6e452/src/Compilers/Core/Portable/DiagnosticAnalyzer/DiagnosticAnalysisContext.cs#L234

Copy link

Choose a reason for hiding this comment

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

Internally, we also use a CWT, so the API already provides a simple way to achieve caching:
Note

Example test analyzer for this API: https://github.com/dotnet/roslyn/blob/3a7a7407ea3c831630ecf2754092c33df3a6e452/src/Compilers/Test/Core/Diagnostics/CommonDiagnosticAnalyzers.cs#L1605

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I can do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

So i don't seem to be able to use that helper. it is keyed off of SourceText. But avoiding the source-text is the point here. I need to keep off of AdditionalFile. Thoughts @mavasani ?

Copy link

Choose a reason for hiding this comment

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

But avoiding the source-text is the point here

I am not sure I understand. SourceText is already strongly held by the AdditionalText: https://github.com/dotnet/roslyn/blob/3a7a7407ea3c831630ecf2754092c33df3a6e452/src/Compilers/Core/Portable/AdditionalTextFile.cs#L49-L53

Isn't the key point here avoiding repeated compute of the data associated with each AdditionalText file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Went deep down the rabit hole of trying to make this keyed off sourcetext. It's painful. Effectively, the data we compute off the source-text holds onto data outside of the source text (specifically, things like the path and other information about the original additionaltext). I went down the path of trying to extract this out (e.g. RawApiData, ApiData, RawApiLine, ApiLine) and have the map point from the SourceText to the raw data. But then there was so much wrapping of this i needed to do to make working with things palatable (and not have additional copies/allocatins happening).

If we make it so that hte context objects allow for storing arbitrary K/V and not just Text/Value or Tree/Value, then this will be trivial to do.

I discussed this with manish, and we decided that would take too much time. So we're going to accept that this is a CWT. However, i'm making it non-static so it has the same lifetime as the analyzer (And whatever owns it).


internal const string Extension = ".txt";
internal const string PublicShippedFileNamePrefix = "PublicAPI.Shipped";
internal const string PublicShippedFileName = PublicShippedFileNamePrefix + Extension;
Expand Down Expand Up @@ -108,7 +116,14 @@ void CheckAndRegisterImplementation(bool isPublic)
{
var errors = new List<Diagnostic>();
// Switch to "RegisterAdditionalFileAction" available in Microsoft.CodeAnalysis "3.8.x" to report additional file diagnostics: https://github.com/dotnet/roslyn-analyzers/issues/3918
if (!TryGetAndValidateApiFiles(compilationContext.Options, compilationContext.Compilation, isPublic, compilationContext.CancellationToken, errors, out ApiData shippedData, out ApiData unshippedData))
if (!TryGetAndValidateApiFiles(
compilationContext.Options,
compilationContext.Compilation,
isPublic,
compilationContext.CancellationToken,
errors,
out var shippedData,
out var unshippedData))
{
compilationContext.RegisterCompilationEndAction(context =>
{
Expand All @@ -124,8 +139,16 @@ void CheckAndRegisterImplementation(bool isPublic)
Debug.Assert(errors.Count == 0);

RegisterImplActions(compilationContext, new Impl(compilationContext.Compilation, shippedData, unshippedData, isPublic, compilationContext.Options));

static bool TryGetAndValidateApiFiles(AnalyzerOptions options, Compilation compilation, bool isPublic, CancellationToken cancellationToken, List<Diagnostic> errors, out ApiData shippedData, out ApiData unshippedData)
return;

static bool TryGetAndValidateApiFiles(
AnalyzerOptions options,
Compilation compilation,
bool isPublic,
CancellationToken cancellationToken,
List<Diagnostic> errors,
[NotNullWhen(true)] out ApiData? shippedData,
[NotNullWhen(true)] out ApiData? unshippedData)
{
return TryGetApiData(options, compilation, isPublic, errors, cancellationToken, out shippedData, out unshippedData)
&& ValidateApiFiles(shippedData, unshippedData, isPublic, errors);
Expand All @@ -147,83 +170,121 @@ static void RegisterImplActions(CompilationStartAnalysisContext compilationConte
}
}

private static ApiData ReadApiData(List<(string path, SourceText sourceText)> data, bool isShippedApi)
private static ApiData ReadApiData(string path, SourceText sourceText, bool isShippedApi)
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this now computes teh ApiData for a single file. handling of multiple files is taken care of by the caller.

{
var apiBuilder = ImmutableArray.CreateBuilder<ApiLine>();
var removedBuilder = ImmutableArray.CreateBuilder<RemovedApiLine>();
var apiBuilder = ArrayBuilder<ApiLine>.GetInstance();
var removedBuilder = ArrayBuilder<RemovedApiLine>.GetInstance();
var maxNullableRank = -1;
var rank = -1;

foreach (var (path, sourceText) in data)
var additionalFileInfo = new AdditionalFileInfo(path, sourceText, isShippedApi);

foreach (var line in sourceText.Lines)
{
int rank = -1;
string text = line.ToString();
if (string.IsNullOrWhiteSpace(text))
continue;

rank++;

foreach (TextLine line in sourceText.Lines)
if (text == NullableEnable)
{
string text = line.ToString();
if (string.IsNullOrWhiteSpace(text))
{
continue;
}
maxNullableRank = rank;
continue;
}

var apiLine = new ApiLine(text, line.Span, additionalFileInfo);
if (text.StartsWith(RemovedApiPrefix, StringComparison.Ordinal))
{
string removedText = text[RemovedApiPrefix.Length..];
removedBuilder.Add(new RemovedApiLine(removedText, apiLine));
}
else
{
apiBuilder.Add(apiLine);
}
}

rank++;
return new ApiData(apiBuilder.ToImmutableAndFree(), removedBuilder.ToImmutableAndFree(), maxNullableRank);
}

if (text == NullableEnable)
{
maxNullableRank = Math.Max(rank, maxNullableRank);
continue;
}
/// <summary>
/// Takes potentially multiple <see cref="ApiData"/> instances, corresponding to different additional text
/// files, and flattens them into the final instance we will use when analyzing the compilation.
/// </summary>
private static ApiData Flatten(ArrayBuilder<ApiData> allData)
{
Debug.Assert(allData.Count > 0);

var apiLine = new ApiLine(text, line.Span, sourceText, path, isShippedApi);
if (text.StartsWith(RemovedApiPrefix, StringComparison.Ordinal))
{
string removedtext = text[RemovedApiPrefix.Length..];
removedBuilder.Add(new RemovedApiLine(removedtext, apiLine));
}
else
{
apiBuilder.Add(apiLine);
}
}
// The common case is that we will have one file corresponding to the shipped data, and one for the
// unshipped data. In that case, just return the instance directly.
if (allData.Count == 1)
return allData[0];

var apiBuilder = ArrayBuilder<ApiLine>.GetInstance();
var removedBuilder = ArrayBuilder<RemovedApiLine>.GetInstance();
var maxNullableRank = -1;

for (int i = 0, n = allData.Count; i < n; i++)
{
var data = allData[i];
apiBuilder.AddRange(data.ApiList);
removedBuilder.AddRange(data.RemovedApiList);
maxNullableRank = Math.Max(maxNullableRank, data.NullableRank);
}

return new ApiData(apiBuilder.ToImmutable(), removedBuilder.ToImmutable(), maxNullableRank);
return new ApiData(apiBuilder.ToImmutableAndFree(), removedBuilder.ToImmutableAndFree(), maxNullableRank);
}

private static bool TryGetApiData(AnalyzerOptions analyzerOptions, Compilation compilation, bool isPublic, List<Diagnostic> errors, CancellationToken cancellationToken, out ApiData shippedData, out ApiData unshippedData)
private static bool TryGetApiData(
AnalyzerOptions analyzerOptions,
Compilation compilation,
bool isPublic,
List<Diagnostic> errors,
CancellationToken cancellationToken,
[NotNullWhen(true)] out ApiData? shippedData,
[NotNullWhen(true)] out ApiData? unshippedData)
{
if (!TryGetApiText(analyzerOptions.AdditionalFiles, isPublic, cancellationToken, out var shippedText, out var unshippedText))
using var allShippedData = ArrayBuilder<ApiData>.GetInstance();
Copy link

Choose a reason for hiding this comment

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

using var allShippedData

This threw me for a loop until I saw this was coming from an implementation in this repo. I assume we don't want a similar disposable pattern for the one in Roslyn?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Roslyn one is disposable as well, just a slightly different pattern (due to compiler team concerns)

Copy link

Choose a reason for hiding this comment

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

Weird, I'm not seeing the Roslyn ones as IDisposable from code inspection, but I'm probably missing it

Copy link
Member Author

Choose a reason for hiding this comment

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

we have a struct disposable wrapper around ours. look for using var _ = ArrayBuilder... in roslyn.sln :)

using var allUnshippedData = ArrayBuilder<ApiData>.GetInstance();

AddApiTexts(
analyzerOptions.AdditionalFiles, isPublic, allShippedData, allUnshippedData, cancellationToken);

if (allShippedData.Count == 0 && allUnshippedData.Count == 0)
{
if (shippedText == null && unshippedText == null)
if (TryGetEditorConfigOptionForMissingFiles(analyzerOptions, compilation, out var silentlyBailOutOnMissingApiFiles) &&
silentlyBailOutOnMissingApiFiles)
{
if (TryGetEditorConfigOptionForMissingFiles(analyzerOptions, compilation, out var silentlyBailOutOnMissingApiFiles) &&
silentlyBailOutOnMissingApiFiles)
{
shippedData = default;
unshippedData = default;
return false;
}

// Bootstrapping public API files.
(shippedData, unshippedData) = (ApiData.Empty, ApiData.Empty);
return true;
shippedData = null;
unshippedData = null;
return false;
}

var missingFileName = (shippedText, isPublic) switch
{
(shippedText: null, isPublic: true) => PublicShippedFileName,
(shippedText: null, isPublic: false) => InternalShippedFileName,
(shippedText: not null, isPublic: true) => PublicUnshippedFileName,
(shippedText: not null, isPublic: false) => InternalUnshippedFileName
};
errors.Add(Diagnostic.Create(isPublic ? PublicApiFileMissing : InternalApiFileMissing, Location.None, missingFileName));
shippedData = default;
unshippedData = default;
return false;
// Bootstrapping public API files.
(shippedData, unshippedData) = (ApiData.Empty, ApiData.Empty);
return true;
}

shippedData = ReadApiData(shippedText, isShippedApi: true);
unshippedData = ReadApiData(unshippedText, isShippedApi: false);
return true;
if (allShippedData.Count > 0 && allUnshippedData.Count > 0)
{
shippedData = Flatten(allShippedData);
unshippedData = Flatten(allUnshippedData);
return true;
}

var missingFileName = (allShippedData.Count == 0, isPublic) switch
{
(true, isPublic: true) => PublicShippedFileName,
(true, isPublic: false) => InternalShippedFileName,
(false, isPublic: true) => PublicUnshippedFileName,
(false, isPublic: false) => InternalUnshippedFileName
};

errors.Add(Diagnostic.Create(isPublic ? PublicApiFileMissing : InternalApiFileMissing, Location.None, missingFileName));
shippedData = null;
unshippedData = null;
return false;
}

private static bool TryGetEditorConfigOption(AnalyzerOptions analyzerOptions, SyntaxTree tree, string optionName, out string optionValue)
Expand Down Expand Up @@ -302,49 +363,32 @@ private static bool TryGetEditorConfigOptionForSkippedNamespaces(AnalyzerOptions
return true;
}

private static bool TryGetApiText(
private static void AddApiTexts(
ImmutableArray<AdditionalText> additionalTexts,
bool isPublic,
CancellationToken cancellationToken,
[NotNullWhen(returnValue: true)] out List<(string path, SourceText text)>? shippedText,
[NotNullWhen(returnValue: true)] out List<(string path, SourceText text)>? unshippedText)
ArrayBuilder<ApiData> allShippedData,
ArrayBuilder<ApiData> allUnshippedData,
CancellationToken cancellationToken)
{
shippedText = null;
unshippedText = null;

foreach (AdditionalText additionalText in additionalTexts)
foreach (var additionalText in additionalTexts)
{
cancellationToken.ThrowIfCancellationRequested();

var file = new PublicApiFile(additionalText.Path, isPublic);

if (file.IsApiFile)
{
SourceText text = additionalText.GetText(cancellationToken);
// if it's not an api file (quick filename check), we can just immediately ignore.
if (!file.IsApiFile)
continue;

if (text is null)
{
continue;
}

var data = (additionalText.Path, text);

if (file.IsShipping)
{
shippedText ??= new();

shippedText.Add(data);
}
else
{
unshippedText ??= new();

unshippedText.Add(data);
}
if (!s_additionalTextToApiData.TryGetValue(additionalText, out var apiData))
{
apiData = ReadApiData(additionalText.Path, additionalText.GetText(cancellationToken), file.IsShipping);
apiData = s_additionalTextToApiData.GetValue(additionalText, _ => apiData);
}
}

return shippedText != null && unshippedText != null;
var resultList = file.IsShipping ? allShippedData : allUnshippedData;
resultList.Add(apiData);
}
}

private static bool ValidateApiFiles(ApiData shippedData, ApiData unshippedData, bool isPublic, List<Diagnostic> errors)
Expand Down