Skip to content

Commit 6b6a0ab

Browse files
authored
Merge pull request #403 from PhysShell/fix/401-extractor-depth
fix(roslyn): a deep if/else-if chain no longer aborts the whole extraction (#401)
2 parents e7d33e3 + f8bd59b commit 6b6a0ab

3 files changed

Lines changed: 236 additions & 2 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,14 @@ jobs:
222222
rustc -O tests/helpers/stage1_stub.rs -o "$RUNNER_TEMP/stage1-stub$ext"
223223
- name: Build the owen launcher
224224
run: dotnet build frontend/roslyn/OwnSharp.Cli/OwnSharp.Cli.csproj -c Release --nologo
225+
# Own.NET#401 on Windows: the wpf-extractor job runs this regression on Linux only, and
226+
# the stack guard it pins (RuntimeHelpers.TryEnsureSufficientExecutionStack) depends on the
227+
# platform's thread stack, so the n = 1000 case is checked on a Windows runner as well.
228+
- name: Deep if / else-if chain does not take the run down, Windows (Own.NET#401)
229+
if: runner.os == 'Windows'
230+
env:
231+
OWN_TIERB_REQUIRED: "1"
232+
run: python tests/test_extractor_deep_flow.py
225233
# The vendored Python core is a PACK-time payload, so a plain build does
226234
# not place it where the launcher looks. Staging it here is harness setup
227235
# for the Python-engine paths, not a production behaviour.
@@ -802,6 +810,14 @@ jobs:
802810
env:
803811
OWN_TIERB_REQUIRED: "1"
804812
run: python tests/test_extractor_columns.py
813+
- name: Deep if / else-if chain does not take the run down (Own.NET#401)
814+
# One method with a tracked local and 31 conditions used to abort the whole extraction
815+
# (System.Text.Json MaxDepth 64). Bodies within spec/OwnIR.md section 4.2 (<= 32) stay analysed;
816+
# deeper ones are skipped and counted, and the core accepts every emitted document.
817+
# REQUIRED here; skips cleanly in the offline Tier-A job.
818+
env:
819+
OWN_TIERB_REQUIRED: "1"
820+
run: python tests/test_extractor_deep_flow.py
805821
- name: Guarded release is not a call-site handoff (INF-S2, #380)
806822
# A callee that releases its parameter only under a guard is a PARTIAL release, so
807823
# its call must not be lowered as the caller's `release` (the fabricated `must`),

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

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3568,6 +3568,35 @@ cc.Filter is null
35683568
// `tracked` local IDisposables. Returns null on any UNMODELLED statement (a `goto`, labeled
35693569
// statement, local function, `lock`/`fixed`, …): the method is then honestly skipped, not
35703570
// guessed. Loops, `try`, `do` and `switch` ARE modelled below.
3571+
// spec/OwnIR.md §4.2: flow bodies nest at most 32 levels (`then` / `else` / `body`; the
3572+
// top-level list is level 0). Both doors reject a document that nests deeper.
3573+
const int OwnIrMaxNestingDepth = 32;
3574+
3575+
// #401: each nested body costs two JSON levels (an object and an array), so a document at the
3576+
// §4.2 limit is about 2 * 32 + 4 levels deep. The default MaxDepth (64) threw on 31 nested
3577+
// `else if`; 128 is the cap §4.2 itself is measured against.
3578+
const int FactsJsonMaxDepth = 128;
3579+
3580+
// The deepest `then` / `else` / `body` nesting of a lowered flow body, counted as §4.2 counts it
3581+
// (an existing list is a level, even an empty one). Flow ops are anonymous objects or
3582+
// dictionaries, so the three keys are read by name.
3583+
static int FlowNestingDepth(IReadOnlyList<object> nodes, int level = 0)
3584+
{
3585+
var deepest = level;
3586+
foreach (var node in nodes)
3587+
{
3588+
foreach (var key in new[] { "then", "else", "body" })
3589+
{
3590+
object? child = node is IDictionary<string, object?> d
3591+
? (d.TryGetValue(key, out var v) ? v : null)
3592+
: node?.GetType().GetProperty(key)?.GetValue(node);
3593+
if (child is IReadOnlyList<object> list)
3594+
deepest = Math.Max(deepest, FlowNestingDepth(list, level + 1));
3595+
}
3596+
}
3597+
return deepest;
3598+
}
3599+
35713600
static List<object>? LowerFlowBody(BlockSyntax block, HashSet<string> tracked, SemanticModel model)
35723601
{
35733602
var nodes = new List<object>();
@@ -3650,6 +3679,12 @@ static bool LowerFlowStmt(StatementSyntax st, HashSet<string> tracked, SemanticM
36503679
bool canEscape = true, List<object>? onThrow = null,
36513680
List<object>? onReturn = null, bool onThrowDefinite = false)
36523681
{
3682+
// #401: the lowering recurses once per nested statement, so a few hundred `else if`
3683+
// overflowed the stack - an uncatchable crash that lost the whole run. Running short of
3684+
// stack means the body is far deeper than spec/OwnIR.md section 4.2 admits (32), which is
3685+
// skipped anyway: bail the method as unmodelled instead.
3686+
if (!System.Runtime.CompilerServices.RuntimeHelpers.TryEnsureSufficientExecutionStack())
3687+
return false;
36533688
switch (st)
36543689
{
36553690
case BlockSyntax b:
@@ -7197,6 +7232,14 @@ or ImplicitObjectCreationExpressionSyntax } init
71977232
statMethodsSkipped++; // unmodelled construct -> honestly skipped
71987233
continue;
71997234
}
7235+
// #401: a body nesting deeper than spec/OwnIR.md §4.2 admits would make BOTH
7236+
// doors reject the whole document, so it is skipped like any other unmodelled
7237+
// shape: this method loses its flow facts, the rest of the run keeps theirs.
7238+
if (FlowNestingDepth(fbody) > OwnIrMaxNestingDepth)
7239+
{
7240+
statMethodsSkipped++;
7241+
continue;
7242+
}
72007243
statMethodsAnalysed++;
72017244
// Stage 2 (spec §5.1): stamp the record's per-overload `sig` when the
72027245
// symbol resolves, so an overloaded method gets its own summary beside
@@ -7337,7 +7380,7 @@ or ImplicitObjectCreationExpressionSyntax } init
73377380
Console.Error.WriteLine($"extractor: guarded_facts self-check failed: {violation}");
73387381
return 2;
73397382
}
7340-
var json = JsonSerializer.Serialize(facts, new JsonSerializerOptions { WriteIndented = true });
7383+
var json = JsonSerializer.Serialize(facts, new JsonSerializerOptions { WriteIndented = true, MaxDepth = FactsJsonMaxDepth });
73417384

73427385
if (reportStats)
73437386
Console.Error.WriteLine(
@@ -7352,7 +7395,7 @@ or ImplicitObjectCreationExpressionSyntax } init
73527395
if (heapEffectsPath is not null)
73537396
File.WriteAllText(heapEffectsPath, JsonSerializer.Serialize(
73547397
HeapEffectFacts.Extract(compilation, parsed),
7355-
new JsonSerializerOptions { WriteIndented = true }));
7398+
new JsonSerializerOptions { WriteIndented = true, MaxDepth = FactsJsonMaxDepth }));
73567399
return 0;
73577400

73587401
// Opt-in recall knob for the flow pass, read deep in InjectThrowEdge (a static field rather than

‎tests/test_extractor_deep_flow.py‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
#!/usr/bin/env python3
2+
"""Own.NET#401 - a deep `if / else if` chain must not take the whole run down.
3+
4+
THE DEFECT
5+
6+
The flow pass lowers every `if` to `{op:"if", then:[...], else:[...]}`, so each `else if`
7+
nests two JSON levels (an object and an array). The facts document was serialised with
8+
System.Text.Json's default MaxDepth of 64, so ONE method with a tracked disposable local and
9+
31 conditions threw `JsonException` at the final serialise: no facts, no SARIF, every other
10+
method in the repository unanalysed (couchbase-net-client, dnSpy).
11+
12+
WHAT THIS TEST PINS (each a separate PASS/KILL criterion of the fix)
13+
14+
P1 n <= 32 conditions (flow nesting <= 32, the spec/OwnIR.md §4.2 limit): the extractor
15+
exits 0, the method IS flow-analysed, its body nests exactly n levels, and the core
16+
accepts the document. A fix that dodges the crash by skipping these methods fails here
17+
(it would lose coverage the spec allows).
18+
P2 n >= 33 (deeper than the spec admits): the extractor exits 0, the method is honestly
19+
skipped and counted in `methods_skipped_unmodelled`, and the core still accepts the
20+
document - so the crash is not merely moved from the extractor to the core.
21+
P3 In every case a sibling method with a real undisposed local is still reported, i.e.
22+
one deep method never costs the rest of the file its analysis.
23+
P4 n = 1000: the lowering recursion itself used to overflow the stack (an uncatchable
24+
crash, the whole run lost) somewhere between 300 and 600 conditions; it must bail the
25+
method as unmodelled instead (P2 and P3 apply).
26+
27+
The boundary n = 32 / 33 is the spec's, not a tuning knob: `then`/`else` bodies are levels,
28+
the top-level body is level 0, so the k-th condition's `then` sits at level k+1 and a chain
29+
of n conditions nests exactly n.
30+
31+
REQUIRED VS SKIPPED: as tests/test_extractor_columns.py - required when OWN_TIERB_REQUIRED=1
32+
(the wpf-extractor CI job), a clean, explained skip when dotnet is absent.
33+
34+
Run: OWN_TIERB_REQUIRED=1 python3 tests/test_extractor_deep_flow.py
35+
"""
36+
37+
from __future__ import annotations
38+
39+
import json
40+
import os
41+
import shutil
42+
import subprocess
43+
import sys
44+
import tempfile
45+
46+
_REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
47+
_EXT = os.path.join(_REPO, "frontend", "roslyn", "OwnSharp.Extractor")
48+
_LIMIT = 32 # spec/OwnIR.md §4.2: a body nesting exactly 32 is accepted, 33 rejected
49+
_CASES = (31, 32, 33, 200, 1000)
50+
51+
52+
def _source(n: int) -> str:
53+
lines = [
54+
"using System.IO;",
55+
"",
56+
"namespace Repro;",
57+
"",
58+
"public static class Chain",
59+
"{",
60+
" public static int Classify(int x)",
61+
" {",
62+
" var ms = new MemoryStream();",
63+
" int r;",
64+
]
65+
for i in range(n):
66+
kw = "if" if i == 0 else "else if"
67+
lines.append(f" {kw} (x == {i}) {{ r = {i}; }}")
68+
lines += [
69+
" else { r = -1; }",
70+
" ms.Dispose();",
71+
" return r;",
72+
" }",
73+
"",
74+
" public static void Leaky()",
75+
" {",
76+
" var leaked = new MemoryStream();",
77+
" leaked.WriteByte(1);",
78+
" }",
79+
"}",
80+
"",
81+
]
82+
return "\n".join(lines)
83+
84+
85+
def _depth(nodes: object, level: int = 0) -> int:
86+
"""Deepest body level, counted like spec §4.2 / ownir._check_flow_coordinates."""
87+
if not isinstance(nodes, list):
88+
return -1
89+
deepest = level
90+
for n in nodes:
91+
if isinstance(n, dict):
92+
for key in ("then", "else", "body"):
93+
deepest = max(deepest, _depth(n.get(key), level + 1))
94+
return deepest
95+
96+
97+
def _run_case(n: int, work: str, fails: list[str]) -> None:
98+
src = os.path.join(work, f"n{n}")
99+
os.makedirs(src)
100+
with open(os.path.join(src, "Chain.cs"), "w", encoding="utf-8", newline="\n") as f:
101+
f.write(_source(n))
102+
facts_path = os.path.join(work, f"n{n}.facts.json")
103+
ext = subprocess.run(
104+
["dotnet", "run", "--project", _EXT, "--",
105+
src, "-o", facts_path, "--flow-locals", "--stats"],
106+
cwd=_REPO, capture_output=True, text=True, encoding="utf-8", errors="replace")
107+
tag = f"n={n}"
108+
if ext.returncode != 0 or not os.path.exists(facts_path):
109+
tail = (ext.stderr or ext.stdout).strip().splitlines()[-3:]
110+
joined = " | ".join(tail)
111+
fails.append(f"{tag}: extractor rc={ext.returncode}, no facts ({joined})")
112+
return
113+
with open(facts_path, encoding="utf-8") as f:
114+
facts = json.load(f)
115+
fns = {fn.get("name"): fn for fn in facts.get("functions", [])}
116+
chain = fns.get("Repro.Chain.Classify")
117+
skipped = (facts.get("stats") or {}).get("methods_skipped_unmodelled")
118+
if n <= _LIMIT:
119+
if chain is None:
120+
fails.append(f"{tag}: P1 Classify was not flow-analysed "
121+
"(coverage the spec allows was lost)")
122+
elif _depth(chain.get("body")) != n:
123+
got = _depth(chain.get("body"))
124+
fails.append(f"{tag}: P1 Classify nests {got} levels, expected {n}")
125+
else:
126+
if chain is not None:
127+
got = _depth(chain.get("body"))
128+
fails.append(f"{tag}: P2 Classify nests {got} > {_LIMIT} but was emitted")
129+
if not isinstance(skipped, int) or skipped < 1:
130+
fails.append(f"{tag}: P2 the skip is not counted "
131+
f"(methods_skipped_unmodelled={skipped!r})")
132+
env = dict(os.environ, PYTHONPATH=_REPO)
133+
core = subprocess.run(
134+
[sys.executable, "-m", "ownlang", "ownir", facts_path,
135+
"--format", "sarif", "--severity", "warning"],
136+
cwd=_REPO, capture_output=True, text=True, encoding="utf-8", errors="replace", env=env)
137+
try:
138+
results = json.loads(core.stdout)["runs"][0]["results"]
139+
except (ValueError, KeyError, IndexError):
140+
fails.append(f"{tag}: core rejected the document (rc={core.returncode}): "
141+
f"{(core.stderr or core.stdout).strip()[-200:]}")
142+
return
143+
if not any("leaked" in r.get("message", {}).get("text", "") for r in results):
144+
fails.append(f"{tag}: P3 the sibling leak in Leaky() is not reported")
145+
146+
147+
def run() -> int:
148+
# Failure text can carry U+FFFD from a crashed extractor; never let printing it fail the run.
149+
if hasattr(sys.stdout, "reconfigure"):
150+
sys.stdout.reconfigure(errors="backslashreplace")
151+
if not shutil.which("dotnet"):
152+
if os.environ.get("OWN_TIERB_REQUIRED") == "1":
153+
print("extractor deep flow: FAIL - dotnet is REQUIRED here "
154+
"(OWN_TIERB_REQUIRED=1) and is not on PATH")
155+
return 1
156+
print("extractor deep flow: SKIP - dotnet not on PATH")
157+
return 0
158+
fails: list[str] = []
159+
work = tempfile.mkdtemp(prefix="own401-")
160+
try:
161+
for n in _CASES:
162+
_run_case(n, work, fails)
163+
finally:
164+
shutil.rmtree(work, ignore_errors=True)
165+
if fails:
166+
print("extractor deep flow: FAIL")
167+
for f in fails:
168+
print(" -", f)
169+
return 1
170+
print(f"extractor deep flow: OK ({len(_CASES)} chain lengths, limit {_LIMIT})")
171+
return 0
172+
173+
174+
if __name__ == "__main__":
175+
raise SystemExit(run())

0 commit comments

Comments
 (0)