From 514289db1eecc075788d30e95f9d6fef6a951bc4 Mon Sep 17 00:00:00 2001 From: PhysShell <45852143+PhysShell@users.noreply.github.com> Date: Fri, 2 Oct 2026 06:55:45 +0500 Subject: [PATCH] fix(extractor): --fix-candidates no longer drops orphaned_awaitables (OWN053) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--fix-candidates` is additive: it adds fix metadata and changes nothing else. It removed a section. The same input, the same build: --flow-locals orphaned_awaitables: 5 sites --flow-locals --fix-candidates orphaned_awaitables: absent so every OWN053 advisory disappeared from a run that asked for fix candidates (corpus/ownership-lab/h29/fx/Orphan.cs: 5 advisories without the flag, 0 with it, on both engines). Root cause: the extractor built its envelope in three places, picked by a nested conditional — `--fix-candidates` ? A : orphans ? B : C. The `--fix-candidates` envelope was written before `orphaned_awaitables` existed and was never given it. Each additive section had to be remembered in every envelope, and one was not. The fix is the mechanism, not the missing line: ONE envelope, built once, in the key order the facts have always had; a section that is not always there is one conditional line. There is no second envelope for the next section to be forgotten in. Output without the flag is unchanged byte for byte, and so is every flag-on output that had nothing to lose: 256 outputs compared with main's build (every C# input CI scans and the two OWN053 fixtures, with and without --flow-locals, flag on and off) — 254 byte-identical, 2 repaired (they gain exactly the section they used to drop), 0 other. Why the S0 control did not see it. The invariant it holds was already the right one — flag-on minus the S0 fields == flag-off, the whole document. The document was the problem: FixCandidatesSample.cs has subscriptions and no other section, and CI scans it without --flow-locals, so nothing the flag could drop was ever there. Now: * tests/fixtures/fix_candidates/AdditiveSections.cs makes every top-level section the extractor can write non-empty under --flow-locals (both OWN053 families included, with no reference directory); * `check_fix_candidates_facts.py --additive-sections` holds the equality on it, reads the section list off the extractor's own envelope (`facts[""] = ...`) and refuses a pair that leaves a section out — so a section added to the extractor is red until the fixture exercises it — and requires the reference to give the same findings for both documents; * the CI step "S0 fix-candidates — extractor metadata (Part A)" runs it. Negative controls: on main's extractor the new check fails (`orphaned_awaitables` is in the flag-off facts and MISSING from the flag-on facts; 3 findings with the flag, 5 without); with the section dropped under the flag in this tree it fails the same way and passes again once restored; an envelope key the fixture does not exercise fails it by name; and the old sample, given to the new mode, is refused as not exercising `services`, `functions` and `orphaned_awaitables`. No OwnIR vocabulary or version change (OWNIR_VERSION 1, LOWERED_VERSION 2), no change to the OWN053 rule, to the state-protocol lowering or to the measurement instrument. spec/OwnIR.md §2 states the producer-side rule. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/ci.yml | 16 ++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 72 ++++----- spec/OwnIR.md | 10 ++ tests/check_fix_candidates_facts.py | 151 +++++++++++++++++- .../fix_candidates/AdditiveSections.cs | 93 +++++++++++ 5 files changed, 294 insertions(+), 48 deletions(-) create mode 100644 tests/fixtures/fix_candidates/AdditiveSections.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a2af56b3..d30d4011 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2205,6 +2205,22 @@ jobs: echo "FAIL: flag-off facts drifted from the pre-S0 golden (byte parity broken)"; exit 1 fi echo "OK: fix-candidate metadata correct; flag-off is byte-identical to the pre-S0 golden" + # ADDITIVE means the flag never REMOVES anything either, and the sample above + # cannot show that: it has subscriptions and no other section. For as long as + # `orphaned_awaitables` existed, the flag-on envelope dropped it — every OWN053 + # site gone from the facts — with this step green. So the same invariant + # (flag-on minus the S0 fields == flag-off, the WHOLE document) is checked on a + # fixture that makes every top-level section the extractor can write non-empty + # under --flow-locals. The checker reads that section list off the extractor's + # own envelope and refuses a pair that leaves one out, so a new section cannot + # arrive without being covered here. + ad=tests/fixtures/fix_candidates/AdditiveSections.cs + dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \ + "$ad" --flow-locals --fix-candidates -o "$RUNNER_TEMP/ad_on.json" + dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \ + "$ad" --flow-locals -o "$RUNNER_TEMP/ad_off.json" + python tests/check_fix_candidates_facts.py --additive-sections \ + "$RUNNER_TEMP/ad_on.json" "$RUNNER_TEMP/ad_off.json" # S0 Part B: the `own-fix subscriptions candidates` collector turns the fix # metadata into a deterministic candidates.json (analysis-only). Reuses fc_on.json. diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 10e5074d..bf923259 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -7280,47 +7280,39 @@ or ImplicitObjectCreationExpressionSyntax } init methods_flow_analysed = statMethodsAnalysed, methods_skipped_unmodelled = statMethodsSkipped, }; -// `fix_candidates_version` is a top-level ADDITIVE metadata field, present ONLY under -// --fix-candidates; it does not move `ownir_version` (the fact-schema vocabulary is -// unchanged — no new resource-kind or analysis-routing value). Without the flag the -// object is byte-for-byte the pre-S0 shape. +// ONE envelope, built once, in the key order the facts have always been written. A section +// that is not always there is ONE conditional line here — never a second envelope. There +// used to be three, picked by a nested conditional (`--fix-candidates` ? A : orphans ? B : C), +// and each additive section had to be remembered in every one of them: the `--fix-candidates` +// envelope was written before `orphaned_awaitables` existed and never got it, so the flag +// that only ADDS fix metadata silently removed every OWN053 site from the facts. // -// Every envelope below stamps the SAME `ownir_version`, the core's current one -// (ownlang/ownir.py OWNIR_VERSION; tests/test_ownir.py reads every stamp in this file). -object facts = emitFixCandidates - ? new - { - ownir_version = 1, - fix_candidates_version = 1, - module = "Extracted", - components, - services = factServices, - functions = flowFunctions, - stats = factStats, - } - // OWN053 (promoted from ownership-semantics-lab H-29): an ADDITIVE top-level list of orphaned awaitables from which - // both engines mint the advisory; absent when there is no site, so such a document stays byte-identical to the - // pre-OWN053 shape (and `ownir_version` does not move: the field is additive, like `fix_candidates_version`). - : OrphanedAwaitables.Sites.Count > 0 - ? new - { - ownir_version = 1, - module = "Extracted", - components, - services = factServices, - functions = flowFunctions, - stats = factStats, - orphaned_awaitables = OrphanedAwaitables.Sites.OrderBy(o => JsonSerializer.Serialize(o), StringComparer.Ordinal).ToList(), - } - : new - { - ownir_version = 1, - module = "Extracted", - components, - services = factServices, - functions = flowFunctions, - stats = factStats, - }; +// Every section is written as `facts[""] = ...` and nowhere else: the S0 additivity +// check (tests/check_fix_candidates_facts.py) reads the section list off these lines, so a +// section added here is one its fixture must exercise. +// +// `ownir_version` is the core's current one (ownlang/ownir.py OWNIR_VERSION). It is spelled +// as this one literal on purpose: tests/test_ownir.py (IR2) reads every stamp in this file. +const int ownir_version = 1; +var facts = new Dictionary(); +facts["ownir_version"] = ownir_version; +// S0: ADDITIVE metadata, present ONLY under --fix-candidates. It does not move the version +// (no new resource-kind or analysis-routing value), and removing the S0 fields from a +// flag-on document gives the flag-off document — every other section included +// (tests/check_fix_candidates_facts.py holds exactly that). +if (emitFixCandidates) + facts["fix_candidates_version"] = 1; +facts["module"] = "Extracted"; +facts["components"] = components; +facts["services"] = factServices; +facts["functions"] = flowFunctions; +facts["stats"] = factStats; +// OWN053 (promoted from ownership-semantics-lab H-29): an ADDITIVE top-level list of orphaned +// awaitables from which both engines mint the advisory. Absent when there is no site, so such +// a document stays byte-identical to the pre-OWN053 shape. +if (OrphanedAwaitables.Sites.Count > 0) + facts["orphaned_awaitables"] = OrphanedAwaitables.Sites + .OrderBy(o => JsonSerializer.Serialize(o), StringComparer.Ordinal).ToList(); // P-037 A2.1: a sidecar record that does not satisfy its own vocabulary is a producer // defect, and a producer defect must not become a facts file. Refuse the whole run // (exit 2, the launcher's "extraction failed, no verdict was produced" tier) rather diff --git a/spec/OwnIR.md b/spec/OwnIR.md index 405c59f3..559cae8f 100644 --- a/spec/OwnIR.md +++ b/spec/OwnIR.md @@ -89,6 +89,16 @@ A newer extractor that introduces either against an un-bumped core therefore fai the run instead of mis-analyzing it — which is why both **must** bump `OWNIR_VERSION` per the table above. +**Additive is a promise about the producer too.** A producer option that adds +metadata must change nothing else in the document it writes. The extractor's +`--fix-candidates` adds `fix_candidates_version`, the component shape fields and +a `fix` block per subscription; take those out of a flag-on document and what is +left **is** the flag-off document — every section, `orphaned_awaitables` (§9) +included. The extractor builds its envelope once for that reason (one conditional +line per optional section, never a second envelope), and +`tests/check_fix_candidates_facts.py --additive-sections` holds the equality on a +fixture that exercises every top-level section the extractor can write. + ## 3. What OwnIR is not Verdict logic never lives in a frontend. The core's diagnostics (OWN0xx) come diff --git a/tests/check_fix_candidates_facts.py b/tests/check_fix_candidates_facts.py index 9e5fac72..35e2fc20 100644 --- a/tests/check_fix_candidates_facts.py +++ b/tests/check_fix_candidates_facts.py @@ -1,14 +1,27 @@ -"""Assert the S0 `--fix-candidates` extractor metadata on FixCandidatesSample.cs. +"""Assert the S0 `--fix-candidates` extractor contract at the fact level. Not a ``test_*`` (it needs the C# extractor to produce the facts, so CI runs the -extractor first and passes the JSON path). Encodes the Part-A extractor contract -at the fact level; exits non-zero on any violation. +extractor first and passes the JSON paths). Exits non-zero on any violation. Usage: python tests/check_fix_candidates_facts.py [] + python tests/check_fix_candidates_facts.py --additive-sections fix_on = FixCandidatesSample.cs scanned WITH --fix-candidates off = the SAME sample WITHOUT the flag (optional; asserts NO fix metadata leaks) + +The first form checks the metadata itself on FixCandidatesSample.cs. The second +checks the other half of the contract — the flag only ADDS — on +tests/fixtures/fix_candidates/AdditiveSections.cs scanned with `--flow-locals`, +flag on and flag off: + + flag-on minus the S0 fields == flag-off (the whole document) + +and refuses a pair that does not exercise every top-level section the extractor +can write (read off the extractor's own envelope), so that the equality cannot +be satisfied by a document with nothing in it. It was: the first form's sample +has subscriptions and no other section, and a flag-on envelope that dropped +`orphaned_awaitables` — every OWN053 site — passed it for as long as it existed. """ from __future__ import annotations @@ -16,11 +29,19 @@ import copy import json import os +import re import sys sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")) -from ownlang.ownir import OWNIR_VERSION +from ownlang.ownir import OWNIR_VERSION, OwnIRError, check_facts + +_ROOT = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..") +# The ONLY top-level field S0 adds. Everything else under --fix-candidates lives inside +# `components[]` (see `_strip_additive`); a second top-level S0 field is a contract change +# and belongs here, in the open. +_ADDITIVE_TOP_LEVEL = ("fix_candidates_version",) +_EXTRACTOR = os.path.join(_ROOT, "frontend", "roslyn", "OwnSharp.Extractor", "Program.cs") _ADDITIVE_COMPONENT_KEYS = ( "qualified_name", @@ -32,9 +53,15 @@ def _strip_additive(facts: dict) -> dict: - """The flag-ON facts with every S0-additive field removed.""" + """The flag-ON facts with every S0-additive field removed — and nothing else. + + This is the whole list of what `--fix-candidates` is allowed to change. It is an + ALLOWLIST of S0's own fields, so the comparison it feeds is over the complete + document: a section the flag drops, reorders or rewrites is a difference, whether + or not anyone thought to name that section in a test.""" f = copy.deepcopy(facts) - f.pop("fix_candidates_version", None) + for k in _ADDITIVE_TOP_LEVEL: + f.pop(k, None) for c in f.get("components", []): for k in _ADDITIVE_COMPONENT_KEYS: c.pop(k, None) @@ -62,6 +89,111 @@ def _fixes(facts: dict, name: str) -> list[dict]: return [s["fix"] for s in (comp.get("subscriptions") or []) if s.get("fix")] +def _additivity_problem(on: dict, off: dict) -> str | None: + """`flag-on minus the S0 fields == flag-off`, or what differs.""" + stripped = _strip_additive(on) + detail = [] + for k in sorted(k for k in set(stripped) | set(off) if stripped.get(k) != off.get(k)): + if k not in stripped: + detail.append(f"`{k}` is in the flag-off facts and MISSING from the flag-on facts") + elif k not in off: + detail.append(f"`{k}` appears only in the flag-on facts and is not an S0 field") + else: + detail.append(f"`{k}` differs") + if not detail and list(stripped) != list(off): + detail.append(f"the top-level key ORDER differs: {list(stripped)} vs {list(off)}") + if not detail: + return None + return ("flag-on minus the S0 fields must equal flag-off: " + "; ".join(detail) + + " — --fix-candidates only adds, it never removes or changes another section") + + +def _extractor_sections() -> list[str]: + """The top-level keys the extractor's envelope can write, read from its source. + + The envelope is built in one place as `facts[""] = ...`. Reading the keys from + there, instead of listing them here, is what makes a NEW section fail this check + until the fixture exercises it.""" + with open(_EXTRACTOR, encoding="utf-8") as fh: + keys = re.findall(r'\bfacts\["([a-z_]+)"\]\s*=', fh.read()) + return list(dict.fromkeys(keys)) + + +def _findings(facts: dict) -> list[tuple[object, ...]] | str: + try: + return sorted((f.code, f.file, f.line, f.column or 0, f.severity, f.message) + for f in check_facts(facts)) + except OwnIRError as e: + return f"refused: {e}" + + +def additive_sections(on_path: str, off_path: str) -> int: + """`--additive-sections`: the flag only adds, over EVERY section the extractor writes.""" + on, off = _load(on_path), _load(off_path) + fails: list[str] = [] + + # 1. the pair really is flag-on / flag-off, at the current version + if on.get("fix_candidates_version") != 1 or "fix_candidates_version" in off: + fails.append("the pair is not (flag-on, flag-off): fix_candidates_version must be 1 " + "in the first document and absent from the second") + components = on.get("components") + if not any(s.get("fix") for c in (components if isinstance(components, list) else []) + for s in c.get("subscriptions") or []): + fails.append("the flag-on facts carry no `fix` block: the pair does not exercise S0") + if on.get("ownir_version") != OWNIR_VERSION or off.get("ownir_version") != OWNIR_VERSION: + fails.append(f"ownir_version must be the core's {OWNIR_VERSION} in both documents: " + f"{on.get('ownir_version')!r} with the flag, " + f"{off.get('ownir_version')!r} without") + + # 2. not vacuous: every section the extractor CAN write is there, and not empty + sections = [k for k in _extractor_sections() if k not in _ADDITIVE_TOP_LEVEL] + readable = {"ownir_version", "module", "components", "functions"} <= set(sections) + if not readable: + fails.append(f"could not read the extractor's envelope from {_EXTRACTOR} (found " + f"{sections}); the envelope moved — update `_extractor_sections`") + for k in sections: + v = off.get(k) + if k not in off or (isinstance(v, (list, dict)) and not v): + fails.append(f"the fixture does not exercise `{k}`: the extractor can write it, " + f"and the flag-off facts have it " + f"{'empty' if k in off else 'absent'}. " + f"Extend tests/fixtures/fix_candidates/AdditiveSections.cs so the " + f"additivity check covers it") + unknown = sorted(set(off) - set(sections)) + if unknown and readable: + fails.append(f"the flag-off facts carry top-level keys this check did not find in the " + f"extractor's envelope: {unknown} — update `_extractor_sections`") + + # 3. the invariant itself, over the whole document + problem = _additivity_problem(on, off) + if problem is not None: + fails.append(problem) + + # 4. and its consequence: the flag changes no verdict and no advisory + got_on, got_off = _findings(on), _findings(off) + if got_on != got_off: + said = [g if isinstance(g, str) else f"{len(g)} finding(s)" for g in (got_on, got_off)] + fails.append(f"the reference gives different findings with the flag ({said[0]}) " + f"and without it ({said[1]})") + elif isinstance(got_off, str): + fails.append(f"the reference refuses the fixture's facts: {got_off}") + else: + orphans = off.get("orphaned_awaitables") or [] + own053 = [f for f in got_off if f[0] == "OWN053"] + if not isinstance(orphans, list) or len(own053) != len(orphans): + fails.append(f"{len(orphans) if isinstance(orphans, list) else '?'} " + f"orphaned_awaitables entr(ies) but {len(own053)} OWN053 advisor(ies)") + + if fails: + for fmsg in fails: + print("FAIL:", fmsg, file=sys.stderr) + return 1 + print(f"fix-candidates additivity: flag-on minus the S0 fields equals flag-off over " + f"all {len(sections)} sections the extractor writes ({', '.join(sections)}); " + f"the reference gives the same {len(got_off)} finding(s) with and without the flag") + return 0 + + def main(on_path: str, off_path: str | None) -> int: on = _load(on_path) fails: list[str] = [] @@ -247,7 +379,8 @@ def only_fix(name: str) -> dict | None: # Additivity, positively: strip every additive field from the flag-ON facts and # the result must EQUAL the flag-off facts (same records, same order, same old # values) -- enabling the metadata changed nothing pre-existing. - check(_strip_additive(on) == off, "flag-on minus additive fields must equal flag-off") + problem = _additivity_problem(on, off) + check(problem is None, problem or "") if fails: for fmsg in fails: @@ -258,7 +391,9 @@ def only_fix(name: str) -> dict | None: if __name__ == "__main__": - if len(sys.argv) not in (2, 3): + if len(sys.argv) == 4 and sys.argv[1] == "--additive-sections": + raise SystemExit(additive_sections(sys.argv[2], sys.argv[3])) + if len(sys.argv) not in (2, 3) or sys.argv[1].startswith("--"): print(__doc__, file=sys.stderr) raise SystemExit(2) raise SystemExit(main(sys.argv[1], sys.argv[2] if len(sys.argv) == 3 else None)) diff --git a/tests/fixtures/fix_candidates/AdditiveSections.cs b/tests/fixtures/fix_candidates/AdditiveSections.cs new file mode 100644 index 00000000..91d99d10 --- /dev/null +++ b/tests/fixtures/fix_candidates/AdditiveSections.cs @@ -0,0 +1,93 @@ +using System; +using System.ComponentModel; +using System.IO; +using System.Threading.Tasks; + +// S0 additivity fixture — read by `tests/check_fix_candidates_facts.py --additive-sections` +// (the CI step "S0 fix-candidates — extractor metadata (Part A)"). +// +// `--fix-candidates` only ADDS: take the S0 fields out of a flag-on document and what is left +// must be the flag-off document. That claim is only as strong as the document it is checked +// on, and FixCandidatesSample.cs has subscriptions and nothing else — so a flag-on envelope +// that dropped a whole section (it dropped `orphaned_awaitables`) passed it. This one file +// makes EVERY top-level section the Roslyn extractor can write non-empty under +// `--flow-locals`, and the checker refuses the pair if a section the extractor can write is +// missing from it. A new section therefore means a new block here, or a red build. +// +// It lives under tests/fixtures/, not frontend/roslyn/samples/: that directory is scanned as +// a whole by jobs that pin its findings. It is self-contained on purpose (no reference +// directory): the two OWN053 families are reached through types declared in this file. +namespace Own.Samples.FixCandidates.Additive +{ + // components[] — a subscription that is never released (under --fix-candidates it also + // carries the S0 `fix` block, and its component the S0 shape fields). + public sealed class Subscriber + { + private readonly INotifyPropertyChanged _source; + + public Subscriber(INotifyPropertyChanged source) + { + _source = source; + _source.PropertyChanged += OnChanged; + } + + private void OnChanged(object? sender, PropertyChangedEventArgs e) { } + } + + // services[] — a DI registration graph. The extraction is syntactic, so the surface only + // has to parse. + public sealed class ScopedThing { } + + public sealed class Holder { public Holder(ScopedThing thing) { } } + + public interface IRegistrar + { + IRegistrar AddScoped(); + IRegistrar AddSingleton(); + } + + public static class Registration + { + public static void Configure(IRegistrar services) + { + services.AddScoped(); + services.AddSingleton(); + } + } + + // functions[] — a flow-sensitive disposable local. + public static class Flow + { + public static long Leak(string path) + { + var stream = new FileStream(path, FileMode.Open); + return stream.Length; + } + } + + // orphaned_awaitables[] — both frozen OWN053 families. + public sealed class Connection : IDisposable + { + private bool _open = true; + + public bool IsOpen => _open; + + public void Dispose() { _open = false; } + } + + public sealed class Store + { + public Task OpenConnectionAsync() => Task.FromResult(new Connection()); + + public Task CommitAsync() => Task.CompletedTask; + } + + public static class Orphans + { + public static void Run(Store store) + { + var connection = store.OpenConnectionAsync(); // A_owned_result: a disposable, never observed + var commit = store.CommitAsync(); // B_protocol_lifecycle: a lifecycle call, never observed + } + } +}