Skip to content

Commit bdb3307

Browse files
PhysShellPhysShell
authored
Merge pull request #318: feat(ownir): carry a real Roslyn source column from the extractor to SARIF
Own.NET#317 (child of #266). own-check reported no column at all, so every one of its findings anchored line-only, while OwnAudit's finding-occurrence/v1 anchors on path + startLine + startColumn. The coordinate now travels end to end: Roslyn node -> OwnIR fact -> Finding -> SARIF region.startColumn. One location authority: LineOf(node) is a projection of PosOf(node) = RangeOf(node) .Start, a single GetLineSpan() call. There is deliberately no parallel ColumnOf -- two independent lookups are two chances to pass two different nodes, and the result would be a well-formed coordinate pointing at a place that does not exist. FixSpanOf routes through the same RangeOf with its serialized shape unchanged. Three carriers converted (dispoFieldPos, pooledFieldRent, rented) so a column cannot be dropped between where it is resolved and where it is emitted. Eighteen records carry one: seven handle-minting flow ops and eleven subs.Add shapes. Release, use, overspan and OWN025 keep their line alone -- mixing one node's line with another node's column is the single error this change exists to prevent. OWNIR_VERSION stays 0: an optional field with a safe default is additive. Verified by CI, not asserted: the extractor compiled, the four coordinates in ColumnAnchorsSample.cs matched real Roslyn in both modes, every subscription record's column equalled its own fix block's span.start_column, and the golden byte-parity gate passed over all 23 regenerated values. Does not close #317. The corpus must be re-measured with two metrics -- occurrence_id coverage and startColumn coverage per producer -- with every differential category empty except "same finding, same path/rule/message/line, startColumn null -> a real column".
2 parents 0ded835 + fd30d20 commit bdb3307

13 files changed

Lines changed: 1157 additions & 58 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,15 @@ jobs:
261261
frontend/roslyn/samples/OwnedCollectionElementSample.cs \
262262
-o "$RUNNER_TEMP/facts.json"
263263
cat "$RUNNER_TEMP/facts.json"
264+
- name: Extractor source columns (Own.NET#317)
265+
# The producer half of #317. Runs the real extractor over a fixture that puts two
266+
# anchor sites on ONE line and asserts each record's exact (line, column) - the
267+
# only place in this repo where an emitted column is checked against the source
268+
# text rather than against another hand-written fixture. REQUIRED here (dotnet is
269+
# present); it skips cleanly in the offline Tier-A job.
270+
env:
271+
OWN_TIERB_REQUIRED: "1"
272+
run: python tests/test_extractor_columns.py
264273
- name: S2 step 10 analyzer-delta verifier (Tier B, full public CLI)
265274
env:
266275
OWN_TIERB_REQUIRED: "1"

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

Lines changed: 157 additions & 37 deletions
Large diffs are not rendered by default.

‎frontend/roslyn/samples/ColumnAnchorsSample.cs‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
using System;
2+
using System.IO;
3+
4+
// Own.NET#317 - the extractor's source COLUMN, and the one property that proves it is
5+
// a real coordinate rather than decoration: TWO anchor sites on ONE line.
6+
//
7+
// Every other sample in this directory puts one finding on one line, so a `column` that
8+
// was always 1, or always the indentation, or always the first site's, would pass all of
9+
// them. Here each pair shares a line and differs only in column, so a column that does
10+
// not come from the right node collapses two findings into one (the bridge's dedupe key
11+
// carries `column` exactly so it cannot) or points at the wrong site.
12+
//
13+
// PLAIN ASCII SPACES ONLY, and no non-ASCII anywhere in this file. Roslyn's
14+
// `LinePosition.Character` counts UTF-16 code units from the line start, expanding no
15+
// tabs and folding no surrogate pairs, so a tab or an astral character would make the
16+
// expected columns a statement about this file's byte layout rather than about the
17+
// extractor. tests/test_extractor_columns.py asserts that too, before it asserts
18+
// anything else.
19+
public class ColumnAnchorsSample
20+
{
21+
public event EventHandler? Alpha;
22+
public event EventHandler? Beta;
23+
24+
// Two `+=` on one line, neither ever `-=`'d -> two OWN001 subscription leaks. The
25+
// record anchors on the EVENT ACCESS (`src.Alpha` / `src.Beta`), which is the node
26+
// the extractor has always taken `line` from - not the statement, and not `+=`.
27+
// `src` is a parameter, so the source is `injected` and both are warnings.
28+
public void SubscribeBoth(ColumnAnchorsSample src)
29+
{
30+
src.Alpha += OnAlpha; src.Beta += OnBeta;
31+
}
32+
33+
private void OnAlpha(object? sender, EventArgs e) { }
34+
35+
private void OnBeta(object? sender, EventArgs e) { }
36+
37+
// Two undisposed IDisposable locals on one line. Neither is `using`-guarded, neither
38+
// escapes (`first.Length` reads THROUGH the local; the identifier's parent is the
39+
// member access, not the return), and neither is disposed - so both leak, in both
40+
// extractor modes:
41+
//
42+
// * without --flow-locals the D1 detector emits two `local-disposable` records,
43+
// each anchored on its own declarator;
44+
// * with --flow-locals the flow pass emits two `acquire` ops instead, again one
45+
// per declarator, and the core reports OWN001 at the acquire.
46+
//
47+
// Both modes are exercised, because the two paths mint their coordinate in different
48+
// code and only one of them was on the obvious route.
49+
public long TwoLeaksOneLine()
50+
{
51+
var first = new MemoryStream(); var second = new MemoryStream();
52+
first.WriteByte(1);
53+
second.WriteByte(2);
54+
return first.Length + second.Length;
55+
}
56+
}

‎ownlang/ownir.py‎

Lines changed: 111 additions & 17 deletions
Large diffs are not rendered by default.

‎rust/crates/own-ir/tests/roundtrip.rs‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,3 +142,47 @@ fn additive_unknown_fields_are_preserved() {
142142
let doc = OwnIr::from_json(text).expect("additive fields are tolerated");
143143
assert_eq!(doc.to_value().expect("serialize"), original);
144144
}
145+
146+
/// Own.NET#317: the optional 1-based `column` is additive, so it has no typed
147+
/// field here - it rides in the flattened `extra` and must come back
148+
/// value-for-value, the same guarantee this file states for every fixture.
149+
///
150+
/// `round_trips_every_python_fixture` already covers this by scanning the fixture
151+
/// directory, but only implicitly: it proves whatever happens to be on disk. This
152+
/// asserts the field by name at each path that carries one, so the guarantee stays
153+
/// legible if the fixture is renamed - and cannot become vacuous if both sides
154+
/// were ever to drop the field together.
155+
#[test]
156+
fn optional_column_survives_the_round_trip() {
157+
let text = fs::read_to_string(fixtures_dir().join("flow_column_anchors.facts.json"))
158+
.expect("the column fixture must exist");
159+
let original: Value = serde_json::from_str(&text).expect("valid JSON");
160+
let back = OwnIr::from_json(&text)
161+
.expect("must parse like Python load()")
162+
.to_value()
163+
.expect("round-trip serialization");
164+
assert_eq!(
165+
back, original,
166+
"the column fixture must round-trip value-for-value"
167+
);
168+
169+
// Named paths rather than a text scan: this asserts WHICH records kept a
170+
// column, so a future edit that moves one silently fails here instead of
171+
// still counting to eight.
172+
for (pointer, expected) in [
173+
("/components/0/subscriptions/0/column", 17), // subscription record
174+
("/components/0/subscriptions/1/column", 9), // disposable field record
175+
("/functions/0/params/0/column", 26), // contract param
176+
("/functions/1/body/0/column", 13), // direct acquire
177+
("/functions/1/body/1/column", 44), // same line, other column
178+
("/functions/1/body/2/column", 17), // alias_join
179+
("/functions/1/body/3/column", 22), // fresh-returning call
180+
("/functions/1/body/4/then/0/column", 21), // acquire inside a branch
181+
] {
182+
assert_eq!(
183+
back.pointer(pointer).and_then(Value::as_i64),
184+
Some(expected),
185+
"column at {pointer} must survive the round-trip"
186+
);
187+
}
188+
}

‎spec/OwnIR.md‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ What bumps the version — the rule that keeps the three producers honest:
5151

5252
| Change | Bumps `OWNIR_VERSION`? |
5353
|---|---|
54-
| Add an **optional** field with a safe default (e.g. `type`, `source_type`) | **No** — an older core reads the record without it (see §4). |
54+
| Add an **optional** field with a safe default (e.g. `type`, `source_type`, `column`) | **No** — an older core reads the record without it (see §4). |
5555
| Add a new **resource kind** discriminator value (§4) | **Yes** — the kind selects the analysis path (§4 routing), so it is a vocabulary change, not additive metadata; a present-but-unknown kind is rejected at load. |
5656
| Add, rename, remove, or change the meaning of a **flow op** (§5) | **Yes** — vocabulary change (see the guard below). |
5757
| Remove or rename a required field, or change a field's semantics | **Yes** — not backward-readable. |
@@ -88,7 +88,7 @@ finding — a frontend cannot silently lose a verdict.
8888

8989
Each component has a `name`, a `file`, and a list historically keyed
9090
`subscriptions` (it is really the list of owned-resource records). Each record
91-
carries a `line` and, optionally, `released` (bool) and a `resource`
91+
carries a `line` and, optionally, a `column`, `released` (bool) and a `resource`
9292
discriminator. An unreleased record is the core's **OWN001** (owned-but-not-
9393
released) at `line`; a released one nets to a balanced acquire/release and stays
9494
silent. The `resource`/`type` fields are additive (§2), so an older core reads
@@ -135,12 +135,50 @@ the reason it read. A non-string value is rejected at load. The Roslyn frontend
135135
currently reads `[OwnIgnore]` on **`IDisposable` field declarations** (the clearest
136136
attribute site — the record anchors at the field); other sites are follow-ups.
137137

138+
### 4.1 `column` — the optional source coordinate (normative)
139+
140+
A record, a contract param, or a flow op MAY carry a `column` beside its `line`:
141+
the **1-based** source column of the *same* node the `line` anchors on. It is
142+
optional and additive, so it does not bump `OWNIR_VERSION` (§2).
143+
144+
Three rules, and they are the whole contract:
145+
146+
1. **Same node.** The column belongs to the node the line came from. Pairing one
147+
node's line with another's column yields a well-formed coordinate pointing at
148+
a place that does not exist — worse than an absent field, because nothing
149+
downstream can detect it. The one place this bites today is the OWN025
150+
(POOL005) verdict, which anchors on the *view* site rather than the acquire:
151+
it therefore reports **no** column at all rather than the acquire's.
152+
2. **Never invented.** A producer that does not know the column omits it. It is
153+
never `0`, never `1`, and never recovered by re-reading the source line —
154+
`Diagnostic._caret_col` does exactly that for its human-readable caret, by
155+
pulling a name out of the message text and falling back to the indentation.
156+
That is a renderer heuristic, not a coordinate the analysis computed.
157+
3. **Validated, not coerced.** `load()` rejects a `column` that is not a 1-based
158+
integer — `0`, negative, `bool`, string, float and array all raise
159+
`OwnIRError`, and the walk recurses into `if`/`while` bodies. `bool` is
160+
rejected explicitly: `True` is an `int` in Python and would otherwise read as
161+
column 1. `check_facts()`, which may be called directly on un-validated facts,
162+
degrades to absent instead.
163+
164+
A flow-local handle is minted on five paths — a contract param, a direct
165+
`acquire`, an `alias_join`, a fresh-returning call `result`, and a branch acquire
166+
hoisted to the function scope — and the column travels all five. An `alias_join`
167+
handle carries one, but a finding still anchors at the source acquire, because
168+
the alias shares its obligation rather than creating a second one.
169+
170+
The column rides to SARIF as `region.startColumn`, alongside `startLine` and only
171+
when a real `startLine` is present. It is what OwnAudit's `finding-occurrence/v1`
172+
physical anchor reads (Own.NET#317, PhysShell/OwnAudit#58); a finding without one
173+
is anchored line-only, which is a degradation rather than a failure.
174+
138175
## 5. Flow bodies (`functions[]`)
139176

140177
A flow function has a `name`, a `file`, and a `body`: an ordered list of flow
141178
ops modelling one method's intra-procedural CFG (P-016). Each op has an `op` and
142-
a `line`. The lowerer mints a globally-unique handle per acquire so a finding
143-
maps back to the exact C# local. The **complete** op vocabulary:
179+
a `line`, and optionally a `column` (§4.1). The lowerer mints a globally-unique
180+
handle per acquire so a finding maps back to the exact C# local. The **complete**
181+
op vocabulary:
144182

145183
| `op` | Fields | Lowers to |
146184
|---|---|---|

‎spec/ownir.schema.json‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,11 @@
4646
}
4747
},
4848
"$defs": {
49+
"sourceColumn": {
50+
"description": "1-based source column of the SAME node `line` anchors on. Optional and additive, so OWNIR_VERSION stays 0 (spec/OwnIR.md §2): a producer that does not report one omits it and no consumer substitutes a value — not 0, not 1, and never recovered by re-reading the source line. Rides to SARIF as region.startColumn, where the OwnAudit occurrence anchor reads it (Own.NET#317).",
51+
"type": "integer",
52+
"minimum": 1
53+
},
4954
"resourceKind": {
5055
"description": "The resource-kind discriminator (spec/OwnIR.md §4). It selects the analysis path, so a present-but-unknown value is rejected at load (fail-loud) and a new kind must bump OWNIR_VERSION. Pinned to ownlang/ownir.py::_KNOWN_RESOURCE_KINDS.",
5156
"type": "string",
@@ -97,6 +102,7 @@
97102
"type": "object",
98103
"properties": {
99104
"line": { "type": "integer" },
105+
"column": { "$ref": "#/$defs/sourceColumn" },
100106
"event": {
101107
"description": "The event/handle identifier for this owned resource (e.g. `bus.CustomerChanged`, `_timer.Tick`); carried into the finding message and rendered output.",
102108
"type": "string"
@@ -163,6 +169,7 @@
163169
"properties": {
164170
"name": { "type": "string" },
165171
"line": { "type": "integer" },
172+
"column": { "$ref": "#/$defs/sourceColumn" },
166173
"effect": { "$ref": "#/$defs/paramEffect" }
167174
}
168175
},
@@ -177,6 +184,7 @@
177184
"properties": {
178185
"op": { "const": "acquire" },
179186
"line": { "type": "integer" },
187+
"column": { "$ref": "#/$defs/sourceColumn" },
180188
"var": { "type": "string" },
181189
"kind": { "type": "string", "enum": ["pool"] }
182190
},
@@ -188,6 +196,7 @@
188196
"properties": {
189197
"op": { "const": "release" },
190198
"line": { "type": "integer" },
199+
"column": { "$ref": "#/$defs/sourceColumn" },
191200
"var": { "type": "string" }
192201
},
193202
"required": ["op", "var"]
@@ -198,6 +207,7 @@
198207
"properties": {
199208
"op": { "const": "use" },
200209
"line": { "type": "integer" },
210+
"column": { "$ref": "#/$defs/sourceColumn" },
201211
"var": { "type": "string" }
202212
},
203213
"required": ["op", "var"]
@@ -208,6 +218,7 @@
208218
"properties": {
209219
"op": { "const": "overspan" },
210220
"line": { "type": "integer" },
221+
"column": { "$ref": "#/$defs/sourceColumn" },
211222
"var": { "type": "string" }
212223
},
213224
"required": ["op", "var"]
@@ -218,6 +229,7 @@
218229
"properties": {
219230
"op": { "const": "return" },
220231
"line": { "type": "integer" },
232+
"column": { "$ref": "#/$defs/sourceColumn" },
221233
"var": { "type": "string" }
222234
},
223235
"required": ["op"]
@@ -228,6 +240,7 @@
228240
"properties": {
229241
"op": { "const": "alias_join" },
230242
"line": { "type": "integer" },
243+
"column": { "$ref": "#/$defs/sourceColumn" },
231244
"var": { "type": "string" },
232245
"src": { "type": "string" }
233246
},
@@ -239,6 +252,7 @@
239252
"properties": {
240253
"op": { "const": "call" },
241254
"line": { "type": "integer" },
255+
"column": { "$ref": "#/$defs/sourceColumn" },
242256
"callee": { "type": "string" },
243257
"sig": { "type": "string" },
244258
"args": { "type": "array", "items": { "type": "string" } },
@@ -252,6 +266,7 @@
252266
"properties": {
253267
"op": { "const": "if" },
254268
"line": { "type": "integer" },
269+
"column": { "$ref": "#/$defs/sourceColumn" },
255270
"then": { "type": "array", "items": { "$ref": "#/$defs/flowOp" } },
256271
"else": { "type": "array", "items": { "$ref": "#/$defs/flowOp" } }
257272
},
@@ -263,6 +278,7 @@
263278
"properties": {
264279
"op": { "const": "while" },
265280
"line": { "type": "integer" },
281+
"column": { "$ref": "#/$defs/sourceColumn" },
266282
"body": { "type": "array", "items": { "$ref": "#/$defs/flowOp" } }
267283
},
268284
"required": ["op"]

‎tests/check_fix_candidates_facts.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,38 @@ def only_fix(name: str) -> dict | None:
114114
s0, s1 = two[0]["span"], two[1]["span"]
115115
check(s0["start_line"] == s1["start_line"], "TwoOnOneLine: same line")
116116
check(s0["start"] != s1["start"], "TwoOnOneLine: spans must differ (full span)")
117+
# #317: and now the RECORD itself tells them apart, not just the S0 fix block.
118+
# Until this change the two subscriptions were indistinguishable to anything
119+
# reading `path + line` — which is exactly what an OwnAudit occurrence anchor
120+
# reads.
121+
check(s0["start_column"] != s1["start_column"],
122+
"TwoOnOneLine: the two spans must differ in COLUMN, not only in the "
123+
"absolute offset — a byte offset is not a coordinate")
124+
125+
# #317, the cross-check that needs no expected values: every subscription record's
126+
# `column` must equal its own fix block's `span.start_column`. The two are computed
127+
# in different places (the `subs.Add` anchor from PosOf(a.Left), the fix span from
128+
# FixSpanOf(a)), and an assignment expression starts exactly where its left side
129+
# does — so agreement is required and disagreement means the anchor took its column
130+
# from a node other than the one it names. This holds for every sample, on every
131+
# run, with nothing hardcoded.
132+
for comp in on.get("components", []):
133+
for sub in comp.get("subscriptions") or []:
134+
fix = sub.get("fix")
135+
if not fix:
136+
continue
137+
span = fix["span"]
138+
check(sub.get("line") == span["start_line"],
139+
f"{comp.get('name')}/{sub.get('event')}: record line {sub.get('line')!r} "
140+
f"!= fix span start_line {span['start_line']!r}")
141+
check("column" in sub,
142+
f"{comp.get('name')}/{sub.get('event')}: subscription record carries no "
143+
"'column' — the extractor is still line-only on the anchor path (#317)")
144+
if "column" in sub:
145+
check(sub["column"] == span["start_column"],
146+
f"{comp.get('name')}/{sub.get('event')}: record column "
147+
f"{sub['column']!r} != fix span start_column "
148+
f"{span['start_column']!r} — two coordinates for one node")
117149

118150
# Wrapped delegate: handler NORMALIZED to the method, teardown still exact.
119151
f = only_fix("WrappedDelegate")

0 commit comments

Comments
 (0)