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

When replacing CodeStyle analyzers do not provide Features analyzers fallback options #75534

Merged
merged 7 commits into from
Oct 24, 2024
Merged

When replacing CodeStyle analyzers do not provide Features analyzers fallback options #75534

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a838b9
Add a HasSdkCodeStyleAnalyzers property to ProjectAttributes
JoeRobich Oct 16, 2024
180455c
Track unmapped AnalyzerReferences added by the project system.
JoeRobich Oct 16, 2024
03eccdd
Selectively treat some Host analyzers as Project analyzers.
JoeRobich Oct 16, 2024
e9f7c04
Allow the DiagnosticComputer to treat some Host analyzers as Project …
JoeRobich Oct 22, 2024
1198405
Added integration tests
JoeRobich Oct 23, 2024
7344695
PR feedback
JoeRobich Oct 24, 2024
dff04fe
Backport test skip from #75612
JoeRobich Oct 24, 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
75 changes: 71 additions & 4 deletions ...ol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.HostStates.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.IO;
using System.Linq;
using Roslyn.Utilities;

Expand Down Expand Up @@ -32,18 +33,84 @@ public IEnumerable<StateSet> GetAllHostStateSets()
private HostAnalyzerStateSets GetOrCreateHostStateSets(Project project, ProjectAnalyzerStateSets projectStateSets)
{
var key = new HostAnalyzerStateSetKey(project.Language, project.Solution.SolutionState.Analyzers.HostAnalyzerReferences);
var hostStateSets = ImmutableInterlocked.GetOrAdd(ref _hostAnalyzerStateMap, key, CreateLanguageSpecificAnalyzerMap, project.Solution.SolutionState.Analyzers);
// Some Host Analyzers may need to be treated as Project Analyzers so that they do not have access to the
// Host fallback options. These ids will be used when building up the Host and Project analyzer collections.
var referenceIdsToRedirect = GetReferenceIdsToRedirectAsProjectAnalyzers(project);
var hostStateSets = ImmutableInterlocked.GetOrAdd(ref _hostAnalyzerStateMap, key, CreateLanguageSpecificAnalyzerMap, (project.Solution.SolutionState.Analyzers, referenceIdsToRedirect));
return hostStateSets.WithExcludedAnalyzers(projectStateSets.SkippedAnalyzersInfo.SkippedAnalyzers);

static HostAnalyzerStateSets CreateLanguageSpecificAnalyzerMap(HostAnalyzerStateSetKey arg, HostDiagnosticAnalyzers hostAnalyzers)
static HostAnalyzerStateSets CreateLanguageSpecificAnalyzerMap(HostAnalyzerStateSetKey arg, (HostDiagnosticAnalyzers HostAnalyzers, ImmutableHashSet<object> ReferenceIdsToRedirect) state)
{
var language = arg.Language;
var analyzersPerReference = hostAnalyzers.GetOrCreateHostDiagnosticAnalyzersPerReference(language);
var analyzersPerReference = state.HostAnalyzers.GetOrCreateHostDiagnosticAnalyzersPerReference(language);

var analyzerMap = CreateStateSetMap(language, [], analyzersPerReference.Values, includeWorkspacePlaceholderAnalyzers: true);
var (hostAnalyzerCollection, projectAnalyzerCollection) = GetAnalyzerCollections(analyzersPerReference, state.ReferenceIdsToRedirect);
var analyzerMap = CreateStateSetMap(language, projectAnalyzerCollection, hostAnalyzerCollection, includeWorkspacePlaceholderAnalyzers: true);

return new HostAnalyzerStateSets(analyzerMap);
}

static (IEnumerable<ImmutableArray<DiagnosticAnalyzer>> HostAnalyzerCollection, IEnumerable<ImmutableArray<DiagnosticAnalyzer>> ProjectAnalyzerCollection) GetAnalyzerCollections(
Copy link
Member

Choose a reason for hiding this comment

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

def don't like returning these as lazy enumerbles. can you make both an ImmutableArray<ImmutableArray<...

Copy link
Member Author

Choose a reason for hiding this comment

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

I could but then we have to do something about the common case where we aren't redirecting and are returning analyzersPerReference.Values which is an IEnumerable<ImmutableArray<DiagnosticAnalyzer>>. The use of these IEnumerable's is also piped into StateManager.CreateStateSetMap. This might be cleanup for a follow up PR.

Copy link
Member

Choose a reason for hiding this comment

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

which is an IEnumerable<ImmutableArray>

Ick :) Can that one change?

Copy link
Member

Choose a reason for hiding this comment

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

at the very least, don't return mutable lists. return an immutable array here. that way we don't have to worry about it changing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use ArrayBuilder instead of List.

ImmutableDictionary<object, ImmutableArray<DiagnosticAnalyzer>> analyzersPerReference,
ImmutableHashSet<object> referenceIdsToRedirectAsProjectAnalyzers)
{
if (referenceIdsToRedirectAsProjectAnalyzers.IsEmpty)
{
return (analyzersPerReference.Values, []);
}

var hostAnalyzerCollection = new List<ImmutableArray<DiagnosticAnalyzer>>();
var projectAnalyzerCollection = new List<ImmutableArray<DiagnosticAnalyzer>>();

foreach (var kvp in analyzersPerReference)
Copy link
Member

@tmat tmat Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Deconstruct: foreach (var (referenceId, analyzers) in analyzersPerReference)

{
if (referenceIdsToRedirectAsProjectAnalyzers.Contains(kvp.Key))
{
projectAnalyzerCollection.Add(kvp.Value);
}
else
{
hostAnalyzerCollection.Add(kvp.Value);
}
}

return (hostAnalyzerCollection, projectAnalyzerCollection);
}
}

private static readonly ImmutableHashSet<string> FeaturesAnalyzerFileNames = [
"Microsoft.CodeAnalysis.Features.dll",
"Microsoft.CodeAnalysis.CSharp.Features.dll",
"Microsoft.CodeAnalysis.VisualBasic.Features.dll",
];

private static ImmutableHashSet<object> GetReferenceIdsToRedirectAsProjectAnalyzers(Project project)
{
if (project.State.HasSdkCodeStyleAnalyzers)
{
// When a project uses CodeStyle analyzers added by the SDK, we remove them in favor of the
// Features analyzers. We need to then treat the Features analyzers as Project analyzers so
// they do not get access to the Host fallback options.
return GetFeaturesAnalyzerReferenceIds(project.Solution.SolutionState.Analyzers);
}

return [];

static ImmutableHashSet<object> GetFeaturesAnalyzerReferenceIds(HostDiagnosticAnalyzers hostAnalyzers)
{
var builder = ImmutableHashSet.CreateBuilder<object>();

foreach (var analyzerReference in hostAnalyzers.HostAnalyzerReferences)
{
var fileName = Path.GetFileName(analyzerReference.FullPath)!;
if (FeaturesAnalyzerFileNames.Contains(fileName))
{
builder.Add(analyzerReference.Id);
}
}

return builder.ToImmutable();
}
}

private sealed class HostAnalyzerStateSets
Expand Down
52 changes: 46 additions & 6 deletions src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ internal sealed partial class ProjectSystemProject
/// </summary>
private readonly HashSet<string> _projectAnalyzerPaths = [];

/// <summary>
Copy link
Member

Choose a reason for hiding this comment

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

Add explanation of what mapping of analyzer references means.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this field name to _sdkCodeStyleAnalyzerPaths which I think is self explanatory.

/// The set of unmapped analyzer reference paths that the project knows about.
/// </summary>
private readonly HashSet<string> _unmappedProjectAnalyzerPaths = [];

/// <summary>
/// Paths to analyzers we want to add when the current batch completes.
/// </summary>
Expand All @@ -81,6 +86,7 @@ internal sealed partial class ProjectSystemProject
private string? _outputFilePath;
private string? _outputRefFilePath;
private string? _defaultNamespace;
private bool _hasSdkCodeStyleAnalyzers;

/// <summary>
/// If this project is the 'primary' project the project system cares about for a group of Roslyn projects that
Expand Down Expand Up @@ -446,14 +452,20 @@ private void UpdateRunAnalyzers()
ChangeProjectProperty(ref _runAnalyzers, runAnalyzers, s => s.WithRunAnalyzers(Id, runAnalyzers));
}

internal bool HasSdkCodeStyleAnalyzers
{
get => _hasSdkCodeStyleAnalyzers;
set => ChangeProjectProperty(ref _hasSdkCodeStyleAnalyzers, value, s => s.WithHasSdkCodeStyleAnalyzers(Id, value));
}

/// <summary>
/// The default namespace of the project.
/// </summary>
/// <remarks>
/// In C#, this is defined as the value of "rootnamespace" msbuild property. Right now VB doesn't
/// In C#, this is defined as the value of "rootnamespace" msbuild property. Right now VB doesn't
/// have the concept of "default namespace", but we conjure one in workspace by assigning the value
/// of the project's root namespace to it. So various features can choose to use it for their own purpose.
///
///
/// In the future, we might consider officially exposing "default namespace" for VB project
/// (e.g.through a "defaultnamespace" msbuild property)
/// </remarks>
Expand All @@ -464,8 +476,8 @@ internal string? DefaultNamespace
}

/// <summary>
/// The max language version supported for this project, if applicable. Useful to help indicate what
/// language version features should be suggested to a user, as well as if they can be upgraded.
/// The max language version supported for this project, if applicable. Useful to help indicate what
/// language version features should be suggested to a user, as well as if they can be upgraded.
/// </summary>
internal string? MaxLangVersion
{
Expand Down Expand Up @@ -857,7 +869,7 @@ public void AddDynamicSourceFile(string dynamicFilePath, ImmutableArray<string>
// Don't get confused by _filePath and filePath.
// VisualStudioProject._filePath points to csproj/vbproj of the project
// and the parameter filePath points to dynamic file such as ASP.NET .g.cs files.
//
//
// Also, provider is free-threaded. so fine to call Wait rather than JTF.
fileInfo = provider.Value.GetDynamicFileInfoAsync(
projectId: Id, projectFilePath: _filePath, filePath: dynamicFilePath, CancellationToken.None).WaitAndGetResult_CanCallOnBackground(CancellationToken.None);
Expand Down Expand Up @@ -948,7 +960,7 @@ private void OnDynamicFileInfoUpdated(object? sender, string dynamicFilePath)
{
if (!_dynamicFilePathMaps.TryGetValue(dynamicFilePath, out fileInfoPath))
{
// given file doesn't belong to this project.
// given file doesn't belong to this project.
// this happen since the event this is handling is shared between all projects
return;
}
Expand All @@ -970,10 +982,18 @@ public void AddAnalyzerReference(string fullPath)

var mappedPaths = GetMappedAnalyzerPaths(fullPath);

bool containsSdkCodeStyleAnalyzers;

using var _ = CreateBatchScope();

using (_gate.DisposableWait())
{
// Track the unmapped analyzer paths
_unmappedProjectAnalyzerPaths.Add(fullPath);

// Determine if we started using SDK CodeStyle analyzers while access to _unmappedProjectAnalyzerPaths is gated.
containsSdkCodeStyleAnalyzers = _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer);
Copy link
Member

Choose a reason for hiding this comment

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

❓ What about

containsSdkCdoeStyleAnalyzers = HasSdkCodeStyleAnalyzers || IsSdkCodeStyleAnalyzer(fullPath);

Copy link
Member

Choose a reason for hiding this comment

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

Can we do containsSdkCodeStyleAnalyzers = _hasSdkCodeStyleAnalyzers | IsSdkCodeStyleAnalyzer(fullPath) instead of enumerating all the paths each time?

Copy link
Member

Choose a reason for hiding this comment

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

agree on tweaking 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.

Changing this to only track the codestyle analyzer paths instead of tracking all analyzer paths. Only one call to IsSdkCodeStyleAnalyzer when adding or removing.


// check all mapped paths first, so that all analyzers are either added or not
foreach (var mappedFullPath in mappedPaths)
{
Expand All @@ -998,6 +1018,8 @@ public void AddAnalyzerReference(string fullPath)
}
}
}

HasSdkCodeStyleAnalyzers = containsSdkCodeStyleAnalyzers;
}

public void RemoveAnalyzerReference(string fullPath)
Expand All @@ -1007,10 +1029,18 @@ public void RemoveAnalyzerReference(string fullPath)

var mappedPaths = GetMappedAnalyzerPaths(fullPath);

bool containsSdkCodeStyleAnalyzers;

using var _ = CreateBatchScope();

using (_gate.DisposableWait())
{
// Track the unmapped analyzer paths
_unmappedProjectAnalyzerPaths.Remove(fullPath);

// Determine if we are still using SDK CodeStyle analyzers while access to _unmappedProjectAnalyzerPaths is gated.
containsSdkCodeStyleAnalyzers = _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer);
Copy link
Member

Choose a reason for hiding this comment

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

💡 Can also simplify this one:

containsSdkCodeStyleAnalyzers = IsSdkCodeStyleAnalyzer(fullPath)
    ? _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer)
    : HasSdkCodeStyleAnalyzers;


// check all mapped paths first, so that all analyzers are either removed or not
foreach (var mappedFullPath in mappedPaths)
{
Expand All @@ -1028,6 +1058,8 @@ public void RemoveAnalyzerReference(string fullPath)
_analyzersRemovedInBatch.Add(mappedFullPath);
}
}

HasSdkCodeStyleAnalyzers = containsSdkCodeStyleAnalyzers;
}

private OneOrMany<string> GetMappedAnalyzerPaths(string fullPath)
Expand Down Expand Up @@ -1061,6 +1093,14 @@ private OneOrMany<string> GetMappedAnalyzerPaths(string fullPath)
_ => false,
};

private void UpdateHasSdkCodeStyleAnalyzers()
Copy link
Member

Choose a reason for hiding this comment

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

Is this used?

{
var containsSdkCodeStyleAnalyzers = _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer);
if (containsSdkCodeStyleAnalyzers == HasSdkCodeStyleAnalyzers)
return;
HasSdkCodeStyleAnalyzers = containsSdkCodeStyleAnalyzers;
}

internal const string RazorVsixExtensionId = "Microsoft.VisualStudio.RazorExtension";
private static readonly string s_razorSourceGeneratorSdkDirectory = CreateDirectoryPathFragment("Sdks", "Microsoft.NET.Sdk.Razor", "source-generators");
private static readonly ImmutableArray<string> s_razorSourceGeneratorAssemblyNames =
Expand Down
5 changes: 3 additions & 2 deletions src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProjectFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ public async Task<ProjectSystemProject> CreateAndAddToWorkspaceAsync(string proj
compilationOutputInfo: new(creationInfo.CompilationOutputAssemblyFilePath),
SourceHashAlgorithms.Default, // will be updated when command line is set
filePath: creationInfo.FilePath,
telemetryId: creationInfo.TelemetryId),
telemetryId: creationInfo.TelemetryId,
hasSdkCodeStyleAnalyzers: project.HasSdkCodeStyleAnalyzers),
compilationOptions: creationInfo.CompilationOptions,
parseOptions: creationInfo.ParseOptions);

Expand Down Expand Up @@ -260,7 +261,7 @@ public void ApplyChangeToWorkspaceWithProjectUpdateState(Func<Workspace, Project
/// <summary>
/// Applies a solution transformation to the workspace and triggers workspace changed event for specified <paramref name="projectId"/>.
/// The transformation shall only update the project of the solution with the specified <paramref name="projectId"/>.
///
///
/// The <paramref name="solutionTransformation"/> function must be safe to be attempted multiple times (and not update local state).
/// </summary>
public void ApplyChangeToWorkspace(ProjectId projectId, Func<CodeAnalysis.Solution, CodeAnalysis.Solution> solutionTransformation)
Expand Down
33 changes: 28 additions & 5 deletions src/Workspaces/Core/Portable/Workspace/Solution/ProjectInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ public sealed class ProjectInfo
/// </summary>
internal bool RunAnalyzers => Attributes.RunAnalyzers;

/// <summary>
/// True if the project contains references to the SDK CodeStyle analyzers.
/// </summary>
internal bool HasSdkCodeStyleAnalyzers => Attributes.HasSdkCodeStyleAnalyzers;

/// <summary>
/// The initial compilation options for the project, or null if the default options should be used.
/// </summary>
Expand Down Expand Up @@ -391,6 +396,11 @@ internal ProjectInfo WithTelemetryId(Guid telemetryId)
return With(attributes: Attributes.With(telemetryId: telemetryId));
}

internal ProjectInfo WithHasSdkCodeStyleAnalyzers(bool hasSdkCodeStyleAnalyzers)
{
return With(attributes: Attributes.With(hasSdkCodeStyleAnalyzers: hasSdkCodeStyleAnalyzers));
}

internal string GetDebuggerDisplay()
=> nameof(ProjectInfo) + " " + Name + (!string.IsNullOrWhiteSpace(FilePath) ? " " + FilePath : "");

Expand All @@ -413,7 +423,8 @@ internal sealed class ProjectAttributes(
Guid telemetryId = default,
bool isSubmission = false,
bool hasAllInformation = true,
bool runAnalyzers = true)
bool runAnalyzers = true,
bool hasSdkCodeStyleAnalyzers = false)
{
/// <summary>
/// Matches names like: Microsoft.CodeAnalysis.Features (netcoreapp3.1)
Expand Down Expand Up @@ -497,6 +508,11 @@ internal sealed class ProjectAttributes(
/// </summary>
public Guid TelemetryId { get; } = telemetryId;

/// <summary>
/// True if the project contains references to the SDK CodeStyle analyzers.
/// </summary>
public bool HasSdkCodeStyleAnalyzers { get; } = hasSdkCodeStyleAnalyzers;

private SingleInitNullable<(string? name, string? flavor)> _lazyNameAndFlavor;
private SingleInitNullable<Checksum> _lazyChecksum;

Expand Down Expand Up @@ -527,7 +543,8 @@ public ProjectAttributes With(
Optional<bool> isSubmission = default,
Optional<bool> hasAllInformation = default,
Optional<bool> runAnalyzers = default,
Optional<Guid> telemetryId = default)
Optional<Guid> telemetryId = default,
Optional<bool> hasSdkCodeStyleAnalyzers = default)
{
var newId = id ?? Id;
var newVersion = version ?? Version;
Expand All @@ -543,6 +560,7 @@ public ProjectAttributes With(
var newHasAllInformation = hasAllInformation.HasValue ? hasAllInformation.Value : HasAllInformation;
var newRunAnalyzers = runAnalyzers.HasValue ? runAnalyzers.Value : RunAnalyzers;
var newTelemetryId = telemetryId.HasValue ? telemetryId.Value : TelemetryId;
var newHasSdkCodeStyleAnalyzers = hasSdkCodeStyleAnalyzers.HasValue ? hasSdkCodeStyleAnalyzers.Value : HasSdkCodeStyleAnalyzers;

if (newId == Id &&
newVersion == Version &&
Expand All @@ -557,7 +575,8 @@ public ProjectAttributes With(
newIsSubmission == IsSubmission &&
newHasAllInformation == HasAllInformation &&
newRunAnalyzers == RunAnalyzers &&
newTelemetryId == TelemetryId)
newTelemetryId == TelemetryId &&
newHasSdkCodeStyleAnalyzers == HasSdkCodeStyleAnalyzers)
{
return this;
}
Expand All @@ -577,7 +596,8 @@ public ProjectAttributes With(
newTelemetryId,
newIsSubmission,
newHasAllInformation,
newRunAnalyzers);
newRunAnalyzers,
newHasSdkCodeStyleAnalyzers);
}

public void WriteTo(ObjectWriter writer)
Expand All @@ -600,6 +620,7 @@ public void WriteTo(ObjectWriter writer)
writer.WriteBoolean(HasAllInformation);
writer.WriteBoolean(RunAnalyzers);
writer.WriteGuid(TelemetryId);
writer.WriteBoolean(HasSdkCodeStyleAnalyzers);

// TODO: once CompilationOptions, ParseOptions, ProjectReference, MetadataReference, AnalyzerReference supports
// serialization, we should include those here as well.
Expand All @@ -623,6 +644,7 @@ public static ProjectAttributes ReadFrom(ObjectReader reader)
var hasAllInformation = reader.ReadBoolean();
var runAnalyzers = reader.ReadBoolean();
var telemetryId = reader.ReadGuid();
var hasSdkCodeStyleAnalyzers = reader.ReadBoolean();

return new ProjectAttributes(
projectId,
Expand All @@ -639,7 +661,8 @@ public static ProjectAttributes ReadFrom(ObjectReader reader)
telemetryId,
isSubmission: isSubmission,
hasAllInformation: hasAllInformation,
runAnalyzers: runAnalyzers);
runAnalyzers: runAnalyzers,
hasSdkCodeStyleAnalyzers: hasSdkCodeStyleAnalyzers);
}

public Checksum Checksum
Expand Down
Loading
Loading