Do not load generators in VS process #74444
Conversation
git pus
git pus
git pus
git pus
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
src/VisualStudio/Core/Impl/SolutionExplorer/AnalyzerItem/AnalyzerItem.cs
Outdated
Show resolved
Hide resolved
| @@ -33,11 +33,11 @@ internal abstract class BaseItem : | |||
| { | |||
| public virtual event PropertyChangedEventHandler PropertyChanged { add { } remove { } } | |||
|
|
|||
| private readonly string _name; | |||
| protected readonly string Name; | |||
There was a problem hiding this comment.
protected as we use it as part of the equality contract of derived classes.
src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/CpsDiagnosticItemSource.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/DiagnosticItem.cs
Outdated
Show resolved
Hide resolved
| private void NotifyPropertyChanged(string propertyName) | ||
| { | ||
| PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); | ||
| } |
There was a problem hiding this comment.
no more notifications, as thsi is now an immutable vlaue.
|
|
||
| public ProjectId ProjectId { get; } | ||
| public DiagnosticDescriptor Descriptor { get; } | ||
| public ReportDiagnostic EffectiveSeverity { get; private set; } |
There was a problem hiding this comment.
private setter removed.
src/Workspaces/Core/Portable/SourceGeneration/IRemoteSourceGenerationService.cs
Outdated
Show resolved
Hide resolved
...sualStudio/Core/Impl/SolutionExplorer/DiagnosticItem/BaseDiagnosticAndGeneratorItemSource.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public override IContextMenuController ContextMenuController => _commandHandler.DiagnosticContextMenuController; | ||
|
|
||
| public override object GetBrowseObject() | ||
| => new BrowseObject(this); | ||
|
|
||
| internal void UpdateEffectiveSeverity(ReportDiagnostic newEffectiveSeverity) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💭 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.
There was a problem hiding this comment.
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.
| /// <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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You should be able to use [DisallowNull] for this.
| private void NotifyPropertyChanged(string propertyName) | ||
| { | ||
| PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); | ||
| } |
There was a problem hiding this comment.
no more notifications, as thsi is now an immutable vlaue.
| CommandHandler = commandHandler; | ||
| _diagnosticAnalyzerService = diagnosticAnalyzerService; | ||
|
|
||
| _listener = listenerProvider.GetListener(FeatureAttribute.SourceGenerators); |
There was a problem hiding this comment.
probably not. if so, can remove in followup if there are no other changes i need to make.
| } | ||
|
|
||
| return; | ||
| var identifies = await GetIdentitiesAsync().ConfigureAwait(false); |
This change updates our VS workspace to eliminate the last known place where we were loading generators 'in process' in the VS host. Specifically, we would load generators 'in proc' to populate this UI:
Note: The files themselves were generated in our OOP process. But we still loaded the generators in-proc to get things like all the generator names.
Loading in proc is highly problematic as it limits us from being able to reload generators when they change on disk. Because VS is a .Net framework app, any attempts to reload will bork as that's just not somethign .Net framework is capable of.
By moving this computation out of process, we now compute it in our .Net Core host. There we are able to use thigns like AssemblyLoadContexts to actually load different versions of an generator independently. Note: the work to actually do that is upcoming. But this blocks the VS side that would have otherwise had significant problems.