From f88fe63a1489e9afdacfa000b475bd937b58be1a Mon Sep 17 00:00:00 2001 From: PhysShell <45852143+PhysShell@users.noreply.github.com> Date: Sun, 18 Oct 2026 09:56:28 +0500 Subject: [PATCH] fix(roslyn): same-named locals in sibling scopes are no longer conflated (#408) The --flow-locals pass keys locals by name. C# lets two different locals share a name when their scopes are disjoint, so a reference to an untracked local named like a tracked one became an op on the tracked local: - in a sibling branch, a `use` with no `acquire` on that path made the core abort the whole document (internal OWN030 "undefined name", exit 2; MessagePack-CSharp ToLZ4BinaryCore); - in a later block, a `use` after the tracked local's release was a false OWN002 use-after-return. Candidates are now remembered by declarator. Every scope that declares an untracked same-named local (its block, or the foreach / for / using / switch / fixed statement it belongs to; `out var` / pattern designations to their block) hides that name from `tracked` inside it. Scopes of two same-named locals never overlap (CS0128 / CS0136), so inside that scope the name can only mean the untracked local. Extractor-only: OwnIR and both cores are unchanged. Closes #408 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 7 + frontend/roslyn/OwnSharp.Extractor/Program.cs | 91 ++++++- tests/test_extractor_sibling_locals.py | 246 ++++++++++++++++++ 3 files changed, 337 insertions(+), 7 deletions(-) create mode 100644 tests/test_extractor_sibling_locals.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e52e923..97afd043 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -818,6 +818,13 @@ jobs: env: OWN_TIERB_REQUIRED: "1" run: python tests/test_extractor_deep_flow.py + - name: Same-named locals in sibling scopes are not conflated (Own.NET#408) + # The flow pass keys locals by name: an untracked local sharing a tracked one's name in a + # sibling branch aborted the whole document in the core (internal OWN030), and in a later + # block read as a false OWN002. REQUIRED here; skips cleanly in the offline Tier-A job. + env: + OWN_TIERB_REQUIRED: "1" + run: python tests/test_extractor_sibling_locals.py - name: Guarded release is not a call-site handoff (INF-S2, #380) # A callee that releases its parameter only under a guard is a PARTIAL release, so # its call must not be lowered as the caller's `release` (the fabricated `must`), diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 499aa257..b9c02d7e 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -3597,6 +3597,63 @@ static int FlowNestingDepth(IReadOnlyList nodes, int level = 0) return deepest; } +// #408: C# lets two DIFFERENT locals share a name when their scopes are disjoint (two branches, two +// blocks, a branch and a loop). The flow pass keys locals by NAME, so every reference to an untracked +// local named like a tracked one became an op on the tracked local: a `use` with no `acquire` on that +// path aborted the whole document in the core (internal OWN030 "undefined name", MessagePack-CSharp), +// and a `use` after the tracked local's release was a false OWN002. Map each scope that declares such +// an untracked same-named local (its block, or the foreach / for / using / switch / fixed statement it +// belongs to) to the names it hides; the lowering drops those names from `tracked` inside it. Scopes of +// two same-named locals never overlap (CS0128 / CS0136), so inside that scope the name can only mean +// the untracked local. Lambda and local-function parameters are not mapped. +static Dictionary>? ShadowingScopes(BlockSyntax body, HashSet tracked, + HashSet candidateDecls) +{ + Dictionary>? map = null; + void Hide(SyntaxNode? scope, string name) + { + if (scope is null || !tracked.Contains(name)) return; + map ??= new Dictionary>(); + if (!map.TryGetValue(scope, out var names)) + map[scope] = names = new HashSet(StringComparer.Ordinal); + names.Add(name); + } + static SyntaxNode? StatementScope(SyntaxNode? owner) => owner switch + { + SwitchSectionSyntax sec => sec.Parent, // a section's locals belong to the whole switch block + _ => owner, + }; + foreach (var node in body.DescendantNodes()) + switch (node) + { + case VariableDeclaratorSyntax d when !candidateDecls.Contains(d): + Hide(d.Parent?.Parent switch + { + LocalDeclarationStatementSyntax ld => StatementScope(ld.Parent), + { } owner => owner, // for / using / fixed statement + null => null, + }, d.Identifier.Text); + break; + case ForEachStatementSyntax fe: + Hide(fe, fe.Identifier.Text); + break; + case SingleVariableDesignationSyntax sv: // `out var x`, `is T x`, deconstruction + Hide(StatementScope(sv.Ancestors().FirstOrDefault(a => a is BlockSyntax or SwitchSectionSyntax)), + sv.Identifier.Text); + break; + } + return map; +} + +static HashSet WithoutShadowed(SyntaxNode scope, HashSet tracked) +{ + if (FlowShadows.Hidden is not { } map || !map.TryGetValue(scope, out var hidden) || !tracked.Overlaps(hidden)) + return tracked; + var narrowed = new HashSet(tracked, StringComparer.Ordinal); + narrowed.ExceptWith(hidden); + return narrowed; +} + static List? LowerFlowBody(BlockSyntax block, HashSet tracked, SemanticModel model) { var nodes = new List(); @@ -3617,6 +3674,8 @@ static bool LowerFlowStatements(IReadOnlyList stmts, int start, bool canEscape, List? onThrow, List? onReturn, bool onThrowDefinite = false) { + if (start == 0 && stmts.Count > 0 && stmts[0].Parent is { } scope) + tracked = WithoutShadowed(scope, tracked); for (var i = start; i < stmts.Count; i++) { var st = stmts[i]; @@ -3685,6 +3744,7 @@ static bool LowerFlowStmt(StatementSyntax st, HashSet tracked, SemanticM // skipped anyway: bail the method as unmodelled instead. if (!System.Runtime.CompilerServices.RuntimeHelpers.TryEnsureSufficientExecutionStack()) return false; + tracked = WithoutShadowed(st, tracked); switch (st) { case BlockSyntax b: @@ -7032,6 +7092,14 @@ when model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol } } var candidates = new HashSet(); + // #408: the DECLARATORS behind `candidates` — the flow pass keys locals by name, so a + // different local that merely shares a tracked name must be told apart by its declarator. + var candidateDecls = new HashSet(); + void AddCandidate(VariableDeclaratorSyntax d) + { + candidates.Add(d.Identifier.Text); + candidateDecls.Add(d); + } var poolBuffers = new HashSet(); // candidates that are ArrayPool buffers var usingMemoryOwners = new HashSet(); // `using`-declared MemoryPool owners var newedDisposables = new HashSet(); // candidates created via `new` (NOT a pool rental / factory) @@ -7045,7 +7113,7 @@ when model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol foreach (var v in ld.Declaration.Variables) if (IsMemoryPoolRent(v.Initializer?.Value, model)) { - candidates.Add(v.Identifier.Text); + AddCandidate(v); usingMemoryOwners.Add(v.Identifier.Text); } continue; @@ -7059,25 +7127,25 @@ or ImplicitObjectCreationExpressionSyntax } init // resource -> never disposing a local of it cannot leak. && !HasEmptyDisposeBody(dt)) { - candidates.Add(v.Identifier.Text); + AddCandidate(v); newedDisposables.Add(v.Identifier.Text); } else if (IsPoolRent(v.Initializer?.Value, model)) // an ArrayPool buffer { - candidates.Add(v.Identifier.Text); + AddCandidate(v); poolBuffers.Add(v.Identifier.Text); } else if (IsOwningFactory(v.Initializer?.Value, model)) // File / crypto Create* factory - candidates.Add(v.Identifier.Text); + AddCandidate(v); else if (IsMemoryPoolRent(v.Initializer?.Value, model)) // MemoryPool IMemoryOwner (Dispose-released, NOT a poolBuffer) - candidates.Add(v.Identifier.Text); + AddCandidate(v); else if (IsFirstPartyDisposableFactory(v.Initializer?.Value, model, out _, out _)) // P-005 D5.2: `var r = FirstPartyFactory()` — a candidate acquire // IFF the core proves the callee returns `fresh` (it emits a `call` // op, not an `acquire`; the core decides). Checked last so `new` / // pool / BCL-factory initializers keep their existing classification. - candidates.Add(v.Identifier.Text); + AddCandidate(v); } // OWN053 (promoted from ownership-semantics-lab H-29): the orphaned-awaitable sites of this body, // always on under --flow-locals (the own-check default); the frozen scope is the detector's own comment. @@ -7090,7 +7158,7 @@ or ImplicitObjectCreationExpressionSyntax } init foreach (var v in usd.Variables) if (IsMemoryPoolRent(v.Initializer?.Value, model)) { - candidates.Add(v.Identifier.Text); + AddCandidate(v); usingMemoryOwners.Add(v.Identifier.Text); } // Eligibility, NOT a deleted guard. This is the gate that drops a method @@ -7226,7 +7294,9 @@ or ImplicitObjectCreationExpressionSyntax } init // shape `params[]` is paired with. var flowNames = new HashSet(tracked, StringComparer.Ordinal); flowNames.UnionWith(ownedParamNames); + FlowShadows.Hidden = ShadowingScopes(mbody, flowNames, candidateDecls); var fbody = LowerFlowBody(mbody, flowNames, model); + FlowShadows.Hidden = null; if (fbody is null || fbody.Count == 0) { statMethodsSkipped++; // unmodelled construct -> honestly skipped @@ -7447,3 +7517,10 @@ static class OrphanedAwaitables internal readonly record struct SourceRange(SourcePos Start, SourcePos End); } + +// #408: the shadowing scopes of the method being lowered (see ShadowingScopes), ambient for the +// recursive lowering so its many signatures stay unchanged; null outside LowerFlowBody. +static class FlowShadows +{ + [ThreadStatic] internal static Dictionary>? Hidden; +} diff --git a/tests/test_extractor_sibling_locals.py b/tests/test_extractor_sibling_locals.py new file mode 100644 index 00000000..191a9166 --- /dev/null +++ b/tests/test_extractor_sibling_locals.py @@ -0,0 +1,246 @@ +#!/usr/bin/env python3 +"""Own.NET#408 - two locals with one name in sibling scopes must not be conflated. + +THE DEFECT + +The `--flow-locals` pass tracks locals by NAME. C# lets two DIFFERENT locals share a name when their +scopes are disjoint (two branches, two blocks, a branch and a loop). When only one of them is a +tracked disposable/pooled local, every reference to the other one was still emitted as an op on +the tracked name: + * in a sibling branch: a `use` with no `acquire` on that path - the core aborted the WHOLE + document (internal OWN030 "undefined name", exit 2; mined on MessagePack-CSharp + ToLZ4BinaryCore); + * in a later block: a `use` after the first local's `release` - a false OWN002 use-after-return. + +WHAT THIS TEST PINS (each a separate PASS/KILL criterion of the fix) + + C1 sibling branches (the MessagePack shape): the core accepts the document, and a leak in another + method of the same file is still reported - one method never costs the file its analysis. + C2 sequential blocks: no OWN002 for the second, unpooled local. + C3 a foreach iteration variable and an `out var` sharing the tracked name: no crash, no OWN002. + C4 both same-named locals tracked: the never-disposed one still leaks (OWN001 at its line). + C5 control: a real use-after-return of ONE local in ONE scope still reports OWN002. + C6 the untracked local declared directly in a `try` block (lowered as a statement list). + +REQUIRED VS SKIPPED: as tests/test_extractor_columns.py - required when OWN_TIERB_REQUIRED=1, +a clean, explained skip when dotnet is absent. + +Run: OWN_TIERB_REQUIRED=1 python3 tests/test_extractor_sibling_locals.py +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import sys +import tempfile + +_REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +_EXT = os.path.join(_REPO, "frontend", "roslyn", "OwnSharp.Extractor") + +_SOURCE = """using System; +using System.Buffers; +using System.Collections.Generic; +using System.IO; + +namespace Repro; + +public static class SiblingLocals +{ + // C1: MessagePack-CSharp ToLZ4BinaryCore shape + public static void Branches(bool pooled, int n) + { + if (pooled) + { + var buf = ArrayPool.Shared.Rent(n); + try { buf[0] = 1; } + finally { ArrayPool.Shared.Return(buf); } + } + else + { + foreach (var i in new[] { 1, 2 }) + { + var buf = new byte[n]; + buf.AsSpan(0, 1).Clear(); + } + } + } + + // C2 + public static void Sequential(int n) + { + { + var seq = ArrayPool.Shared.Rent(n); + try { seq[0] = 1; } + finally { ArrayPool.Shared.Return(seq); } + } + { + var seq = new byte[n]; + seq[0] = 2; + } + } + + // C3 + public static void LoopAndOut(bool a, int n, List items, string text) + { + if (a) + { + var it = ArrayPool.Shared.Rent(n); + try { it[0] = 1; } + finally { ArrayPool.Shared.Return(it); } + } + else if (n > 0) + { + foreach (var it in items) + it[0] = 3; + } + else + { + if (int.TryParse(text, out var it)) + Console.WriteLine(it); + } + } + + // C4 + public static void BothTracked(bool a) + { + if (a) + { + var both = new MemoryStream(); + both.WriteByte(1); + both.Dispose(); + } + else + { + var both = new MemoryStream(); + both.WriteByte(2); + } + } + + // C5 + public static void RealUseAfterReturn(int n) + { + var real = ArrayPool.Shared.Rent(n); + ArrayPool.Shared.Return(real); + real[0] = 1; + } + + // C6: the untracked local is declared directly in a `try` block (lowered as a statement list) + public static void TryBlock(bool pooled, int n) + { + if (pooled) + { + var tb = ArrayPool.Shared.Rent(n); + try { tb[0] = 1; } + finally { ArrayPool.Shared.Return(tb); } + } + else + { + try + { + var tb = new byte[n]; + tb[0] = 2; + } + finally { Console.WriteLine(); } + } + } + + // C1: a sibling method with a plain leak + public static void Leaky() + { + var leaked = new MemoryStream(); + leaked.WriteByte(1); + } +} +""" + + +def _line_of(marker: str) -> int: + for i, line in enumerate(_SOURCE.splitlines(), 1): + if marker in line: + return i + raise AssertionError(marker) + + +def run() -> int: + if hasattr(sys.stdout, "reconfigure"): + sys.stdout.reconfigure(errors="backslashreplace") + if not shutil.which("dotnet"): + if os.environ.get("OWN_TIERB_REQUIRED") == "1": + print("extractor sibling locals: FAIL - dotnet is REQUIRED here " + "(OWN_TIERB_REQUIRED=1) and is not on PATH") + return 1 + print("extractor sibling locals: SKIP - dotnet not on PATH") + return 0 + fails: list[str] = [] + work = tempfile.mkdtemp(prefix="own408-") + try: + src = os.path.join(work, "src") + os.makedirs(src) + with open(os.path.join(src, "Repro.cs"), "w", encoding="utf-8", newline="\n") as f: + f.write(_SOURCE) + facts = os.path.join(work, "facts.json") + ext = subprocess.run( + ["dotnet", "run", "--project", _EXT, "--", src, "-o", facts, "--flow-locals"], + cwd=_REPO, capture_output=True, text=True, encoding="utf-8", errors="replace") + if ext.returncode != 0 or not os.path.exists(facts): + tail = " | ".join((ext.stderr or ext.stdout).strip().splitlines()[-3:]) + print(f"extractor sibling locals: FAIL - extractor rc={ext.returncode} ({tail})") + return 1 + env = dict(os.environ, PYTHONPATH=_REPO) + core = subprocess.run( + [sys.executable, "-m", "ownlang", "ownir", facts, + "--format", "sarif", "--severity", "warning"], + cwd=_REPO, capture_output=True, text=True, encoding="utf-8", errors="replace", env=env) + try: + results = json.loads(core.stdout)["runs"][0]["results"] + except (ValueError, KeyError, IndexError): + tail = (core.stderr or core.stdout).strip()[-240:] + print(f"extractor sibling locals: FAIL - C1 the core rejected the document " + f"(rc={core.returncode}): {tail}") + return 1 + found = [(r.get("ruleId"), r["locations"][0]["physicalLocation"]["region"]["startLine"], + r["message"]["text"]) for r in results] + + def has(rule: str, line: int, local: str) -> bool: + return any(r == rule and ln == line and f"'{local}'" in t for r, ln, t in found) + + def any_on(local: str, rule: str) -> bool: + return any(r == rule and f"'{local}'" in t for r, _, t in found) + + if not has("OWN001", _line_of("var leaked ="), "leaked"): + fails.append("C1 the sibling leak in Leaky() is not reported") + if any_on("buf", "OWN002") or any_on("buf", "OWN001"): + fails.append("C1 a finding on 'buf' in Branches(): " + repr( + [x for x in found if "'buf'" in x[2]])) + if any_on("seq", "OWN002"): + fails.append("C2 false OWN002 on 'seq': the unpooled second local was conflated") + if any("'it'" in t for _, _, t in found): + fails.append("C3 a finding on 'it': " + repr([x for x in found if "'it'" in x[2]])) + if not has("OWN001", _second_line("var both = new MemoryStream();"), "both"): + fails.append("C4 the never-disposed second 'both' no longer leaks: " + repr( + [x for x in found if "'both'" in x[2]])) + if any("'tb'" in t for _, _, t in found): + fails.append("C6 a finding on 'tb': " + repr([x for x in found if "'tb'" in x[2]])) + if not any_on("real", "OWN002"): + fails.append("C5 the real use-after-return on 'real' is no longer reported") + finally: + shutil.rmtree(work, ignore_errors=True) + if fails: + print("extractor sibling locals: FAIL") + for f in fails: + print(" -", f) + return 1 + print("extractor sibling locals: OK (6 criteria)") + return 0 + + +def _second_line(marker: str) -> int: + hits = [i for i, line in enumerate(_SOURCE.splitlines(), 1) if marker in line] + return hits[1] + + +if __name__ == "__main__": + raise SystemExit(run())