Fix duplicate diagnostic counts and middleware cancellation - #366
Merged
davidkallesen merged 7 commits intoJul 29, 2026
Conversation
AssemblyHelper.GetSystemVersion() returns the full four-part assembly version. The fourth component is the build height added by tools such as Nerdbank.GitVersioning, which is not something a caller wants when presenting a package version. The overload takes the number of components to include and validates it is between 1 and 4, so callers can ask for SemVer-style "Major.Minor.Patch" directly instead of trimming the string themselves. Includes tests and the regenerated CodeDoc.
Picks up the remarks for the all-numeric top-level label rejection added in 5d45738, which noted that it also disambiguates host names from IPv4 addresses. The generated CodeDoc was not regenerated in that commit.
- InternetBrowserHelper: S4036 "use an absolute path". Launching the user's default browser goes through the shell, so resolving an absolute path is not applicable. - NetworkInformationHelper: S1313 "magic numbers should not be used", twice. The literals are well-known network constants and are clearer inline than named.
Assert.Contains over a string sequence without a comparer uses the default equality comparer, which the analyzers flag as culture-dependent. Passing StringComparer.Ordinal states the intent and keeps the assertions culture-neutral. No behavioural change: every one of these compares C# syntax tokens, header names or literal test data, all of which are ordinal by nature.
MSBuild reports every diagnostic twice: inline while the target runs, and again in
the end-of-build recap under "(CoreCompile target) ->". Neither -clp:NoSummary nor
-tl:off removes the recap, and ParseBuildOutputHelper incremented a counter per
regex match, so every occurrence count was exactly doubled.
Consumers see this directly. atc-coding-rules-updater generates temporary
suppressions annotated with an occurrence count, and a solution with 7 real
violation sites was reported as 14.
Parsing is now per line and identical diagnostics are counted once. The key is the
whole normalised line, not the matched fragment: the patterns start at ": error " /
": warning " and therefore exclude the file and position, so keying on the fragment
would collapse two genuine occurrences of the same rule at different positions.
Normalisation also strips a leading MSBuild node id ("1>"), because at higher
verbosity the inline copy carries one while the recap copy is indented.
Adds six tests: three duplicate shapes, two guarding that legitimate duplicates
still count separately (same code at different positions, same message from
different projects), and one over the real captured output of a two-project
solution asserting 14 lines yield 7 occurrences.
…g out where wrong S8949 flagged four call sites that dropped the request's cancellation token. They need two different answers, not one. KeepAliveMiddleware writes the response body for a health ping, so it now passes context.RequestAborted: if the caller has hung up there is nothing to gain by writing it. RequestResponseLoggerMiddleware passes CancellationToken.None, explicitly. Those three call sites run after the response has been produced - they rewind the buffered swap stream, copy it back into the real response body and restore httpContext.Response.Body. Honouring the request's cancellation there would abort the copy part-way on a client disconnect, leaving a truncated response, an unrestored Body (the assignment follows the await) and an incomplete log entry, which is the one thing that middleware exists to produce. A comment records that this is deliberate, since the analyzer will keep suggesting RequestAborted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
✨ Features
GetSystemVersion(int fieldCount)onAssemblyHelperMajor.Minor.Patchdirectly🐛 Fixes
ParseBuildOutputHelper-clp:NoSummarynor-tl:offremoves the recap copycontext.RequestAbortedinKeepAliveMiddlewareCancellationToken.Nonein the request/response logger📦 Dependencies
🔧 Configuration
.editorconfigsectionInternetBrowserHelperNetworkInformationHelper🎨 Styling
StringComparer.OrdinaltoAssert.Containson strings📝 Documentation
System.mdfor theIsHostNameRFC 1123 remarksGetSystemVersionoverloadNotes
atc-coding-rules-updaterreported 7 real sites as 14 beforeAppDomainExtensionsTests.GetAssemblyInformationsfailsorigin/main