Show duplicate item errors due to implicit items in design-time builds #675
Conversation
In design-time builds, remove duplicate items so that we can get a valid compilation and don't get a flood of errors due to having no references Fixes dotnet#664
|
@srivatsn @nguerrera @natidea @davkean for review Escrow template: ScenarioIn #624, we added a better error message for the below scenario. However, the better error message was not generated in design-time builds, so when a project is opened the error list will show the less specific error message from the compiler, as well as many other errors that types can't be resolved, etc., because the design-time build failed and so the design-time compilation doesn't have any references. This change adds the more helpful error to the design-time build and prevents the other errors from showing up in the design-time build by de-duplicating the items (only in the design-time build). Scenario for error message in general:
BugWorkaroundsPerform a build in VS, or on the command line RiskLow to medium - Tests should cover any impacted scenarios for the command line. VS design time experience was manually validated. Performance ImpactLow - Runs an additional target during design-time build to check for duplicates items. Regression Analysisn/a |
|
Tagging @MattGertz for RC3. |
|
@srivatsn Can you get this on the RC3 mail thread I started this morning so we can get it in front of JoC? |
| if (DefaultItemsEnabled && DefaultItemsOfThisTypeEnabled) | ||
| { | ||
| var duplicateItems = Items.GroupBy(i => i.ItemSpec).Where(g => g.Count() > 1).ToList(); | ||
| var itemGroups = Items.GroupBy(i => i.ItemSpec); |
There was a problem hiding this comment.
i => i.ItemSpec [](start = 47, length = 15)
For non-windows platforms, I think we need to handle "duplicate" items with different casing (e.g. Class1.cs and class1.cs) by making this Items.GroupBy(i => i.ItemSpec, StringComparer.OrdinalIgnoreCase)
There was a problem hiding this comment.
I would think it would be the opposite: e.g. case-insensitive better for Windows, case-sensitive better for Linux. It would be valid (though asking for serious trouble) on some file systems to have two different files differing only by case and we`d insert an error where compilation would have succeeded. I am not sure what the best answer is here.
There was a problem hiding this comment.
Actually I changed my mind. If we try to make this case insensitive, then even the implicit glob **/*.cs would fail if a user had both Class1.cs and class1.cs on Linux.
I think the implication of leaving it case sensitive is that on Windows, a user could still get a duplicate sources error from compiler even with this fix. (E.g. if they have implicit **/*.cs, <Compile Include="Class1.cs" /> and the file is actually named class1.cs)
There was a problem hiding this comment.
A customer ran into a similar issue - #679. Basically they had different casing for NetStandard.Library and we weren't deduping.
|
Proactively merging to get a build out for shiproom. |
…0190601.4 (dotnet#675) - Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.0.0-preview6.19301.4 - Microsoft.AspNetCore.Mvc.Analyzers - 3.0.0-preview6.19301.4
In design-time builds, remove duplicate items so that we can get a valid compilation and don't get a flood of errors due to having no references
Fixes #664