|
| 1 | +<!-- SPDX-License-Identifier: MIT --> |
| 2 | +<!-- Copyright (c) Robert Vokac and contributors --> |
| 3 | + |
| 4 | +# `UriParser` gains `Register`, and its hooks become `protected` — #1997 group A-4 (SR-AUD-146) |
| 5 | + |
| 6 | +**This is a public source break and a vtable and layout change.** It lands under **SA-2** (five |
| 7 | +conditions, all discharged below) and **SA-15.3**, which lifted SA-3's exclusion of vtable changes |
| 8 | +on 2026-08-20. It is the change that exclusion was blocking. |
| 9 | + |
| 10 | +## The defect, in three parts |
| 11 | + |
| 12 | +`System::UriParser` is the extensibility point for custom URI schemes. This port had it in a state |
| 13 | +where **it could not extend anything**: |
| 14 | + |
| 15 | +1. **`Register` did not exist at all.** There was no way to register a custom parser, which is the |
| 16 | + type's entire purpose. A subclass could be written and then had nowhere to go — the same shape |
| 17 | + #1997 group A-3 found for `UriCreationOptions`, one member over. |
| 18 | +2. **Three override hooks were `public`** where .NET's are `protected` — `GetComponents`, |
| 19 | + `IsBaseOf`, `IsWellFormedOriginalString`. They exist to be **overridden**, never **called**, and |
| 20 | + .NET says so in a comment of its own, describing its internal forwarders as existing *"to avoid |
| 21 | + `protected internal` signatures in the public docs"* (`UriSyntax.cs:245-246`). Published as |
| 22 | + public, any caller holding a `UriParser&` could invoke another parser's hook directly. |
| 23 | +3. **`OnRegister` was absent**, so even if a parser could be registered it could not observe its |
| 24 | + own registration. |
| 25 | + |
| 26 | +## What landed |
| 27 | + |
| 28 | +| Member | .NET | Before | After | |
| 29 | +|---|---|---|---| |
| 30 | +| `Register(parser, scheme, port)` | `public static` | **absent** | present, and **observable** | |
| 31 | +| `IsKnownScheme(scheme)` | `public static` | present | present, now consults the registry | |
| 32 | +| `OnRegister(scheme, port)` | `protected virtual` | **absent** | present, empty base body | |
| 33 | +| `GetComponents` | `protected virtual` | **public** | `protected` | |
| 34 | +| `IsBaseOf` | `protected virtual` | **public** | `protected` | |
| 35 | +| `IsWellFormedOriginalString` | `protected virtual` | **public** | `protected` | |
| 36 | +| `SchemeName` | `internal` | absent | `protected` accessor, private field | |
| 37 | + |
| 38 | +`sizeof(System::UriParser)` **8 → 48** (vptr + `std::string scheme_` + `intcs port_`), `alignof` 8. |
| 39 | +**Every consumer deriving from `UriParser` must rebuild.** Measured: **zero** derivations in `cna` |
| 40 | +and `mobile-eggbert`, and zero mentions of the type in either. |
| 41 | + |
| 42 | +## The registration is observable, which is the whole point |
| 43 | + |
| 44 | +A `Register` that validated its arguments and stored into a table nothing reads would be |
| 45 | +**accepted and ignored** — the SR-AUD-168 defect this repository keeps finding. After a successful |
| 46 | +call, **`IsKnownScheme(schemeName)` answers `true`**. That is .NET's own linkage: `Register` reaches |
| 47 | +`s_table` through `FetchSyntax`, and `IsKnownScheme` reads the same table through `GetSyntax`. |
| 48 | +Mutation M1 breaks that link and is caught by four cases. |
| 49 | + |
| 50 | +`Register` takes a **`std::shared_ptr<UriParser>`** rather than a reference, because .NET's static |
| 51 | +table holds a strong reference for the life of the process and entries are never removed. A raw |
| 52 | +pointer would leave the registry holding something the caller may destroy, and a reference could |
| 53 | +not express the null argument .NET rejects. |
| 54 | + |
| 55 | +## Rules transcribed rather than derived |
| 56 | + |
| 57 | +* **A one-character scheme is refused, though it is a valid scheme name.** .NET writes |
| 58 | + `ArgumentOutOfRangeException.ThrowIfEqual(schemeName.Length, 1)` *and* calls |
| 59 | + `Uri.CheckSchemeName`, which accepts a single letter — the two rules disagree on exactly one |
| 60 | + input and the narrower one runs first. Deriving the check from `CheckSchemeName` alone silently |
| 61 | + accepts `Register(p, "a", 80)`. Both halves are asserted in one case so the asymmetry reads as |
| 62 | + deliberate. |
| 63 | +* **The port range is `0..65535` plus the single sentinel `-1`.** .NET's test is |
| 64 | + `(uint)defaultPort > 0xFFFF && defaultPort != -1`, and **the cast is the rule**: every other |
| 65 | + negative value becomes a very large unsigned number and is rejected. A naive `port > 0xFFFF` |
| 66 | + accepts `-2`; mutation M4 writes exactly that and is caught. |
| 67 | +* **`OnRegister` runs before the scheme is stored.** .NET assigns `syntax._scheme` on the line |
| 68 | + *after* the callback (`UriSyntax.cs:175-176`), so during it the parser does not yet know its own |
| 69 | + scheme — which is what makes the parameter load-bearing rather than a convenience. Mutation M2 |
| 70 | + reorders the two and is caught. |
| 71 | +* **Two distinct `InvalidOperationException`s, because they are two different questions:** has |
| 72 | + *this parser* already been registered (`net_uri_NeedFreshParser`), and has *this scheme* already |
| 73 | + been taken (`net_uri_AlreadyRegistered`). Both texts are .NET's verbatim. Collapsing them would |
| 74 | + leave a caller unable to tell which of the two mistakes they made. |
| 75 | + |
| 76 | +## A mistake of my own, recorded rather than quietly fixed |
| 77 | + |
| 78 | +.NET keeps built-ins and customs in **one** table, so its single `oldSyntax != null` test refuses |
| 79 | +`Register(p, "http", 80)` by the same statement that refuses a repeated custom scheme. This port |
| 80 | +**must** split them — it has no parser objects for the sixteen built-ins — and that split makes it |
| 81 | +possible to check only one of the two. **A first cut of this ticket did exactly that and would have |
| 82 | +let a caller claim `gopher`.** The split is a permanent feature of this port's shape, so the same |
| 83 | +mistake is available to every later change; mutation M5 removes the built-in check and is caught, |
| 84 | +and the test that catches it says why it exists. |
| 85 | + |
| 86 | +## Migration |
| 87 | + |
| 88 | +The three hooks are now `protected`, so a subclass that wants its own hook reachable from outside |
| 89 | +**publishes a forwarder** — which is exactly what .NET's `InternalGetComponents`, |
| 90 | +`InternalIsBaseOf` and friends are. The one first-party site (`modules/uri/tests`) was migrated |
| 91 | +that way: |
| 92 | + |
| 93 | +```cpp |
| 94 | +class TestParser final : public UriParser { |
| 95 | +public: |
| 96 | + bool CallIsBaseOf(const Uri& b, const Uri& r) { return IsBaseOf(b, r); } // was: p.IsBaseOf(...) |
| 97 | +}; |
| 98 | +``` |
| 99 | +
|
| 100 | +## SA-2's five conditions |
| 101 | +
|
| 102 | +1. **Migration note** — this file. |
| 103 | +2. **Per-spelling negative consumer fixture** — |
| 104 | + `test/consumer/uri_parser_protected_hooks_negative.cpp`, **5 sites**. Set grows **52 / 264 → |
| 105 | + 53 / 269**. Site 2 is `IsWellFormedOriginalString`, the hook most likely to survive a careless |
| 106 | + migration because a caller who only wanted *"does this parser accept the string"* has no reason |
| 107 | + to think of it as an override point. |
| 108 | +3. **Downstream ticket** — not needed: measured **zero** `UriParser` sites in either consumer, so |
| 109 | + there is nothing to file. Recorded here rather than left unstated. |
| 110 | +4. **Full gate** — **17,689 / 38, 0 failed, 0 skipped** (+6; `SharpRuntimeTests_Uri` 308 → 314; no |
| 111 | + other executable moved). Module graph unchanged at **41 / 94**. |
| 112 | +5. **Measured impact against `cna` and `mobile-eggbert`** — zero sites in both, neither edited. |
| 113 | +
|
| 114 | +## SA-15.3's fourth condition |
| 115 | +
|
| 116 | +**Enumerate every `catch` clause whose meaning changes: there are none.** No exception type was |
| 117 | +introduced, reparented or removed; `Register` throws `ArgumentNullException`, |
| 118 | +`ArgumentOutOfRangeException` and `InvalidOperationException`, all pre-existing and all reached |
| 119 | +from a member that did not exist before, so no existing clause can change what it receives. |
| 120 | +
|
| 121 | +## What is deliberately still absent, and why |
| 122 | +
|
| 123 | +.NET declares four more `protected virtual` hooks: `OnNewUri`, `InitializeAndValidate`, `Resolve` |
| 124 | +and (already present) the three above. They are **not** added: |
| 125 | +
|
| 126 | +* **`OnNewUri`** would be an override point with no caller — this port's `System::Uri` performs its |
| 127 | + own parse and never consults a `UriParser`, so nothing would ever invoke it. |
| 128 | +* **`InitializeAndValidate` and `Resolve`** take an `out UriFormatException` parameter with no C++ |
| 129 | + counterpart that is not invented, and .NET's `InitializeAndValidate` body reaches `uri._syntax` |
| 130 | + — private state this port's `Uri` does not have. |
| 131 | +
|
| 132 | +**That nothing calls the hooks at all is declared in the header rather than left to be discovered.** |
| 133 | +`OnRegister` is the single exception and the only one: `Register` calls it, so a subclass really can |
| 134 | +observe its own registration. That is the difference between an extensibility point and a shape. |
| 135 | +
|
| 136 | +## Evidence |
| 137 | +
|
| 138 | +Eight mutations, **all caught**. M8 — republishing a hook — is **invisible to gtest**, because a |
| 139 | +widened hook behaves identically wherever both spellings compile; it is caught by the negative |
| 140 | +fixture, sites 2 and 5, verified by running the checker with the mutation applied. |
| 141 | +
|
| 142 | +**One mutation verdict was harness noise and is recorded rather than counted.** A "clean" re-run of |
| 143 | +M7 asserted on a substring that occurs **twice** (`toLowerInvariant` is called from both `Register` |
| 144 | +and `IsKnownScheme`), so the edit never applied and the run reported NOT CAUGHT. Re-targeted at |
| 145 | +`Register`'s call alone it is caught. An ambiguous anchor is a harness state, not a finding. |
0 commit comments