PR 3: chore(quality): mechanical analyzer-driven cleanup (453 files)#176
Open
rlorenzo wants to merge 10 commits into
Open
PR 3: chore(quality): mechanical analyzer-driven cleanup (453 files)#176rlorenzo wants to merge 10 commits into
rlorenzo wants to merge 10 commits into
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 300 files, which is 150 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (300)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
0d26c81 to
438134a
Compare
rlorenzo
force-pushed
the
tools/codeql-roslyn-sonar-resharper
branch
2 times, most recently
from
078b830 to
ccde16a
Compare
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
438134a to
11799f3
Compare
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
11799f3 to
2580a27
Compare
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
==========================================
+ Coverage 42.96% 43.02% +0.05%
==========================================
Files 877 877
Lines 51468 51415 -53
Branches 4802 4808 +6
==========================================
+ Hits 22113 22120 +7
+ Misses 28831 28771 -60
Partials 524 524
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
5 times, most recently
from
7f36555 to
918ce04
Compare
Contributor
Author
|
@bsedwards The CodeQL issues reported in the files changed section are all existing issues and were not introduced by this PR. Been running this code on TEST for a while, and things seem to be working fine. I did extensive local dev testing, and nothing is broken so far. |
3 tasks
- Bulk-applied via `jb cleanupcode --profile=OptimizeUsings` then `dotnet format`. Removes unused usings and sorts/groups remaining ones — compiler-equivalent, no behavior change. - Adds Viper.sln.DotSettings with OptimizeUsings, ShortenReferences, and RemoveRedundancies profiles for this and follow-up PRs. - Reproduction note: cleanupcode deadlocks on MSBuild's shared compiler server while the dev server runs; set DOTNET_USE_COMPILER_SERVER=0 to bypass.
- Bulk-applied via `jb cleanupcode --profile=ShortenReferences` then `dotnet format`. Replaces inline `Namespace.Type` with imported `Type` (or `using Alias = Namespace.Type;` for ambiguous names). Compiler-equivalent — no behavior change. - PhotoExportService.cs gets the largest single diff because it juggles four DocumentFormat.OpenXml sub-namespaces with conflicting type names (`Picture`, `Paragraph`, `Run`, etc.); aliases at the top replace the inline full qualifiers throughout.
- Drop dead `?.` and `!= null` guards in 10 files where the analyzer proves the value is non-null (DI-injected helpers, catch parameters, values guaranteed by preceding null-checks). - ViteProxyHelpers.cs: suppress CA1508 with `#pragma warning disable` on the inner check of a double-checked locking pattern. The outer guard may have raced; CA1508's dataflow analysis doesn't model thread interleaving, so this is a known false positive.
- Strip empty `<param name="X"></param>` tags whose corresponding parameter doesn't exist (or where the tag adds no doc value). Removes ~24 InvalidXmlDocComment findings. - Fix malformed XML: escape literal `<T>` as `<T>` in three doc comments (HttpHelper, IamApi, UinformService); promote two `// <summary>` typos in IamApi to `///`; demote orphan `///` block on Program.cs SetAwsCredentials (top-level local function doesn't accept doc comments) to plain comment. - Rename three orphan `<param>` tags to match real parameters (UserHelper.HasPermission, RAPSSecurityService.IsAllowedTo, VMACSExport.ExportToInstances). Partial-coverage cases (methods with descriptive `<param>` tags for some but not all parameters) intentionally left alone — their existing documentation has real value, and selectively adding empty tags or deleting good ones would be a net loss.
Fixes the 29 PossibleMultipleEnumeration findings flagged by ReSharper. Each call site enumerated an IEnumerable two-or-more times (e.g., `source.Any()` then `source.First()`, or `source.Sum(a)` then `source.Sum(b)`). Materialising once with `.ToList()` is semantically equivalent and avoids re-evaluating deferred queries. - Test files: cast assertion targets to lists so xUnit's `Assert.NotEmpty` + `Assert.Equal(N, x.Count())` doesn't iterate twice. - VueTableDefault.cs: `data`, `skipColumns`, `altColumnNames` are enumerated 3+ times per helper and 3 times across helpers from `InvokeAsync`. Materialise at both layers. - EvaluationPolicyService.IsRequiredFor*: `rotationWeeks` is enumerated by `.Any()`, `.FirstOrDefault(currentWeek)`, and `.FirstOrDefault(nextWeek)`. Materialise at top of method. - CMSContentController.GetContentBlockByFn: switch `Any()/First()` to `Count == 0` / indexer. - CliniciansController.FilterCliniciansByPermissions: rename param to `cliniciansSource`, materialise once into `clinicians`. - Smaller fixes in RotationsController, EvaluationReportService.
- Bulk-applied via `jb cleanupcode --profile=RemoveRedundancies` then `dotnet format`. Removes redundant argument default values, redundant member initializers, redundant `else` after `return`, redundant anonymous-type property names, and other rewrites in the ReSharper "Remove code redundancies" category — all compiler-equivalent, no behavior change. - Updates `Viper.sln.DotSettings`: the umbrella `<CSRemoveCodeRedundancies>` flag alone is a no-op. ReSharper also requires the (sparsely-documented) sibling `<RemoveCodeRedundancies>` flag plus per-rule sub-flags (`<CSRemoveRedundantArgumentDefaultValues>`, `<CSRemoveRedundantInitializers>`) to actually trigger the rewrites. - Fixes a minor follow-up: `CliniciansController.cs:569` was changed to `clinicians.Count` (List property) from `clinicians.Count()` (LINQ). The IEnumerable to List materialisation in `9823a4c9` made `clinicians` a List, which Sonar S2971 then flagged as preferring the property. No new lint rule violations introduced by this commit.
- Real cleanups across CMS / ClinicalScheduler / Effort / RAPS / Students: drop dead null-checks Roslyn flow analysis can prove unreachable, collapse a duplicated return path, simplify the cache lookup that was capturing a never-assigned outer variable, fix the stale XML doc param tag left behind by the IEnumerable materialisation rename, remove a virtual call from a DTO constructor, and split a SqlCommand object initializer outside the using statement. - Replace anonymous-type byte[]? gymnastics in PhotoExportService with a named record so CodeQL stops flagging the casts as redundant. - Gate now supports `--exclude-rule` and ships defaults for rules that fire false positives on ASP.NET Core / EF surfaces (DTO accessors bound at runtime by JSON / MVC, record positional properties used via record pattern Equals, and the EF nav-property NRT contract that Roslyn intentionally distrusts because Include() can be missing).
The UserHelper.GetByLoginId cache populator doesn't need the ICacheEntry parameter; using `_` silences the new ReSharper UnusedParameter.Local finding the previous commit introduced when it collapsed the closure.
Aligns the lines already modified in this branch with the closure work done on codeql/3-correctness so the two branches do not conflict on merge. - CMSFile.cs: seal the class to close cs/virtual-call-in-constructor (no inheritor in C#, no mocking, no UseLazyLoadingProxies). - CompetencyController.UpdateComptency: drop the now-dead `CompetencyId <= 0` check. The preceding FindAsync already returns NotFound when the row is missing; an existing row's identity column is always a positive int. - Program.cs SetAwsCredentials: trim the RegionEndpoint value, look it up with BindingFlags.IgnoreCase, and fall back to USWest1 when the name is non-empty but does not match any known region. The previous ternary could leave profile.Region as null on unknown region names.
…ines The xunit.v3 migration on main introduces analyzer xUnit1051, which recommends test calls that accept a CancellationToken pass `TestContext.Current.CancellationToken` instead of relying on the default. This branch's bulk cleanup happened to touch 16 such lines across two test files, so the PR-scoped ReSharper gate now flags them as new findings at PR-touched lines. - ReportsControllerTests: pass `ct:` to the no-arg controller calls that validate BadRequest/parameter-error paths (10 sites). - EmailNotificationTest: pass the token to `_context.Persons.AddAsync` in 6 test setup blocks. The remaining xUnit1051 warnings elsewhere in the test project are pre-existing on main and not blocked by this PR.
rlorenzo
force-pushed
the
quality/analyzer-bulk-cleanup
branch
from
a350ca9 to
282bb6d
Compare
Contributor
Author
|
@bsedwards, This has been updated to not conflict with the changes coming in for the CodeQL security fixes at #190 |
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
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
Part 3 of 6. Stacks on top of PR #175. This is the elephant — 453 files changed, but they're almost all mechanical, tool-applied rewrites with no behavior change.
Commits (each is one analyzer rule's bulk fix)
optimize using directivesshorten reference qualifiersNamespace.Type→ importedTyperemove dead null-checksclean up stale XML doc commentsmaterialize IEnumerableremove code redundanciesjb cleanupcode --profile=RemoveRedundanciesclear ReSharper PR-gate + CodeQL review findingsdiscard unused entry paramWhy bundled
Splitting these by area would create artificial PRs that the reviewer still couldn't read line-by-line. They share the same review style — "trust the tool, spot-check the result" — so they're better together.
Conflict resolution notes
Two cherry-pick conflicts hit because later refactors (in PR #5/#6) modified files this PR also touches:
web/Areas/CMS/Data/Codecs.cs(refactored later in PR 6) — applied just theSystem.IO.Stream→Streamqualifier shortening to the pre-refactor file.web/Areas/Students/Services/StudentGroupService.cs(refactored later in PR 6) — applied just theid => id != null→!string.IsNullOrEmpty(id)and SqlException qualifier fixes; skipped thesealed recordchange which targets a record introduced by the PR 6 refactor.web/Areas/Effort/Services/EvaluationReportService.cs: skipped — thematerialize IEnumerablefix targetsCalculateWeightedAverage, a helper that gets introduced by PR 5. The fix is moved into PR 5 atop that extraction.PR stack
Test plan
npm run verify:buildsucceeds locallynpm run testpasses