Skip to content

fix: stop the obsolete leading-zero setter calling an obsolete method - #460

Merged
twcclegg merged 2 commits into
mainfrom
fix/codeql-obsolete-api-calls
Sep 6, 2026
Merged

fix: stop the obsolete leading-zero setter calling an obsolete method#460
twcclegg merged 2 commits into
mainfrom
fix/codeql-obsolete-api-calls

Conversation

@twcclegg

@twcclegg twcclegg commented Sep 5, 2026

Copy link
Copy Markdown
Owner

The four cs/call-to-obsolete-method alerts. One is a real deprecated-calls-deprecated chain and is fixed; three are tests that deliberately cover a deprecated API and should be dismissed rather than rewritten.

What actually triggers this query

Reading the query itself (csharp/ql/src/API Abuse/CallToObsoleteMethod.ql) rather than guessing at it decided every call site:

from MethodCall c, Method m
where
  m = c.getTarget() and
  m.getAnAttribute() instanceof ObsoleteAttribute and
  not c.getEnclosingCallable().(Attributable).getAnAttribute() instanceof ObsoleteAttribute

Three consequences:

  • #pragma warning disable CS0618 does nothing here. All four sites were already inside a pragma with an explanatory comment. CodeQL reads the AST, not pragmas — which is why these alerts persist despite a 0-warning build.
  • Only MethodCall counts. Constructor calls and property accesses are invisible to it, which is why the repo's other CS0618-suppressed obsolete usages (new EmbeddedResourceMetadataLoader(), the PhoneRegex ctor, MessageBeingBuilt.ItalianLeadingZero) are not flagged. Exactly four obsolete method calls exist; exactly four alerts.
  • [Obsolete] on a property does not reach its accessors — which is precisely why Phonenumber.cs:170 was flagged. It is deprecated code calling deprecated code, but getEnclosingCallable() returns the setter accessor, whose own attribute set is empty.

Fixed: Phonenumber.cs:170

SetItalianLeadingZero's own [Obsolete] names SetNumberOfLeadingZeros as the replacement, and its entire body is MessageBeingBuilt.NumberOfLeadingZeros = value ? 1 : 0; return this;. The setter discarded that return value, so calling the replacement directly is byte-identical. Line 170 was the method's only caller in the repo. The property, its [Obsolete] marker and the public signature are unchanged.

Added TestItalianLeadingZeroSetterWritesNumberOfLeadingZeros, which goes through the ItalianLeadingZero property setter itself rather than SetNumberOfLeadingZeros directly. Nothing in the existing suite exercised that exact line — TestNonEqualWithItalianLeadingZeroSetToTrue, the test whose name suggests it does, calls SetNumberOfLeadingZeros directly on both sides — so the fixed line previously shipped with no regression coverage of its own.

Recommend dismissing: the three TestMedataManager.cs sites (68, 81, 88)

MetadataManager.SetMetadataLoader's [Obsolete] message is "Not intended for external use and will become internal in a future release." It names no replacement — the member is slated to become internal, not superseded — so there is nothing to migrate to, and SetMetadataLoader_RoutesLookupsThroughCustomLoader / SetMetadataLoader_RejectsNull exist precisely to cover that entry point. The comment above the pragma now says so explicitly.

Two ways to make the alerts disappear were considered and rejected:

  • Marking the test methods [Obsolete] to satisfy the query's exclusion — analyzer-gaming, and it wouldn't even work: line 88's call sits inside a lambda (Assert.Throws<...>(() => ...)), so getEnclosingCallable() returns the lambda, which can carry no attribute.
  • Adding a non-obsolete internal alias for the tests to call — adds API surface and destroys the coverage the tests exist for.

Suggested dismissal reason: won't fix — test deliberately covers a deprecated API that has no replacement.

Verification

  • dotnet build csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 — 0 warnings, 0 errors.
  • dotnet test csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 — 491 passed, 0 failed, 0 skipped.

No [Obsolete] markers removed, no public API changes.

Note for reviewers: the new test was added in a sandbox with no .NET SDK and no network access to install one, so it could not be built/run locally to confirm it compiles and passes — checked by hand against the existing test file's own conventions (same #pragma warning disable CS0618 pattern used elsewhere in this repo). CI on this PR is the actual gate for that one commit.

Resolves the cs/call-to-obsolete-method CodeQL alerts. The query excludes a
call whose *enclosing callable* is itself marked [Obsolete], but attributes on
a property do not reach its accessors, so the back-compat shim below was
flagged even though it is deprecated code calling deprecated code.

PhoneNumber.Builder.ItalianLeadingZero's setter now writes through
SetNumberOfLeadingZeros(value ? 1 : 0) instead of SetItalianLeadingZero(value).
SetItalianLeadingZero's entire body is that same write plus `return this`
(discarded here), and its own [Obsolete] message names SetNumberOfLeadingZeros
as the replacement, so this is the documented migration and the observable
behaviour is unchanged. The property, its [Obsolete] marker and the public
signature all stay exactly as they were; line 170 was SetItalianLeadingZero's
only caller.

The three MetadataManager.SetMetadataLoader calls in TestMedataManager are
deliberately left alone. That member's [Obsolete] message names no
replacement - it is slated to become internal rather than to be superseded -
so there is nothing to migrate to, and the two tests exist specifically to
cover that entry point. They keep their narrowly scoped CS0618 pragma; the
comment above it now records why the CodeQL alerts on those lines should be
dismissed rather than "fixed" by rewriting the tests away from the API under
test.
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.59%. Comparing base (8ad7fa2) to head (5f28309).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #460      +/-   ##
==========================================
+ Coverage   87.51%   87.59%   +0.07%     
==========================================
  Files          43       43              
  Lines        3886     3885       -1     
  Branches      991      991              
==========================================
+ Hits         3401     3403       +2     
+ Misses        280      278       -2     
+ Partials      205      204       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The existing coverage near this code (TestNonEqualWithItalianLeadingZeroSetToTrue)
calls SetNumberOfLeadingZeros directly, so nothing in the suite actually invokes
the obsolete ItalianLeadingZero property setter this PR just changed to stop
calling SetItalianLeadingZero. Add a test that goes through the property itself.
@twcclegg
twcclegg force-pushed the fix/codeql-obsolete-api-calls branch from a14a0d9 to 5f28309 Compare September 5, 2026 19:38
@twcclegg
twcclegg merged commit 672de6c into main Sep 6, 2026
8 checks passed
@twcclegg
twcclegg deleted the fix/codeql-obsolete-api-calls branch September 6, 2026 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants