Produce warning on duplicate analyzer references #63264
Conversation
ghost
added
Area-Compilers
Community
Youssef1313
force-pushed
the
issues/62610
branch
from
81be1f8 to
2e80a60
Compare
Youssef1313
force-pushed
the
issues/62610
branch
from
2e80a60 to
a9326fa
Compare
| <!-- TODO: Remove once we consume an SDK with the fix from https://github.com/dotnet/wpf/pull/6799 --> | ||
| <Target Name="RemoveDuplicateAnalyzers" BeforeTargets="CoreCompile"> | ||
| <ItemGroup> | ||
| <FilteredAnalyzer Include="@(Analyzer->Distinct())" /> | ||
| <Analyzer Remove="@(Analyzer)" /> | ||
| <Analyzer Include="@(FilteredAnalyzer)" /> | ||
| </ItemGroup> | ||
| </Target> |
There was a problem hiding this comment.
I think the root source of duplication is:
This produces a whole ItemGroup in the wpftmp project with analyzers:
|
@RikkiGibson This is green now :) |
jaredpar
left a comment
jaredpar
left a comment
There was a problem hiding this comment.
Can't use a hash set cause of deteriminism issues but everything else looks good.
eng/targets/Imports.targets
Outdated
| </ItemGroup> | ||
| </Target> | ||
|
|
||
| <!-- TODO: Remove once we consume an SDK with the fix from https://github.com/dotnet/wpf/pull/6799 --> |
There was a problem hiding this comment.
Nit: can you match the general format for a work around comment as used in the rest of the file
| <!-- TODO: Remove once we consume an SDK with the fix from https://github.com/dotnet/wpf/pull/6799 --> | |
| <!-- Workaround for https://github.com/dotnet/wpf/pull/6799 --> |
| }; | ||
|
|
||
| var resolvedReferences = ArrayBuilder<AnalyzerFileReference>.GetInstance(); | ||
| var resolvedReferences = PooledHashSet<AnalyzerFileReference>.GetInstance(); |
There was a problem hiding this comment.
Can't use a hash set here because it introduces non-determinism into the compilation. Think we need two collections here: the resolved list and the hash set.
|
@RikkiGibson PTAL |
|
It looks like a test baseline needs to be updated. |
|
@RikkiGibson @jaredpar Anything else needed here? |
|
Thanks for the ping @Youssef1313. I think we should do a few checks before merging here. Other projects could get that same issue with WPF projects that we had to work around if they start consuming this compiler. I'd like to test-insert this change to VS to verify the impact, but I won't be able to until our internal build gets fixed. I'll follow up when I have more info. |
|
Have created a test insertion (internal only) for this. |
|
@RikkiGibson Test insertion is passing. |
|
Would like to merge as soon as we can verify dotnet/wpf#6792 (comment). |
|
I think we are good to go here since this should be going into RC1 at the same time as the WPF fix. |
|
Thanks @Youssef1313! |
5 participants
Fixes #62610