Skip to content

fix: preserve Java normalization semantics for line terminators - #507

Closed
wmundev wants to merge 1 commit into
mainfrom
codex/bug-review-2026-10-28-parity
Closed

wmundev wants to merge 1 commit into
mainfrom
codex/bug-review-2026-10-28-parity

Conversation

@wmundev

@wmundev wmundev commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Normalizing a vanity number containing a line terminator currently converts its letters to keypad digits even though Java drops those letters. For example, Normalize("1800FLOWERS\n") returns 18003569377 in C# and 1800 in Java. This is a P2 parity defect with high confidence, affecting callers of normalization with copied or multiline input.

The string and StringBuilder guards now reject the five line terminators excluded by Java's non-DOTALL VALID_ALPHA_PHONE_PATTERN: LF, CR, NEL, line separator and paragraph separator. Ordinary spaces, tabs, vertical tabs and form feeds continue to allow vanity conversion. The additional scan runs only after finding three ASCII letters; no regex or public API is added.

Java reference: PhoneNumberUtil.java at 33be5664343676b27761213d8f5500d682568ea7, specifically VALID_ALPHA_PHONE_PATTERN and normalize(StringBuilder) at lines 804–811. The existing normalization tests are in the corresponding PhoneNumberUtilTest.java.

Validation:

  • Freshly compiled the Java baseline and executed 22 equivalent cases: six terminator forms at three positions, plus four whitespace controls. All passed.
  • Added a C# regression to the corresponding synthetic-metadata test class; observed it fail before the fix with expected 1800, actual 18003569377, and pass afterward.
  • dotnet test csharp/PhoneNumbers.slnx: 1,008 tests passed (453 library + 51 extensions tests on each of net8.0 and net10.0).
  • dotnet build csharp --no-restore: all three library TFMs built, zero warnings/errors, including trim/AOT analyzers.
  • Executed the same 22 cases against the modern DLL and the hash-verified netstandard2.0 DLL using an external harness; all passed.
  • Before/after PhoneNumberWorkflowBenchmark.ParseOnly over 1,000 numbers: 358.6 / 335.5 microseconds, 348.12 KB allocated in both. Five measured iterations, one launch each; broad timing intervals overlap, so this is a limited regression check, not evidence of a speedup or of vanity-specific throughput.
  • Release package validation and clean-rebuild reproducibility checked with CI=true and VersionPrefix=9.0.42; all three packaged DLL hashes match across both builds.
  • Diff whitespace checked with core.whitespace=cr-at-eol, preserving the files' existing line endings.

Review baselines: C# main dcaee5a42ab993d26a4bac47ef1cac4f1150ade2; Java master 33be5664343676b27761213d8f5500d682568ea7. The C# metadata is v9.0.42 and Java's checked-in metadata is v9.0.41; these comparisons do not consult metadata. This review does not establish whole-library parity. No Java source or generated metadata changed.

@wmundev wmundev added the bug label Oct 28, 2026
@codecov

codecov Bot commented Oct 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.86%. Comparing base (dcaee5a) to head (c52c670).

Files with missing lines Patch % Lines
csharp/PhoneNumbers/PhoneNumberUtil.cs 33.33% 1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
- Coverage   87.96%   87.86%   -0.10%     
==========================================
  Files          43       43              
  Lines        3896     3899       +3     
  Branches      995      997       +2     
==========================================
- Hits         3427     3426       -1     
- Misses        271      273       +2     
- Partials      198      200       +2     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wmundev wmundev closed this Oct 28, 2026
@wmundev
wmundev deleted the codex/bug-review-2026-10-28-parity branch October 28, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant