Add file watchers for analyzer references #74745
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.ProjectSystem; | ||
|
|
||
| internal sealed class FileWatchedPortableExecutableReferenceFactory | ||
| internal sealed class FileWatchedReferenceFactory<TReference> | ||
| where TReference : class |
There was a problem hiding this comment.
made generic, so we can share the majority of logic across PortableExecutableReference and AnalyzerReference
| referenceDirectories.Add(Path.Combine(dotnetRoot, "packs")); | ||
| } | ||
|
|
||
| if (PlatformInformation.IsWindows) |
There was a problem hiding this comment.
this moved to logic only run for the PortableExecutionReference case, not the AnalyzerFileReference case. I'm not 100% sure if that's what we want, or if we should always be watching tehse special folders for both cases. @jasonmalinowski @dibarbet ?
There was a problem hiding this comment.
where does an AnalyzerFileReference come from? (e.g. what is the user scenario)?
There was a problem hiding this comment.
I assume we shouldn't be creating multiple file watchers for the same directories and file kinds though (I don't think we should have multiple file watchers and then file change events for the same directory and types)?
There was a problem hiding this comment.
it generally comes from a user having an <AnalyzerReference> in their project file. :)
There was a problem hiding this comment.
hm - I would think then this probably only applies to PortableExecutableReference and probably isn't needed for AnalyzerFileReference. Not 100% sure on that though
There was a problem hiding this comment.
yeah, i'll talk to @jasonmalinowski to make sure.
There was a problem hiding this comment.
I would expect we do have this same logic for both, although it may have less of a benefit in this case. The reason for these general directory watches is we expect a lot of references to come from them, and since they don't really change often it saves us a lot of overhead watching each file separately. If the SDK ends up with a dozen or so analyzers (if it doesn't already?) multiply that across a bunch of projects and you have a nice win.
Hopefully it's easy enough to refactor?
|
|
||
| if (PlatformInformation.IsWindows) | ||
| { | ||
| referenceDirectories.Add(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86), "Reference Assemblies", "Microsoft", "Framework")); |
There was a problem hiding this comment.
this moved into code specific to watching for PEReferences. Not sure if we need the same code for AnalyzerFileReferences. @jasonmalinowski i need your thoughts here :)
| Func<ProjectSystemProjectFactory, TReference, TReference> createNewReference, | ||
| Func<ProjectUpdateState, TReference, TReference, ProjectUpdateState> updateState, | ||
| Func<Solution, ProjectId, TReference, TReference, Solution> updateSolution) | ||
| where TReference : class |
There was a problem hiding this comment.
extracted a common generic helper so we can have the same logic for PERefs and AnalyzerRefs above.
| static project => project.MetadataReferences.OfType<PortableExecutableReference>(), | ||
| static reference => reference.FilePath!, | ||
| static (@this, reference) => CreateMetadataReference_NoLock(reference.FilePath!, reference.Properties, @this.SolutionServices), | ||
| static (projectUpdateState, oldReference, newReference) => projectUpdateState |
There was a problem hiding this comment.
nit: I see all these lambdas together and wonder if named arguments would improve the readability.
There was a problem hiding this comment.
@JoeRobich sure. I can add that :)
|
Not a blocker - would you add the user scenario/feedback that prompted this change/design reevaluation? |
@jaredpar do you have a good link you'd like to use as the primary here? Basically, this is about the feedback we keep getting from people developing generators, and how they hate having to close/open VS after making every change. |
@CyrusNajmabadi You can just leave a link to roslyn chat in С# discord 😅 |
|
@jasonmalinowski ptal. |
|
|
||
| static ImmutableArray<WatchedDirectory> GetAdditionalWatchedDirectories() | ||
| { | ||
|
|
This is the primary issue that gets feedback. Can see the discussion there as well as the linked issues to this one. |
|
@jasonmalinowski ptal |
|
@jasonmalinowski ptal. |
| }); | ||
|
|
||
| static ImmutableArray<WatchedDirectory> GetAdditionalWatchedDirectories() | ||
| { |
| FileWatchedReferenceFactory.StartWatchingReference(reference, reference.FilePath!); | ||
| FileWatchedPortableExecutableReferenceFactory.StartWatchingReference(reference, reference.FilePath!); | ||
|
|
||
| // Remove file watchers for any references we're no longer watching. |
There was a problem hiding this comment.
can add in followup.
We want to know when analyzers change on disk so that we can reload and rerun them. The way this will happen is as follows: