Close IDE/CA build-enforcement gaps in the C# 14 style contract - #87
Merged
Merged
Conversation
Researched current C# 14 / .NET 10 style and analysis-rule guidance across seven angles (Microsoft Learn, the dotnet/roslyn option-default source, and 2026 analyzer-ecosystem status) to find rules this repo's TreatWarningsAsErrors + EnforceCodeStyleInBuild gate was not actually enforcing. The mechanism: EnforceCodeStyleInBuild only runs IDE-rule analyzers at build time, it does not promote their severity, and roughly two dozen relevant rules ship at Suggestion or Silent by default - a silent no-op in CI no matter how the project is configured, until each one gets an explicit severity here. Adds explicit severities/values for: pattern-matching preference (IDE0019, IDE0020, IDE0038, IDE0078, IDE0260), local-function-over-lambda and capture-free static local functions/lambdas (IDE0039, IDE0062, IDE0320), readonly struct/member (IDE0250, IDE0251), throw-expression and deconstruction simplification (IDE0016, IDE0042), redundant interpolation and compound assignment (IDE0071, IDE0054, IDE0074), dead-store elimination (IDE0059), parentheses-for-clarity (IDE0047, IDE0048), unbound-generic nameof for C# 14 (IDE0340), ArgumentNullException/ArgumentOutOfRangeException/ ObjectDisposedException throw-helper preference (CA1510-CA1513), and base/interface parameter-name parity kept at suggestion to match IDE1006's own rename-safety rationale (CA1725). Adds SYSLIB1054 as a build error to guard against a future hand-written [DllImport] - a BannedSymbols.txt/RS0030 entry for the same symbol was tried and reverted, because it also flags LibraryImportGenerator's own generated DllImport-based fallback stub for non-fully-blittable P/Invoke signatures, which RS0030 does not exempt the way the compiler's own SYSLIB1054 diagnostic does. Also flips csharp_style_prefer_primary_constructors to false to make the repo's existing explicit-constructor convention a documented choice rather than an unpinned SDK default, and enables dotnet_style_prefer_auto_properties and dotnet_style_prefer_conditional_expression_over_assignment/return at suggestion pending a manual pass. Adds three naming rules that previously matched nothing: non-private fields and enum members (the existing non_private_members rule only covers properties/methods/events), const and static readonly fields (previously fell through to the private-field _camelCase rule for the private subset), and an Async-suffix rule for async methods and local functions of any accessibility. Makes eng/Shipping.props's AOT/trim posture explicit (EnableTrimAnalyzer, EnableAotAnalyzer, EnableSingleFileAnalyzer, IsTrimmable - already implied by IsAotCompatible, restated so the guarantee survives a future SDK change to what that implies) and adds VerifyReferenceTrimCompatibility alongside the existing AOT reference check. dotnet format applied the ~24 resulting violations across the codebase. One of its rewrites (a lambda-to-local-function conversion in LiveProbeStateTests.cs, IDE0039) left a following continuation-line argument indented with a single space instead of the file's tab convention in two spots; fixed by hand after `dotnet format --verify-no-changes` and a full `dotnet build` both passed anyway, which is worth knowing about for any future large-scale dotnet format run on this codebase. Verified: clean build (0 warnings, 0 errors), dotnet format --verify-no-changes clean, and 4167/4168 tests pass. The one remaining failure (PackedConsumerBindingTests.Packaged_AOT_consumer_publishes_and_runs_only_as_a_standalone_executable) is a pre-existing environment gap (vswhere.exe not on PATH, breaking the native AOT link step) reproduced identically on main before this change. Every change here is low blast-radius by design (autofixable or additive, confirmed against a real build). Higher-blast-radius candidates from the same research pass - AnalysisMode<Category> escalations, CA1852/CA1063/CA1051, a Meziantou.Analyzer version bump, a VSTHRD analyzer addition, stricter expression-bodied-member and collection-expression severities, System.Threading.Lock migration, and CA1062 - are intentionally deferred pending their own trial builds and review.
The IDE-wide reformat commit (09df927) mixed genuine style fixes with several unrelated regressions: - Silently dropped the `partial` modifier from five classes hosting `[LuaFunction]`/`[LuaGlobal]` members (BenchFunctions, CoexistencePluginAFunctions, CoexistencePluginBFunctions, LiveFunctions, ProbeConsole), breaking the LuaBindings source generator's companion emission for each. - Renamed the CE 7.7 bootstrap's `CESDK` namespace to `LiveProbe` and dropped its `using LiveProbe;`, which would have broken Cheat Engine's hard-coded `CESDK.CESDK` host lookup. - Removed two `using CheatEngine.SDK.Engine.Generated;` directives that a source-generator-blind IDE inspection flagged as unused, and renamed a namespace in a LiveProbe.Tests stub that is intentionally paired with a source-linked file in the `LiveProbe` namespace. - Line-wrapped three `<Target>` entries in CompatibilitySuppressions.xml, breaking ApiCompat's exact-match lookup against already-approved 2.0.0 breaking changes on AddressResolutionOptions. - Padded a markdown table in src/CheatEngine.SDK/README.md past a doc-consistency test's exact substring check. - Swapped `static const char` for `static constexpr char` in the native Lua bridge, invalidating its pinned SHA-256 proof record. This commit reverts each regression and lets dotnet format finish applying the remaining, legitimate IDE0055/IDE0048 whitespace and clarity fixes across the tree. It also splits GovernanceWorkflowTests.Dependency_submit_step_submits_only_a_snapshot_of_this_run, which had grown past MA0051's 60-line limit, extracting BuildDependencySnapshot as a helper. The packages.lock.json refresh across projects reflects the Meziantou.Analyzer 3.0.290 pin already recorded in Directory.Packages.props by the reformat commit. Verified: dotnet build (0 warnings, 0 errors), dotnet format --verify-no-changes (clean), and dotnet test --solution CheatEngine.SDK.slnx --fail-skips on (4167/4168; the sole remaining failure is PackedConsumerBindingTests' Native AOT publish, which needs the MSVC/vswhere.exe linker toolchain this environment does not have and is unrelated to this change).
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: CheatEngineNet/CheatEngine.SDK/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates analyzer settings and the Meziantou.Analyzer package across the solution. It explicitly enables trimming and AOT analysis checks in shipping properties. It also changes several code expressions and internal state declarations, adds test support for Lua synchronization, and updates tests and documentation. Most other edits reformat existing code, tables, and comments without changing their described behavior. Merge Risk: ⚪ Minimal · up to The lock-file update matches the repository’s analyzer version pin; no actionable merge risk is identified. 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Full details: Public Api And CompatibilityExplanation The PR changes Resolution Revert the namespace-only change to Comment |
tests/CheatEngine.SDK.AotProbe isn't part of CheatEngine.SDK.slnx, so the solution-wide restore in the previous commit never touched its packages.lock.json. It was still pinned to Meziantou.Analyzer 3.0.283 while every other lock file in the tree had already moved to the 3.0.290 GlobalPackageReference, which failed CI's locked-mode restore (NU1004) in the Lock files, Detect NuGet dependencies, and Native AOT publication probe jobs.
|
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
(Suggestion/Silent) despite EnforceCodeStyleInBuild, and fills in three
naming-convention rules that previously matched nothing (async-suffix,
const/static-readonly fields, non-private fields).
eng/Shipping.props (IsTrimmable, EnableTrimAnalyzer, EnableAotAnalyzer,
EnableSingleFileAnalyzer, VerifyReferenceTrimCompatibility).
on this branch mid-review: five classes silently lost the
partialmodifier their LuaBindings-generated members depend on, the CE 7.7
bootstrap's
CESDKnamespace got renamed (breaking CE's hard-codedCESDK.CESDKhost lookup), twousing CheatEngine.SDK.Engine.Generated;directives were dropped as apparently-unused, three
CompatibilitySuppressions.xml
<Target>entries got line-wrapped(breaking ApiCompat's exact-match lookup), a markdown table drifted past
a doc-consistency test's exact substring check, and one line in the
native Lua bridge changed in a way that invalidated its pinned SHA-256
proof record.
Test plan
dotnet build CheatEngine.SDK.slnx -c Debug— 0 warnings, 0 errorsdotnet format CheatEngine.SDK.slnx --verify-no-changes— cleandotnet test --solution CheatEngine.SDK.slnx -c Debug --fail-skips on— 4167/4168; the sole failure (
PackedConsumerBindingTestsNativeAOT publish) reproduces identically on
mainand needs theMSVC/vswhere.exe linker toolchain this environment lacks
.editorconfigwith build-enforced analyzer severities, style preferences, and naming rules. AddedSYSLIB1054as an error and enabled required API analyzer files.eng/Shipping.props, including referenced-assembly trim-compatibility verification.Meziantou.Analyzerto 3.0.290 in central package management and lock files, including the AotProbe lock file.synchronizestand-in, refine bridge-export assertions, and extract a dependency snapshot helper.partialmodifiers and generator imports, a corrected bootstrap namespace, compatibility suppression updates, and a native Lua bridge change. The supplied change details do not describe each repair.mainand requires an unavailable MSVC/vswhere.exe linker toolchain.