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.

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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -597,9 +597,9 @@ public async Task<VersionStamp> GetSemanticVersionAsync(CancellationToken cancel
{
var docVersion = await _lazyLatestDocumentTopLevelChangeVersion.GetValueAsync(cancellationToken).ConfigureAwait(false);

// This is unfortunate, however the impact of this is that *any* change to our project-state version will
// This is unfortunate, however the impact of this is that *any* change to our project-state version will
// cause us to think the semantic version of the project has changed. Thus, any change to a project property
// that does *not* flow into the compiler still makes us think the semantic version has changed. This is
// that does *not* flow into the compiler still makes us think the semantic version has changed. This is
// likely to not be too much of an issue as these changes should be rare, and it's better to be conservative
// and assume there was a change than to wrongly presume there was not.
return docVersion.GetNewerVersion(this.Version);
Expand Down Expand Up @@ -675,6 +675,9 @@ public async Task<VersionStamp> GetSemanticVersionAsync(CancellationToken cancel
[DebuggerBrowsable(DebuggerBrowsableState.Collapsed)]
public bool RunAnalyzers => this.ProjectInfo.RunAnalyzers;

[DebuggerBrowsable(DebuggerBrowsableState.Collapsed)]
internal bool HasSdkCodeStyleAnalyzers => this.ProjectInfo.HasSdkCodeStyleAnalyzers;

private ProjectState With(
ProjectInfo? projectInfo = null,
TextDocumentStates<DocumentState>? documentStates = null,
Expand Down Expand Up @@ -736,6 +739,9 @@ public ProjectState WithHasAllInformation(bool hasAllInformation)
public ProjectState WithRunAnalyzers(bool runAnalyzers)
=> (runAnalyzers == RunAnalyzers) ? this : WithNewerAttributes(Attributes.With(runAnalyzers: runAnalyzers, version: Version.GetNewerVersion()));

internal ProjectState WithHasSdkCodeStyleAnalyzers(bool hasSdkCodeStyleAnalyzers)
=> (hasSdkCodeStyleAnalyzers == HasSdkCodeStyleAnalyzers) ? this : WithNewerAttributes(Attributes.With(hasSdkCodeStyleAnalyzers: hasSdkCodeStyleAnalyzers, version: Version.GetNewerVersion()));

public ProjectState WithChecksumAlgorithm(SourceHashAlgorithm checksumAlgorithm)
{
if (checksumAlgorithm == ChecksumAlgorithm)
Expand Down Expand Up @@ -962,7 +968,7 @@ public ProjectState UpdateDocuments(ImmutableArray<DocumentState> oldDocuments,

var newDocumentStates = DocumentStates.SetStates(newDocuments);

// When computing the latest dependent version, we just need to know how
// When computing the latest dependent version, we just need to know how
GetLatestDependentVersions(
newDocumentStates,
AdditionalDocumentStates,
Expand Down
11 changes: 11 additions & 0 deletions src/Workspaces/Core/Portable/Workspace/Solution/Solution.cs
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,17 @@ internal Solution WithRunAnalyzers(ProjectId projectId, bool runAnalyzers)
return WithCompilationState(CompilationState.WithRunAnalyzers(projectId, runAnalyzers));
}

/// <summary>
/// Create a new solution instance with the project specified updated to have
/// the specified hasSdkCodeStyleAnalyzers.
/// </summary>
internal Solution WithHasSdkCodeStyleAnalyzers(ProjectId projectId, bool hasSdkCodeStyleAnalyzers)
{
CheckContainsProject(projectId);

return WithCompilationState(CompilationState.WithHasSdkCodeStyleAnalyzers(projectId, hasSdkCodeStyleAnalyzers));
}

/// <summary>
/// Creates a new solution instance with the project documents in the order by the specified document ids.
/// The specified document ids must be the same as what is already in the project; no adding or removing is allowed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,16 @@ public SolutionCompilationState WithRunAnalyzers(
forkTracker: true);
}

/// <inheritdoc cref="SolutionState.WithHasSdkCodeStyleAnalyzers"/>
internal SolutionCompilationState WithHasSdkCodeStyleAnalyzers(
ProjectId projectId, bool hasSdkCodeStyleAnalyzers)
{
return ForkProject(
this.SolutionState.WithHasSdkCodeStyleAnalyzers(projectId, hasSdkCodeStyleAnalyzers),
translate: null,
forkTracker: true);
}

/// <inheritdoc cref="SolutionState.WithProjectDocumentsOrder"/>
public SolutionCompilationState WithProjectDocumentsOrder(
ProjectId projectId, ImmutableList<DocumentId> documentIds)
Expand Down Expand Up @@ -574,7 +584,8 @@ public SolutionCompilationState WithProjectAttributes(ProjectInfo.ProjectAttribu
.WithProjectDefaultNamespace(projectId, attributes.DefaultNamespace)
.WithHasAllInformation(projectId, attributes.HasAllInformation)
.WithRunAnalyzers(projectId, attributes.RunAnalyzers)
.WithProjectChecksumAlgorithm(projectId, attributes.ChecksumAlgorithm);
.WithProjectChecksumAlgorithm(projectId, attributes.ChecksumAlgorithm)
.WithHasSdkCodeStyleAnalyzers(projectId, attributes.HasSdkCodeStyleAnalyzers);
}

public SolutionCompilationState WithProjectInfo(ProjectInfo info)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,24 @@ public StateChange WithRunAnalyzers(ProjectId projectId, bool runAnalyzers)
return ForkProject(oldProject, newProject);
}

/// <summary>
/// Create a new solution instance with the project specified updated to have
/// the specified hasSdkCodeStyleAnalyzers.
/// </summary>
internal StateChange WithHasSdkCodeStyleAnalyzers(ProjectId projectId, bool hasSdkCodeStyleAnalyzers)
{
var oldProject = GetRequiredProjectState(projectId);
var newProject = oldProject.WithHasSdkCodeStyleAnalyzers(hasSdkCodeStyleAnalyzers);

if (oldProject == newProject)
{
return new(this, oldProject, newProject);
}

// fork without any change on compilation.
return ForkProject(oldProject, newProject);
}

/// <summary>
/// Create a new solution instance with the project specified updated to include
/// the specified project references.
Expand Down