Skip to content

Commit a08b733

Browse files
authored
Merge pull request #387 from PhysShell/fix/fix-candidates-additive-envelope
fix(extractor): --fix-candidates no longer drops orphaned_awaitables (OWN053)
2 parents 7053439 + 514289d commit a08b733

5 files changed

Lines changed: 294 additions & 48 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2205,6 +2205,22 @@ jobs:
22052205
echo "FAIL: flag-off facts drifted from the pre-S0 golden (byte parity broken)"; exit 1
22062206
fi
22072207
echo "OK: fix-candidate metadata correct; flag-off is byte-identical to the pre-S0 golden"
2208+
# ADDITIVE means the flag never REMOVES anything either, and the sample above
2209+
# cannot show that: it has subscriptions and no other section. For as long as
2210+
# `orphaned_awaitables` existed, the flag-on envelope dropped it — every OWN053
2211+
# site gone from the facts — with this step green. So the same invariant
2212+
# (flag-on minus the S0 fields == flag-off, the WHOLE document) is checked on a
2213+
# fixture that makes every top-level section the extractor can write non-empty
2214+
# under --flow-locals. The checker reads that section list off the extractor's
2215+
# own envelope and refuses a pair that leaves one out, so a new section cannot
2216+
# arrive without being covered here.
2217+
ad=tests/fixtures/fix_candidates/AdditiveSections.cs
2218+
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
2219+
"$ad" --flow-locals --fix-candidates -o "$RUNNER_TEMP/ad_on.json"
2220+
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
2221+
"$ad" --flow-locals -o "$RUNNER_TEMP/ad_off.json"
2222+
python tests/check_fix_candidates_facts.py --additive-sections \
2223+
"$RUNNER_TEMP/ad_on.json" "$RUNNER_TEMP/ad_off.json"
22082224
22092225
# S0 Part B: the `own-fix subscriptions candidates` collector turns the fix
22102226
# metadata into a deterministic candidates.json (analysis-only). Reuses fc_on.json.

‎frontend/roslyn/OwnSharp.Extractor/Program.cs‎

Lines changed: 32 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -7280,47 +7280,39 @@ or ImplicitObjectCreationExpressionSyntax } init
72807280
methods_flow_analysed = statMethodsAnalysed,
72817281
methods_skipped_unmodelled = statMethodsSkipped,
72827282
};
7283-
// `fix_candidates_version` is a top-level ADDITIVE metadata field, present ONLY under
7284-
// --fix-candidates; it does not move `ownir_version` (the fact-schema vocabulary is
7285-
// unchanged — no new resource-kind or analysis-routing value). Without the flag the
7286-
// object is byte-for-byte the pre-S0 shape.
7283+
// ONE envelope, built once, in the key order the facts have always been written. A section
7284+
// that is not always there is ONE conditional line here — never a second envelope. There
7285+
// used to be three, picked by a nested conditional (`--fix-candidates` ? A : orphans ? B : C),
7286+
// and each additive section had to be remembered in every one of them: the `--fix-candidates`
7287+
// envelope was written before `orphaned_awaitables` existed and never got it, so the flag
7288+
// that only ADDS fix metadata silently removed every OWN053 site from the facts.
72877289
//
7288-
// Every envelope below stamps the SAME `ownir_version`, the core's current one
7289-
// (ownlang/ownir.py OWNIR_VERSION; tests/test_ownir.py reads every stamp in this file).
7290-
object facts = emitFixCandidates
7291-
? new
7292-
{
7293-
ownir_version = 1,
7294-
fix_candidates_version = 1,
7295-
module = "Extracted",
7296-
components,
7297-
services = factServices,
7298-
functions = flowFunctions,
7299-
stats = factStats,
7300-
}
7301-
// OWN053 (promoted from ownership-semantics-lab H-29): an ADDITIVE top-level list of orphaned awaitables from which
7302-
// both engines mint the advisory; absent when there is no site, so such a document stays byte-identical to the
7303-
// pre-OWN053 shape (and `ownir_version` does not move: the field is additive, like `fix_candidates_version`).
7304-
: OrphanedAwaitables.Sites.Count > 0
7305-
? new
7306-
{
7307-
ownir_version = 1,
7308-
module = "Extracted",
7309-
components,
7310-
services = factServices,
7311-
functions = flowFunctions,
7312-
stats = factStats,
7313-
orphaned_awaitables = OrphanedAwaitables.Sites.OrderBy(o => JsonSerializer.Serialize(o), StringComparer.Ordinal).ToList(),
7314-
}
7315-
: new
7316-
{
7317-
ownir_version = 1,
7318-
module = "Extracted",
7319-
components,
7320-
services = factServices,
7321-
functions = flowFunctions,
7322-
stats = factStats,
7323-
};
7290+
// Every section is written as `facts["<key>"] = ...` and nowhere else: the S0 additivity
7291+
// check (tests/check_fix_candidates_facts.py) reads the section list off these lines, so a
7292+
// section added here is one its fixture must exercise.
7293+
//
7294+
// `ownir_version` is the core's current one (ownlang/ownir.py OWNIR_VERSION). It is spelled
7295+
// as this one literal on purpose: tests/test_ownir.py (IR2) reads every stamp in this file.
7296+
const int ownir_version = 1;
7297+
var facts = new Dictionary<string, object?>();
7298+
facts["ownir_version"] = ownir_version;
7299+
// S0: ADDITIVE metadata, present ONLY under --fix-candidates. It does not move the version
7300+
// (no new resource-kind or analysis-routing value), and removing the S0 fields from a
7301+
// flag-on document gives the flag-off document — every other section included
7302+
// (tests/check_fix_candidates_facts.py holds exactly that).
7303+
if (emitFixCandidates)
7304+
facts["fix_candidates_version"] = 1;
7305+
facts["module"] = "Extracted";
7306+
facts["components"] = components;
7307+
facts["services"] = factServices;
7308+
facts["functions"] = flowFunctions;
7309+
facts["stats"] = factStats;
7310+
// OWN053 (promoted from ownership-semantics-lab H-29): an ADDITIVE top-level list of orphaned
7311+
// awaitables from which both engines mint the advisory. Absent when there is no site, so such
7312+
// a document stays byte-identical to the pre-OWN053 shape.
7313+
if (OrphanedAwaitables.Sites.Count > 0)
7314+
facts["orphaned_awaitables"] = OrphanedAwaitables.Sites
7315+
.OrderBy(o => JsonSerializer.Serialize(o), StringComparer.Ordinal).ToList();
73247316
// P-037 A2.1: a sidecar record that does not satisfy its own vocabulary is a producer
73257317
// defect, and a producer defect must not become a facts file. Refuse the whole run
73267318
// (exit 2, the launcher's "extraction failed, no verdict was produced" tier) rather

‎spec/OwnIR.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,16 @@ A newer extractor that introduces either against an un-bumped core therefore fai
8989
the run instead of mis-analyzing it — which is why both **must** bump
9090
`OWNIR_VERSION` per the table above.
9191

92+
**Additive is a promise about the producer too.** A producer option that adds
93+
metadata must change nothing else in the document it writes. The extractor's
94+
`--fix-candidates` adds `fix_candidates_version`, the component shape fields and
95+
a `fix` block per subscription; take those out of a flag-on document and what is
96+
left **is** the flag-off document — every section, `orphaned_awaitables` (§9)
97+
included. The extractor builds its envelope once for that reason (one conditional
98+
line per optional section, never a second envelope), and
99+
`tests/check_fix_candidates_facts.py --additive-sections` holds the equality on a
100+
fixture that exercises every top-level section the extractor can write.
101+
92102
## 3. What OwnIR is not
93103

94104
Verdict logic never lives in a frontend. The core's diagnostics (OWN0xx) come

‎tests/check_fix_candidates_facts.py‎

Lines changed: 143 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,47 @@
1-
"""Assert the S0 `--fix-candidates` extractor metadata on FixCandidatesSample.cs.
1+
"""Assert the S0 `--fix-candidates` extractor contract at the fact level.
22
33
Not a ``test_*`` (it needs the C# extractor to produce the facts, so CI runs the
4-
extractor first and passes the JSON path). Encodes the Part-A extractor contract
5-
at the fact level; exits non-zero on any violation.
4+
extractor first and passes the JSON paths). Exits non-zero on any violation.
65
76
Usage:
87
python tests/check_fix_candidates_facts.py <fix_on.json> [<off.json>]
8+
python tests/check_fix_candidates_facts.py --additive-sections <on.json> <off.json>
99
1010
fix_on = FixCandidatesSample.cs scanned WITH --fix-candidates
1111
off = the SAME sample WITHOUT the flag (optional; asserts NO fix metadata leaks)
12+
13+
The first form checks the metadata itself on FixCandidatesSample.cs. The second
14+
checks the other half of the contract — the flag only ADDS — on
15+
tests/fixtures/fix_candidates/AdditiveSections.cs scanned with `--flow-locals`,
16+
flag on and flag off:
17+
18+
flag-on minus the S0 fields == flag-off (the whole document)
19+
20+
and refuses a pair that does not exercise every top-level section the extractor
21+
can write (read off the extractor's own envelope), so that the equality cannot
22+
be satisfied by a document with nothing in it. It was: the first form's sample
23+
has subscriptions and no other section, and a flag-on envelope that dropped
24+
`orphaned_awaitables` — every OWN053 site — passed it for as long as it existed.
1225
"""
1326

1427
from __future__ import annotations
1528

1629
import copy
1730
import json
1831
import os
32+
import re
1933
import sys
2034

2135
sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), ".."))
2236

23-
from ownlang.ownir import OWNIR_VERSION
37+
from ownlang.ownir import OWNIR_VERSION, OwnIRError, check_facts
38+
39+
_ROOT = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")
40+
# The ONLY top-level field S0 adds. Everything else under --fix-candidates lives inside
41+
# `components[]` (see `_strip_additive`); a second top-level S0 field is a contract change
42+
# and belongs here, in the open.
43+
_ADDITIVE_TOP_LEVEL = ("fix_candidates_version",)
44+
_EXTRACTOR = os.path.join(_ROOT, "frontend", "roslyn", "OwnSharp.Extractor", "Program.cs")
2445

2546
_ADDITIVE_COMPONENT_KEYS = (
2647
"qualified_name",
@@ -32,9 +53,15 @@
3253

3354

3455
def _strip_additive(facts: dict) -> dict:
35-
"""The flag-ON facts with every S0-additive field removed."""
56+
"""The flag-ON facts with every S0-additive field removed — and nothing else.
57+
58+
This is the whole list of what `--fix-candidates` is allowed to change. It is an
59+
ALLOWLIST of S0's own fields, so the comparison it feeds is over the complete
60+
document: a section the flag drops, reorders or rewrites is a difference, whether
61+
or not anyone thought to name that section in a test."""
3662
f = copy.deepcopy(facts)
37-
f.pop("fix_candidates_version", None)
63+
for k in _ADDITIVE_TOP_LEVEL:
64+
f.pop(k, None)
3865
for c in f.get("components", []):
3966
for k in _ADDITIVE_COMPONENT_KEYS:
4067
c.pop(k, None)
@@ -62,6 +89,111 @@ def _fixes(facts: dict, name: str) -> list[dict]:
6289
return [s["fix"] for s in (comp.get("subscriptions") or []) if s.get("fix")]
6390

6491

92+
def _additivity_problem(on: dict, off: dict) -> str | None:
93+
"""`flag-on minus the S0 fields == flag-off`, or what differs."""
94+
stripped = _strip_additive(on)
95+
detail = []
96+
for k in sorted(k for k in set(stripped) | set(off) if stripped.get(k) != off.get(k)):
97+
if k not in stripped:
98+
detail.append(f"`{k}` is in the flag-off facts and MISSING from the flag-on facts")
99+
elif k not in off:
100+
detail.append(f"`{k}` appears only in the flag-on facts and is not an S0 field")
101+
else:
102+
detail.append(f"`{k}` differs")
103+
if not detail and list(stripped) != list(off):
104+
detail.append(f"the top-level key ORDER differs: {list(stripped)} vs {list(off)}")
105+
if not detail:
106+
return None
107+
return ("flag-on minus the S0 fields must equal flag-off: " + "; ".join(detail)
108+
+ " — --fix-candidates only adds, it never removes or changes another section")
109+
110+
111+
def _extractor_sections() -> list[str]:
112+
"""The top-level keys the extractor's envelope can write, read from its source.
113+
114+
The envelope is built in one place as `facts["<key>"] = ...`. Reading the keys from
115+
there, instead of listing them here, is what makes a NEW section fail this check
116+
until the fixture exercises it."""
117+
with open(_EXTRACTOR, encoding="utf-8") as fh:
118+
keys = re.findall(r'\bfacts\["([a-z_]+)"\]\s*=', fh.read())
119+
return list(dict.fromkeys(keys))
120+
121+
122+
def _findings(facts: dict) -> list[tuple[object, ...]] | str:
123+
try:
124+
return sorted((f.code, f.file, f.line, f.column or 0, f.severity, f.message)
125+
for f in check_facts(facts))
126+
except OwnIRError as e:
127+
return f"refused: {e}"
128+
129+
130+
def additive_sections(on_path: str, off_path: str) -> int:
131+
"""`--additive-sections`: the flag only adds, over EVERY section the extractor writes."""
132+
on, off = _load(on_path), _load(off_path)
133+
fails: list[str] = []
134+
135+
# 1. the pair really is flag-on / flag-off, at the current version
136+
if on.get("fix_candidates_version") != 1 or "fix_candidates_version" in off:
137+
fails.append("the pair is not (flag-on, flag-off): fix_candidates_version must be 1 "
138+
"in the first document and absent from the second")
139+
components = on.get("components")
140+
if not any(s.get("fix") for c in (components if isinstance(components, list) else [])
141+
for s in c.get("subscriptions") or []):
142+
fails.append("the flag-on facts carry no `fix` block: the pair does not exercise S0")
143+
if on.get("ownir_version") != OWNIR_VERSION or off.get("ownir_version") != OWNIR_VERSION:
144+
fails.append(f"ownir_version must be the core's {OWNIR_VERSION} in both documents: "
145+
f"{on.get('ownir_version')!r} with the flag, "
146+
f"{off.get('ownir_version')!r} without")
147+
148+
# 2. not vacuous: every section the extractor CAN write is there, and not empty
149+
sections = [k for k in _extractor_sections() if k not in _ADDITIVE_TOP_LEVEL]
150+
readable = {"ownir_version", "module", "components", "functions"} <= set(sections)
151+
if not readable:
152+
fails.append(f"could not read the extractor's envelope from {_EXTRACTOR} (found "
153+
f"{sections}); the envelope moved — update `_extractor_sections`")
154+
for k in sections:
155+
v = off.get(k)
156+
if k not in off or (isinstance(v, (list, dict)) and not v):
157+
fails.append(f"the fixture does not exercise `{k}`: the extractor can write it, "
158+
f"and the flag-off facts have it "
159+
f"{'empty' if k in off else 'absent'}. "
160+
f"Extend tests/fixtures/fix_candidates/AdditiveSections.cs so the "
161+
f"additivity check covers it")
162+
unknown = sorted(set(off) - set(sections))
163+
if unknown and readable:
164+
fails.append(f"the flag-off facts carry top-level keys this check did not find in the "
165+
f"extractor's envelope: {unknown} — update `_extractor_sections`")
166+
167+
# 3. the invariant itself, over the whole document
168+
problem = _additivity_problem(on, off)
169+
if problem is not None:
170+
fails.append(problem)
171+
172+
# 4. and its consequence: the flag changes no verdict and no advisory
173+
got_on, got_off = _findings(on), _findings(off)
174+
if got_on != got_off:
175+
said = [g if isinstance(g, str) else f"{len(g)} finding(s)" for g in (got_on, got_off)]
176+
fails.append(f"the reference gives different findings with the flag ({said[0]}) "
177+
f"and without it ({said[1]})")
178+
elif isinstance(got_off, str):
179+
fails.append(f"the reference refuses the fixture's facts: {got_off}")
180+
else:
181+
orphans = off.get("orphaned_awaitables") or []
182+
own053 = [f for f in got_off if f[0] == "OWN053"]
183+
if not isinstance(orphans, list) or len(own053) != len(orphans):
184+
fails.append(f"{len(orphans) if isinstance(orphans, list) else '?'} "
185+
f"orphaned_awaitables entr(ies) but {len(own053)} OWN053 advisor(ies)")
186+
187+
if fails:
188+
for fmsg in fails:
189+
print("FAIL:", fmsg, file=sys.stderr)
190+
return 1
191+
print(f"fix-candidates additivity: flag-on minus the S0 fields equals flag-off over "
192+
f"all {len(sections)} sections the extractor writes ({', '.join(sections)}); "
193+
f"the reference gives the same {len(got_off)} finding(s) with and without the flag")
194+
return 0
195+
196+
65197
def main(on_path: str, off_path: str | None) -> int:
66198
on = _load(on_path)
67199
fails: list[str] = []
@@ -247,7 +379,8 @@ def only_fix(name: str) -> dict | None:
247379
# Additivity, positively: strip every additive field from the flag-ON facts and
248380
# the result must EQUAL the flag-off facts (same records, same order, same old
249381
# values) -- enabling the metadata changed nothing pre-existing.
250-
check(_strip_additive(on) == off, "flag-on minus additive fields must equal flag-off")
382+
problem = _additivity_problem(on, off)
383+
check(problem is None, problem or "")
251384

252385
if fails:
253386
for fmsg in fails:
@@ -258,7 +391,9 @@ def only_fix(name: str) -> dict | None:
258391

259392

260393
if __name__ == "__main__":
261-
if len(sys.argv) not in (2, 3):
394+
if len(sys.argv) == 4 and sys.argv[1] == "--additive-sections":
395+
raise SystemExit(additive_sections(sys.argv[2], sys.argv[3]))
396+
if len(sys.argv) not in (2, 3) or sys.argv[1].startswith("--"):
262397
print(__doc__, file=sys.stderr)
263398
raise SystemExit(2)
264399
raise SystemExit(main(sys.argv[1], sys.argv[2] if len(sys.argv) == 3 else None))

0 commit comments

Comments
 (0)