Add warning waves infrastructure to Razor compiler#13016
Add warning waves infrastructure to Razor compiler#13016chsienki merged 2 commits intodotnet:mainfrom
Conversation
Implement the plumbing for Razor warning waves, modeled after C#'s warning waves feature. This PR adds the infrastructure only — no new warnings are introduced yet. Changes: - Add WarningLevel (uint) to RazorDiagnosticDescriptor and RazorDiagnostic - 0 = always reported, >0 = suppressed when exceeding configured level - Add RazorWarningLevel (uint) to RazorConfiguration and RazorCodeGenerationOptions - Default derived from RazorLanguageVersion major version - Add central diagnostic filtering in CodeRenderingContext.GetDiagnostics() - Add GetDefaultWarningLevel() to RazorLanguageVersion - Parse/validate build_property.RazorWarningLevel in source generator (RZ3601 for invalid values) - Add ReportDiagnostics overload for ImmutableArray<Diagnostic> Tests: - CodeRenderingContextTest: 9 tests for warning level filtering - Source generator tests: validation of RazorWarningLevel parsing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| // Filter out diagnostics whose warning level exceeds the configured level. | ||
| // Diagnostics with level 0 are always reported regardless of the configured level. | ||
| using var filtered = new PooledArrayBuilder<RazorDiagnostic>(capacity: _diagnostics.Count); |
There was a problem hiding this comment.
Oh, was SelectAndOrderByAsArray not a thing?
There was a problem hiding this comment.
Never mind the old question, that was dumb. But, the new question is:
Is OrderByAsArray stable in netstandard2.0? Does it matter in this context?
There was a problem hiding this comment.
Looks like the stability is the same as the original, so never mind
There was a problem hiding this comment.
BTW, it feels like this method is pretty high traffic. In which case, maybe your original change is better.
| Error "{Id}": "{MessageFormat}" | ||
| """; | ||
| private string DebuggerToString() | ||
| => $"""Error "{Id}" (level {WarningLevel}): "{MessageFormat}" """; |
There was a problem hiding this comment.
Is the extra space at the end intentional?
There was a problem hiding this comment.
This wasn't the change that I was expecting.
davidwengier
left a comment
There was a problem hiding this comment.
Presumably you'll follow up with an SDK change after this is merged to pass the property through?
| razorLanguageVersion = RazorLanguageVersion.Latest; | ||
| } | ||
|
|
||
| uint razorWarningLevel = razorLanguageVersion.GetDefaultWarningLevel(); |
There was a problem hiding this comment.
Should the default here be 10, so if the SDK doesn't pass the RazorWarningLevel property through to the compiler they essentially get the same behaviour as now?
There was a problem hiding this comment.
I went back and forth on this. I ended up here because I think we should always aim to be the language version. In older SDKs we'll be using older language versions, which didn't have WarningWaves, and in newer ones we'll have the property.
There is a question of do we actually set it in the SDK at all, or just leave it empty by default and use language version, and then if the user wants to set it we'll pick up the value.
It comes down to a bit philosophical of where we store the default value: in the compiler or in the targets. I prefer this way, but open to changing it.
There was a problem hiding this comment.
To be clear, because I wasn't in my first comment, I wasn't thinking about the value of RazorWarningLevel provided by the SDK (and I don't think we need to go and necessarily set a value for it in the SDK), I was talking about for when the SDK doesn't set <CompilerVisibleMetadata Include="RazorWarningLevel">, which is what this default would be for. In other words, what this is now is "RazorWarningLevel defaults to RazorLangVersion", what I was wondering about is "RazorWarningLevel defaults to RazorLangVersion, unless you're using an SDK that doesn't know about RazorWarningLevel at all, in which case it defaults to 10 to baseline to current behaviour". Or something.
There was a problem hiding this comment.
Oh yeah, that's a really good catch on the distinction, let me update it.
b276092 to
b78889f
Compare
- Use Where().OrderByAsArray() for non-destructive filtering in GetDiagnostics - Revert uint back to int for consistency with Roslyn - Fix copypasta bug in WithSuppressUniqueIds (RootNamespace -> SuppressUniqueIds) - Remove trailing space in debugger display string - Extract CreateDescriptor helper and refactor all descriptors in RazorDiagnostics.cs - Make lambdas static in IncrementalValueProviderExtensions - Use PooledArrayBuilder(capacity: 2) in source generator RazorProviders - Extract ParseRazorLanguageVersion and ParseRazorWarningLevel helper methods - Distinguish absent vs empty RazorWarningLevel: absent (old SDK) defaults to 0, empty (new SDK) defaults to language version - Fix encoding issues in comments (em dash -> dash) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@ToddGrun Addressed your feedback, let me know if you have anything else. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Implement the plumbing for Razor warning waves, modeled after C#'s warning waves feature. This PR adds the infrastructure only - no new warnings are introduced yet.
Design: #12713
Changes
Core Infrastructure
RazorDiagnosticDescriptor- Addeduint WarningLevelproperty (0 = always reported)RazorDiagnostic- ExposedWarningLevelfrom descriptorRazorConfiguration- Addeduint RazorWarningLevelparameter (default 0)RazorCodeGenerationOptions- ThreadedRazorWarningLevelthrough options, builder, and allWith*methodsRazorLanguageVersion- AddedGetDefaultWarningLevel()(returns major version as uint)RazorProjectEngine- Wires config warning level into code gen optionsDiagnostic Filtering
CodeRenderingContext.GetDiagnostics()- Central filtering: suppresses diagnostics whereWarningLevel > configured level. Level 0 diagnostics are always reported.Source Generator
build_property.RazorWarningLevelfrom MSBuildRZ3601for invalid valuesTests
CodeRenderingContextTest- 9 tests for warning level filtering (levels 0-13, multi-level, always-on)RazorWarningLevelparsing (invalid, valid, empty)