Skip to content

Commit 80b3e1e

Browse files
committed
refactor(component-model)!: six attributes get .NET's get-only shape (#2403)
Six System::ComponentModel attributes published a BARE MUTABLE PUBLIC DATA MEMBER where .NET publishes a get-only property, and most had no Yes/No/Default statics and no equality members at all. WHAT MAKES THIS MORE THAN A STYLE POINT IS THAT THE PORT WAS INCONSISTENT WITH ITSELF INSIDE ONE FILE. CategoryAttribute, BrowsableAttribute, DisplayNameAttribute and DescriptionAttribute -- in that very header and its neighbour -- already had the correct shape: private member, getXxxProperty(), the static set, and Equals/GetHashCode/getIsDefaultAttributeProperty. The other six did not: ReadOnlyAttribute public bool IsReadOnly, Yes/No, NO Default ImmutableObjectAttribute public bool Immutable, no statics LocalizableAttribute public bool IsLocalizable, no statics MergablePropertyAttribute public bool AllowMerge, no statics NotifyParentPropertyAttribute public bool NotifyParent, no statics RefreshPropertiesAttribute public Refresh RefreshProperties_, no statics All six are now `final` for .NET's `sealed`, with a get-only accessor, the full static set, and the three equality members. REFRESHPROPERTIES BECOMES A TOP-LEVEL ENUM. .NET declares it in its own file (RefreshProperties.cs); this port nested it as RefreshPropertiesAttribute::Refresh, differing in BOTH the name and the scope. THE DEFAULTS ARE NOT UNIFORM AND THAT IS .NET'S. MergablePropertyAttribute.Default is YES (MergablePropertyAttribute.cs:12) where the other four boolean attributes default to NO, and BrowsableAttribute.Default is Yes too, which this port already had right. A repair that "harmonised" the five would be wrong HERE AND NOWHERE ELSE, so the asymmetry is pinned in one case rather than scattered as five unrelated literals. ONE DIVERGENCE IS DELIBERATE AND IS PINNED. .NET's GetHashCode for all six is base.GetHashCode() -- IDENTITY -- while its Equals is VALUE-based, so two equal .NET instances can hash differently: a hash-contract violation in the reference itself. This port does not reproduce it, and the reason is WRITTEN DOWN RATHER THAN CHOSEN -- System/Attribute.hpp's own doc-comment states the house rule ("A subclass that needs value equality must override BOTH Equals and GetHashCode ... Overriding only one breaks the equals/hashCode contract") and the four already-correct siblings use a value hash. Reproducing .NET here would contradict the port's own stated rule and its own neighbours. MUTATION M7 ADOPTS .NET'S IDENTITY HASH AND IS CAUGHT. THE MODULE'S WHOLE PRIOR COVERAGE FOR THESE SIX WAS CONSTRUCTOR ROUND-TRIPS THROUGH THE PUBLIC FIELD. Nothing asserted the statics, the equality members or the defaults, so .NET's Default values -- the actual contract of a metadata attribute -- were unpinned. Six cases now pin them. That is the point worth carrying: component-model HAS its own test executable, so "has tests" is not "is covered". Landed under SA-8 with SA-2's five conditions. NO VTABLE CHANGE: Equals, GetHashCode and getIsDefaultAttributeProperty are already virtual on System::Attribute, so these are overrides of existing slots, not new ones. First-party migration: 8 sites, all in this module's own test file, every one named by the compiler. Negative fixture with 8 sites -- a read AND A WRITE for ReadOnlyAttribute, a read for each of the other five, and one for the enum's scope. The WRITE earns its own site because it is the spelling a careless migration would have kept longest: it compiled silently against the old shape while changing the attribute's meaning. Set 49/248 -> 50/256. Downstream, measured: zero System::ComponentModel sites in both consumers, recorded as #2404. Seven mutations, ALL CAUGHT, three at compile time -- the only way C++ reports a shape. A PROCESS NOTE WORTH KEEPING. An early build of this change appeared to report ZERO ERRORS and was nearly taken at face value. The cause: `cmake --build ... -k 200` is NOT keep-going -- `-k` must follow `--`, so CMake printed its usage banner and exited. That is CLAUDE.md rule 2's own recorded #2395 trap in a new form, and A MIS-PASSED -k IS WORSE THAN NONE, because it looks like success. Gate 17,637 / 38 executables: 17,637 run, 17,637 passed, 0 failed, 0 skipped, recounted from the per-executable logs with every executable run separately and continuing past failures. +6 on 17,631, in SharpRuntimeTests_ComponentModel (98 -> 104); no other executable moved. Zero build warnings at --parallel 2; module boundaries and the generated catalogue green.
1 parent c86259b commit 80b3e1e

8 files changed

Lines changed: 580 additions & 54 deletions

File tree

CLAUDE.md

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

NEXT.md

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33

44
# NEXT.md
55

6-
> **Test-count floor, 2026-08-19 — 17,631 / 38, AND THE GATE IS GREEN.** The complete
7-
> 38-executable gate reads **17,631 run: 17,631 passed, 0 failed, 0 skipped**, recounted from the
6+
> **Test-count floor, 2026-08-20 — 17,637 / 38, AND THE GATE IS GREEN.** The complete
7+
> 38-executable gate reads **17,637 run: 17,637 passed, 0 failed, 0 skipped**, recounted from the
88
> per-executable logs with every executable run separately and continuing past failures, zero build
99
> warnings at `--parallel 2`. Every checkpoint below this one ends with *"the gate is not green"*;
1010
> this one does not. Two of the three historical failure sources were environmental and are simply
@@ -51,11 +51,19 @@
5151
> name claimed "DiffersAcrossProcesses" while its body only checked within one. §4b has the
5252
> remaining candidates.
5353
>
54-
> **BOTH WORK QUEUES ARE EMPTY AGAIN.** Six tickets were filed and closed on 2026-08-19 by this
55-
> sweep — #2397, #2398, #2399, #2400 (downstream record), #2401 and #2402. `ticket` has **0
56-
> `todo`**; `task` has **0** unclassified (14,979 ignored / 1,082 ported / 140 ignore). Ticket
57-
> totals: **2,385 done, 9 blocked, 1 needs_user, 5 wontfix**. Graph **41 / 94** (#2401 added one
58-
> private edge), negative fixtures **49 / 248** (#2399 added one). What remains *blocked* needs the user or an external event,
54+
> **#2403 — `System::ComponentModel`, and the pattern that found it is pattern (2) below.** Six
55+
> attributes published a **bare mutable public data member** where .NET publishes a get-only
56+
> property, most with **no statics and no equality members at all** — while four siblings **in the
57+
> same header** already had the correct shape. The module's whole prior coverage for those six was
58+
> constructor round-trips through the public field, so .NET's `Default` values — the actual contract
59+
> of a metadata attribute — were unpinned, and `MergablePropertyAttribute::Default` is `Yes` where
60+
> its four siblings are `No`.
61+
>
62+
> **BOTH WORK QUEUES ARE EMPTY AGAIN.** Eight tickets were filed and closed by this sweep — #2397,
63+
> #2398, #2399, #2400, #2401, #2402 on 2026-08-19, then #2403 and #2404 on 2026-08-20. `ticket` has
64+
> **0 `todo`**; `task` has **0** unclassified (14,979 ignored / 1,082 ported / 140 ignore). Ticket
65+
> totals: **2,387 done, 9 blocked, 1 needs_user, 5 wontfix**. Graph **41 / 94** (#2401 added one
66+
> private edge), negative fixtures **50 / 256** (#2399 and #2403 added one each). What remains *blocked* needs the user or an external event,
5967
> and each is itemised in §2 below. **§4b says where to look next, and the method that found all
6068
> three of today's tickets.**
6169

@@ -164,6 +172,7 @@ coverage" and the difference matters:
164172
| Module | Component | Where it is exercised today |
165173
|---|---|---|
166174
| `text-regular-expressions` | `Text.RegularExpressions` | a section of `tests/integration/System/Text/TextRemainingTests.cpp` — **this is the one #2397 measured, and it held four divergences** |
175+
| *(not in this table)* `component-model` | `ComponentModel` | **has** its own tests, and #2403 still found six attributes with a mutable public representation — so **"has tests" is not "is covered"** |
167176
| `io-compression-zip` | `IO.Compression.Zip` | `tests/integration/System/IO/Compression/CompressionTests.cpp` (145 cases in the file, covering the whole namespace) |
168177
| `security-cryptography-random` | `Security.Cryptography.Random` | **exhausted — measured by #2398 and #2399, and both found real defects.** Its three shipped cases could not fail: each asserted a buffer's size *after* filling a buffer sized before the call |
169178
| `storage` | `Storage` | `tests/integration/Task39RemainingTests.cpp` (78 cases in the file) — one header, `StoragePaths` |
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — six `System::ComponentModel` attributes get .NET's shape (ticket #2403)
5+
6+
*2026-08-20.* Six attributes published a **bare mutable public data member** where .NET publishes a
7+
**get-only property**, and most were missing their `Yes`/`No`/`Default` statics and their equality
8+
members entirely.
9+
10+
Landed under **SA-8** (*"where this port publishes a mutable or public representation and .NET's is
11+
private, readonly or absent, match .NET and migrate the first-party sites"*) with **SA-2**'s five
12+
conditions, since removing a public data member is a public source break.
13+
14+
**No vtable change**: `Equals`, `GetHashCode` and `getIsDefaultAttributeProperty` are already
15+
`virtual` on `System::Attribute`, so these are overrides of existing slots, not new ones.
16+
17+
**Downstream, measured:** **zero** sites of any kind in `cna` and in `mobile-eggbert`. Downstream
18+
record: **#2404**.
19+
20+
---
21+
22+
## 1. What made this more than a style point
23+
24+
**The port was inconsistent with itself inside one file.** `CategoryAttribute`,
25+
`BrowsableAttribute`, `DisplayNameAttribute` and `DescriptionAttribute` — in that very header and
26+
its neighbour — already had the correct shape: private member, `getXxxProperty()`, the static set,
27+
and `Equals`/`GetHashCode`/`getIsDefaultAttributeProperty`. The other six did not.
28+
29+
| Type | Was | Now (= .NET) |
30+
|---|---|---|
31+
| `ReadOnlyAttribute` | `public bool IsReadOnly`; `Yes`/`No`, **no `Default`** | `getIsReadOnlyProperty()`; `Yes`/`No`/`Default` |
32+
| `ImmutableObjectAttribute` | `public bool Immutable`; **no statics** | `getImmutableProperty()`; `Yes`/`No`/`Default` |
33+
| `LocalizableAttribute` | `public bool IsLocalizable`; no statics | `getIsLocalizableProperty()`; `Yes`/`No`/`Default` |
34+
| `MergablePropertyAttribute` | `public bool AllowMerge`; no statics | `getAllowMergeProperty()`; `Yes`/`No`/`Default` |
35+
| `NotifyParentPropertyAttribute` | `public bool NotifyParent`; no statics | `getNotifyParentProperty()`; `Yes`/`No`/`Default` |
36+
| `RefreshPropertiesAttribute` | `public Refresh RefreshProperties_`; no statics | `getRefreshPropertiesProperty()`; `All`/`Repaint`/`Default` |
37+
38+
All six are now `final`, matching .NET's `public sealed class`, and all six gained
39+
`Equals`/`GetHashCode`/`getIsDefaultAttributeProperty`.
40+
41+
## 2. `RefreshProperties` is a top-level enum
42+
43+
.NET declares `public enum RefreshProperties` in its **own file** (`RefreshProperties.cs`). This
44+
port nested it as `RefreshPropertiesAttribute::Refresh`, differing in **both the name and the
45+
scope**. It is now top-level, with .NET's values (`None = 0`, `All = 1`, `Repaint = 2`).
46+
47+
**Migration:** `RefreshPropertiesAttribute::Refresh::All` becomes `RefreshProperties::All`.
48+
49+
## 3. The defaults are not uniform, and that is .NET's
50+
51+
**`MergablePropertyAttribute::Default` is `Yes`** (`MergablePropertyAttribute.cs:12`), where the
52+
other four boolean attributes default to `No`. `BrowsableAttribute::Default` is `Yes` too
53+
(`BrowsableAttribute.cs:32`), and this port already had that right.
54+
55+
A repair that "harmonised" the five would be wrong **here and nowhere else**, which is why the
56+
asymmetry carries a pin of its own — asserted in one case, so it reads as an asymmetry rather than
57+
as five unrelated literals.
58+
59+
## 4. One divergence is deliberate: `GetHashCode`
60+
61+
**.NET's `GetHashCode` for all six is `base.GetHashCode()` — identity — while its `Equals` is
62+
value-based.** Two equal .NET instances can therefore hash differently: a hash-contract violation in
63+
the reference itself.
64+
65+
**This port does not reproduce it, and the reason is written down rather than chosen.**
66+
`System/Attribute.hpp`'s own doc-comment states the house rule in terms:
67+
68+
> A subclass that needs value equality must override **both** `Equals` and `GetHashCode` … Overriding
69+
> only one breaks the equals/hashCode contract.
70+
71+
and the four already-correct siblings in this very header use a value-based hash. Reproducing .NET
72+
here would contradict the port's own stated rule *and* its own neighbours. The divergence is pinned:
73+
mutation M7 replaces the value hash with .NET's identity hash and **is caught**.
74+
75+
## 5. What a caller has to change
76+
77+
| Was | Now |
78+
|---|---|
79+
| `attr.IsReadOnly` | `attr.getIsReadOnlyProperty()` |
80+
| `attr.Immutable` | `attr.getImmutableProperty()` |
81+
| `attr.IsLocalizable` | `attr.getIsLocalizableProperty()` |
82+
| `attr.AllowMerge` | `attr.getAllowMergeProperty()` |
83+
| `attr.NotifyParent` | `attr.getNotifyParentProperty()` |
84+
| `attr.RefreshProperties_` | `attr.getRefreshPropertiesProperty()` |
85+
| `RefreshPropertiesAttribute::Refresh::X` | `RefreshProperties::X` |
86+
87+
**There is no replacement for *writing* one, deliberately.** .NET publishes no setter, and a caller
88+
that needs a different value constructs a different attribute — or uses the `Yes`/`No`/`Default`
89+
statics, which #2403 also completed. A metadata attribute a caller can retarget after construction
90+
is not metadata, and **the write is the spelling that would have survived a careless migration
91+
longest**, because it compiled silently against the old shape while changing the attribute's
92+
meaning. It gets its own negative-fixture site for that reason.
93+
94+
## 6. Testing
95+
96+
First-party migration: **8 sites**, all in this module's own test file, every one named by the
97+
compiler. Negative consumer fixture
98+
`test/consumer/component_model_attribute_representation_negative.cpp`, **8 sites** — a read and a
99+
**write** for `ReadOnlyAttribute`, a read for each of the other five, and one for the enum's scope.
100+
Fixture set **49 / 248 → 50 / 256**.
101+
102+
**The module's whole prior coverage for these six was constructor round-trips through the public
103+
field.** Nothing asserted the statics, the equality members or the defaults — so .NET's `Default`
104+
values, which are the actual contract of a metadata attribute, were unpinned. Six cases now pin
105+
them.
106+
107+
Seven mutations, **all caught**, three of them at compile time (`static_assert` for the seals, and
108+
the enum's scope) — which is the only way C++ reports a shape.
109+
110+
**A process note worth keeping.** An early build of this change appeared to report **zero errors**
111+
and was very nearly taken at face value. The cause: `cmake --build … -k 200` is not keep-going —
112+
`-k` must follow `--`, and CMake was printing its usage banner and exiting. This is `CLAUDE.md` rule
113+
2's recorded #2395 trap in a new form: *the build stops at the first failing translation unit unless
114+
`-k` is passed*, and a mis-passed `-k` is worse than none, because it looks like success.

0 commit comments

Comments
 (0)