refactor(text,text-json,globalization,core): one UTF-8 scalar decode, in one place (#2354)

#2014 found five copies of this runtime's UTF-8 scalar decode, factored the rule
into System/detail/Utf8Scalar.hpp and moved the two .cpp ones. #2354 was the
follow-up for the three header-inline copies it left alone as "entangled with
those types' own decode loops".

THE TICKET NAMED THREE COPIES AND THERE WERE SIX. Measured by grepping for the
continuation-byte test across every module:

  UnicodeEncoding.hpp   named    moved
  UTF32Encoding.hpp     named    moved
  Rune.hpp              named    moved
  Utf8JsonWriter.cpp    NOT      moved -- byte-for-byte identical to the shared one
  IdnMapping.cpp        NOT      moved
  UTF8Encoding.cpp      NOT      moved -- see below

Zero copies remain. That the count could be wrong by a factor of two is the
finding, not a footnote: a rule duplicated often enough that nobody can enumerate
its copies is exactly the shape that produced the defects #2014 and the IdnMapping
repair were written to fix -- "\xC2\x41" decoding to a garbage code point,
"\xC0\x80" decoding straight through to real U+0000.

THE COPIES WERE NOT GRATUITOUS, AND THAT IS PRESERVED RATHER THAN COLLAPSED.
Three doors want three different things, and they differ on exactly one input
class: a STRUCTURALLY VALID encoding of a value that is not a Unicode scalar -- a
surrogate, or a value above U+10FFFF.

  Rune::TryGetRuneAt   reports failure, consuming the SEQUENCE'S OWN LENGTH, so a
                       caller can step over the whole thing
  any Encoding         substitutes U+FFFD over ONE BYTE, so it emits one
                       replacement per byte, which is .NET's replacement
                       DecoderFallback
  IdnMapping           throws, because a domain name comes from untrusted input

So the shared header has TWO entry points, not one. TryDecodeUtf8Scalar reports;
DecodeUtf8Scalar is five lines on top of it and substitutes. Collapsing the two
would have been a silent behaviour change in whichever door lost.

THE COPY #2014 COULD NOT MOVE. UTF8Encoding.cpp's wellFormedUtf8Length reads a
(pointer, end) range rather than a std::string and returns a validity LENGTH
rather than a code point, because its well-formed bytes pass through unchanged and
there is no re-encoding step to fold a substitution into. That was a real
obstacle. The answer is that the range is now a PARAMETER of the shared decode.
`size` is an exclusive bound, so a caller decoding a sub-range gets a truncated
sequence reported as ill-formed instead of read past.

IdnMapping's two hand-spelled extra rejections are subsumed exactly: a 2-byte lead
below 0xC2 is an overlong encoding, and a 4-byte lead above 0xF4 encodes a value
above U+10FFFF.

THE TEST THAT DID NOT EXIST. Rune::TryGetRuneAt had NO test anywhere in the
repository, and the mutation that collapses its length onto the Encoding contract
passed all 17,296 tests then present. Five cases added:

  Text           311 -> 314   the Rune/Encoding contrast, per-branch overlong rows
  Globalization  681 -> 683   the throwing contract, and the two subsumed rules

Two files rather than one because modules/text does not depend on Globalization,
and a refactor is not a reason to add a public component edge. The module graph is
unchanged at 41 modules / 92 edges, verified.

Five mutations, all caught -- but TWO of them only after a new assertion, and both
are recorded rather than quietly fixed, because they measure how much of this rule
was untested while it was duplicated six ways: collapsing the two contracts, and
dropping the 3-byte branch's overlong rejection (the 2-byte row alone missed it,
since each branch carries its own test).

No behaviour change at any door, no signature change, no layout change, nothing to
migrate. -179 lines. System::Text::detail still re-exports both entry points, so
every caller written against the old spelling is unchanged.

Also closes #2282 as MOOT: its subject, UnitySerializationHolder::GetRealObject(),
was removed outright by #2281 under SA-9, so there is no misleading message left to
repair and no replacement text left to defer.

Gate: 17,301 run, 17,301 passed, 0 failed, 0 skipped across 38 executables, GREEN.

docs/Utf8ScalarDecodeSingleDefinition.md

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,99 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# One UTF-8 scalar decode, in one place (ticket #2354)
+
+*2026-08-18.* `System/detail/Utf8Scalar.hpp` is now the **only** definition of this runtime's
+UTF-8 scalar decode. Six copies existed across four modules; #2014 moved two, and #2354 moved the
+remaining four.
+
+**No behaviour changes.** Every door produces byte-identical output for every input. There is no
+signature change, no layout change and nothing to migrate; this note is a design record, not a
+migration note.
+
+---
+
+## 1. Where the copies were
+
+| Copy | Module | Named by the ticket? | Fate |
+|---|---|---|---|
+| `ASCIIEncoding.cpp` | `text` | — | moved by #2014 |
+| `Latin1Encoding.cpp` | `text` | — | written against the shared rule by #2014 |
+| `UnicodeEncoding.hpp` | `text` | ✅ | moved |
+| `UTF32Encoding.hpp` | `text` | ✅ | moved |
+| `Rune.hpp` | `text` | ✅ | moved |
+| `Utf8JsonWriter.cpp` | `text-json` | ❌ | moved — byte-for-byte identical |
+| `IdnMapping.cpp` | `globalization` | ❌ | moved |
+| `UTF8Encoding.cpp` | `text` | ❌ | moved — see §3 |
+
+**The ticket named three and there were six.** That is the finding, not a footnote: a rule
+duplicated often enough that nobody could enumerate its copies is exactly the shape that produced
+the defects #2014 and the IdnMapping repair were written to fix (`"\xC2\x41"` decoding to a
+garbage code point, `"\xC0\x80"` decoding straight through to real U+0000).
+
+## 2. Three contracts over one rule
+
+The copies were not gratuitous. Three doors genuinely want three different things, and they
+differ on exactly one input class: a **structurally valid** encoding of a value that is **not a
+Unicode scalar** — a surrogate, or a value above `U+10FFFF`.
+
+| Door | On ill-formed input | Consumes |
+|---|---|---|
+| `Rune::TryGetRuneAt` | reports `false` | the **sequence's own length** for a non-scalar, `1` for a structural break |
+| any `Encoding` | substitutes `U+FFFD` | always `1`, so one replacement per byte, as .NET's replacement `DecoderFallback` does |
+| `IdnMapping` | throws `ArgumentException` | — |
+
+Collapsing that difference would have been a silent behaviour change in whichever door lost, so
+it is a **parameter of the contract** rather than an accident of which copy you read:
+
+```cpp
+bool TryDecodeUtf8Scalar(const char* s, std::size_t size, std::size_t i,
+                         std::uint32_t& codePoint, std::size_t& length);   // reports
+void DecodeUtf8Scalar(const std::string& s, std::size_t i,
+                      std::uint32_t& codePoint, std::size_t& length);      // substitutes
+```
+
+`DecodeUtf8Scalar` is defined as `if (!Try(...)) { codePoint = 0xFFFD; length = 1; }` — five
+lines, and the substituting contract is now visible instead of being spread through five
+`return` statements.
+
+`IdnMapping`'s two hand-spelled extra rejections are subsumed exactly: a 2-byte lead below
+`0xC2` is an overlong encoding, and a 4-byte lead above `0xF4` encodes a value above `U+10FFFF`.
+
+## 3. The copy #2014 could not move
+
+`UTF8Encoding.cpp`'s `wellFormedUtf8Length` reads a `(pointer, end)` range rather than a
+`std::string`, and returns a validity **length** rather than a code point — its well-formed bytes
+pass through unchanged, so there is no re-encoding step to fold a substitution into. That was a
+real obstacle, and the answer is that the range is now a **parameter** of the shared decode
+rather than a reason to keep a sixth copy. `size` is an exclusive bound, so a caller decoding a
+sub-range gets a truncated sequence reported as ill-formed instead of read past.
+
+## 4. The test that did not exist
+
+`Rune::TryGetRuneAt` had **no test anywhere in the repository**, and a mutation collapsing its
+length onto the `Encoding` contract passed all 17,296 tests then present. That gap is what makes
+a refactor like this dangerous, and it is closed first:
+
+* `modules/text/tests/System/Text/Utf8SharedScalarDecodeTests.cpp`
+* `modules/globalization/tests/System/Globalization/IdnMappingUtf8DecodeTests.cpp`
+
+Two files rather than one because `modules/text` does not depend on `Globalization`, and a
+refactor is not a reason to add a public component edge. **The module graph is unchanged at
+41 modules / 92 edges.**
+
+| Mutation | Caught |
+|---|---|
+| Collapse the two contracts (a non-scalar reports length 1) | ✅ — **only after** the new pin; nothing caught it before |
+| The substituting form propagates the `Try` length | ✅ |
+| Drop the 3-byte branch's overlong rejection | ✅ — **only after** adding per-branch overlong rows; the 2-byte row alone missed it |
+| Drop the surrogate rejection | ✅ (three pre-existing suites) |
+| `UTF8Encoding` returns a length even on failure | ✅ |
+
+Two of the five needed a new assertion. Both are recorded here rather than quietly fixed, because
+they are the measurement of how much of this rule was untested while it was duplicated six ways.
+
+## 5. Net effect
+
+−179 lines. One definition. `System::Text::detail` still re-exports both entry points, so every
+caller written against the old spelling is unchanged.

docs/Utf8ScalarDecodeSingleDefinition.md

@@ -9,16 +9,14 @@
 namespace System::detail {
 
     /**
-     * @brief One UTF-8 scalar decode, in one place.
+     * @brief One UTF-8 scalar decode, in one place — the reporting form.
      *
      * This runtime stores `System::String` as UTF-8, so every component that converts *characters*
      * rather than *storage bytes* has to decode a scalar first — and by ticket #2014 five copies
-     * of that decode had accumulated across `modules/text`: `ASCIIEncoding.cpp`,
-     * `UTF8Encoding.cpp` (as a validity-length variant), `UnicodeEncoding.hpp`,
-     * `UTF32Encoding.hpp` and `Rune.hpp`. Adding a sixth for `Latin1Encoding` is exactly the
-     * duplication this repository keeps having to repair, so #2014 factored the rule out here and
-     * moved the two `.cpp` copies onto it. The three header-inline copies are a separate,
-     * recorded follow-up (**#2354**), because they are entangled with those types' own loops.
+     * of that decode had accumulated across `modules/text`. #2014 factored the rule out here and
+     * moved the two `.cpp` copies onto it; **ticket #2354 (2026-08-18) moved the last three**,
+     * which are header-inline and entangled with their types' own loops: `UnicodeEncoding.hpp`,
+     * `UTF32Encoding.hpp` and `Rune.hpp`. There is now exactly one definition.
      *
      * **It lives in `Core.Base` as of ticket #2106**, not in `modules/text`. `System::BinaryData`
      * needed the same decode to give `ToString()` .NET's replacement behaviour, and
@@ -27,40 +25,52 @@ namespace System::detail {
      * already depends on. `Core.Base` is that place. `System::Text::detail` re-exports these
      * names, so every existing caller is unchanged.
      *
-     * @param s          The UTF-8 text.
-     * @param i          Index of the first byte of the sequence; must be `< s.size()`.
-     * @param codePoint  Receives the decoded scalar, or `U+FFFD` for an ill-formed sequence.
-     * @param length     Receives the number of bytes consumed — always `1` for an ill-formed
-     *                   sequence, so a caller resumes one byte later and cannot loop forever.
+     * **Why there are two forms.** `Rune::TryGetRuneAt` must *report* an ill-formed sequence,
+     * while an `Encoding` must *substitute* `U+FFFD` and carry on; and the two disagree about
+     * one thing that a single function cannot express — how many bytes a **structurally valid**
+     * sequence that encodes a **non-scalar** (a surrogate, or a value above `U+10FFFF`) consumes.
+     * `Rune` reports the sequence's own length there, so a caller can skip the whole thing; the
+     * substituting form always reports `1`, so one replacement character is emitted per byte, as
+     * .NET's replacement fallback does. Collapsing that difference would have been a silent
+     * behaviour change in whichever door lost, so it is a parameter of the contract instead.
      *
-     * Conformance, unchanged from the copies it replaces: continuation bytes are validated,
-     * overlong encodings are rejected, and a surrogate or an out-of-range scalar decodes to
-     * `U+FFFD`.
+     * @param s          The UTF-8 bytes.
+     * @param size       Exclusive end of the readable range; a caller decoding a sub-range
+     *                   passes that sub-range's end, and a truncated sequence at the boundary
+     *                   is reported as ill-formed rather than read past.
+     * @param i          Index of the first byte of the sequence; must be `< size`.
+     * @param codePoint  Receives the decoded scalar; untouched unless the call returns `true`.
+     * @param length     Receives the number of bytes to advance by — the sequence's length on
+     *                   success; `1` for a structurally ill-formed sequence (bad lead byte, bad
+     *                   or missing continuation byte, overlong encoding), so a caller resumes one
+     *                   byte later and cannot loop forever; and the sequence's own length for a
+     *                   structurally valid encoding of a non-scalar value.
+     * @return @c true when a Unicode scalar was decoded.
      */
-    inline void DecodeUtf8Scalar(const std::string& s, std::size_t i,
-                                 std::uint32_t& codePoint, std::size_t& length) {
+    [[nodiscard]] inline bool TryDecodeUtf8Scalar(const char* s, std::size_t size, std::size_t i,
+                                                  std::uint32_t& codePoint, std::size_t& length) {
         auto isContinuation = [](unsigned char b) { return (b & 0xC0) == 0x80; };
         const unsigned char c0 = static_cast<unsigned char>(s[i]);
         std::uint32_t cp;
         std::size_t len;
         if (c0 < 0x80) {
             cp = c0;
             len = 1;
-        } else if ((c0 & 0xE0) == 0xC0 && i + 1 < s.size() &&
+        } else if ((c0 & 0xE0) == 0xC0 && i + 1 < size &&
                    isContinuation(static_cast<unsigned char>(s[i + 1]))) {
             cp = (static_cast<std::uint32_t>(c0 & 0x1F) << 6) |
                  (static_cast<unsigned char>(s[i + 1]) & 0x3F);
             len = 2;
-            if (cp < 0x80) { codePoint = 0xFFFD; length = 1; return; }
-        } else if ((c0 & 0xF0) == 0xE0 && i + 2 < s.size() &&
+            if (cp < 0x80) { length = 1; return false; }   // overlong
+        } else if ((c0 & 0xF0) == 0xE0 && i + 2 < size &&
                    isContinuation(static_cast<unsigned char>(s[i + 1])) &&
                    isContinuation(static_cast<unsigned char>(s[i + 2]))) {
             cp = (static_cast<std::uint32_t>(c0 & 0x0F) << 12) |
                  ((static_cast<unsigned char>(s[i + 1]) & 0x3F) << 6) |
                  (static_cast<unsigned char>(s[i + 2]) & 0x3F);
             len = 3;
-            if (cp < 0x800) { codePoint = 0xFFFD; length = 1; return; }
-        } else if ((c0 & 0xF8) == 0xF0 && i + 3 < s.size() &&
+            if (cp < 0x800) { length = 1; return false; }  // overlong
+        } else if ((c0 & 0xF8) == 0xF0 && i + 3 < size &&
                    isContinuation(static_cast<unsigned char>(s[i + 1])) &&
                    isContinuation(static_cast<unsigned char>(s[i + 2])) &&
                    isContinuation(static_cast<unsigned char>(s[i + 3]))) {
@@ -69,19 +79,45 @@ namespace System::detail {
                  ((static_cast<unsigned char>(s[i + 2]) & 0x3F) << 6) |
                  (static_cast<unsigned char>(s[i + 3]) & 0x3F);
             len = 4;
-            if (cp < 0x10000) { codePoint = 0xFFFD; length = 1; return; }
+            if (cp < 0x10000) { length = 1; return false; }  // overlong
         } else {
-            codePoint = 0xFFFD;
             length = 1;
-            return;
+            return false;
         }
         if (cp > 0x10FFFF || (cp >= 0xD800 && cp <= 0xDFFF)) {
-            codePoint = 0xFFFD;
-            length = 1;
-            return;
+            length = len;   // structurally valid, but not a scalar -- see the note above
+            return false;
         }
         codePoint = cp;
         length = len;
+        return true;
+    }
+
+    /** @brief `std::string` overload of the reporting form. */
+    [[nodiscard]] inline bool TryDecodeUtf8Scalar(const std::string& s, std::size_t i,
+                                                  std::uint32_t& codePoint, std::size_t& length) {
+        return TryDecodeUtf8Scalar(s.data(), s.size(), i, codePoint, length);
+    }
+
+    /**
+     * @brief One UTF-8 scalar decode — the substituting form every `Encoding` uses.
+     *
+     * Ill-formed input decodes to `U+FFFD` over a single byte, matching .NET's default
+     * `DecoderFallback`. See `TryDecodeUtf8Scalar` for why the length differs between the two
+     * forms on one specific class of input.
+     *
+     * @param s          The UTF-8 text.
+     * @param i          Index of the first byte of the sequence; must be `< s.size()`.
+     * @param codePoint  Receives the decoded scalar, or `U+FFFD` for an ill-formed sequence.
+     * @param length     Receives the number of bytes consumed — always `1` for an ill-formed
+     *                   sequence, so a caller resumes one byte later and cannot loop forever.
+     */
+    inline void DecodeUtf8Scalar(const std::string& s, std::size_t i,
+                                 std::uint32_t& codePoint, std::size_t& length) {
+        if (!TryDecodeUtf8Scalar(s, i, codePoint, length)) {
+            codePoint = 0xFFFD;
+            length    = 1;
+        }
     }
 
     /**

modules/core/include/System/detail/Utf8Scalar.hpp

@@ -2,6 +2,7 @@
 // Copyright (c) Robert Vokac and contributors
 // Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
 #include "System/Globalization/IdnMapping.hpp"
+#include "System/detail/Utf8Scalar.hpp"
 #include <algorithm>
 #include <stdexcept>
 #include <string>
@@ -14,59 +15,30 @@ namespace System::Globalization {
 // UTF-8 helpers
 // ---------------------------------------------------------------------------
 
-namespace {
-    // A continuation byte must match the 10xxxxxx bit pattern (RFC 3629).
-    bool isContinuationByte(unsigned char c) { return (c & 0xC0) == 0x80; }
-}
-
 std::u32string IdnMapping::utf8ToCodePoints(const std::string& s) {
     std::u32string out;
     out.reserve(s.size());
+    // Each branch here previously trusted the continuation bytes' low 6 bits without checking
+    // that their top 2 bits are 10xxxxxx, and never rejected overlong encodings, surrogate code
+    // points, or out-of-range results -- so malformed UTF-8 was silently misinterpreted as some
+    // other, unrelated code point. Confirmed with a standalone repro: "\xC2\x41" (a valid
+    // 2-byte lead byte followed by 'A') produced a garbage Punycode result with no exception.
+    // That matters here specifically, since domain names routinely originate from untrusted
+    // input.
+    //
+    // Ticket #2354 (2026-08-18) found this copy, which the ticket itself did not name. It is a
+    // THIRD contract over the same rule -- Rune reports, an Encoding substitutes, and this one
+    // throws -- and all three are now spellings of System/detail/Utf8Scalar.hpp's one decode.
+    // The two extra rejections this copy spelled out by hand are subsumed exactly: a 2-byte
+    // lead below 0xC2 is an overlong encoding, and a 4-byte lead above 0xF4 is above U+10FFFF.
     size_t i = 0;
     while (i < s.size()) {
-        unsigned char c = static_cast<unsigned char>(s[i]);
-        char32_t cp;
-        // Each branch below previously trusted the continuation bytes' low 6 bits without
-        // checking their top 2 bits are actually 10xxxxxx, and never rejected overlong
-        // encodings, surrogate code points, or out-of-Unicode-range results -- so malformed
-        // UTF-8 (e.g. a valid lead byte followed by an ordinary ASCII byte instead of a real
-        // continuation byte) was silently misinterpreted as some other, unrelated code point
-        // instead of being rejected. Confirmed via a standalone repro before this fix: "\xC2\x41"
-        // (a valid 2-byte lead byte followed by 'A', not a continuation byte) produced a
-        // garbage Punycode result with no exception at all. This matters for IdnMapping
-        // specifically since domain names routinely originate from untrusted input.
-        if (c < 0x80) {
-            cp = c; ++i;
-        } else if ((c & 0xE0) == 0xC0) {
-            if (c < 0xC2 || i + 1 >= s.size() || !isContinuationByte(static_cast<unsigned char>(s[i+1])))
-                throw System::ArgumentException("IdnMapping: invalid UTF-8 sequence.");
-            cp = (char32_t(c & 0x1F) << 6) | (static_cast<unsigned char>(s[i+1]) & 0x3F);
-            i += 2;
-        } else if ((c & 0xF0) == 0xE0) {
-            if (i + 2 >= s.size() ||
-                !isContinuationByte(static_cast<unsigned char>(s[i+1])) ||
-                !isContinuationByte(static_cast<unsigned char>(s[i+2])))
-                throw System::ArgumentException("IdnMapping: invalid UTF-8 sequence.");
-            cp = (char32_t(c & 0x0F) << 12) | ((static_cast<unsigned char>(s[i+1]) & 0x3F) << 6)
-               | (static_cast<unsigned char>(s[i+2]) & 0x3F);
-            if (cp < 0x800 || (cp >= 0xD800 && cp <= 0xDFFF))
-                throw System::ArgumentException("IdnMapping: invalid UTF-8 sequence.");
-            i += 3;
-        } else if ((c & 0xF8) == 0xF0) {
-            if (c > 0xF4 || i + 3 >= s.size() ||
-                !isContinuationByte(static_cast<unsigned char>(s[i+1])) ||
-                !isContinuationByte(static_cast<unsigned char>(s[i+2])) ||
-                !isContinuationByte(static_cast<unsigned char>(s[i+3])))
-                throw System::ArgumentException("IdnMapping: invalid UTF-8 sequence.");
-            cp = (char32_t(c & 0x07) << 18) | ((static_cast<unsigned char>(s[i+1]) & 0x3F) << 12)
-               | ((static_cast<unsigned char>(s[i+2]) & 0x3F) << 6) | (static_cast<unsigned char>(s[i+3]) & 0x3F);
-            if (cp < 0x10000 || cp > 0x10FFFF)
-                throw System::ArgumentException("IdnMapping: invalid UTF-8 sequence.");
-            i += 4;
-        } else {
+        std::uint32_t cp  = 0;
+        std::size_t   len = 0;
+        if (!System::detail::TryDecodeUtf8Scalar(s, i, cp, len))
             throw System::ArgumentException("IdnMapping: invalid UTF-8 sequence.");
-        }
-        out.push_back(cp);
+        out.push_back(static_cast<char32_t>(cp));
+        i += len;
     }
     return out;
 }