Skip to content

Commit efd1c14

Browse files
committed
fix(uri): UriTypeConverter::ConvertFrom returns std::optional<Uri> (#1999)
Rule-14 sweep. The by-value Uri return type cannot express .NET's null, so ConvertFrom("") forwarded the empty string straight to the Uri constructor and threw where .NET returns null (UriTypeConverter.cs:40-51). The recorded gate and the recorded cost are both corrected. Section 14.5 wrote an approval sentence because this is "a vtable-slot signature change"; SA-10, granted after that record, names RETURN TYPE explicitly and routes it through SA-2's five conditions -- SA-3's exclusion is a change to the vtable's SHAPE, not a signature within an existing slot. And the record's "mandatory migration for every override" is measurably zero: there are no overrides and no derivations anywhere, in-tree or downstream, so the migration was four test call sites. The widening is exactly one input wide, and that is .NET's own boundary -- its comment says the malformed case is left to the constructor. A separate pin asserts it, because "return the empty state on any failure" is the plausible over-correction. The kind is now spelled RelativeOrAbsolute, matching the reference. It is behaviourally identical to the one-argument Uri(text) this used to call, so the change is documentation rather than behaviour, and the note says so. Four mutations, all caught. M4 only after a case was added: every test converted an absolute URI, so requiring UriKind::Absolute never failed -- a relative input is the only one the kind discriminates, which is exactly why .NET passes RelativeOrAbsolute. M2 was invalid as first written (-Werror=unused-parameter) and was reformulated rather than counted. Fixture set 43/223 -> 44/226. Site 3's diagnostic is "invalid covariant return type" rather than the "does not override" predicted, and it is the more precise of the two. Downstream measured: 0 sites in cna, 0 in mobile-eggbert. Gate: 17,517 run, 17,517 passed, 0 failed, 0 skipped across 38 executables (+2 on 17,515; SharpRuntimeTests_Uri 287 -> 289; no other executable moved). Module graph unchanged at 41/93.
1 parent 4d5e897 commit efd1c14

6 files changed

Lines changed: 284 additions & 12 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `UriTypeConverter::ConvertFrom` returns `std::optional<Uri>` (ticket #1999)
5+
6+
*2026-08-19.* `System::UriTypeConverter::ConvertFrom` returns `std::optional<Uri>` instead of
7+
`Uri`, so an **empty** input returns the empty state rather than throwing.
8+
9+
**This is a public virtual signature change**, landed under **SA-10** with SA-2's five conditions
10+
discharged.
11+
12+
---
13+
14+
## 1. What was wrong
15+
16+
The return type was a by-value `Uri`, which **cannot express .NET's `null`**. So an empty string
17+
was forwarded straight to the `Uri` constructor and threw `UriFormatException`. .NET
18+
short-circuits it:
19+
20+
```csharp
21+
if (value is string uriString)
22+
{
23+
if (string.IsNullOrEmpty(uriString))
24+
{
25+
return null;
26+
}
27+
28+
// Let the Uri constructor throw any informative exceptions
29+
return new Uri(uriString, UriKind.RelativeOrAbsolute);
30+
} // UriTypeConverter.cs:40-51
31+
```
32+
33+
## 2. The widening is exactly one input wide
34+
35+
.NET's own comment — *"Let the Uri constructor throw any informative exceptions"* — says the empty
36+
case is the **only** one it short-circuits. A malformed string still throws.
37+
38+
That boundary is pinned by its own test, because "return the empty state on any failure" is the
39+
plausible over-correction, and it is mutation M3.
40+
41+
## 3. Why SA-10 and not an approval
42+
43+
The design record (`docs/SystemUriNamespaceReviewPlan.md` §14.5) called this *"a vtable-slot
44+
signature change, plus mandatory migration for every override"* and wrote an approval sentence.
45+
Two things have changed since:
46+
47+
* **SA-10 was granted afterwards**, and its list names **"return type"** explicitly, routing such
48+
a change through SA-2's five conditions rather than a fresh ask. SA-3's exclusion is a change to
49+
the vtable's **shape** — adding or removing a virtual, or changing the base class — not a
50+
signature within an existing slot.
51+
* **The migration cost is measurably zero.** There are **no overrides and no derivations** of
52+
`UriTypeConverter` anywhere: not in `modules/`, not in `test/` or `tests/`, and not in either
53+
downstream consumer. The only call sites were four in this repository's own tests.
54+
55+
What a *future* override would lose is pinned as site 3 of the negative fixture.
56+
57+
## 4. To migrate
58+
59+
```cpp
60+
Uri uri = converter.ConvertFrom(text); // was
61+
auto uri = converter.ConvertFrom(text); // now
62+
if (uri) { /* … uri->getHostProperty() … */ }
63+
```
64+
65+
An empty `text` no longer throws — it yields an empty optional.
66+
67+
## 5. One detail made explicit rather than changed
68+
69+
The kind is now spelled `UriKind::RelativeOrAbsolute`, matching the reference. It is
70+
**behaviourally identical** to the one-argument `Uri(text)` this used to call: with that kind, both
71+
guards in `Uri(string, UriKind)` are inert and it simply parses. The change is documentation, not
72+
behaviour — stated so a reader does not look for an effect that is not there.
73+
74+
## 6. Evidence
75+
76+
Four mutations, **all caught**:
77+
78+
| Mutation | Caught by |
79+
|---|---|
80+
| M1 — the empty input is no longer short-circuited | `Fix1999_ConvertFromEmptyReturnsTheEmptyStateInsteadOfThrowing` |
81+
| M2 — every input returns the empty state | four cases |
82+
| M3 — a *malformed* input is short-circuited too | `Decl1999_OnlyTheEmptyInputIsShortCircuited` |
83+
| M4 — the kind becomes `Absolute` | `Fix1999_ARelativeUriIsAccepted` — **only after that case was added** |
84+
85+
**M3** is the plausible over-correction — "return the empty state on any failure" — and the reason
86+
§2's boundary has a test of its own.
87+
88+
**M4 is the one worth recording.** It went **uncaught** at first: every case converted an
89+
*absolute* URI, so requiring `UriKind::Absolute` never failed. A relative input is the only one
90+
the kind discriminates, which is precisely why .NET passes `RelativeOrAbsolute` — and the new case
91+
also shows the relative URI round-trips, which is why `ConvertTo` uses `OriginalString`.
92+
93+
M2 was invalid as first written — it left `text` unused and `-Werror=unused-parameter` rejected it
94+
— and was reformulated rather than counted.
95+
96+
Negative consumer fixture: `test/consumer/uri_typeconverter_optional_negative.cpp`, three sites,
97+
all rejected. Fixture set grows to **44 fixtures / 226 sites**. Site 2 is the spelling most likely
98+
to survive a careless migration — calling straight through the result, which used to be a `Uri`.
99+
100+
Site 3's diagnostic is **"invalid covariant return type"** rather than the "does not override" I
101+
predicted, and it is the more precise of the two: gcc reads the old signature as an *attempted
102+
covariant override* of the new one and rejects it on the spot, naming the reason.
103+
104+
Gate: **17,517 run, 17,517 passed, 0 failed, 0 skipped** across 38 executables — `+2` on 17,515
105+
(`SharpRuntimeTests_Uri` 287 → 289; one pin inverted in place, two cases added). No other
106+
executable moved. Module graph unchanged at 41/93.
107+
108+
## 7. Downstream, measured
109+
110+
`UriTypeConverter` appears in **zero** places in `cna` and **zero** in `mobile-eggbert`. Neither
111+
repository was modified, and no downstream ticket is needed.

modules/uri/include/System/UriTypeConverter.hpp

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Copyright (c) Robert Vokac and contributors
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#pragma once
5+
#include <optional>
56
#include <string>
67
#include "System/Uri.hpp"
78

@@ -42,13 +43,39 @@ namespace System {
4243
/**
4344
* @brief Converts a string to a Uri.
4445
*
45-
* C++ counterpart of .NET UriTypeConverter.ConvertFrom(ITypeDescriptorContext, CultureInfo, object).
46+
* C++ counterpart of .NET UriTypeConverter.ConvertFrom(ITypeDescriptorContext,
47+
* CultureInfo, object).
48+
*
4649
* @param text The string representation of a URI.
47-
* @return A Uri constructed from @p text.
48-
* @throws System::UriFormatException if @p text is empty or malformed.
50+
* @return A Uri constructed from @p text, or **`std::nullopt` when @p text is empty**.
51+
* @throws System::UriFormatException if @p text is malformed.
52+
*
53+
* Ticket #1999 / SR-AUD-148 (U-I). The return type was a by-value `Uri`, which **cannot
54+
* express .NET's `null`**, so an empty string was forwarded straight to the `Uri`
55+
* constructor and threw `UriFormatException`. .NET returns null:
56+
* @code
57+
* if (value is string uriString)
58+
* {
59+
* if (string.IsNullOrEmpty(uriString))
60+
* {
61+
* return null;
62+
* }
63+
* // Let the Uri constructor throw any informative exceptions
64+
* return new Uri(uriString, UriKind.RelativeOrAbsolute);
65+
* } // UriTypeConverter.cs:40-51
66+
* @endcode
67+
* The empty case is the ONLY one .NET short-circuits -- its comment says outright that a
68+
* malformed string is left to the constructor -- so `std::optional` widens exactly one
69+
* input and nothing else.
70+
*
71+
* @note The kind is spelled **explicitly** as `RelativeOrAbsolute`, matching the
72+
* reference. It is behaviourally identical to the one-argument `Uri(text)` this used to
73+
* call -- with that kind the two guards in `Uri(string, UriKind)` are both inert -- so
74+
* the change is documentation rather than behaviour.
4975
*/
50-
[[nodiscard]] virtual Uri ConvertFrom(const std::string& text) const {
51-
return Uri(text);
76+
[[nodiscard]] virtual std::optional<Uri> ConvertFrom(const std::string& text) const {
77+
if (text.empty()) return std::nullopt;
78+
return Uri(text, UriKind::RelativeOrAbsolute);
5279
}
5380

5481
/**

modules/uri/tests/System/UriTypeConverterTests.cpp

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Copyright (c) Robert Vokac and contributors
33
// Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
44
#include <gtest/gtest.h>
5+
#include <optional>
56
#include "System/UriFormatException.hpp"
67
#include "System/UriTypeConverter.hpp"
78

@@ -19,14 +20,44 @@ TEST(UriTypeConverterTest, CanConvertTo) {
1920
}
2021

2122
TEST(UriTypeConverterTest, ConvertFromString) {
23+
// MIGRATED by #1999: ConvertFrom returns std::optional<Uri>, because a by-value Uri cannot
24+
// express .NET's null.
2225
UriTypeConverter c;
23-
Uri uri = c.ConvertFrom("http://example.com");
24-
EXPECT_EQ(uri.getHostProperty(), "example.com");
26+
auto uri = c.ConvertFrom("http://example.com");
27+
ASSERT_TRUE(uri.has_value());
28+
EXPECT_EQ(uri->getHostProperty(), "example.com");
2529
}
2630

27-
TEST(UriTypeConverterTest, ConvertFromEmptyThrows) {
31+
TEST(UriTypeConverterTest, Fix1999_ConvertFromEmptyReturnsTheEmptyStateInsteadOfThrowing) {
32+
// INVERTED by #1999 / SR-AUD-148. This asserted the divergence: an empty string was forwarded
33+
// straight to the Uri constructor and threw. .NET short-circuits it and returns null --
34+
// "if (string.IsNullOrEmpty(uriString)) { return null; }" (UriTypeConverter.cs:44-47).
2835
UriTypeConverter c;
29-
EXPECT_THROW(c.ConvertFrom(""), System::UriFormatException);
36+
std::optional<Uri> result;
37+
EXPECT_NO_THROW(result = c.ConvertFrom(""));
38+
EXPECT_FALSE(result.has_value());
39+
}
40+
41+
TEST(UriTypeConverterTest, Fix1999_ARelativeUriIsAccepted) {
42+
// Added after mutation M4 -- "use UriKind::Absolute" -- went UNCAUGHT: every other case
43+
// converts an ABSOLUTE URI, so requiring Absolute never failed. A relative input is the only
44+
// one the kind discriminates, and .NET passes UriKind.RelativeOrAbsolute precisely so this
45+
// works (UriTypeConverter.cs:50).
46+
UriTypeConverter c;
47+
std::optional<Uri> relative;
48+
EXPECT_NO_THROW(relative = c.ConvertFrom("/path/to/resource"));
49+
ASSERT_TRUE(relative.has_value());
50+
EXPECT_FALSE(relative->getIsAbsoluteUriProperty());
51+
EXPECT_EQ(c.ConvertTo(*relative), "/path/to/resource")
52+
<< "and it round-trips, which is why ConvertTo uses OriginalString";
53+
}
54+
55+
TEST(UriTypeConverterTest, Decl1999_OnlyTheEmptyInputIsShortCircuited) {
56+
// The widening is exactly one input wide, and that is .NET's own boundary: its comment says
57+
// "Let the Uri constructor throw any informative exceptions", so a MALFORMED string must
58+
// still throw rather than return the empty state.
59+
UriTypeConverter c;
60+
EXPECT_THROW((void)c.ConvertFrom("http://exa mple.com/"), System::UriFormatException);
3061
}
3162

3263
TEST(UriTypeConverterTest, ConvertToString) {
@@ -40,8 +71,9 @@ TEST(UriTypeConverterTest, RoundTrip) {
4071
UriTypeConverter c;
4172
Uri original("https://test.org/api");
4273
std::string s = c.ConvertTo(original);
43-
Uri restored = c.ConvertFrom(s);
44-
EXPECT_EQ(restored.getHostProperty(), "test.org");
74+
auto restored = c.ConvertFrom(s);
75+
ASSERT_TRUE(restored.has_value());
76+
EXPECT_EQ(restored->getHostProperty(), "test.org");
4577
}
4678

4779
// ---------------------------------------------------------------------------

plan.sqlite3

0 Bytes
Binary file not shown.
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
// SPDX-License-Identifier: MIT
2+
// Copyright (c) Robert Vokac and contributors
3+
//
4+
// Negative compile fixture for ticket #1999 (SR-AUD-148, cause U-I).
5+
//
6+
// #1999 changed System::UriTypeConverter::ConvertFrom's return type from `Uri` to
7+
// `std::optional<Uri>`. The by-value Uri CANNOT EXPRESS .NET's null, so an empty string was
8+
// forwarded straight to the Uri constructor and threw UriFormatException. .NET returns null:
9+
//
10+
// if (value is string uriString)
11+
// {
12+
// if (string.IsNullOrEmpty(uriString)) { return null; }
13+
// // Let the Uri constructor throw any informative exceptions
14+
// return new Uri(uriString, UriKind.RelativeOrAbsolute);
15+
// } // UriTypeConverter.cs:40-51
16+
//
17+
// The empty case is the ONLY one .NET short-circuits, so the widening is exactly one input wide.
18+
//
19+
// This is a public VIRTUAL signature change, which is why it lands under SA-10 (whose list names
20+
// "return type" explicitly) with SA-2's five conditions, rather than under SA-3 -- SA-3's
21+
// exclusion is a change to the vtable's SHAPE (adding or removing a virtual, or changing the
22+
// base), not a signature within an existing slot. Measured, there are ZERO overrides and ZERO
23+
// derivations anywhere, so the design record's "mandatory migration for every override" cost is
24+
// zero; what a future override would lose is site 3 below.
25+
//
26+
// Migration: bind the result to `auto` (or `std::optional<Uri>`) and test `has_value()`.
27+
//
28+
// Records: docs/Migration-UriTypeConverterOptional.md,
29+
// docs/NegativeConsumerFixtureValidation.md.
30+
//
31+
// NEGATIVE-FIXTURE: component=Uri
32+
#include <optional>
33+
#include <string>
34+
#include <type_traits>
35+
36+
#include "System/UriTypeConverter.hpp"
37+
38+
#ifndef SHARP_RUNTIME_NEGATIVE_SITE
39+
#define SHARP_RUNTIME_NEGATIVE_SITE 0
40+
#endif
41+
42+
using System::Uri;
43+
using System::UriTypeConverter;
44+
45+
int main() {
46+
const UriTypeConverter converter;
47+
48+
#if SHARP_RUNTIME_NEGATIVE_SITE == 1
49+
// NEGATIVE(uritypeconverter-by-value-result): conversion from
50+
// | cannot convert
51+
// | no viable conversion
52+
Uri byValue = converter.ConvertFrom("http://example.com");
53+
(void)byValue;
54+
#else
55+
auto byValue = converter.ConvertFrom("http://example.com");
56+
(void)byValue;
57+
#endif
58+
59+
#if SHARP_RUNTIME_NEGATIVE_SITE == 2
60+
// NEGATIVE(uritypeconverter-direct-member-access): base operand of '->' has non-pointer type
61+
// | no member named
62+
// | base operand of
63+
// THE SPELLING MOST LIKELY TO SURVIVE A CARELESS MIGRATION: calling straight through the
64+
// result, which used to be a Uri and is now an optional.
65+
const std::string host = converter.ConvertFrom("http://example.com").getHostProperty();
66+
(void)host;
67+
#else
68+
const std::string host = converter.ConvertFrom("http://example.com")->getHostProperty();
69+
(void)host;
70+
#endif
71+
72+
#if SHARP_RUNTIME_NEGATIVE_SITE == 3
73+
// NEGATIVE(uritypeconverter-override-old-signature): invalid covariant return type
74+
// | does not override
75+
// | marked 'override', but does not override
76+
// What a FUTURE override loses. Measured, there are none today -- in this repository or in
77+
// either downstream consumer -- which is why the recorded migration cost was zero.
78+
//
79+
// The diagnostic is "invalid covariant return type" rather than "does not override", and that
80+
// is more precise than expected: gcc reads the old signature as an ATTEMPTED covariant
81+
// override of the new one and rejects it on the spot, naming the reason.
82+
struct MyConverter final : UriTypeConverter {
83+
Uri ConvertFrom(const std::string& text) const override { return Uri(text); }
84+
};
85+
(void)sizeof(MyConverter);
86+
#else
87+
struct MyConverter final : UriTypeConverter {
88+
std::optional<Uri> ConvertFrom(const std::string& text) const override {
89+
return text.empty() ? std::optional<Uri>{} : std::optional<Uri>{Uri(text)};
90+
}
91+
};
92+
(void)sizeof(MyConverter);
93+
#endif
94+
95+
// UNCHANGED, and asserted so the fixture proves what did NOT break: ConvertTo still returns a
96+
// string by value, the type is still polymorphic, and the empty input no longer throws.
97+
static_assert(std::is_same_v<decltype(converter.ConvertTo(std::declval<const Uri&>())),
98+
std::string>,
99+
"#1999 must not have touched ConvertTo");
100+
static_assert(std::is_polymorphic_v<UriTypeConverter>, "still polymorphic");
101+
return converter.ConvertFrom("").has_value() ? 1 : 0;
102+
}

0 commit comments

Comments
 (0)