Skip to content

Commit 71e7f6a

Browse files
aaronburtleAniruddh25RubenCerna2079
authored
Fix CS8632 warnings in Service.Tests (#3610)
## Why make this change? Closes #3543 ## What is this change? Changed one line in `src/Service.Tests/Azure.DataApiBuilder.Service.Tests.csproj`: ``` - <Nullable>disable</Nullable> + <Nullable>annotations</Nullable> ``` and we include a comment explaining the choice and linking back to issue #3543. #### Why this particular approach? `Azure.DataApiBuilder.Service.Tests.csproj` declared `<Nullable>disable</Nullable>`, but ~25 files in the project override it with a per-file `#nullable enable` directive. The issue arises when files used `string?` syntax **without** the per-file override, producing CS8632 noise in build logs and a latent risk of hard build breaks if `TreatWarningsAsErrors=True` ever propagated into the project. #### Options considered | # | Approach | Why rejected (or chosen) | |---|---|---| | **A** | Strip `?` from every offending file | High churn across many test files, easy to regress, and *loses* real intent (those `?`s were added deliberately by file authors who used `#nullable enable`). Doesn't actually fix the underlying mismatch, a new file added tomorrow could re-introduce the warning. | | **B** | `<Nullable>enable</Nullable>` to match the rest of the repo (`Directory.Build.props` sets `enable`) | Turns on **flow-analysis** warnings (CS8600 / CS8602 / CS8618 / CS8625, etc.) across thousands of lines of legacy test code that was written under `disable`. Because `Directory.Build.props` sets `TreatWarningsAsErrors=True`, every one of those becomes a build break. The scope to clean up is enormous and out of scope for this cleanup issue. | | **C** | `<Nullable>annotations</Nullable>` | Exactly the C# language mode designed for this scenario. Permits `?` syntax in the annotations context everywhere (no more CS8632), **without** enabling flow-analysis warnings on the unaudited bulk of the test code. The 25 files that already have `#nullable enable` keep their stronger setting. | #### Why `annotations` is the right C# language mode here The C# nullable-context feature has two independent dimensions: the **annotations context** (do `?` and `!` mean anything?) and the **warnings context** (do flow-analysis diagnostics fire?). The four `<Nullable>` MSBuild values map to combinations of those two: | Value | Annotations | Warnings | |---|---|---| | `disable` | off → `?` triggers CS8632 | off | | `warnings` | off → `?` triggers CS8632 | on | | **`annotations`** | **on → `?` is legal everywhere** | **off → no CS8600/CS8602/etc. noise** | | `enable` | on | on | `annotations` is the only mode that fixes the reported problem (CS8632) without creating a much larger one (flow-analysis warnings turned into errors across legacy code). #### Per-file `#nullable enable` directives are unchanged The 25 files that declare `#nullable enable` (e.g. `AuthorizationResolverUnitTests.cs`, `DabCacheServiceIntegrationTests.cs`, the `Mcp/` and `Embedding*` test files, etc.) have been audited for full nullable correctness and keep their stronger setting. Leaving them alone: - preserves their existing nullable flow analysis, - makes them defensive rather than load-bearing, they no longer silently mask a project-level inconsistency, - avoids churn unrelated to this issue. ## How was this tested? 1. **Baseline.** Clean rebuild of `Service.Tests` with the change: `0 Warning(s), 0 Error(s)`. 2. **Regression probe.** Temporarily added `private static string? __nullableRegressionProbe = null;` to `RequestParserUnitTests.cs` — one of the files called out in the issue, which has **no** `#nullable enable` directive. Rebuilt: - CS8632 count: **0** - The only complaints were CA1805 / CS0414 about the unused probe field itself, unrelated to nullability and confirming the project setting tolerates `?` in files without the per-file directive. 3. **Final clean rebuild after reverting the probe:** `0 Warning(s), 0 Error(s)`. Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
1 parent 84c58b7 commit 71e7f6a

1 file changed

Lines changed: 8 additions & 1 deletion

File tree

src/Service.Tests/Azure.DataApiBuilder.Service.Tests.csproj

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,14 @@
33
<PropertyGroup>
44
<TargetFrameworks>net10.0</TargetFrameworks>
55
<IsPackable>false</IsPackable>
6-
<Nullable>disable</Nullable>
6+
<!--
7+
Nullable=annotations permits nullable reference type syntax (e.g. `string?`)
8+
everywhere without emitting CS8632, while keeping nullable flow-analysis
9+
warnings (CS8600/CS8602/etc.) suppressed for the bulk of the legacy test code.
10+
Individual files that have been audited may opt in to full nullable checks
11+
via `#nullable enable`. See issue #3543.
12+
-->
13+
<Nullable>annotations</Nullable>
714
<OutputPath>$(BaseOutputPath)\tests</OutputPath>
815
<!-- ASPDEPR008: IWebHostBuilder/IWebHost are obsolete on net10. The
916
ConfigurationTests fixtures still consume them via Program.cs

0 commit comments

Comments
 (0)