Skip to content

Commit 52d2edd

Browse files
committed
fix(core): IsCompatibilitySwitchSet consults the registry (#2250, SR-AUD-103)
It returned false unconditionally, without consulting the AppContext switch registry at all -- so a switch a caller had explicitly SET TO TRUE still reported as unset. Transcribed from AppDomain.cs:171-174. Landed under SA-10, which covers both changes: a return type and a noexcept are exactly what it names. NEITHER CHANGE COULD LAND WITHOUT THE OTHER, which is why the ticket was gated on both. The nullable return is not stylistic: a C++ bool cannot distinguish an explicitly-FALSE switch from an UNSET one, which is the entire reason .NET's is bool?. Keeping bool would have made the forward pointless -- both states would still have collapsed to false. The noexcept drop is not a relaxation: AppContext::TryGetSwitch raises for an empty name and takes a mutex whose lock() can throw, so forwarding from a noexcept member would have turned BOTH into std::terminate. The drop is the only safe way to forward at all, and the empty-name diagnostic now reaches the caller where the unconditional false swallowed it. Implementation note: the body is out of line in AppDomain.cpp, for the same reason SetData/GetData already are -- AppContext.hpp includes AppDomain.hpp for BaseDirectory, so the include cannot run the other way. Adding it to the header produced exactly that cycle, and the existing pattern was there to follow. Both gated pins were written for this moment and are inverted; a third case was added for the distinction the nullable return exists for. Fixture set: 33 fixtures / 187 sites. Gate 17,284 run, 0 failed.
1 parent ba15593 commit 52d2edd

8 files changed

Lines changed: 232 additions & 30 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

audit/AUDIT_FINDINGS_INDEX.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `IsCompatibilitySwitchSet` consults the registry (ticket #2250)
5+
6+
*2026-08-18.* `AppDomain::IsCompatibilitySwitchSet` returned `false` unconditionally, without
7+
consulting the `AppContext` switch registry at all — so a switch a caller had explicitly **set to
8+
true** still reported as unset.
9+
10+
Landed under `docs/StandingApprovals.md` **SA-10** with SA-2's five conditions. **Two changes had
11+
to land together.**
12+
13+
---
14+
15+
## 1. What changed
16+
17+
| | Was | Is |
18+
|---|---|---|
19+
| the body | `return false;` | .NET's `TryGetSwitch(value, out result) ? result : default(bool?)` |
20+
| return type | `bool` | **`std::optional<bool>`** |
21+
| `noexcept` | yes | **no** |
22+
| an explicitly-**false** switch | `false` | `std::optional<bool>(false)` |
23+
| a switch **never set** | `false` — indistinguishable | **`std::nullopt`** |
24+
| an **empty** switch name | `false`, swallowed | **`ArgumentException`** |
25+
26+
Transcribed from `AppDomain.cs:171-174`.
27+
28+
## 2. Why both changes were required, together
29+
30+
**The nullable return** is not a stylistic choice: a C++ `bool` cannot distinguish an
31+
explicitly-false switch from an unset one, which is the entire reason .NET's is `bool?`. Keeping
32+
`bool` would have made the forward pointless — both states would still have collapsed to `false`.
33+
34+
**The `noexcept` drop** is not a relaxation either: `AppContext::TryGetSwitch` raises
35+
`System::ArgumentException` for an empty switch name and takes a `std::mutex` whose `lock()` can
36+
throw. Forwarding from a `noexcept` member would have turned **both into `std::terminate`**. The
37+
drop is the only safe way to forward at all.
38+
39+
That is why neither could land without the other, and why the ticket was gated on both.
40+
41+
## 3. To migrate
42+
43+
```cpp
44+
// before
45+
if (domain.IsCompatibilitySwitchSet(name)) { ... }
46+
47+
// after
48+
const auto set = domain.IsCompatibilitySwitchSet(name);
49+
if (set.has_value() && *set) { ... } // explicitly on
50+
if (!set.has_value()) { ... } // never set — a NEW state you can now see
51+
// or, for the old collapsed answer:
52+
if (domain.IsCompatibilitySwitchSet(name).value_or(false)) { ... }
53+
```
54+
55+
An empty switch name now raises instead of quietly answering `false`.
56+
57+
## 4. An implementation note
58+
59+
The body is **out of line**, in `AppDomain.cpp`, for the same reason `SetData`/`GetData` already
60+
are: `System/AppContext.hpp` includes `AppDomain.hpp` for `BaseDirectory`, so the include cannot
61+
run the other way. Adding the include to the header produced exactly that cycle, and the existing
62+
pattern was already there to follow.
63+
64+
## 5. Downstream, measured
65+
66+
Neither `cna` nor `mobile-eggbert` calls `IsCompatibilitySwitchSet` — **zero sites in both**.
67+
Neither repository was modified.

modules/core/include/System/AppDomain.hpp

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#pragma once
55
#include <functional>
66
#include <string>
7+
#include <optional>
78
#include "System/ArgumentException.hpp"
89
#include "System/EventArgs.hpp"
910
#include "System/MarshalByRefObject.hpp"
@@ -290,25 +291,39 @@ namespace System {
290291
/**
291292
* @brief Determines whether a compatibility switch is set.
292293
*
293-
* C++ counterpart of .NET AppDomain.IsCompatibilitySwitchSet(string).
294-
* Always returns false in this port: it does NOT consult the
295-
* System::AppContext switch registry that SetData/GetData above now share.
294+
* C++ counterpart of .NET `AppDomain.IsCompatibilitySwitchSet(string)`
295+
* (`AppDomain.cs:171-174`), transcribed:
296296
*
297-
* @warning This is a known, ticketed divergence, not an oversight. .NET
298-
* forwards to AppContext.TryGetSwitch and returns @c bool? — and this port
299-
* cannot follow it without two changes that need explicit approval, which
300-
* ticket #2250 carries: the return type must become nullable to keep
301-
* "explicitly false" distinguishable from "unset", and the @c noexcept must
302-
* go, because AppContext::TryGetSwitch throws System::ArgumentException for
303-
* an empty switch name (and takes a mutex, which can throw too). Forwarding
304-
* while still declared @c noexcept would turn both into std::terminate.
297+
* ```csharp
298+
* return AppContext.TryGetSwitch(value, out bool result) ? result : default(bool?);
299+
* ```
300+
*
301+
* @par Ticket #2250 made both approval-bound changes together
302+
* It used to `return false` unconditionally, without consulting the `System::AppContext`
303+
* switch registry at all — so a switch a caller had explicitly **set to true** still
304+
* reported as unset. Following .NET needed two changes that had to land together, and
305+
* SA-10 covers both:
306+
*
307+
* - **the return type is `std::optional<bool>`**, because a C++ `bool` cannot
308+
* distinguish an explicitly-false switch from an unset one — which is precisely the
309+
* distinction `bool?` exists to carry;
310+
* - **the `noexcept` is gone**, because `AppContext::TryGetSwitch` raises
311+
* `System::ArgumentException` for an empty switch name and takes a `std::mutex` whose
312+
* `lock()` can throw. Forwarding from a `noexcept` member would have turned both into
313+
* `std::terminate`, so the drop is not a stylistic relaxation but the only safe way to
314+
* forward at all.
305315
*
306316
* @param value The name of the compatibility switch.
307-
* @return Always false.
317+
* @return The switch's value, or `std::nullopt` if it is not set.
318+
* @throws System::ArgumentException if @p value is empty, exactly as
319+
* `AppContext::TryGetSwitch` does — the diagnostic now reaches the caller instead
320+
* of being swallowed by an unconditional `false`.
321+
*
322+
* @note The body is out of line for the same reason `SetData`/`GetData` above are:
323+
* `System/AppContext.hpp` includes this header for `BaseDirectory`, so the include
324+
* may not run the other way.
308325
*/
309-
[[nodiscard]] bool IsCompatibilitySwitchSet(const std::string& /*value*/) const noexcept {
310-
return false;
311-
}
326+
[[nodiscard]] std::optional<bool> IsCompatibilitySwitchSet(const std::string& value) const;
312327

313328
// -----------------------------------------------------------------------
314329
// Additional static methods

modules/core/src/System/AppDomain.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,18 @@ AppDomain::AppDomain() {
6565
#endif
6666
}
6767

68+
// #2250 / SR-AUD-103. Transcribed from .NET's AppDomain.cs:171-174:
69+
// return AppContext.TryGetSwitch(value, out bool result) ? result : default(bool?);
70+
//
71+
// It used to `return false` unconditionally without consulting the switch registry at all, so a
72+
// switch a caller had explicitly SET TO TRUE still reported as unset. Out of line for the same
73+
// reason SetData/GetData below are: AppContext.hpp includes AppDomain.hpp for BaseDirectory.
74+
std::optional<bool> AppDomain::IsCompatibilitySwitchSet(const std::string& value) const {
75+
bool result = false;
76+
if (AppContext::TryGetSwitch(value, result)) return result;
77+
return std::nullopt;
78+
}
79+
6880
void AppDomain::SetData(const std::string& name, void* data) {
6981
AppContext::SetData(name, data);
7082
}

modules/core/tests/System/AppDomainTests.cpp

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
// that the two fixtures, which live in different executables, do not share a
1919
// name in filters and logs.
2020
#include <gtest/gtest.h>
21+
#include <optional>
2122

2223
#include <string>
2324

@@ -70,30 +71,58 @@ TEST(AppDomainDataPolicyTests, SetData_NullValue_IsStoredAndReadBack) {
7071
}
7172

7273
// ---------------------------------------------------------------------------
73-
// SR-AUD-103, switch half / ticket #2250 (needs_user). This pins the CURRENT
74-
// divergence deliberately: IsCompatibilitySwitchSet does not consult AppContext,
75-
// because forwarding needs a nullable return type and a noexcept drop, both of
76-
// which need approval. If #2250 is approved, this is the test that must change.
74+
// SR-AUD-103, switch half / ticket #2250 — SHIPPED, and both pins below inverted
75+
//
76+
// IsCompatibilitySwitchSet used to `return false` unconditionally, without consulting the switch
77+
// registry at all -- so a switch a caller had explicitly SET TO TRUE still reported as unset.
78+
// .NET's is `AppContext.TryGetSwitch(value, out bool result) ? result : default(bool?)`
79+
// (`AppDomain.cs:171-174`).
80+
//
81+
// Following it needed two approval-bound changes TOGETHER, and SA-10 covers both: the return type
82+
// had to become nullable, because a C++ bool cannot distinguish an explicitly-FALSE switch from
83+
// an UNSET one -- which is the whole reason .NET's is `bool?` -- and the noexcept had to go,
84+
// because AppContext::TryGetSwitch raises for an empty name and takes a mutex whose lock() can
85+
// throw. Forwarding from a noexcept member would have turned both into std::terminate, so the
86+
// drop is the only safe way to forward at all rather than a stylistic relaxation.
7787
// ---------------------------------------------------------------------------
7888

79-
TEST(AppDomainDataPolicyTests, IsCompatibilitySwitchSet_DoesNotYetConsultAppContext) {
89+
TEST(AppDomainDataPolicyTests, Fix2250_IsCompatibilitySwitchSetConsultsAppContext) {
8090
AppContext::SetSwitch("AppDomainDataPolicyTests.switchOn", true);
8191
ASSERT_TRUE([] {
8292
bool enabled = false;
8393
return AppContext::TryGetSwitch("AppDomainDataPolicyTests.switchOn", enabled) && enabled;
8494
}()) << "positive control: AppContext itself must report the switch as set";
85-
EXPECT_FALSE(AppDomain::CurrentDomain().IsCompatibilitySwitchSet("AppDomainDataPolicyTests.switchOn"));
95+
EXPECT_EQ(std::optional<bool>(true),
96+
AppDomain::CurrentDomain().IsCompatibilitySwitchSet("AppDomainDataPolicyTests.switchOn"));
8697
}
8798

88-
TEST(AppDomainDataPolicyTests, IsCompatibilitySwitchSet_IsStillNoexcept) {
89-
// The reference and the argument are bound outside the noexcept operand on
90-
// purpose: CurrentDomain() is not itself noexcept, and neither is the
91-
// const char* -> std::string conversion, so either one inside the operand
92-
// would answer a different question.
99+
TEST(AppDomainDataPolicyTests, Fix2250_ExplicitlyFalseIsNotUnset) {
100+
// THE DISTINCTION THE NULLABLE RETURN EXISTS FOR, and the reason a bool could not have
101+
// carried the repair: these two states used to be one, and both used to read as `false`.
102+
AppContext::SetSwitch("AppDomainDataPolicyTests.switchOff", false);
103+
AppDomain& domain = AppDomain::CurrentDomain();
104+
105+
EXPECT_EQ(std::optional<bool>(false),
106+
domain.IsCompatibilitySwitchSet("AppDomainDataPolicyTests.switchOff"));
107+
EXPECT_EQ(std::nullopt,
108+
domain.IsCompatibilitySwitchSet("AppDomainDataPolicyTests.neverSet"));
109+
EXPECT_NE(domain.IsCompatibilitySwitchSet("AppDomainDataPolicyTests.switchOff"),
110+
domain.IsCompatibilitySwitchSet("AppDomainDataPolicyTests.neverSet"))
111+
<< "explicitly false and unset were indistinguishable before #2250";
112+
}
113+
114+
TEST(AppDomainDataPolicyTests, Fix2250_ItIsNoLongerNoexceptAndTheDiagnosticReachesTheCaller) {
115+
// The reference and the argument are bound outside the noexcept operand on purpose:
116+
// CurrentDomain() is not itself noexcept, and neither is the const char* -> std::string
117+
// conversion, so either one inside the operand would answer a different question.
93118
AppDomain& domain = AppDomain::CurrentDomain();
94119
const std::string name("AppDomainDataPolicyTests.switchOn");
95-
EXPECT_TRUE(noexcept(domain.IsCompatibilitySwitchSet(name)));
96-
EXPECT_NO_THROW((void)domain.IsCompatibilitySwitchSet(std::string()));
120+
EXPECT_FALSE(noexcept(domain.IsCompatibilitySwitchSet(name)));
121+
122+
// An empty name used to be swallowed by the unconditional `false`. It now reaches the caller
123+
// as AppContext::TryGetSwitch's own diagnostic -- which is what the noexcept drop buys, and
124+
// why keeping the noexcept would have meant std::terminate instead.
125+
EXPECT_THROW((void)domain.IsCompatibilitySwitchSet(std::string()), System::ArgumentException);
97126
}
98127

99128
// ---------------------------------------------------------------------------

plan.sqlite3

4 KB
Binary file not shown.
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// SPDX-License-Identifier: MIT
2+
// Copyright (c) Robert Vokac and contributors
3+
//
4+
// Negative compile fixture for ticket #2250 (SR-AUD-103, switch half).
5+
//
6+
// #2250 made AppDomain::IsCompatibilitySwitchSet consult the AppContext switch
7+
// registry, as .NET does (`AppDomain.cs:171-174`). It used to `return false`
8+
// unconditionally, so a switch a caller had explicitly SET TO TRUE still
9+
// reported as unset.
10+
//
11+
// Two approval-bound changes had to land together, and SA-10 covers both:
12+
// * the return type is std::optional<bool>, because a C++ bool cannot
13+
// distinguish an explicitly-FALSE switch from an UNSET one -- which is the
14+
// whole reason .NET's is bool?;
15+
// * the noexcept is gone, because AppContext::TryGetSwitch raises for an
16+
// empty name and takes a mutex whose lock() can throw. Forwarding from a
17+
// noexcept member would have been std::terminate, so the drop is the only
18+
// safe way to forward rather than a stylistic relaxation.
19+
//
20+
// Migration: hold the result in `auto` / std::optional<bool> and ask
21+
// has_value(); or spell the old collapsed answer with value_or(false).
22+
//
23+
// Records: docs/Migration-AppDomainCompatibilitySwitch.md,
24+
// docs/NegativeConsumerFixtureValidation.md.
25+
//
26+
// NEGATIVE-FIXTURE: component=Core.Base allow=int128-extension
27+
#include <optional>
28+
#include <string>
29+
#include <type_traits>
30+
31+
#include "System/AppDomain.hpp"
32+
33+
#ifndef SHARP_RUNTIME_NEGATIVE_SITE
34+
#define SHARP_RUNTIME_NEGATIVE_SITE 0
35+
#endif
36+
37+
using System::AppDomain;
38+
39+
int main() {
40+
AppDomain& domain = AppDomain::CurrentDomain();
41+
const std::string name("consumer-fixture-switch");
42+
43+
#if SHARP_RUNTIME_NEGATIVE_SITE == 1
44+
// NEGATIVE(appdomain-switch-assigned-to-bool): conversion from
45+
// | cannot convert
46+
// | no viable conversion
47+
{ bool set = domain.IsCompatibilitySwitchSet(name); (void)set; }
48+
#else
49+
{ const std::optional<bool> set = domain.IsCompatibilitySwitchSet(name); (void)set; }
50+
#endif
51+
52+
#if SHARP_RUNTIME_NEGATIVE_SITE == 2
53+
// NEGATIVE(appdomain-switch-still-noexcept): static assertion failed
54+
// | static_assert
55+
// The shape that breaks a consumer SILENTLY: a noexcept assertion, or a function whose own
56+
// specification is COMPUTED from this one.
57+
static_assert(noexcept(domain.IsCompatibilitySwitchSet(name)),
58+
"IsCompatibilitySwitchSet is expected to be noexcept");
59+
#else
60+
static_assert(!noexcept(domain.IsCompatibilitySwitchSet(name)),
61+
"#2250: it forwards to a throwing, mutex-taking call");
62+
#endif
63+
64+
#if SHARP_RUNTIME_NEGATIVE_SITE == 3
65+
// NEGATIVE(appdomain-switch-return-still-bool): static assertion failed
66+
// | static_assert
67+
static_assert(std::is_same_v<decltype(domain.IsCompatibilitySwitchSet(name)), bool>,
68+
"the switch query is expected to return bool");
69+
#else
70+
static_assert(std::is_same_v<decltype(domain.IsCompatibilitySwitchSet(name)),
71+
std::optional<bool>>,
72+
"#2250: explicitly-false and unset must be distinguishable");
73+
#endif
74+
75+
// UNCHANGED, and asserted so the fixture proves the change was surgical: the neighbouring
76+
// stub accessors keep their types and their noexcept.
77+
static_assert(noexcept(domain.getShadowCopyFilesProperty()), "neighbour untouched");
78+
return 0;
79+
}

0 commit comments

Comments
 (0)