Skip to content

Commit 7729e87

Browse files
committed
fix(core): System::ValueType is no longer directly constructible (#2322, SR-AUD-068)
'System::ValueType v;' compiled, where .NET declares 'public abstract class ValueType'. Landed under SA-8, option B of the review's three, and the same shape as #2339. PROTECTED, NOT ABSTRACT, AND DELIBERATELY SO. A C++ class becomes abstract only by having a pure virtual, and .NET's ValueType.ToString() has a real body -- so making one pure here would invent surface the reference does not have. The protected constructor gets the property that matters: base yes, object no. A static_assert records that is_abstract_v<ValueType> is intentionally false, so a later reader does not 'finish the job'. The copy and move members are protected too, closing the slicing route the review did not enumerate. A PREMISE THE REVIEW GOT WRONG, corrected here: it recorded that 'the only derived types anywhere are SimpleValueType and ConcreteValueType in ValueTypeTests.cpp'. That was true of DERIVATIONS and missed five direct INSTANTIATIONS in Batch15TypesTests.cpp, which the compiler found immediately. WHAT IS NOT REPAIRED AND CANNOT BE, now declared rather than left to be rediscovered as bugs: .NET's Equals and GetHashCode compare FIELDS and its ToString returns the RUNTIME TYPE NAME, where this port returns identity, an address-derived hash and the literal 'System.ValueType'. All three are reflection, and a C++ base class can neither enumerate a derived class's fields nor learn its name. A test pins all three. Fixture set: 23 fixtures / 155 sites. Downstream #2370: zero occurrences in either consumer. Gate 17,299 run, 0 failed.
1 parent ffa96a3 commit 7729e87

8 files changed

Lines changed: 256 additions & 13 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: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `System::ValueType` is no longer directly constructible (ticket #2322)
5+
6+
*2026-08-18.* `System::ValueType v;` compiled. .NET declares `public abstract class ValueType`,
7+
so C# rejects the equivalent.
8+
9+
Landed under `docs/StandingApprovals.md` **SA-8** with SA-2's five conditions. Same shape as
10+
#2339 (`Attribute`). No layout, vtable or `noexcept` change.
11+
12+
---
13+
14+
## 1. What changed
15+
16+
| Spelling | Was | Is |
17+
|---|---|---|
18+
| `System::ValueType v;` | compiled | **rejected** |
19+
| `new System::ValueType()` | compiled | **rejected** |
20+
| `ValueType sliced = derived;` | compiled | **rejected** — the copy member went protected too |
21+
| `std::is_default_constructible_v<ValueType>` | `true` | **`false`** |
22+
| `class MyValue : public ValueType {};` || **unchanged**, and the only intended use |
23+
24+
## 2. Protected, not abstract — deliberately
25+
26+
.NET's class is `abstract`. A C++ class becomes abstract only by having a **pure virtual**, and
27+
.NET's `ValueType.ToString()` has a real body — so making one pure here would invent surface the
28+
reference does not have. The protected constructor gets the property that actually matters: the
29+
type can still be a base, and can no longer be an object. A `static_assert` records that
30+
`is_abstract_v<ValueType>` is deliberately `false`.
31+
32+
## 3. What was NOT repaired, and cannot be
33+
34+
.NET's `Equals` and `GetHashCode` compare **fields**, and its `ToString()` returns
35+
`this.GetType().ToString()` — the **runtime type name**. This port returns identity, an
36+
address-derived hash, and the literal `"System.ValueType"`.
37+
38+
All three are **reflection**, permanently out of scope per `CLAUDE.md`, and a C++ base class can
39+
neither enumerate a derived class's fields nor learn its name. A test pins all three as
40+
declarations rather than leaving them to be rediscovered as bugs.
41+
42+
A derived type that needs value semantics must override `Equals` **and** `GetHashCode` together —
43+
both are already `virtual`, so #2322 invented no hook for it — and `ToString` if it wants its own
44+
name.
45+
46+
## 4. A premise the review got wrong
47+
48+
The #2322 review recorded that *"the only derived types anywhere are `SimpleValueType` and
49+
`ConcreteValueType` in `ValueTypeTests.cpp`"*. That was true of **derivations** and missed five
50+
direct **instantiations** in `Batch15TypesTests.cpp`, which the compiler found immediately. They
51+
now go through a local probe, which is what a caller has to do too.
52+
53+
## 5. To migrate
54+
55+
Derive:
56+
57+
```cpp
58+
// before
59+
System::ValueType v;
60+
61+
// after
62+
class MyValue : public System::ValueType {};
63+
MyValue v;
64+
```
65+
66+
A reference or pointer to the base is unaffected.
67+
68+
## 6. Downstream, measured
69+
70+
Neither `cna` nor `mobile-eggbert` mentions `System::ValueType` — **zero occurrences in both**.
71+
Neither repository was modified. The downstream ticket is **#2370**.

modules/core/include/System/ValueType.hpp

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,42 @@ namespace System {
1414
/**
1515
* @brief Provides the base class for value types.
1616
*
17-
* C++ counterpart of .NET System.ValueType. In .NET, ValueType is the implicit
18-
* base of all struct types; it overrides Equals and GetHashCode to perform
19-
* field-by-field comparison via reflection. In C++ there is no reflection, so
20-
* the defaults here fall back to object identity. Concrete value-type structs
21-
* should override Equals() and GetHashCode() to match .NET field-equality semantics.
17+
* C++ counterpart of .NET System.ValueType, which is `public abstract class ValueType`. In .NET
18+
* it is the implicit base of every struct type and overrides Equals/GetHashCode to compare
19+
* field-by-field through reflection.
20+
*
21+
* @par The constructor is protected, since ticket #2322
22+
* `System::ValueType v;` used to compile. .NET's class is **abstract**, so C# rejects the
23+
* equivalent. A C++ class becomes abstract only by having a pure virtual, and .NET's
24+
* `ValueType.ToString()` has a real body — so making one pure here would be inventing surface
25+
* the reference does not have. The protected constructor is the faithful move instead, and it
26+
* gets the property that matters: the type can still be a base, and can no longer be an object.
27+
* The copy and move members are protected with it, so the base cannot be reached by a slice.
28+
*
29+
* @par The identity defaults are a PERMANENT DEVIATION, not a TODO
30+
* .NET's Equals and GetHashCode compare fields, and its ToString returns the **runtime type
31+
* name** (`this.GetType().ToString()`); this port returns the literal `"System.ValueType"`. All
32+
* three are **reflection**, which `CLAUDE.md` lists as permanently out of scope, and a C++ base
33+
* class cannot enumerate a derived class's fields or learn its name. The gap is therefore not
34+
* closable rather than merely unclosed.
35+
*
36+
* A derived type that needs value semantics must override `Equals` **and** `GetHashCode`
37+
* together — both are already `virtual`, so no new hook was invented for it — and `ToString` if
38+
* it wants its own name. Overriding only one of the first two breaks the equals/hash contract
39+
* silently.
2240
*/
2341
class ValueType {
42+
protected:
43+
/**
44+
* @brief Protected default constructor. See the class doc-comment for why this, and not an
45+
* abstract class, is the faithful counterpart of .NET's `public abstract`.
46+
*/
47+
ValueType() = default;
48+
ValueType(const ValueType&) = default;
49+
ValueType(ValueType&&) = default;
50+
ValueType& operator=(const ValueType&) = default;
51+
ValueType& operator=(ValueType&&) = default;
52+
2453
public:
2554
virtual ~ValueType() = default;
2655

@@ -45,7 +74,9 @@ class ValueType {
4574
/**
4675
* @brief Returns a string representation of this instance.
4776
*
48-
* C++ counterpart of .NET ValueType.ToString().
77+
* C++ counterpart of .NET ValueType.ToString(), which returns `this.GetType().ToString()` —
78+
* the RUNTIME TYPE NAME. This literal is a permanent deviation: naming the runtime type is
79+
* reflection. A derived type that wants its own name must override this.
4980
*/
5081
virtual std::string ToString() const { return "System.ValueType"; }
5182
};

modules/core/tests/System/Batch15TypesTests.cpp

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -165,28 +165,40 @@ TEST(BadImageFormatExceptionExtraTests, DefaultCtor_EmptyFileName) {
165165
// ===========================================================================
166166
// ValueType
167167
// ===========================================================================
168+
//
169+
// #2322 made System::ValueType's constructor protected, matching .NET's `public abstract class
170+
// ValueType`. These five tests were direct instantiations of the base -- and the #2322 review
171+
// did NOT find them: it recorded that "the only derived types anywhere are SimpleValueType and
172+
// ConcreteValueType in ValueTypeTests.cpp", which was true of DERIVATIONS and missed these
173+
// INSTANTIATIONS in a different file. They now go through a local probe, which is what a caller
174+
// has to do too.
175+
176+
namespace {
177+
/// The minimal legal way to reach the base after #2322.
178+
class ProbeValueType : public System::ValueType {};
179+
} // namespace
168180

169181
TEST(ValueTypeTests, DefaultCtor_Works) {
170-
System::ValueType vt;
182+
ProbeValueType vt;
171183
(void)vt;
172184
SUCCEED();
173185
}
174186
TEST(ValueTypeTests, GetHashCode_ReturnsInt) {
175-
System::ValueType vt;
187+
ProbeValueType vt;
176188
int h = vt.GetHashCode();
177189
(void)h;
178190
SUCCEED();
179191
}
180192
TEST(ValueTypeTests, ToString_NonEmpty) {
181-
System::ValueType vt;
193+
ProbeValueType vt;
182194
EXPECT_FALSE(vt.ToString().empty());
183195
}
184196
TEST(ValueTypeTests, Equals_SameObject) {
185-
System::ValueType vt;
197+
ProbeValueType vt;
186198
EXPECT_TRUE(vt.Equals(vt));
187199
}
188200
TEST(ValueTypeTests, Equals_DifferentObjects_False) {
189-
System::ValueType a, b;
201+
ProbeValueType a, b;
190202
EXPECT_FALSE(a.Equals(b));
191203
}
192204

modules/core/tests/System/ValueTypeTests.cpp

Lines changed: 40 additions & 0 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 <type_traits>
56
#include "System/ValueType.hpp"
67

78
using System::ValueType;
@@ -55,3 +56,42 @@ TEST(ValueTypeTest, BaseToString) {
5556
EXPECT_EQ(p->ToString(), "3");
5657
delete p;
5758
}
59+
60+
// ---------------------------------------------------------------------------
61+
// #2322 / SR-AUD-068 — the base is no longer directly constructible
62+
// ---------------------------------------------------------------------------
63+
64+
TEST(ValueTypeContractTests, Fix2322_TheBaseIsNoLongerDirectlyConstructible) {
65+
// .NET declares `public abstract class ValueType`, so C# rejects the equivalent of
66+
// `System::ValueType v;`. This port compiled it until #2322.
67+
static_assert(!std::is_default_constructible_v<ValueType>,
68+
"#2322: the constructor is protected");
69+
static_assert(!std::is_copy_constructible_v<ValueType>,
70+
"#2322: and the copy member went with it, so the base cannot be sliced out");
71+
static_assert(std::is_default_constructible_v<SimpleValueType>,
72+
"a derived type must still be constructible -- that is the point");
73+
74+
// NOT abstract, and the distinction is deliberate rather than an oversight: a C++ class is
75+
// abstract only by having a pure virtual, and .NET's ValueType.ToString() has a real body, so
76+
// making one pure here would invent surface the reference does not have. The protected
77+
// constructor gets the property that matters -- base yes, object no.
78+
static_assert(!std::is_abstract_v<ValueType>,
79+
"#2322 deliberately did NOT invent a pure virtual to force abstractness");
80+
}
81+
82+
TEST(ValueTypeContractTests, Decl2322_TheIdentityDefaultsArePermanentDeviations) {
83+
// NOT A REPAIR -- A DECLARATION. .NET's Equals and GetHashCode compare fields and its
84+
// ToString returns the runtime type name. All three are reflection, permanently out of scope
85+
// per CLAUDE.md, and a C++ base class can neither enumerate a derived class's fields nor
86+
// learn its name.
87+
SimpleValueType a, b;
88+
EXPECT_FALSE(a.Equals(b)) << "identity, not value -- .NET would compare fields";
89+
EXPECT_TRUE(a.Equals(a));
90+
91+
// The ToString literal is the third of the three, and the one most easily mistaken for a bug.
92+
EXPECT_EQ("System.ValueType", a.ToString())
93+
<< "the runtime type name is reflection; a derived type must override this itself";
94+
95+
// Both hooks a derived type needs are already virtual, which is why #2322 invented none.
96+
static_assert(std::is_polymorphic_v<ValueType>, "Equals/GetHashCode/ToString are virtual");
97+
}

plan.sqlite3

8 KB
Binary file not shown.
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// SPDX-License-Identifier: MIT
2+
// Copyright (c) Robert Vokac and contributors
3+
//
4+
// Negative compile fixture for ticket #2322 (SR-AUD-068).
5+
//
6+
// #2322 made System::ValueType's constructor protected. .NET declares
7+
// `public abstract class ValueType`, so C# rejects the equivalent of
8+
// `System::ValueType v;`; this port compiled it.
9+
//
10+
// It is protected rather than abstract on purpose: a C++ class becomes
11+
// abstract only by having a pure virtual, and .NET's ValueType.ToString() has a
12+
// real body, so making one pure here would invent surface the reference does
13+
// not have. The protected constructor gets the property that matters -- the
14+
// type can still be a base, and can no longer be an object -- and the copy and
15+
// move members went protected with it, so the base cannot be sliced out either.
16+
//
17+
// Each spelling is compiled on its own below; the #else branches are the
18+
// migrated ones. Migration: derive.
19+
//
20+
// Records: docs/Migration-ValueTypeProtectedConstructor.md,
21+
// docs/NegativeConsumerFixtureValidation.md.
22+
//
23+
// NEGATIVE-FIXTURE: component=Core.Base
24+
#include <type_traits>
25+
26+
#include "System/ValueType.hpp"
27+
28+
#ifndef SHARP_RUNTIME_NEGATIVE_SITE
29+
#define SHARP_RUNTIME_NEGATIVE_SITE 0
30+
#endif
31+
32+
using System::ValueType;
33+
34+
namespace {
35+
/// The migrated shape: derive from the base rather than instantiating it.
36+
class MyValue : public ValueType {};
37+
} // namespace
38+
39+
int main() {
40+
#if SHARP_RUNTIME_NEGATIVE_SITE == 1
41+
// NEGATIVE(valuetype-direct-instantiation): is protected within this context
42+
// | protected
43+
ValueType direct;
44+
(void)direct;
45+
#else
46+
MyValue direct;
47+
(void)direct;
48+
#endif
49+
50+
#if SHARP_RUNTIME_NEGATIVE_SITE == 2
51+
// NEGATIVE(valuetype-heap-instantiation): is protected within this context
52+
// | protected
53+
ValueType* heap = new ValueType();
54+
delete heap;
55+
#else
56+
ValueType* heap = new MyValue();
57+
delete heap;
58+
#endif
59+
60+
#if SHARP_RUNTIME_NEGATIVE_SITE == 3
61+
// NEGATIVE(valuetype-copy-slice): is protected within this context
62+
// | protected
63+
// | use of deleted function
64+
MyValue derived;
65+
ValueType sliced = derived;
66+
(void)sliced;
67+
#else
68+
MyValue derived;
69+
const ValueType& notSliced = derived;
70+
(void)notSliced;
71+
#endif
72+
73+
#if SHARP_RUNTIME_NEGATIVE_SITE == 4
74+
// NEGATIVE(valuetype-still-default-constructible): static assertion failed
75+
// | static_assert
76+
// The shape that breaks a consumer SILENTLY: a trait query, or a template constrained on one.
77+
static_assert(std::is_default_constructible_v<ValueType>,
78+
"ValueType is expected to be default-constructible");
79+
#else
80+
static_assert(!std::is_default_constructible_v<ValueType>,
81+
"#2322: the constructor is protected");
82+
static_assert(std::is_default_constructible_v<MyValue>,
83+
"#2322: a derived type is still constructible -- that is the point");
84+
static_assert(!std::is_abstract_v<ValueType>,
85+
"#2322 deliberately did NOT invent a pure virtual to force abstractness");
86+
#endif
87+
88+
return 0;
89+
}

0 commit comments

Comments
 (0)