Fix spurious warnings in dependency node #2435
Conversation
…mework projects and due to incorrect IDs for P2P dependencies
|
Tagging @MattGertz for 15.3 |
| return id.Replace('/', '\\'); | ||
| return id | ||
| .Replace('/', '\\') | ||
| .Replace("..", "__"); |
There was a problem hiding this comment.
Is there no API that can do this normalization for us?
There was a problem hiding this comment.
This is a targeted fix to workaround the fact that CPS calls MakeRooted which alters the ID. Long term perhaps we should consider replacing the id with a more opaque hash.
There was a problem hiding this comment.
This could be problematic. This ID becomes the canonical name of the IVsHierarchy item, and there are places we expect to be able to extract the original file path from the canonical name (for example, analyzer support in Solution Explorer).
There was a problem hiding this comment.
You shouldn't be interpreting the moniker - that is only for the project system itself to reason about. You should be using hierarchy properties to retrieve any data you need.
There was a problem hiding this comment.
So was this regressed given we merged this in?
There was a problem hiding this comment.
I have not seen a regression in Analyzers. I believe Analyzers use an absolute path so it would not be impacted by this.
There was a problem hiding this comment.
Okay, I thought about this more, I can see why analyzers did this originally, analyzers look like the following in the project file:
<ItemGroup>
<Analyzer Include="Analyzer.dll" />
</ItemGroup>This looks a lot like like source files:
<ItemGroup>
<Compile Include="Foo.cs"/>
</ItemGroup>I cansee why we return the analyzer's canonical name as the same as the path of the analyzer.
However, I will state - we should not be expecting or interpreting that outside of the project system. There are other APIs for retrieving a file path given a canonical/mkdocument name, we should be using that. We're theoretically allowed to return whatever we want for a canonical name, for example, that canonical name would be problematic in the following:
<ItemGroup>
<Analyzer Include="Analyzer.dll" />
<None Include="Analyzer.dll" />
</ItemGroup>These are going to have the same canonical name even though they are supposed to be unique.
Customer scenario
Customers observe spurious and confusing warning icons in dependency node when there should not be any, or do not observe warning icons when there should be. This collection of small targeted fixes addresses the most common cases, including:
One side-effect: P2P references to incompatible projects should have a warning icon but does not (however, there are warning messages in the error list). Three separate bugs in NuGet, SDK and Project System are preventing this scenario from working properly, and will need to be addressed later or post 15.3. I've filed #2436 and NuGet/Home#5405 to track fixing this. This seems like a fair tradeoff since users are still warned and the build will fail if user tries to setup an incompatible dependency. Additionally, this side effect is not a regression since this did not work in 15.2 either.
Bugs this fixes:
Fixes #2242
Bug 431059
Workarounds, if any
None
Risk
Low, targeted fixes that only affect dependency node. Postponed more complicated fixes to SDK (#2436)
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Yes. Dependency node was rewritten and we are working through some behavior changes.
Root cause analysis:
Incorrect IDs is a subtle bug occurring because a CPS method is calling
UnconfiguredProject.MakeRooted(see UnattachedProjectItemTreeNode.cs:75 ). The warning on all .NET Framework projects is because the heuristic to determine if TargetFrameworks are compatible is unable to find information for these projects -- this fix changes the heuristic to only report warnings when it can find information.How was the bug found?
Internal Dogfooders
/cc @dotnet/project-system