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

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 1 commit
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
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