Check for missing hierarchies when mapping to Roslyn projects #19559
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes dotnet#19517. In order to shows the "Analyzers" node under the "References" node in Solution Explorer and show the correct sets of analyzer assemblies and diagnostics, we need to map the node for the project to the actual Roslyn `AbstractProject` instance. This is done by matching up various properties of the `IVsHierarchy` for the node to the same properties on the `IVsHierarchy` returned by `AbstractProject.Hierarchy`. This code path was initially written this `Hierarchy` property would never return null; its value was supplied by the project system. However, Delayed Project Load (DPL) bypasses the project system until the user expands the project node in Solution Explorer, and as a result we may now encounter `AbstractProjects` where `Hierarchy` returns null. Currently if we run across one of these we'll throw a `NullReferenceException` which has the end effect of preventing the user from expanding the project node in Solution Explorer, and may even result in a crash. This commit adds in the null check. If the `Hierarchy` property is null then it can't be the project we're looking for.
|
@Pilchie @dotnet/roslyn-ide Please review. |
tmat
reviewed
May 16, 2017
| @@ -62,7 +62,8 @@ public bool TryGetProjectId(IVsHierarchyItem hierarchyItem, string targetFramewo | |||
| // project files are in the same folder--they both use the full path to the _folder_ as the canonical | |||
| // name. To distinguish them we also examine the "regular" name, which will necessarily be different | |||
| // if the two projects are in the same folder. | |||
| if (p.Hierarchy.TryGetCanonicalName((uint)VSConstants.VSITEMID.Root, out string projectCanonicalName) | |||
| if (p.Hierarchy != null | |||
There was a problem hiding this comment.
Add a comment explaining when Hierarchy is null.
Update the code comment to explain why we need to check for null.
|
Adding @MattGertz for approval, pending tests. |
|
@dotnet-bot test windows_release_unit32_prtest please |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to shows the "Analyzers" node under the "References" node in
Solution Explorer and show the correct sets of analyzer assemblies and
diagnostics, we need to map the node for the project to the actual
Roslyn
AbstractProjectinstance. This is done by matching up variousproperties of the
IVsHierarchyfor the node to the same properties onthe
IVsHierarchyreturned byAbstractProject.Hierarchy.This code path was initially written this
Hierarchyproperty wouldnever return null; its value was supplied by the project system.
However, Delayed Project Load (DPL) bypasses the project system until
the user expands the project node in Solution Explorer, and as a result
we may now encounter
AbstractProjectswhereHierarchyreturns null.Currently if we run across one of these we'll throw a
NullReferenceExceptionwhich has the end effect of preventing theuser from expanding the project node in Solution Explorer, and may even
result in a crash.
This commit adds in the null check. If the
Hierarchyproperty is nullthen it can't be the project we're looking for.
Customer scenario
Customer opens a solution with DPL enabled. When they expand a project node in Solution Explorer, nothing happens (the arrow indicates that the project is expanded, but nothing shows up under the project node).
Subsequently searching in Solution Explorer may lead to a crash.
Bugs this fixes:
Fixes #19517.
Workarounds, if any
Disable DPL for the solution.
Risk
Low; we're adding a null check that always should have been there.
Performance impact
Low; just the cost an additional null check when the user initially expands a project.
Is this a regression from a previous update?
Yes.
Root cause analysis:
This code path predates DPL, and until DPL the property in question couldn't be null. A subsequent refactoring to support the new project system made use of the property, but I didn't realize the null check was now necessary.
How was the bug found?
Internal customer report