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

Do not load generators in VS process #74444

Merged
merged 33 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
c0de5f3
Comment formatting
CyrusNajmabadi Jul 18, 2024
f4e75f7
Formatting
CyrusNajmabadi Jul 18, 2024
f939303
In progress
CyrusNajmabadi Jul 18, 2024
0abf15f
In progress
CyrusNajmabadi Jul 18, 2024
3f0a8f9
In progress
CyrusNajmabadi Jul 18, 2024
e7f86d9
In progress
CyrusNajmabadi Jul 18, 2024
e680b53
Host side'
CyrusNajmabadi Jul 18, 2024
1e80bfd
Host side'
CyrusNajmabadi Jul 18, 2024
da19817
Host side'
CyrusNajmabadi Jul 18, 2024
4b62941
Host side'
CyrusNajmabadi Jul 18, 2024
417f3e2
Host side
CyrusNajmabadi Jul 18, 2024
b6ceb4a
Remote side
CyrusNajmabadi Jul 18, 2024
d56dd56
Accessibility
CyrusNajmabadi Jul 18, 2024
f98bd36
Tests
CyrusNajmabadi Jul 18, 2024
50fb7e2
Cast
CyrusNajmabadi Jul 18, 2024
7790fe3
In progress'
CyrusNajmabadi Jul 18, 2024
5da25eb
UI thread
CyrusNajmabadi Jul 18, 2024
8340b71
Tests
CyrusNajmabadi Jul 18, 2024
521c1ad
Make private
CyrusNajmabadi Jul 18, 2024
1c83cf6
Make private
CyrusNajmabadi Jul 18, 2024
02fa4de
Docs
CyrusNajmabadi Jul 18, 2024
972d236
Simplify
CyrusNajmabadi Jul 18, 2024
d2687f3
Clear the items
CyrusNajmabadi Jul 18, 2024
7a54020
Docs
CyrusNajmabadi Jul 18, 2024
1f98ad8
fix
CyrusNajmabadi Jul 18, 2024
0ef1d34
Merge branch 'generatorCleanup' into loadGenerators
CyrusNajmabadi Jul 18, 2024
51a7663
Merge branch 'generatorCleanup' into loadGenerators
CyrusNajmabadi Jul 18, 2024
236d952
Merge branch 'generatorCleanup' into loadGenerators
CyrusNajmabadi Jul 18, 2024
d71c784
Merge branch 'generatorCleanup' into loadGenerators
CyrusNajmabadi Jul 18, 2024
8a23513
Merge branch 'main' into loadGenerators
CyrusNajmabadi Jul 19, 2024
cf36ddd
Proper fallback to in proc
CyrusNajmabadi Jul 19, 2024
d503540
Merge remote-tracking branch 'upstream/main' into loadGenerators
CyrusNajmabadi Jul 22, 2024
ad64b9e
PR feedback
CyrusNajmabadi Jul 22, 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

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.Internal.VisualStudio.PlatformUI;
using Microsoft.VisualStudio.Shell;

Expand All @@ -18,27 +20,24 @@ internal sealed partial class CpsDiagnosticItemSource : BaseDiagnosticAndGenerat
private readonly IVsHierarchyItem _item;
private readonly string _projectDirectoryPath;

/// <summary>
/// The analyzer reference that has been found. Once it's been assigned a non-null value, it'll never be assigned null again.
/// </summary>
private AnalyzerReference? _analyzerReference;
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to base class. note that this comment is important. it was an invariant of this subclass that it would only set this once. We check for thsi invariant in the base class now.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to use [DisallowNull] for this.


public event PropertyChangedEventHandler? PropertyChanged;

public CpsDiagnosticItemSource(
IThreadingContext threadingContext,
Workspace workspace,
string projectPath,
ProjectId projectId,
IVsHierarchyItem item,
IAnalyzersCommandHandler commandHandler,
IDiagnosticAnalyzerService analyzerService)
: base(workspace, projectId, commandHandler, analyzerService)
IDiagnosticAnalyzerService analyzerService,
IAsynchronousOperationListenerProvider listenerProvider)
: base(threadingContext, workspace, projectId, commandHandler, analyzerService, listenerProvider)
{
_item = item;
_projectDirectoryPath = Path.GetDirectoryName(projectPath);

_analyzerReference = TryGetAnalyzerReference(Workspace.CurrentSolution);
if (_analyzerReference == null)
this.AnalyzerReference = TryGetAnalyzerReference(Workspace.CurrentSolution);
if (this.AnalyzerReference == null)
{
// The ProjectId that was given to us was found by enumerating the list of projects in the solution,
// thus the project must have already been added to the workspace at some point. As long as the project
Expand All @@ -55,7 +54,7 @@ public CpsDiagnosticItemSource(

if (analyzerReference != null)
{
_analyzerReference = analyzerReference;
this.AnalyzerReference = analyzerReference;
UnsubscribeFromEvents();
}
}
Expand All @@ -81,8 +80,6 @@ private void IVsHierarchyItem_PropertyChanged(object sender, PropertyChangedEven

public override object SourceItem => _item;

public override AnalyzerReference? AnalyzerReference => _analyzerReference;

private void OnWorkspaceChangedLookForAnalyzer(object sender, WorkspaceChangeEventArgs e)
{
// If the project has gone away in this change, it's not coming back, so we can stop looking at this point
Expand All @@ -99,7 +96,7 @@ private void OnWorkspaceChangedLookForAnalyzer(object sender, WorkspaceChangeEve
var analyzerReference = TryGetAnalyzerReference(e.NewSolution);
if (analyzerReference != null)
{
_analyzerReference = analyzerReference;
this.AnalyzerReference = analyzerReference;
UnsubscribeFromEvents();

PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(HasItems)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ internal sealed class CpsDiagnosticItemSourceProvider(
if (hierarchy.GetCanonicalName(itemId, out var projectCanonicalName) == VSConstants.S_OK)
{
return new CpsDiagnosticItemSource(
_workspace, projectCanonicalName, projectId, item, _commandHandler, _diagnosticAnalyzerService);
_threadingContext, _workspace, projectCanonicalName, projectId, item, _commandHandler, _diagnosticAnalyzerService, _listenerProvider);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public string Tags

[BrowseObjectDisplayName(nameof(SolutionExplorerShim.Effective_severity))]
public string EffectiveSeverity
=> MapReportDiagnosticToText(_diagnosticItem.EffectiveSeverity);
=> MapReportDiagnosticToText(_diagnosticItem._effectiveSeverity);

public override string GetClassName()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// See the LICENSE file in the project root for more information.

using System;
using System.ComponentModel;
using System.Threading;
using System.Threading.Tasks;
using System.Xml.Linq;
Expand All @@ -30,29 +29,16 @@ internal sealed partial class DiagnosticItem(

public ProjectId ProjectId { get; } = projectId;
public DiagnosticDescriptor Descriptor { get; } = descriptor;
public ReportDiagnostic EffectiveSeverity { get; private set; } = effectiveSeverity;

public override event PropertyChangedEventHandler? PropertyChanged;
private readonly ReportDiagnostic _effectiveSeverity = effectiveSeverity;

public override ImageMoniker IconMoniker
=> MapEffectiveSeverityToIconMoniker(this.EffectiveSeverity);
=> MapEffectiveSeverityToIconMoniker(_effectiveSeverity);

public override IContextMenuController ContextMenuController => _commandHandler.DiagnosticContextMenuController;

public override object GetBrowseObject()
=> new BrowseObject(this);

internal void UpdateEffectiveSeverity(ReportDiagnostic newEffectiveSeverity)
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 no longer update these in place (it makes equality/hash semantics non-ideal, which does impact how the observable collection and virtual tree view operate).

instead, if things do change (the rare case) we rebuild the collection, with all the data immutable in it.

Copy link
Member

Choose a reason for hiding this comment

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

💭 Will this cause the solution explorer to collapse nodes that are expanded when the children change? We've seen bugs of that form in other contexts before, so just asking.

Copy link
Member Author

Choose a reason for hiding this comment

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

missed thsi. the answer is likely 'yes'. If we feel like that's important to address, we can potentially do it by breaking out the portion of the anlaysis to determine if we think an actual change to the list happened, versus a property change to an item in the list. that said. only the item itself collapses. not the parent. so it's notthat bad imo.

{
if (EffectiveSeverity != newEffectiveSeverity)
{
EffectiveSeverity = newEffectiveSeverity;

NotifyPropertyChanged(nameof(EffectiveSeverity));
NotifyPropertyChanged(nameof(IconMoniker));
}
}

private static ImageMoniker MapEffectiveSeverityToIconMoniker(ReportDiagnostic effectiveSeverity)
=> effectiveSeverity switch
{
Expand All @@ -64,11 +50,6 @@ private static ImageMoniker MapEffectiveSeverityToIconMoniker(ReportDiagnostic e
_ => default,
};

private void NotifyPropertyChanged(string propertyName)
{
PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

no more notifications, as thsi is now an immutable vlaue.


internal void SetRuleSetSeverity(ReportDiagnostic value, string pathToRuleSet)
{
var ruleSetDocument = XDocument.Load(pathToRuleSet);
Expand All @@ -88,7 +69,7 @@ internal Task<Solution> GetSolutionWithUpdatedAnalyzerConfigSeverityAsync(Report
public override int GetHashCode()
=> Hash.Combine(this.Name,
Hash.Combine(this.ProjectId,
Hash.Combine(this.Descriptor.GetHashCode(), (int)this.EffectiveSeverity)));
Hash.Combine(this.Descriptor.GetHashCode(), (int)_effectiveSeverity)));

public override bool Equals(object obj)
=> Equals(obj as DiagnosticItem);
Expand All @@ -102,6 +83,6 @@ public bool Equals(DiagnosticItem? other)
this.Name == other.Name &&
this.ProjectId == other.ProjectId &&
this.Descriptor.Equals(other.Descriptor) &&
this.EffectiveSeverity == other.EffectiveSeverity;
_effectiveSeverity == other._effectiveSeverity;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Shared.TestHooks;

namespace Microsoft.VisualStudio.LanguageServices.Implementation.SolutionExplorer;

Expand All @@ -11,19 +13,22 @@ internal sealed partial class LegacyDiagnosticItemSource : BaseDiagnosticAndGene
private readonly AnalyzerItem _item;

public LegacyDiagnosticItemSource(
IThreadingContext threadingContext,
AnalyzerItem item,
IAnalyzersCommandHandler commandHandler,
IDiagnosticAnalyzerService diagnosticAnalyzerService)
IDiagnosticAnalyzerService diagnosticAnalyzerService,
IAsynchronousOperationListenerProvider listenerProvider)
: base(
threadingContext,
item.AnalyzersFolder.Workspace,
item.AnalyzersFolder.ProjectId,
commandHandler,
diagnosticAnalyzerService)
diagnosticAnalyzerService,
listenerProvider)
{
_item = item;
this.AnalyzerReference = item.AnalyzerReference;
}

public override object SourceItem => _item;

public override AnalyzerReference? AnalyzerReference => _item.AnalyzerReference;
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,17 @@ namespace Microsoft.VisualStudio.LanguageServices.Implementation.SolutionExplore
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class LegacyDiagnosticItemSourceProvider(
IThreadingContext threadingContext,
[Import(typeof(AnalyzersCommandHandler))] IAnalyzersCommandHandler commandHandler,
IDiagnosticAnalyzerService diagnosticAnalyzerService) : AttachedCollectionSourceProvider<AnalyzerItem>
IDiagnosticAnalyzerService diagnosticAnalyzerService,
IAsynchronousOperationListenerProvider listenerProvider) : AttachedCollectionSourceProvider<AnalyzerItem>
{
protected override IAttachedCollectionSource? CreateCollectionSource(AnalyzerItem item, string relationshipName)
{
if (relationshipName == KnownRelationships.Contains)
{
return new LegacyDiagnosticItemSource(
item, commandHandler, diagnosticAnalyzerService);
threadingContext, item, commandHandler, diagnosticAnalyzerService, listenerProvider);
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,13 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.SolutionExplorer

Dim listenerProvider = workspace.GetService(Of IAsynchronousOperationListenerProvider)
Dim source As IAttachedCollectionSource = New CpsDiagnosticItemSource(
workspace.GetService(Of IThreadingContext),
workspace,
project.FilePath,
project.Id,
New MockHierarchyItem() With {.CanonicalName = "\net472\analyzerdependency\" + analyzerPath},
New FakeAnalyzersCommandHandler, workspace.GetService(Of IDiagnosticAnalyzerService))
New FakeAnalyzersCommandHandler, workspace.GetService(Of IDiagnosticAnalyzerService),
listenerProvider)

Assert.True(source.HasItems)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,11 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.SolutionExplorer
workspace.OnAnalyzerReferenceAdded(projectId, analyzerReference)

Return New LegacyDiagnosticItemSource(
workspace.GetService(Of IThreadingContext),
New AnalyzerItem(New AnalyzersFolderItem(workspace, projectId, Nothing, Nothing), analyzerReference, Nothing),
New FakeAnalyzersCommandHandler,
workspace.GetService(Of IDiagnosticAnalyzerService))
workspace.GetService(Of IDiagnosticAnalyzerService),
workspace.GetService(Of IAsynchronousOperationListenerProvider))
End Function

Private Shared Function CreateSourceGeneratedFilesItemSource(workspace As EditorTestWorkspace, generatorItem As SourceGeneratorItem) As Shell.IAttachedCollectionSource
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ ValueTask<ImmutableArray<string>> GetContentsAsync(
/// </summary>
ValueTask<bool> HasGeneratorsAsync(
Checksum solutionChecksum, ProjectId projectId, ImmutableArray<Checksum> analyzerReferenceChecksums, string language, CancellationToken cancellationToken);

/// <summary>
/// Returns the identities for all source generators found in the <see cref="AnalyzerReference"/> with <see
/// cref="AnalyzerFileReference.FullPath"/> equal to <paramref name="analyzerReferenceFullPath"/>.
/// </summary>
ValueTask<ImmutableArray<SourceGeneratorIdentity>> GetSourceGeneratorIdentitiesAsync(
Checksum solutionChecksum, ProjectId projectId, string analyzerReferenceFullPath, CancellationToken cancellationToken);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.Runtime.Serialization;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis;

Expand Down Expand Up @@ -32,4 +35,14 @@ public static SourceGeneratorIdentity Create(ISourceGenerator generator, Analyze
return new SourceGeneratorIdentity(
assemblyName.Name!, analyzerReference.FullPath, assemblyName.Version!, generatorType.FullName!);
}

public static ImmutableArray<SourceGeneratorIdentity> GetIdentities(
AnalyzerReference analyzerReference, string language)
{
using var _ = ArrayBuilder<SourceGeneratorIdentity>.GetInstance(out var result);
foreach (var generator in analyzerReference.GetGenerators(language))
result.Add(Create(generator, analyzerReference));

return result.ToImmutableAndClear();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,21 @@ await assetProvider.GetAssetHelper<AnalyzerReference>().GetAssetsAsync(

return false;
}

public ValueTask<ImmutableArray<SourceGeneratorIdentity>> GetSourceGeneratorIdentitiesAsync(
Checksum solutionChecksum,
ProjectId projectId,
string analyzerReferenceFullPath,
CancellationToken cancellationToken)
{
return RunServiceAsync(solutionChecksum, solution =>
Copy link
Member Author

Choose a reason for hiding this comment

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

remote side of this call is trivial. we just retrieve hte AnalyzerFileRef requested, fetch its generators and get the identities from that.

{
var project = solution.GetRequiredProject(projectId);
var analyzerReference = project.AnalyzerReferences
.OfType<AnalyzerFileReference>()
.First(r => r.FullPath == analyzerReferenceFullPath);

return ValueTaskFactory.FromResult(SourceGeneratorIdentity.GetIdentities(analyzerReference, project.Language));
}, cancellationToken);
}
}
Loading