feat(extractor): inter-procedural consume contract → use-after-handoff OWN002 (recall 8/10→9/11)#55
Conversation
…002 (recall 8/10 -> 9/11)
The extractor treated every argument-pass as an ESCAPE (untracked), so a stream
handed to a consumer that owns it (`Archive(s)`) vanished and a later touch of `s`
was invisible -- the use-after-handoff arm of ownership-handoff-consume was a miss.
The OwnIR bridge already models consume contracts end-to-end (params with an
inferred `effect`, a `call` op that moves ownership across the call, pinned by
tests/fixtures/ownir/handoff_contract.facts.json + test_ownir). This teaches the
extractor to emit those facts:
- ContractedCallee: a call whose first-party target owns a by-value IDisposable
parameter carries an ownership contract. Passing a tracked local to it is a
handoff, lowered to a `{op:"call", callee, args}` op (not an escape); the bridge
moves ownership per the callee's inferred contract (a body that releases its
param => consume), so a use after the move is OWN002. The cut is the SIGNATURE,
not whole-program points-to -- like Rust's move.
- By-value IDisposable parameters are tracked as owned obligations and emitted in
the function's `params`, so the consumer's body (use + Dispose of the param) lets
the bridge infer the contract and discharge it (no false leak on the consumer).
- A resource local passed to a contracted call is exempted from the escape set.
New fixture ownership-handoff-use (a pure use-after-handoff, no leak arm) is a miss
before and a catch (OWN002) after: recall 8/10 -> 9/11. ownership-handoff-consume
now fires both arms (OWN001+OWN002). Specificity 11/11, 0 FP, CI floor --min-recall 9.
Precision: the consumer shape (a by-value IDisposable param) appears nowhere else in
the corpus or samples, so the machinery engages only on the two handoff fixtures --
no other after.cs or dog-food scan can newly fire, and a `call` op is emitted only
for callees the extractor also lowers as functions (callee name == function name, so
no OWN040). The bridge half was validated locally (hand-built facts -> check_facts ->
the exact OWN001/OWN002 verdicts); case.own pins the logic (test_corpus 7/7,
test_ownir 88/88). The C# emission is validated by the corpus-benchmark + wpf-extractor
CI jobs (no local .NET SDK).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR extends the Roslyn ChangesOWN002 Inter-procedural Ownership-Handoff Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89fee9eff4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| if (e is not InvocationExpressionSyntax inv | ||
| || model.GetSymbolInfo(inv).Symbol is not IMethodSymbol sym | ||
| || sym.DeclaringSyntaxReferences.Length == 0) // first-party (declared in source) |
There was a problem hiding this comment.
When the resolved target is a source-declared interface, abstract, partial-without-body, or extern method with an IDisposable parameter, this check returns a callee name even though the flow pass only emits functions for methods with method.Body. A call such as consumer.Consume(s) against a scanned interface therefore lowers to a call with no matching signature; the OwnIR bridge reports an unmapped OWN040 and the CLI exits with an internal facts error instead of treating the argument as an escape or skipping the contract.
Useful? React with 👍 / 👎.
| var callArgs = callInv.ArgumentList.Arguments | ||
| .Select(a => a.Expression is IdentifierNameSyntax aid | ||
| ? aid.Identifier.Text : a.Expression.ToString()) | ||
| .ToList(); |
There was a problem hiding this comment.
The callee signature emitted below contains only resourceParams, but this serializes every C# argument. For a valid consumer like Consume(int mode, Stream s), the facts contain two call args while Consume has one OwnIR parameter, so the core raises/skips OWN041 and never applies the consume effect; the handoff is then lost, causing the new use-after-handoff case to be missed or reported as a leak whenever the consuming method has any non-disposable parameter.
Useful? React with 👍 / 👎.
| bool consumedArg = idn.Parent is ArgumentSyntax | ||
| && idn.Parent.Parent is ArgumentListSyntax | ||
| && idn.Parent.Parent.Parent is InvocationExpressionSyntax cinv | ||
| && ContractedCallee(cinv, model) is not null; |
There was a problem hiding this comment.
This exemption applies to any contracted invocation argument, but LowerFlowStmt emits a call only when the whole statement is an InvocationExpressionSyntax. If the handoff is inside var n = Consume(s) or return Consume(s), the local is no longer treated as escaped here, yet no call op is generated to move ownership, so the analyzer can miss the use-after-handoff or produce a spurious leak for resources that were actually handed off.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 1398-1405: The consumedArg check in the contracted invocation
exemption is too broad and doesn't verify that the specific argument is bound to
a by-value IDisposable parameter. Modify the consumedArg assignment to not only
check if the argument is inside a contracted invocation (using
ContractedCallee), but also verify that the specific argument is actually bound
to a by-value IDisposable parameter of that contracted method. This requires
examining the parameter binding for the argument (idn) to ensure it corresponds
to a by-value IDisposable parameter before exempting it from escape rules.
- Around line 822-823: The key generation for ContractedCallee in the method
containing the return statement at line 822-823 and the function key generation
at lines 1423-1425 both lack namespace qualification, causing collisions for
methods with identical names and containing types across different namespaces.
Include the namespace information from the symbol's containing namespace in both
locations to fully qualify the generated keys. This ensures that the
ContractedCallee embedded in the call op at line 456 can reliably match to
function definitions and maintain correct inter-procedural contracts for
overloaded or same-named methods across different namespaces.
- Around line 455-463: The argument extraction logic in the Select statement
(lines 459-461) only preserves bare IdentifierNameSyntax and converts all other
expressions to string via ToString(), which loses the underlying identifier for
wrapped expressions like parentheses, casts, and null-forgiving operators.
Create a helper method that recursively unwraps expressions to extract the
underlying identifier name: for ParenthesizedExpressionSyntax unwrap the inner
expression, for CastExpressionSyntax extract the operand identifier, and for
PostfixUnaryExpressionSyntax with null-forgiving operator extract the operand
identifier. Update the Select statement to use this helper method instead of the
current conditional, falling back to ToString() only when no identifier can be
found after unwrapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bec7af97-b5e2-47d8-b0b6-4e9e38cb1dd0
📒 Files selected for processing (9)
.github/workflows/ci.ymlcorpus/real-world/ownership-handoff-use/after.cscorpus/real-world/ownership-handoff-use/before.cscorpus/real-world/ownership-handoff-use/case.owncorpus/real-world/ownership-handoff-use/expected-diagnostics.txtcorpus/real-world/ownership-handoff-use/notes.mddocs/notes/corpus-benchmark.mddocs/proposals/P-012-bug-corpus-mining.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
| if (es.Expression is InvocationExpressionSyntax callInv | ||
| && ContractedCallee(callInv, model) is { } callee) | ||
| { | ||
| var callArgs = callInv.ArgumentList.Arguments | ||
| .Select(a => a.Expression is IdentifierNameSyntax aid | ||
| ? aid.Identifier.Text : a.Expression.ToString()) | ||
| .ToList(); | ||
| nodes.Add(new { op = "call", callee, args = callArgs, line = LineOf(callInv) }); | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i "Program.cs" | grep -i "OwnSharp.Extractor"Repository: PhysShell/Own.NET
Length of output: 107
🏁 Script executed:
wc -l frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 112
🏁 Script executed:
sed -n '450,470p' frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 1280
🏁 Script executed:
# Check for imports related to semantic model and symbols
head -100 frontend/roslyn/OwnSharp.Extractor/Program.cs | grep -E "using|Microsoft.CodeAnalysis"Repository: PhysShell/Own.NET
Length of output: 196
🏁 Script executed:
# Look for the method containing these lines to understand context better
sed -n '440,480p' frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 2562
🏁 Script executed:
# Look at the method signature that contains this code
sed -n '400,445p' frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 2605
🏁 Script executed:
# Check if ContractedCallee or similar methods use model for symbol resolution
rg "ContractedCallee|GetSymbolInfo" frontend/roslyn/OwnSharp.Extractor/Program.cs -A 3 -B 1Repository: PhysShell/Own.NET
Length of output: 3625
🏁 Script executed:
# Check what symbol types are available (ILocalSymbol, IParameterSymbol)
rg "ILocalSymbol|IParameterSymbol|ISymbol" frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 181
🏁 Script executed:
# Check if ParenthesizedExpressionSyntax, CastExpressionSyntax are used elsewhere
rg "ParenthesizedExpressionSyntax|CastExpressionSyntax|SuppressNullableWarningExpression" frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 109
🏁 Script executed:
# Check if IParameterSymbol is used in the codebase
rg "IParameterSymbol" frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Look for patterns of unwrapping expressions (e.g., stripping casts, parentheses)
rg -A 5 "while.*true|switch.*case" frontend/roslyn/OwnSharp.Extractor/Program.cs | head -80Repository: PhysShell/Own.NET
Length of output: 571
🏁 Script executed:
# Check if the issue is real: look for actual test cases or comments about parenthesized/cast arguments
rg -i "parenthes|cast|suppress.*null|argument" frontend/roslyn/OwnSharp.Extractor/Program.cs | head -20Repository: PhysShell/Own.NET
Length of output: 1436
🏁 Script executed:
# Verify the current behavior: does a.Expression.ToString() lose symbol info for complex expressions?
# Also check if there are any existing tests or examples of this problem
find frontend -name "*.cs" -type f | xargs grep -l "OWN002\|ownership.*transfer" | head -5Repository: PhysShell/Own.NET
Length of output: 151
🏁 Script executed:
# Check what Microsoft.CodeAnalysis symbol types are imported/available
# IParameterSymbol should be in Microsoft.CodeAnalysis namespace
rg "namespace Microsoft.CodeAnalysis" --type cs | head -5Repository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Let's look at a full example of how GetSymbolInfo().Symbol is used to extract names
sed -n '200,250p' frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 2922
🏁 Script executed:
# Check if there are any tests that demonstrate the issue (call args with parentheses/casts/null-forgiving)
find . -name "*test*" -o -name "*Test*" | grep -i own | head -10Repository: PhysShell/Own.NET
Length of output: 83
🏁 Script executed:
# Verify the proposed fix would compile by checking if all required syntax types exist
rg "PostfixUnaryExpressionSyntax\|SyntaxKind.SuppressNullableWarningExpression" --type csRepository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Check the exact C# syntax namespace to verify PostfixUnaryExpressionSyntax and SuppressNullableWarningExpression exist
rg "PostfixUnaryExpressionSyntax" --type cs -cRepository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Let's verify what happens with ToString() on various expression types
# Check if there's any test or existing usage that shows the problem
sed -n '455,475p' frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 1207
🏁 Script executed:
# Let's check if IParameterSymbol can be imported from Microsoft.CodeAnalysis
# by looking at the imports and the actual types used
head -20 frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 1217
🏁 Script executed:
# Verify the semantic correctness: does the proposed FlowArgName method use valid types?
# Check if CastExpressionSyntax and ParenthesizedExpressionSyntax are already used
rg "CastExpressionSyntax|ParenthesizedExpressionSyntax" frontend/roslyn/OwnSharp.Extractor/Program.cs -cRepository: PhysShell/Own.NET
Length of output: 63
🌐 Web query:
PostfixUnaryExpressionSyntax SuppressNullableWarningExpression C# Roslyn API
💡 Result:
In the.NET Compiler Platform (Roslyn) API, the null-forgiving (or suppression) operator (!) used in C# is represented in the syntax tree as a PostfixUnaryExpressionSyntax node [1][2][3]. Specifically, this node has a SyntaxKind of SuppressNullableWarningExpression [4]. Key details regarding this API representation include: 1. Syntax Node Structure: The PostfixUnaryExpressionSyntax class is a subclass of ExpressionSyntax [1]. When the C# compiler encounters the ! operator (e.g., variable!), it parses it as a PostfixUnaryExpressionSyntax node where the OperatorToken is the ! token [1]. 2. SyntaxKind Distinction: While other postfix operators (like post-increment x++ or post-decrement x--) are also represented by the PostfixUnaryExpressionSyntax class, the compiler distinguishes the null-forgiving operator by its SyntaxKind, which is defined as SuppressNullableWarningExpression in the Microsoft.CodeAnalysis.CSharp.SyntaxKind enumeration [5][4]. 3. Usage in Analysis: When building Roslyn analyzers or refactorings, you can identify these expressions by checking the Kind of the syntax node [6][4]. For example, in Roslyn's own source code, utility methods often identify this operator by checking if the node is a PostfixUnaryExpressionSyntax with the specific SuppressNullableWarningExpression kind [7]. 4. Compiler Context: This syntax node represents the developer's intent to suppress nullable reference type warnings for that specific expression [2][3][8]. The compiler uses this information during semantic analysis to avoid reporting diagnostic warnings that would otherwise occur if the expression were considered potentially null [2][8]. Because it is a purely syntactic construct for static analysis, it does not change the emitted IL code [9].
Citations:
- 1: https://docs.nunit.org/api/Microsoft.CodeAnalysis.CSharp.Syntax.PostfixUnaryExpressionSyntax.html
- 2: https://github.com/dotnet/roslyn/blob/main/docs/features/nullable-reference-types.md
- 3: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/nullable-warnings
- 4: https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.csharp.syntaxkind?view=roslyn-dotnet-5.0.0
- 5: https://github.com/dotnet/roslyn/blob/77dc97b1312bf5969e6f8d36312871aa75ed92cc/src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs
- 6: https://learn.microsoft.com/en-us/dotnet/csharp/roslyn-sdk/work-with-syntax
- 7: https://source.dot.net/Microsoft.CodeAnalysis.CSharp.Workspaces/src/roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs.html
- 8: https://github.com/dotnet/docs/blob/main/docs/csharp/nullable-references.md
- 9: is vs !is dotnet/roslyn#41649
🏁 Script executed:
# Check the actual Microsoft.CodeAnalysis version in use (from project file)
find . -name "*.csproj" | xargs grep -h "Microsoft.CodeAnalysis" | head -10Repository: PhysShell/Own.NET
Length of output: 142
🏁 Script executed:
# Verify whether the suggested fix's approach (unwrapping syntax) is sound
# Let's trace how downstream uses the callArgs (the "call" op)
rg '"call"' frontend/roslyn/OwnSharp.Extractor/Program.cs -B 5 -A 5Repository: PhysShell/Own.NET
Length of output: 580
🏁 Script executed:
# Check the Python side to see how it processes args from "call" ops
find . -name "*.py" -type f | xargs grep -l "call.*args\|ownership.*transfer" 2>/dev/null | head -5Repository: PhysShell/Own.NET
Length of output: 161
🏁 Script executed:
# Check how the "call" ops are processed downstream in the Python code
cat ownlang/ownir.py | head -100Repository: PhysShell/Own.NET
Length of output: 5424
🏁 Script executed:
# Look for how args are resolved in the Python processing
rg "call.*args|symbol.*lookup" ownlang/ --type py -A 3 -B 1Repository: PhysShell/Own.NET
Length of output: 2309
🏁 Script executed:
# Verify the issue: check if args field is used for symbol resolution
rg '"args"' ownlang/ --type py -A 2 -B 2Repository: PhysShell/Own.NET
Length of output: 717
🏁 Script executed:
# Verify the localmap lookup mechanism to confirm args must be exact variable names
grep -A 10 "localmap" ownlang/ownir.py | head -20Repository: PhysShell/Own.NET
Length of output: 1368
🏁 Script executed:
# Double-check the current code behavior with a concrete example
# If we have `foo((x))` or `foo((int)x)`, what happens with ToString()?
python3 - <<'PY'
# Simulate what happens with current code
expr_text = "(x)"
expr_tostring = "(x)" # ToString() on parenthesized expression includes parens
localmap = {"x": "x_local"} # Actual local name
# Current code behavior:
arg_value = expr_tostring
resolved = localmap.get(arg_value, arg_value)
print(f"Expression: {expr_text}")
print(f"Current code resolves to: {resolved}")
print(f"Expected (local name 'x'): {localmap.get('x', 'x')}")
print(f"Match: {resolved == 'x_local'}")
PYRepository: PhysShell/Own.NET
Length of output: 155
Preserve symbol-backed argument identity when lowering call ops.
Lines 459–461 only keep bare identifiers; parenthesized, cast, and null-forgiving forms become raw text via ToString(). Downstream (ownlang/ownir.py) resolves args via localmap.get(str(a)), so wrapper expressions lose the underlying variable name, breaking symbol-based ownership tracking and causing missed OWN002 checks or spurious caller leaks.
Suggested fix
+static string? FlowArgName(ExpressionSyntax expr, SemanticModel model)
+{
+ ExpressionSyntax e = expr;
+ while (true)
+ {
+ switch (e)
+ {
+ case ParenthesizedExpressionSyntax p:
+ e = p.Expression;
+ continue;
+ case CastExpressionSyntax c:
+ e = c.Expression;
+ continue;
+ case PostfixUnaryExpressionSyntax u
+ when u.IsKind(SyntaxKind.SuppressNullableWarningExpression):
+ e = u.Operand;
+ continue;
+ }
+ break;
+ }
+ return model.GetSymbolInfo(e).Symbol switch
+ {
+ ILocalSymbol l => l.Name,
+ IParameterSymbol p => p.Name,
+ _ => null,
+ };
+}
...
- var callArgs = callInv.ArgumentList.Arguments
- .Select(a => a.Expression is IdentifierNameSyntax aid
- ? aid.Identifier.Text : a.Expression.ToString())
- .ToList();
+ var callArgs = callInv.ArgumentList.Arguments
+ .Select(a => FlowArgName(a.Expression, model) ?? a.Expression.ToString())
+ .ToList();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (es.Expression is InvocationExpressionSyntax callInv | |
| && ContractedCallee(callInv, model) is { } callee) | |
| { | |
| var callArgs = callInv.ArgumentList.Arguments | |
| .Select(a => a.Expression is IdentifierNameSyntax aid | |
| ? aid.Identifier.Text : a.Expression.ToString()) | |
| .ToList(); | |
| nodes.Add(new { op = "call", callee, args = callArgs, line = LineOf(callInv) }); | |
| return true; | |
| if (es.Expression is InvocationExpressionSyntax callInv | |
| && ContractedCallee(callInv, model) is { } callee) | |
| { | |
| var callArgs = callInv.ArgumentList.Arguments | |
| .Select(a => FlowArgName(a.Expression, model) ?? a.Expression.ToString()) | |
| .ToList(); | |
| nodes.Add(new { op = "call", callee, args = callArgs, line = LineOf(callInv) }); | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 455 - 463, The
argument extraction logic in the Select statement (lines 459-461) only preserves
bare IdentifierNameSyntax and converts all other expressions to string via
ToString(), which loses the underlying identifier for wrapped expressions like
parentheses, casts, and null-forgiving operators. Create a helper method that
recursively unwraps expressions to extract the underlying identifier name: for
ParenthesizedExpressionSyntax unwrap the inner expression, for
CastExpressionSyntax extract the operand identifier, and for
PostfixUnaryExpressionSyntax with null-forgiving operator extract the operand
identifier. Update the Select statement to use this helper method instead of the
current conditional, falling back to ToString() only when no identifier can be
found after unwrapping.
| return sym.ContainingType is { } ct ? $"{ct.Name}.{sym.Name}" : sym.Name; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
find . -name "Program.cs" -path "*/OwnSharp.Extractor/*"Repository: PhysShell/Own.NET
Length of output: 109
🏁 Script executed:
# Check the file size first
wc -l ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 114
🏁 Script executed:
# Read the section around lines 822-823
sed -n '810,835p' ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 1322
🏁 Script executed:
# Read the section around lines 1423-1425
sed -n '1410,1435p' ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 1111
🏁 Script executed:
# Look for more context around line 822 to understand the function
sed -n '800,830p' ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 1797
🏁 Script executed:
# Get better context around line 1423 - let's see what that code is doing
sed -n '1415,1430p' ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 670
🏁 Script executed:
# Find where MethodName function is defined
rg -n "static.*MethodName" ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 133
🏁 Script executed:
# Look at the MethodName function definition
sed -n '324,340p' ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 863
🏁 Script executed:
# Search for how the ContractedCallee function's return value is used
rg -n "ContractedCallee" ./frontend/roslyn/OwnSharp.Extractor/Program.cs -A 3 -B 3Repository: PhysShell/Own.NET
Length of output: 1641
🏁 Script executed:
# Check if these keys are used to match callees across the codebase
# Look for how flowFunctions are used and how the names are matched
rg -n "flowFunctions" ./frontend/roslyn/OwnSharp.Extractor/Program.cs -B 2 -A 2Repository: PhysShell/Own.NET
Length of output: 723
🏁 Script executed:
# Check if there's any namespace information captured elsewhere
rg -n "Namespace\|namespace" ./frontend/roslyn/OwnSharp.Extractor/Program.cs | head -20Repository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Look at more context around line 1423 to see the full structure
sed -n '1380,1440p' ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 3336
🏁 Script executed:
# Search for how the callee names are matched or looked up in the bridge/Python side
# Check if there's any matching logic that uses these names
rg -n "name.*callee\|callee.*name" ./frontend/roslyn/OwnSharp.Extractor/Program.cs -iRepository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Check if there's full namespace information captured anywhere
rg -n "NamespaceName\|Namespace()" ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Search for any overload handling or signature matching in the codebase
rg -n "overload\|signature\|Parameters" ./frontend/roslyn/OwnSharp.Extractor/Program.cs -i | head -30Repository: PhysShell/Own.NET
Length of output: 43
🏁 Script executed:
# Look at the broader context of how ContractedCallee is used
# Check what happens with the callee variable returned
sed -n '450,475p' ./frontend/roslyn/OwnSharp.Extractor/Program.csRepository: PhysShell/Own.NET
Length of output: 1610
Include namespace qualification in ContractedCallee and function key generation to prevent collisions.
Lines 822 and 1423 generate Type.Method keys without namespace qualification. Methods with identical names and containing types across different namespaces, or method overloads, will serialize to the same key. The call op at line 456 embeds the ContractedCallee result without file context, making reliable matching to function definitions (which do include file info at line 1423) impossible when collisions occur. This breaks inter-procedural contracts for overloaded or same-named methods across namespaces.
Also applies to: 1423-1425
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 822 - 823, The
key generation for ContractedCallee in the method containing the return
statement at line 822-823 and the function key generation at lines 1423-1425
both lack namespace qualification, causing collisions for methods with identical
names and containing types across different namespaces. Include the namespace
information from the symbol's containing namespace in both locations to fully
qualify the generated keys. This ensures that the ContractedCallee embedded in
the call op at line 456 can reliably match to function definitions and maintain
correct inter-procedural contracts for overloaded or same-named methods across
different namespaces.
…olves OWN040 + review) The call-op design crashed CI: UnitOfWorkFlowSample's consumer ConsumeUnitOfWork(UnitOfWork uow) is a by-value IDisposable param the type-name blast-radius grep missed, and its body is skipped by the flow pass, so the emitted `call` op referenced an undeclared callee -> the bridge raised an unmapped OWN040 and the extractor exited with an internal facts error. Codex (P1) and CodeRabbit flagged the same, plus arg/param arity (OWN041) and an over-broad escape exemption. Replace the whole call-op / params / signature machinery with the pool-Return pattern: a call to a first-party CONSUMER (a method whose own body disposes a by-value IDisposable parameter) is modelled as a RELEASE of the matching argument at the call site (ConsumeReleaseArg + DisposesLocal). A use of the argument after the call is then a use-after-handoff (OWN002), via the existing flow machinery. This dissolves every review finding by construction: - no `call` op -> no undeclared-callee OWN040, no arg/param OWN041 (P1/P2); - the escape exemption is gated on the SAME ConsumeReleaseArg as the release and on the `Consume(s);` statement form the pass actually lowers, so an argument is exempted iff it is released -- never a tracked-but-unreleased false leak, and only the arg bound to the disposed by-value param is exempted (P3 + CodeRabbit parameter-aware); - a callee with no body (interface/abstract/extern) or that doesn't dispose the param yields null -> the argument stays an ordinary escape (no crash); - no cross-call callee-name key, so no namespace-collision concern. Same verdicts as before: ownership-handoff-consume before -> OWN001+OWN002, after clean; ownership-handoff-use before -> OWN002, after clean. Recall 9/11, 0 FP, floor --min-recall 9. Validated locally: facts -> check_facts -> the exact verdicts; test_corpus 7/7; test_ownir 88/88. Docs updated to the call-site-release framing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 711-715: Remove the early return statement in the
ConsumeReleaseArg block that comes after nodes.Add(new { op = "release", var =
carg, line = LineOf(expr) }). Instead of returning immediately after emitting
the release fact, allow the code flow to continue so that use facts can be
processed for other tracked locals in the same invocation. This ensures all
tracked arguments are properly checked and OWN002 violations are not suppressed.
- Around line 807-811: The code checks only the first element of
sym.DeclaringSyntaxReferences using index [0], which can miss valid method
bodies in partial method declarations where the first reference might lack a
body. Instead of checking only DeclaringSyntaxReferences[0].GetSyntax(), iterate
through all DeclaringSyntaxReferences to find the first one that is a
BaseMethodDeclarationSyntax and has a valid body (either a Body or
ExpressionBody property), ensuring all partial declarations are properly
evaluated before returning null.
- Around line 813-819: The current parameter-argument matching in the loop fails
when named arguments are used because it relies on positional index matching
between sym.Parameters[i] and inv.ArgumentList.Arguments[i]. Instead of using
index-based matching, check each argument in the ArgumentList for a NameColon
property; when NameColon is present, match the argument to its target parameter
by comparing the name against the parameter names in sym.Parameters, otherwise
use the positional index. This ensures that named arguments like Method(paramB:
val2, paramA: val1) are correctly mapped to their intended parameters rather
than being matched by position.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8db03da2-0799-43c1-9207-fdfaf76d194b
📒 Files selected for processing (4)
corpus/real-world/ownership-handoff-use/notes.mddocs/notes/corpus-benchmark.mddocs/proposals/P-012-bug-corpus-mining.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
✅ Files skipped from review due to trivial changes (3)
- corpus/real-world/ownership-handoff-use/notes.md
- docs/proposals/P-012-bug-corpus-mining.md
- docs/notes/corpus-benchmark.md
…, named args) Three correctness fixes from CodeRabbit's review of the call-site-release design (none hit the current fixtures, but all real): - Multi-arg calls (Major): EmitFlowExpr no longer `return`s after a consume release, so other tracked arguments of the same call (`Consume(s, t)`) still get their `use` -> a use-after-handoff on a co-argument is no longer hidden. The consumed arg is excluded from the use set (it is a release, never also a use). - Partial-method bodies (Minor): scan ALL DeclaringSyntaxReferences for the first with a body instead of only [0], so a consumer split across partial declarations is still recognised. - Named arguments (Major): map argument -> parameter by NameColon when `name:` is used, else by position, so `Consume(sink: s)` / reordered named args resolve to the right (disposed by-value IDisposable) parameter rather than by raw index. The helper now returns the full set of consumed arguments (ConsumeReleaseArgs); EmitFlowExpr releases each and the escape exemption uses .Contains(nm). Verdicts unchanged: ownership-handoff-consume before -> OWN001+OWN002, use before -> OWN002, both after clean (validated via check_facts); test_corpus 7/7. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
…ugh a forwarding consumer (#68) The D4 frontier: cross-method use-after-dispose through a FORWARDING consumer. The inter-procedural consume-handoff (#55) caught a use-after-handoff when the callee disposes a by-value IDisposable parameter directly, but stopped one hop short — a parameter merely forwarded to another call was left ambiguous (ownir.py's `passed` branch: "awaiting a later transitive pass"). This is that pass. The extractor's consumer detection (ConsumesParam) is now transitive: a parameter is consumed if the body disposes it directly OR forwards it to another first-party consumer that consumes it — following Consume -> Inner -> Dispose through the chain (incl. directly-called local functions), cycle-guarded per parameter, binding each callee's body with its own tree's SemanticModel so cross-file chains resolve. So Consume(s) is still a call-site release and a use after it trips OWN002. The emitted facts match the direct handoff, so the core is unchanged. Conservative: a parameter handed to an unknown or borrowing callee is not consumed (no false release, no false OWN002). Deferred-body safety (Codex/CodeRabbit): the scan (ImmediateInvocations, shared with DisposesLocal) does NOT descend into nested lambda / local-function bodies — a dispose/forward stored in a callback runs deferred, not at the call boundary, so it is not a phantom handoff. Pinned by a silent control (DeferredHandoffNoFalsePositive) and by the new corpus case ownership-handoff-use-transitive (before -> OWN002, after silent), which lifts the benchmark recall floor 10 -> 11. ruff + mypy --strict clean, corpus 9/9, ownir 116/116; corpus benchmark green at recall 11. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
2 participants
What
Teaches the extractor to catch an inter-procedural use-after-handoff — corpus recall 8/10 → 9/11, the last arm of the handoff gad.
The extractor treated every argument-pass as an escape (untracked). So a stream handed to a consumer that owns it —
Archive(s)whereArchive(Stream source)disposessource— simply vanished, and a later touch ofswas invisible. That's the use-after-handoff arm ofownership-handoff-consume, missed for that reason.Mechanism — a call-site release (the pool-
Returnpattern): a call to a first-party consumer — a method whose own body disposes a by-valueIDisposableparameter — is modelled as a release of the matching argument at the call site. A use of that argument after the call is then a use-after-release (OWN002), via the existing flow machinery. The argument is also exempted from the escape set so it stays tracked through the handoff.ConsumeReleaseArg+DisposesLocal— resolve the call's first-party target, and if its body disposes a by-valueIDisposableparameter, return the matching argument's local name.ConsumeReleaseArgand on the bareConsume(s);statement form the flow pass actually lowers — so an argument is exempted iff it is also released (never a tracked-but-unreleased false leak), and only the arg bound to the disposed by-value param is exempted.It is inter-procedural — the signal is the callee's own body — but there is no cross-call signature table, so a callee with no body (interface/abstract/extern) or that doesn't dispose the param yields nothing and the argument stays an ordinary escape. No dangling
callop, no bridge crash.The proof (benchmark, CI)
New fixture
ownership-handoff-use(a pure use-after-handoff — no leak arm) is a miss before and a catch (OWN002) after: recall 8/10 → 9/11.ownership-handoff-consumenow fires both arms (OWN001+OWN002). Specificity stays 0 FP, CI floor--min-recall 9.Validated locally (no .NET SDK): the exact facts the extractor emits →
check_facts→OWN001/OWN002(before) and silence (after), for both fixtures;case.ownpins the logic (test_corpus7/7); the bridge machinery is intact (test_ownir88/88). The C# emission is validated by thecorpus-benchmark+wpf-extractorCI jobs.Files
frontend/roslyn/OwnSharp.Extractor/Program.cs—ConsumeReleaseArg/DisposesLocal, theEmitFlowExprrelease branch, the escape exemption.corpus/real-world/ownership-handoff-use/— the pure use-after-handoff fixture..github/workflows/ci.yml— floor--min-recall 9.docs/notes/corpus-benchmark.md(the→ 9/11ratchet) +docs/proposals/P-012-bug-corpus-mining.md.Remaining recall backlog (2): a cross-method use-after-dispose and an injected-source region-escape.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Summary by CodeRabbit
Tests
OWN002scenario.Bug Fixes
Documentation
Chores