Initial UnusedReferenceService API#47136
Initial UnusedReferenceService API#47136JoeRobich merged 5 commits intodotnet:features/UsedAssemblyReferencesfrom
Conversation
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
It's hard to tell what this all means without an impl of those interfaces. Can you also explain the big picture here and how these are expected to be used by the ps?
src/Features/Core/Portable/UnusedReferences/IUnusedReferencesService.cs
Outdated
Show resolved
Hide resolved
| Task<Project> UpdateReferencesAsync( | ||
| Project project, | ||
| ImmutableArray<ReferenceUpdate> referenceUpdates, | ||
| CancellationToken cancellationToken); |
There was a problem hiding this comment.
Not clear to me yet what this is needed for
There was a problem hiding this comment.
The Unused Reference UI would invoke this to apply the changes the user has choosen. Removing direct references, adding transitive references, or updating a TrestAsUsed reference attribute.
There was a problem hiding this comment.
a few more questions:
- "Removing direct references". If they have a Project instance, why can't htey just do the removal themselve and give us back the new Project instance?
- "or updating a TrestAsUsed reference attribute." I feel like we want a bit on MetadataREference to say that it is TreatAsUsed. With that bit all hte UI has to do is fork teh Project instance and replace the MetadataReferences with teh appropriate forked versions with that bit set.
- "adding transitive references,". Can you expand on this. Why is that part of an 'unused reference ui'?
There was a problem hiding this comment.
@CyrusNajmabadi Thanks for your questions. My goal for this service is that it provides the high-level Project and Package references that are unused and allows those high-level references to be updated.
- So, the UI will invoke this method with the list of changes that the user requested. This method will then work with the project system to ensure that the Project and Package references are updated accordingly.
- A single Project or Package references is likely composed of many MetadataReferences. I am also unsure how an updated metadata reference results in an updated project file. Unless you are saying each of these updates could be encoded as changes in a project file.
- It is possible that a no types from a directly referenced Project or Package are used in the compilation, but types from a tansitive reference it brings along are used. We could provide a means to remove the unused direct reference and directly add the transitive reference in these cases.
There was a problem hiding this comment.
It is possible that a no types from a directly referenced Project or Package are used in the compilation, but types from a tansitive reference it brings along are used. We could provide a means to remove the unused direct reference and directly add the transitive reference in these cases.
i don't think that's possible... can you give an example. i believe the compielr needs all references to be direct to understand anything.
There was a problem hiding this comment.
I think "meta packages" (Microsoft.Net.Compilers.Toolset are the simplest example where nothing is being used from the direct reference, but transitive references are being used.
|
@CyrusNajmabadi Not sure, but were you thinking that from the VisualStudio layer we have access to the PackageUninstallService as well as the ability to manipulate the VsProject project reference without relying on the Project System for this work? |
| /// <summary> | ||
| /// Gets the current selected TargetFramework for the specified project. | ||
| /// </summary> | ||
| string GetTargetFramworkMoniker(ProjectId projectId); |
There was a problem hiding this comment.
is there a reason we need to know the current selecte tfm, as opposed to all the tfms the project supports?
There was a problem hiding this comment.
should this be async?
src/Features/Core/Portable/UnusedReferences/IReferenceCleanupService.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/UnusedReferences/IReferenceCleanupService.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Microsoft.VisualStudio.LanguageServices.csproj
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/UnusedReferences/UnusedReferencesService.cs
Outdated
Show resolved
Hide resolved
| var targetFrameworkMoniker = referenceUpdateService.GetTargetFramworkMoniker(project.Id); | ||
| var projectAssetsFilePath = await referenceUpdateService.GetProjectAssetsFilePathAsync(project.FilePath, targetFrameworkMoniker, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| var projectAssetsText = File.ReadAllText(projectAssetsFilePath); |
There was a problem hiding this comment.
- definitely has to be a protected region. we have IOUtilities for thsi purpose.
- should also be done prior doing the expensive compilation work.
- What is the failure mode for GetUnusedReferencesAsync? if we can't read IO, do we propagate that higher? or do we catch and expose the result in way that gives this information to the caller.
src/Features/Core/Portable/UnusedReferences/UnusedReferencesService.cs
Outdated
Show resolved
Hide resolved
| return targetFrameworkMoniker ?? string.Empty; | ||
| } | ||
|
|
||
| return string.Empty; |
There was a problem hiding this comment.
need to doc failure mode for this API.
667a79c to
58dbb45
Compare
There was a problem hiding this comment.
A single project.assets.json is generated containing the dependency information for all TFMs.
58dbb45 to
59ae847
Compare
|
@ocallesp & @CyrusNajmabadi I've updated this PR based on feedback and it is ready for another review. |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
i think this is good enough to move forward with integration. we may run into issues later, but we can cross that bridge then :)
|
It appears integration test running is broken in this feature branch because it hasn't been updated and the Integration machines are now running VS 16.8 P2 which had breaking changes. |
This adds the types needed for the Project System to implement their share of the feature's logic.
CC: @ocallesp
The goal of this api is to allow a UI dialog and a compilation end analyzer to be written. The UI dialog would be invoked from the Solution Explorer context menu for project and solution nodes and provide the user with the ability to remove unused references. The analyzer would during a build and emit diagnostics based on the presense of unused references.
The IUnusedReferenceService would be the Roslyn entry point to this analysis and provide the means to update these unused references.
The IReferenceUpdateService would be implemented by the Project System and MEF imported to allow us to query information from the build and to update references in project files.