Fix three matcher/formatter parity gaps vs. upstream Java source - #408
Merged
Conversation
…erGroupsRemainGrouped The C# port of PhoneNumberMatcher.AllNumberGroupsRemainGrouped (used by STRICT_GROUPING/EXACT_GROUPING leniency) was missing two pieces of logic present in the upstream Java since this was first ported: - It never skipped past the country calling code before searching for the first formatted group, so a decoy occurrence of that group's digits earlier in the candidate (e.g. in leading junk before the true country code) could be matched instead of the real one. - The "no separator after the NDC" fast-path compared the candidate directly against the national significant number whenever a digit followed the NDC match, without first checking (as Java does) that the region actually has a national destination-code prefix at all. Both are restored to match the current Java source exactly.
PhoneNumberMatcher.ContainsMoreThanOneSlash had been reduced to a bare "are there two or more slashes" check, dropping the number parameter and the "permitted if the extra slashes come after the country calling code" carve-out that the upstream Java has always had (e.g. a German number followed by a slash-separated date, "+49/69/2013", is fine; a literal date with no country code, "1/05/2013", is not). It also silently treated a slash at index 0 as "no slash" (`firstSlashIndex > 0` instead of `>= 0`). This affects STRICT_GROUPING/EXACT_GROUPING leniency in FindNumbers/IsNumberMatch, which call this to decide whether a candidate should be rejected. Restored to match Java's containsMoreThanOneSlashInNationalNumber exactly.
…neNumberMatcher
Java's PhoneNumberUtil.formattingRuleHasFirstGroupOnly() (rule is empty, or
just the first-group placeholder optionally wrapped in one pair of
parentheses) is shared by AsYouTypeFormatter and
PhoneNumberMatcher.isNationalPrefixPresentIfRequired. The C# port had two
independent, both-wrong copies of this check instead:
- AsYouTypeFormatter's private copy matched against the literal "$1"
placeholder, but this port's metadata expands "$FG" to the .NET
replacement token "${1}" (see BuildMetadataFromXml), so real rules like
"(${1})" (e.g. Colombia's mobile format) never matched. As a result AYTF
could wrongly keep formats that require a national prefix, or discard
ones that don't, while filtering candidate formats as digits come in.
- PhoneNumberMatcher.IsNationalPrefixPresentIfRequired used a different,
home-grown check (substring before "${1}", normalize digits, check
emptiness) that only inspects what comes *before* the placeholder and
ignores anything after it, unlike Java's whole-string check.
Added PhoneNumberUtil.FormattingRuleHasFirstGroupOnly as the single
correct port (checked against "${1}", matching this port's stored rule
format) and pointed both call sites at it, mirroring how Java shares one
implementation between the two.
📊 Benchmark Results
PR branch
PR base
|
…OneSlash PR #408 CI was failing with CS1503 on netstandard2.0: PhoneNumberMatcher.cs(647,75): error CS1503: Argument 1: cannot convert from 'char' to 'string' string.Contains(char) doesn't exist on netstandard2.0 (only Contains(string) does), but net8.0/net10.0's Roslyn analyzers flag Contains(string) with a single-char literal as CA1847 (prefer the char overload), and TreatWarningsAsErrors turns that into a build failure on the modern TFMs. Split the call with #if NETSTANDARD2_0, matching the existing polyfill convention in this repo (see PhoneNumberUtil.netstandard.cs) so each TFM uses the overload it actually has/prefers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #408 +/- ##
==========================================
+ Coverage 77.26% 77.28% +0.02%
==========================================
Files 39 39
Lines 4548 4561 +13
Branches 1129 1134 +5
==========================================
+ Hits 3514 3525 +11
- Misses 783 785 +2
Partials 251 251 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ompat PR #408 changed ContainsMoreThanOneSlash's signature from (string candidate) to (PhoneNumber number, string candidate) to fix a correctness bug (matching Java's testContainsMoreThanOneSlashInNationalNumber, which needs to know whether the first slash falls after the country calling code). That's a wanted behavioral fix, but it silently broke the public API surface, which this repo keeps frozen until a major version rev (see #375) and enforces via EnablePackageValidation/ApiCompat during `dotnet pack`. CI caught it as CP0002 against the 9.0.37 baseline for all three TFMs. Restore the old 1-arg overload verbatim (same body that shipped in 9.0.37) as a compatibility shim alongside the new, correct 2-arg overload. Nothing in production code called the old signature (only the test file, which already uses the 2-arg form), so this purely protects hypothetical external consumers without touching the fixed logic.
This was referenced Aug 25, 2026
This was referenced Aug 28, 2026
Closed
Closed
Merged
This was referenced Sep 7, 2026
Closed
Closed
This was referenced Sep 9, 2026
Closed
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
Small porting-parity pass comparing
PhoneNumberMatcher/AsYouTypeFormatter/PhoneNumberUtilagainst the current upstream Java source (google/libphonenumber). Three genuine behavioral gaps found and fixed, each with a regression test that fails without the fix:AllNumberGroupsRemainGrouped: was missing Java's step of skipping past the country calling code before locating the national-destination-code boundary, and a guard requiring the region to have a national prefix before applying the no-separator fast path.ContainsMoreThanOneSlash: reduced to a bare "≥2 slashes" check; restored Java's carve-out permitting a slash that falls right after the country calling code (e.g. a number followed by a date), and fixed an off-by-one for a slash at index 0.FormattingRuleHasFirstGroupOnly: existed as two separate, both-incorrect copies (inAsYouTypeFormatterand inlined inPhoneNumberMatcher), neither matching this port's"${1}"placeholder token — so metadata rules like Colombia's"(${1})"were mishandled. Consolidated into one correct shared helper onPhoneNumberUtil, mirroring Java's single shared implementation.Each change is a minimal, targeted port of the current Java logic — no redesign or unrelated cleanup.
Test plan
dotnet test PhoneNumbers.Test/PhoneNumbers.Test.csproj— 412/412 passing on net8.0 and net10.0