Skip to content

Commit 17708a0

Browse files
authored
refactor(core): move the LEGACY_ENTITY_NAMES parity check out of an include_str test (#3204)
`legacy_entity_name_tests::legacy_entity_names_match_the_lookup_arms` recovered the match-arm keys by `include_str!`-ing its own module and comparing them to `LEGACY_ENTITY_NAMES`. That is a source-text assertion: AGENTS.md bans them, and #3195's Rust gate flags this exact line. Running that gate against `main` + #3198 reports one violation repo-wide, and this is it. The site is one hour old. It arrived with #3124 (`cf840556`), in the follow-up to a review that asked for the const to stop being a second hand-maintained list -- so the gate is not discovering old debt, it is catching a fix that reached for the wrong instrument. MOVED RATHER THAN MARKED. `@source-text-assertion-ok` would have been defensible: the test carries `assert!(!from_source.is_empty())`, so the vacuity failure the rule exists to catch cannot happen there. But a marker excuses a violation, and the violation is avoidable. `scripts/check-legacy-entity-coverage.mjs` already parses this file's arm keys for its own purposes, so it gains a `LEGACY_ENTITY_NAMES` comparison and the Rust test goes away. Same call the repo already made for `check-clash-degenerate-reason-parity.mjs`: a claim about two SOURCES belongs in a lint, where reading both is the honest thing rather than the banned thing. The gate version is STRICTER than the test it replaces: - an arm with no const entry -> named, exit 1 - a const entry with no arm -> named, exit 1 (the test had this too) - the const renamed or reshaped -> "no names extracted", exit 1 All three probed, and all three are regression tests in check-legacy-entity-coverage.test.mjs, which mutates a copy of the real source in a temp tree and asserts the gate turns red. The third matters most: two empty sets agree about everything, and the test being replaced could only guard its own half of that. `every_listed_name_resolves_as_legacy` stays -- it reads no source text and pins that the const holds arm keys rather than base-type names. VERIFIED #3198's gate against this tree: OK (593 .rs files, 110 reads, 0 assertions). So #3198 lands green with no marker and no allowlist row. cargo test --workspace --no-fail-fast: 2156 passed, 0 failed. cargo clippy --workspace --exclude ifc-lite-wasm --all-targets -- -D warnings: clean. It caught the orphaned `use std::collections::BTreeSet` that `cargo test` was happy to leave -- the warning-vs-error gap AGENTS.md warns about. 590 script tests, check-source-text-assertions, check-test-wiring: all green. `pnpm lint`'s unused-locals step fails in this worktree, and does so IDENTICALLY on unmodified main with the same install state -- an `--ignore-scripts` install leaves no `dist/`, so those packages cannot compile standalone. Environmental, null-probed, not caused by this diff. Credit to @BIMvoice: the const and its parity idea are his, from the #3124 follow-up. Only where the check lives changes.
1 parent cf84055 commit 17708a0

3 files changed

Lines changed: 98 additions & 34 deletions

File tree

rust/core/src/legacy_entities.rs

Lines changed: 13 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -254,42 +254,21 @@ pub const LEGACY_ENTITY_NAMES: &[&str] = &[
254254
#[cfg(test)]
255255
mod legacy_entity_name_tests {
256256
use super::*;
257-
use std::collections::BTreeSet;
258257

259-
/// The two-lists-that-must-agree guard for [`LEGACY_ENTITY_NAMES`]: the
260-
/// arm keys are recovered from this module's own source text, so a newly
261-
/// added arm whose key never reaches the const is caught here rather than
262-
/// silently shrinking every caller's universe.
258+
/// The arm/const parity check that used to live here is now
259+
/// `scripts/check-legacy-entity-coverage.mjs`, which already reads this
260+
/// file to derive the arm keys and gained a `LEGACY_ENTITY_NAMES`
261+
/// comparison to go with it.
262+
///
263+
/// It moved because the only way to state it in-crate was `include_str!`
264+
/// of this module's own source, which is a source-text assertion — banned
265+
/// by AGENTS.md and flagged by `check-rust-source-text-assertions` (#3195).
266+
/// The repo had already made the same call for
267+
/// `check-clash-degenerate-reason-parity.mjs`: a claim about two SOURCES
268+
/// belongs in a lint. The gate is also strictly stronger here, since it
269+
/// fails on drift in BOTH directions and its own harness proves it cannot
270+
/// pass by extracting nothing.
263271
///
264-
/// (Deliberately phrased without an example arm: the key pattern is what
265-
/// `scripts/check-legacy-entity-coverage.mjs` scans for, and a made-up
266-
/// name in a comment reads to it as a real arm naming no entity.)
267-
#[test]
268-
fn legacy_entity_names_match_the_lookup_arms() {
269-
let src = include_str!("legacy_entities.rs");
270-
let mut from_source: BTreeSet<&str> = BTreeSet::new();
271-
for line in src.lines() {
272-
let trimmed = line.trim();
273-
let Some(rest) = trimmed.strip_prefix('"') else {
274-
continue;
275-
};
276-
let Some(end) = rest.find('"') else { continue };
277-
if !rest[end + 1..].trim_start().starts_with("=>") {
278-
continue;
279-
}
280-
from_source.insert(&rest[..end]);
281-
}
282-
assert!(
283-
!from_source.is_empty(),
284-
"arm-key extraction found nothing -- the parser, not the table, is broken"
285-
);
286-
let from_const: BTreeSet<&str> = LEGACY_ENTITY_NAMES.iter().copied().collect();
287-
assert_eq!(
288-
from_source, from_const,
289-
"LEGACY_ENTITY_NAMES has drifted from get_legacy_entity_info's match arms"
290-
);
291-
}
292-
293272
/// Every listed name really is legacy -- the cheap direction, but it also
294273
/// pins that the const holds arm keys and not, say, base-type names.
295274
#[test]

scripts/check-legacy-entity-coverage.mjs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,23 @@ export function legacyKeys(rustSource) {
8787
return new Set([...rustSource.matchAll(/"(IFC[A-Z0-9]+)"\s*=>\s*Some\(/g)].map((m) => m[1]));
8888
}
8989

90+
/**
91+
* The names in the `LEGACY_ENTITY_NAMES` const — the public mirror of the match
92+
* arms, and what `dump_rooted_type_sweep.rs` feeds into the cross-language
93+
* rooted-type universe (#3124).
94+
*
95+
* Bounded to the const's own `&[` … `];` block, so a name appearing only in a
96+
* doc comment or in a match arm elsewhere in the file cannot pad it.
97+
*/
98+
export function legacyNameConst(rustSource) {
99+
const start = rustSource.indexOf('pub const LEGACY_ENTITY_NAMES');
100+
if (start === -1) return new Set();
101+
const open = rustSource.indexOf('[', start);
102+
const close = rustSource.indexOf('];', open);
103+
if (open === -1 || close === -1) return new Set();
104+
return new Set([...rustSource.slice(open, close).matchAll(/"(IFC[A-Z0-9]+)"/g)].map((m) => m[1]));
105+
}
106+
90107
/**
91108
* Uppercase names `IfcType::from_str` resolves to a real variant.
92109
*
@@ -198,6 +215,36 @@ export function checkCoverage({ legacySource, schemaSource, oldTables, tableSize
198215
);
199216
}
200217

218+
// THE CONST MUST MIRROR THE ARMS. `LEGACY_ENTITY_NAMES` is public and feeds
219+
// the cross-language rooted-type universe, so an arm that never reaches it
220+
// shrinks that universe silently — which is how the three stratum leaves
221+
// stayed divergent with both halves of that gate green (#3124 review).
222+
//
223+
// This lives HERE rather than in a Rust test because the only way to state it
224+
// in-crate is `include_str!` of the module's own source — a source-text
225+
// assertion, which AGENTS.md bans and `check-rust-source-text-assertions`
226+
// (#3195) flags. Same call the repo already made for
227+
// `check-clash-degenerate-reason-parity.mjs`: a claim about two SOURCES
228+
// belongs in a lint, where reading both is the honest thing rather than the
229+
// banned thing.
230+
const constNames = legacyNameConst(legacySource);
231+
if (constNames.size === 0) {
232+
failures.push(
233+
`no names extracted from LEGACY_ENTITY_NAMES in ${LEGACY_REL} — the extractor has drifted, and two empty sets would otherwise "agree"`,
234+
);
235+
} else {
236+
const missingFromConst = [...keys].filter((k) => !constNames.has(k)).sort();
237+
const extraInConst = [...constNames].filter((k) => !keys.has(k)).sort();
238+
if (missingFromConst.length > 0)
239+
failures.push(
240+
`${LEGACY_REL} has match arms absent from LEGACY_ENTITY_NAMES: ${missingFromConst.join(', ')} — every consumer of that const, the rooted-type sweep's universe included, is blind to them`,
241+
);
242+
if (extraInConst.length > 0)
243+
failures.push(
244+
`LEGACY_ENTITY_NAMES lists ${extraInConst.join(', ')}, which is not a match arm in ${LEGACY_REL}`,
245+
);
246+
}
247+
201248
for (const key of [...keys].sort()) {
202249
if (allTableNames.has(key)) continue;
203250
if (KEYS_ABSENT_FROM_EVERY_BUNDLED_TABLE.has(key)) continue;

scripts/check-legacy-entity-coverage.test.mjs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,44 @@ test("an arm whose key names no entity is reported — the #3172 misspelling", (
102102
assert.match(out, /IfcElectricDistributionPoint .*has no arm in/);
103103
});
104104

105+
test('a match arm absent from LEGACY_ENTITY_NAMES is reported', () => {
106+
// The const is public and feeds the cross-language rooted-type universe
107+
// (dump_rooted_type_sweep.rs), so an arm that never reaches it makes that
108+
// sweep structurally blind to the name -- which is how the three stratum
109+
// leaves stayed divergent with both halves of that gate green (#3124 review).
110+
const real = readFileSync(join(ROOT, LEGACY_REL), 'utf8');
111+
const i = real.indexOf('pub const LEGACY_ENTITY_NAMES');
112+
assert.notEqual(i, -1, 'const anchor drifted');
113+
const target = '"IFCPRESENTATIONSTYLEASSIGNMENT",';
114+
const j = real.indexOf(target, i);
115+
assert.notEqual(j, -1, 'mutation anchor drifted');
116+
const { status, out } = runOn({ [LEGACY_REL]: real.slice(0, j) + real.slice(j + target.length) });
117+
assert.equal(status, 1, out);
118+
assert.match(out, /match arms absent from LEGACY_ENTITY_NAMES.*IFCPRESENTATIONSTYLEASSIGNMENT/);
119+
});
120+
121+
test('a LEGACY_ENTITY_NAMES entry with no match arm is reported', () => {
122+
// The other direction. A name in the const that no arm produces would put a
123+
// phantom into the sweep's universe and read as a real legacy entity.
124+
const real = readFileSync(join(ROOT, LEGACY_REL), 'utf8');
125+
const i = real.indexOf('pub const LEGACY_ENTITY_NAMES');
126+
const j = real.indexOf('[', i) + 1;
127+
const { status, out } = runOn({ [LEGACY_REL]: real.slice(0, j) + '\n "IFCPHANTOMENTITY",' + real.slice(j) });
128+
assert.equal(status, 1, out);
129+
assert.match(out, /IFCPHANTOMENTITY, which is not a match arm/);
130+
});
131+
132+
test('a broken LEGACY_ENTITY_NAMES extractor fails instead of passing vacuously', () => {
133+
// Two empty sets agree about everything. If the const is renamed or the
134+
// block shape changes, this must fail rather than silently compare nothing.
135+
const real = readFileSync(join(ROOT, LEGACY_REL), 'utf8');
136+
const { status, out } = runOn({
137+
[LEGACY_REL]: real.replace('pub const LEGACY_ENTITY_NAMES', 'pub const RENAMED_CONST'),
138+
});
139+
assert.equal(status, 1, out);
140+
assert.match(out, /no names extracted from LEGACY_ENTITY_NAMES/);
141+
});
142+
105143
test('a broken legacy-arm extractor fails instead of passing vacuously', () => {
106144
const { status, out } = runOn({ [LEGACY_REL]: '// every arm gone\n' });
107145
assert.equal(status, 1, out);

0 commit comments

Comments
 (0)