Add an analyzer to detect when a duplicate razor file is provided #11498
Conversation
This can cause other source generator errors, so we should make this easier to diagnose by including an analyzer that explicitly tells the user that this is an error scenario.
| @@ -8,8 +8,7 @@ | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" /> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit" /> | |||
There was a problem hiding this comment.
Note that these specific verifiers are deprecated. See https://github.com/dotnet/roslyn-sdk/tree/main/src/Microsoft.CodeAnalysis.Testing#obsolete-packages.
|
I'm surprised this is a valid scenario in msbuild, or in the compiler command line args, or in a Roslyn project, in order to make it this far! |
| </data> | ||
| <data name="DuplicateRazorFileIncludedMessage" xml:space="preserve"> | ||
| <value>File '{0}' was included in the build twice. This will cause generator build issues.</value> |
There was a problem hiding this comment.
Nit: Should this be more specific and say "was included as an additional file" rather than just "in the build"? Including it in the build twice isn't actually a problem if the items have different item types, eg EmbeddedResource and AdditionalFiles, right?
Also "more than once" rather than "twice" because it could actually be three times 😛
| #pragma warning disable RS1041 // Compiler extensions should be implemented in assemblies targeting netstandard2.0 | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| #pragma warning restore RS1041 // Compiler extensions should be implemented in assemblies targeting netstandard2.0 | ||
| public class DuplicateRazorFileIncludedAnalyzer : DiagnosticAnalyzer |
| Location.Create(additionalFile.Path, new TextSpan(0, 0), new LinePositionSpan(LinePosition.Zero, LinePosition.Zero)), | ||
| additionalFile.Path); | ||
|
|
||
| additionalFilesContext.ReportDiagnostic(diagnostic); |
There was a problem hiding this comment.
this will cause errors in source generation
The source generator will still run and report errors though, right? Wouldn't it be better if this was part of the SG pipeline then?
What's the advantage of a separate analyzer anyway?
There was a problem hiding this comment.
There is a general feeling that we should be moving towards analyzers producing diagnostics over generators as they play better in the incremental space.
There was a problem hiding this comment.
Perhaps the source generator could also detect this situation and bail early to avoid producing superfluous diagnostics.
There was a problem hiding this comment.
There is a general feeling that we should be moving towards analyzers producing diagnostics over generators as they play better in the incremental space.
Thinking about this recommendation in general, it's hard to imagine all the diagnostics that are produced by the razor SG now would instead be produced by some analyzer, that sounds like a lot of duplicate (and hence error-prone) logic.
| if (additionalFile.Path.EndsWith(".cshtml", StringComparison.Ordinal) || | ||
| additionalFile.Path.EndsWith(".razor", StringComparison.Ordinal)) | ||
| { | ||
| if (!includedFiles.Add(additionalFile.Path)) |
There was a problem hiding this comment.
Can it happen that the paths are equivalent but not normalized at this point? For example:
Components\Pages\Counter.razor
Components/Pages/Counter.razor
This check would not catch such duplicates.
| [Theory] | ||
| [InlineData("Duplicate.cshtml")] | ||
| [InlineData("Duplicate.razor")] | ||
| public async Task Analyzer_ReportsDiagnostic_WhenDuplicateRazorFileIsIncluded(string fileName) |
There was a problem hiding this comment.
Consider adding a test for the razor source generator as well to see what the errors would be.
I actually cannot reproduce any issues there. I've created a project which passes duplicate AdditionalFiles to Csc but don't see any errors from the razor source generator. What is needed to hit an error?
There was a problem hiding this comment.
So, I really don't like this approach. I understand the desire to make an analyzer based on some of the conversations we've had about diagnostics in generators, but I don't think this should be the forcing function to change.
Today the razor generator already issues diagnostics, so I don't really want the logic smeared across two places, and the main issue with outputting diagnostics in a generator is based on the location changing within a syntax tree. These aren't based on that anyway, so it's pretty trivial to just make a node that comes off of additional files and issues the diagnostic in an incrementally correct way without needing to add an entire new analyzer.
Inspired by a recent customer issue that had this as its root cause, we will now issue an error when there are multiple duplicate razor files provided to the compilation via AdditionalFiles, as this will cause errors in source generation. We do this as a bespoke analyzer, rather than as part of the generator pipeline, as this is best practice and the approach we want to take going forward.