diff --git a/docs/notes/incremental-computation.md b/docs/notes/incremental-computation.md new file mode 100644 index 00000000..4b27b6fb --- /dev/null +++ b/docs/notes/incremental-computation.md @@ -0,0 +1,104 @@ +# Discovery note — incremental computation: when, how, and why not yet + +- **Type:** discovery note (thinking on the record — *not* a proposal, *not* a + commitment, *not* normative). Lives outside `proposals/` because it has no design + shape yet: it records a direction we considered and the conditions under which it + becomes worth a real proposal. +- **Trigger:** came across [Differential Datalog (DDlog)](https://github.com/vmware-archive/differential-datalog) + — incremental Datalog compiled to Rust on top of + [differential-dataflow](https://github.com/TimelyDataflow/differential-dataflow). + Note the org: `vmware-archive` — the project is **archived / unmaintained**. +- **Related:** [P-002](../proposals/P-002-verification-backend.md) (a backend that + consumes facts — Datalog-adjacent), [P-005](../proposals/P-005-idisposable-ownership.md) + (D5 ownership-transfer-through-callee — the first interprocedural itch), + [P-006](../proposals/P-006-di-lifetimes.md) (DI / captive-dependency — call-graph + reasoning), [P-011](../proposals/P-011-editor-tooling.md) (editor tooling — the + live-feedback use case), [P-013](../proposals/P-013-distribution-surface.md) (the + **"one checker"** discipline this must not break). Strategy hub: + [`../ROADMAP.md`](../ROADMAP.md). + +## The idea in one line + +Compute diagnostics from a **change** to the input facts (one edited file → a few +delta facts) rather than re-deriving everything from scratch — so the tool can give +near-instant feedback on a large solution and re-check a PR by its diff. + +## Why it is conceptually a good fit + +Own.NET's core is already **fact-based**. OwnIR is relational tuples +(`acquire`/`borrow`/`use`/`release`/`escape`, `Loan(owner, binding, kind)`), and the +core's rules (R1-R10, lifetime L1-L3) are logical inference rules over those tuples. +That is *exactly* the Datalog idiom, and fact-based static analysis is the +mainstream home of Datalog: Doop (Java points-to on Soufflé), CodeQL (QL, an +object-oriented Datalog). So "express the core as Datalog rules" is a recognized +architecture for our class of tool, and incremental Datalog (DDlog) is the version +of that which also gives delta-in → delta-out for free. + +## Why it is *not* the move right now + +1. **The current scope is intraprocedural, so incrementality is nearly free + already.** A finding depends only on one method/class's facts (ROADMAP: + "intraprocedural first"). A change does not ripple across the program, so a + trivial **file-hash cache** ("re-extract and re-check only the files that + changed") captures ~all of the practical win with none of the machinery. + Differential dataflow earns its keep on *cross-program* derivations (transitive + closures, joins over the whole call graph) — which we deliberately don't do yet. +2. **DDlog specifically is archived.** Adopting an unmaintained DSL + compiler is a + standing supply-chain and maintenance risk. +3. **It would violate "one checker."** We already span two legs (C# extractor + + Python core). A DDlog engine adds a **third language (Rust) and a second + inference engine** — and the load-bearing invariant (P-013:19-21, ROADMAP) is + that the Python core is the single source of truth and everything else only + *produces or consumes* OwnIR facts. A second thing that decides verdicts is + precisely what the project refuses. + +## When it *does* earn its keep (the two gates) + +Revisit seriously only when at least one is actually on the table: + +- **Gate A — we go interprocedural / whole-program.** The deferred itches: + ownership transfer through a callee ([P-005](../proposals/P-005-idisposable-ownership.md) + D5), DI lifetimes and call-graph reasoning ([P-006](../proposals/P-006-di-lifetimes.md)). + There a change to one method's signature ripples across the call graph and naive + recomputation gets expensive — the classic regime where incremental dataflow + (incremental points-to / call-graph) pays off. +- **Gate B — we want live IDE feedback.** [P-011](../proposals/P-011-editor-tooling.md): + on every keystroke, re-running the whole solution (a real target is hundreds of + files) is wasteful; you want edit → delta diagnostics. + +Until one of these is real, incrementality is over-engineering and the file-hash +cache is the honest answer. + +## If/when it matters — aim here, not at archived DDlog + +- **Roslyn is already incremental on the extraction side.** Incremental parsing + + semantic models come for free once the extractor uses a workspace + ([P-001](../proposals/P-001-csharp-extractor.md) / P-014 Tier B). That covers + *fact extraction* without any new engine; only the *inference* core would need + its own incrementality. +- **Salsa (Rust)** — the demand-driven, query-memoizing incremental framework behind + rust-analyzer (which *is* incremental static analysis for an IDE). For the Gate B + / P-011 future it fits more naturally than Datalog (query → cache → invalidate on + input change), and it is battle-tested in production. +- **differential-dataflow directly** — if we genuinely want delta-in/delta-out, build + on the maintained engine (it powers Materialize) rather than archived DDlog layered + over it. +- **Soufflé** — Datalog→C++, maintained, used by Doop. The right pick if the goal is + "express the core declaratively as Datalog" for speed/clarity, *not* for + incrementality (it is not incremental by default). + +## The non-negotiable guardrail + +Whatever the engine, incremental computation is an **optimization of how facts and +verdicts are recomputed — never a new decider.** It MUST yield bit-for-bit the same +verdicts as the batch Python core, proven by differential testing (batch vs +incremental over the corpus). The moment an incremental engine becomes a second +place where a verdict is decided, we have broken "one checker" (P-013) — which the +project's own manifesto treats as fatal. + +## Decision + +**Park it.** No action now; the file-hash cache covers today's intraprocedural +scope. Promote this note to a real proposal only when Gate A or Gate B lands — and +when it does, default to Salsa (IDE) or maintained differential-dataflow / Soufflé +(heavy inference) over archived DDlog, under the identical-verdicts guardrail. diff --git a/docs/proposals/P-014-semantic-resolution.md b/docs/proposals/P-014-semantic-resolution.md new file mode 100644 index 00000000..3d978b57 --- /dev/null +++ b/docs/proposals/P-014-semantic-resolution.md @@ -0,0 +1,380 @@ +# P-014 — Project-local semantic resolution for the C# extractor + +- **Status:** draft (P0 — the extractor is currently unusable on real desktop code) +- **Depends on:** + - [P-001](P-001-csharp-extractor.md) — the seam this deepens. P-001 defines the + Roslyn-extractor → OwnIR → Python-core pipeline (P-001:71-77) *and* owns the + `ownir_version` stamp / additive-fact discipline (P-001:93-97). Both stay intact. + - [P-004](P-004-wpf-lifetime-profile.md) — consumes the cleaned signal; keeps the + long-lived-owner lifetime judgment (P-004:14-16, WPF005 → OWN014 at P-004:35,45). + - [P-005](P-005-idisposable-ownership.md) — the disposable-field/local checks + benefit from the same `SemanticModel` later (a follow-up increment, not this one). + - `spec/OwnCore.md`, `spec/Diagnostics.md`, `spec/Lifetimes.md` — the fact + vocabulary and the diagnostic catalogue. **Unchanged by this proposal** (see + *Relationship to the spec & docs*). +- **Strategy hub:** [`docs/ROADMAP.md`](../ROADMAP.md). + +## Motivation + +Run the v0 extractor on a real WPF/desktop solution (GTM, ~hundreds of `.cs`) and +it produces a *wall* of false positives. Measured on that codebase: of **326** +"event subscribed but never unsubscribed" findings, **105 (32%)** are pure +arithmetic — `sum += value`, `sumSbor += p.Sbor.Cost`, `totalItem.Quantity += +item.Quantity`, and even **for-loop steps** (`for (int startIndex = 0; …; +startIndex += batchSize)` flagged four times as an "event"). The remaining 221 +are real subscriptions, but they are drowned in the noise — and many of *those* +are not leaks either (a `Window` subscribed to its own `Loaded`/`Closing`). + +Two distinct failures, both predicted by our own docs: + +1. **It matches arithmetic as event subscriptions.** A syntax-only Roslyn pass + cannot tell `event += handler` from `decimal += decimal` — both parse to the + same `AddAssignmentExpression`. `IsHandler` (Program.cs:91-92) accepts any + `IdentifierName`/`MemberAccess` RHS, so every numeric accumulation becomes a + "leak". On a real codebase this is the *majority* of the noise. +2. **It can't see external types.** `grid.View.CellValueChanged += …` on a + DevExpress control, `Closing += Window_Closing` on a WPF `Window` — the + declaring type lives in an assembly the syntax pass never opened, so the + extractor guesses, and guesses "leak". + +This is exactly the `event += without -=` cell the reality-matrix tags **⚠️ +heuristic** with "false positives" (ROADMAP.md:100), and exactly the +*confidently-wrong* tooling the philosophy section rejects: *"honestly skipping +beats confidently lying; the market for confident-but-wrong tooling is already +saturated"* (ROADMAP.md:49-51). The cause is structural: the `event +=` fact is +**type-dependent**, and v0 is **type-free**. No amount of syntactic cleverness +closes that gap — a tightened heuristic (e.g. "RHS must be a method declared in +the class") still lies on `total += obj.Foo` when `Foo` also names a method, and +still cannot see an external event. The only real fix is to let the extractor +*see types*. + +## Scope + +Build the type-dependent facts from a Roslyn **`SemanticModel`** instead of raw +syntax. Concretely, `lhs += rhs` becomes a subscription fact **iff** the model +binds `lhs` to an event (`IEventSymbol`) — equivalently, the model classifies the +node as an `IEventAssignmentOperation` — otherwise it is not emitted. Three tiers, +in cost order. + +### Tier A — project-local compilation (default, no third-party refs) + +Replace the per-file `CSharpSyntaxTree.ParseText(...)` loop (Program.cs:131-155, +one isolated tree per file) with a single `CSharpCompilation` over **all** input +source trees, referencing the extractor runtime's framework assemblies via +`AppContext.GetData("TRUSTED_PLATFORM_ASSEMBLIES")` (zero-config — they are on disk +wherever `dotnet` runs) — *nothing else*. Compiling all inputs together means +cross-file *and* cross-project in-project types/events resolve for free, with no +project-to-project references and no MSBuild. Per candidate `+=`, take +`GetSemanticModel(tree)` and inspect the LHS symbol / operation. (Killing the +numeric FPs needs no references at all — a declared `decimal sumSbor` binds to an +`IFieldSymbol` regardless of whether `decimal` resolves, and an `IFieldSymbol` is +not an `IEventSymbol`; the framework set only *promotes* BCL events from +"unresolved" to a clean fact.) Scoping the compilation to one project of a large +monorepo is a noted future capability — see *Open questions*; the default is one +compilation over everything passed in. This already resolves locals, fields, in-project types and primitives: + +- `sum += value` → `sum` binds to a `decimal` local/field → not an event → + **silent** (cause #1 gone — all 105 numeric FPs drop); +- an in-project `event Foo += h` → `IEventSymbol` → real subscription fact; +- BCL events (`INotifyPropertyChanged.PropertyChanged`, + `INotifyCollectionChanged.CollectionChanged`) resolve from the framework + reference → real subscription fact; +- a DevExpress `grid.CellValueChanged += h` → declaring type unresolved → handled + by the fallback below. + +We **never reference or parse DevExpress (or any third-party) source.** Their +types simply stay unresolved, and unresolved is not "leak". + +### Tier B — full references (opt-in flag) + +When the user *wants* external events checked, resolve the project's real +references. Two mechanisms, traded off in *Open questions*: + +- **`MSBuildWorkspace`** — open the `.csproj`/`.sln`; the build system resolves + references (PackageReferences, project-to-project, framework, transitive). This + is the principled "load all the target's dependencies" with no fake project. + *Cost:* needs a matching .NET SDK + a prior `restore`, pulls in + `Microsoft.CodeAnalysis.Workspaces.MSBuild` + MSBuild assemblies (via + `MSBuildLocator`), is SDK-version-sensitive, and is heavier in CI. +- **`MetadataReference` over the target's built `bin/**/*.dll`** — point the + compilation at the already-compiled output. Roslyn reads assembly **metadata** + (public types and events) directly — **no source, no decompiler.** *Cost:* + requires a prior successful build (populated `bin`), and discovering the right + TFM output folder; over-references harmlessly (referencing unused DLLs is free). + +Either way it is **metadata only** — never third-party source, never a decompiler. + +### Unresolved fallback (first-class, honest) + +When a symbol cannot be resolved (an error type — Tier A on an external event, +Tier B on a missing reference), the extractor **MUST NOT emit a leak, and MUST NOT +fall back to the old syntactic guess.** It emits a distinct *informational* +diagnostic, in our voice: + +```text +OWN050: cannot verify 'grid.View.CellValueChanged' — its declaring type is an +unresolved reference (build the project or pass references); leakage analysis +skipped +``` + +This is the ROADMAP's "honestly skipping" made literal: it is not a leak claim +and not a clean bill of health — it is *unchecked*, and we say exactly that. It is +counted separately from findings and never fails a build. **Severity is +`warning`** — see *Severity & plumbing*; there is no `info`/`note` tier in the +core and this proposal does not invent one. + +> Note: an early sketch phrased this as "…skipped; not a leak". We drop the +> "not a leak" clause — it reads as a verdict ("we checked, it's fine") when the +> truth is the opposite of a verdict. "leakage analysis skipped" is the honest +> framing. + +## Severity & plumbing (the non-obvious part) + +Two facts about the current core constrain the implementation: + +1. **The core has exactly two severities, `ERROR` and `WARNING`** (`Severity` + enum, `ownlang/diagnostics.py:28-30`); `Diagnostic` defaults to `ERROR` + (diagnostics.py:79). In `ownir.py`, `severity` is a presentation string scoped + to `error`/`warning` (render_finding, ownir.py:174-183). There is **no level + below warning.** The unresolved diagnostic is therefore a `warning`. + +2. **`check_facts()` drops every diagnostic whose `severity != Severity.ERROR`** + (`ownir.py:326-329`). A warning emitted through the *normal* diagnostic path + would be silently discarded and never reach the C# bridge. The unresolved + channel must therefore be produced as a `Finding` through a path that + **bypasses** that filter — mirroring how `DI001` is produced and appended + directly via `findings.extend(_di_findings(facts))` (ownir.py:377; helper at + ownir.py:390-412). The subscription-leak message itself is built in the + `check_facts` if/elif chain (ownir.py:344-372); the new channel plugs in as a + sibling helper, not inside that chain. + +Code id: **OWN050** — deliberately a fresh band, not a gap-fill. OWN001-041 are +all *faults*; this is a *coverage note* ("we could not check"), so it sits outside +the fault series rather than squatting an unrelated gap (OWN025-029 would read as +buffer-policy codes, since OWN015-024 is the buffer block). OWN050 opens a "C# +front-end / resolution coverage" band (051+ reserved for future front-end coverage +notes). Stays in the `OWN` namespace (the user sees one prefix; the `DI001` code is +the captive-dependency check's, not a precedent for a new front-end prefix). Add +the title to the `TITLES` map (a code with no title renders empty but does not +crash, diagnostics.py:88-90). + +## Rollout (incremental — one fact at a time) + +The `SemanticModel` infrastructure (one `CSharpCompilation`, per-tree +`GetSemanticModel`) is built once, but applied **first to the event-subscription +fact only** — the highest-noise check. The disposable-field, local-disposable and +pool detectors keep their current name/suffix matching (`IsDisposableType`, +Program.cs:123-127) for now; they are lower-volume and lower-FP, and migrating +them to symbol-based type resolution is a clean follow-up increment once the +compilation plumbing has proven itself. This keeps P-014 a contained, reviewable +change rather than a rewrite of every detector. + +## Build plan (Tier A, incremental) + +Ordered; each task is independently reviewable with an explicit acceptance check. +T1→T2 are sequential (infra then fix); T3/T4 land with T2; T5 (docs) any time; T6 +(validation) last. + +### T0 — dependency prep +- `OwnSharp.Extractor.csproj`: confirm `Microsoft.CodeAnalysis.CSharp` is the only + Roslyn package needed (Tier A uses TPA `MetadataReference`s — **no** Workspaces / + MSBuild package). Update the syntax-only comment (csproj:24). +- **Accept:** builds, no new heavy dependency. + +### T1 — one compilation + semantic model (infra, no behaviour change) +- Refactor Program.cs:131-155: parse every input `.cs` into a `SyntaxTree` (keep + `path:`), build **one** `CSharpCompilation.Create(...)` over all trees with + references = `AppContext.GetData("TRUSTED_PLATFORM_ASSEMBLIES")` split on + `Path.PathSeparator` → `MetadataReference.CreateFromFile`, options + `new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)`. Error-tolerant: + ignore compile diagnostics. +- Get `compilation.GetSemanticModel(tree)` per tree; thread it into the + class/assignment loop. Leave every detector's logic unchanged for now. +- **Accept:** extractor runs on GTM + `frontend/roslyn/samples` and emits the + **same facts as before** (pure regression — model is built but unused); no crash + on unresolved symbols; runtime acceptable (one parse of all files + symbol tables). + +### T2 — semantic event discriminator (the fix) +- In the event loop (Program.cs:174-189) replace the `IsHandler` + AddAssignment + gate with symbol-based classification of `a.Left`: + - `model.GetSymbolInfo(a.Left).Symbol is IEventSymbol` (equivalently + `model.GetOperation(a) is IEventAssignmentOperation`) → **subscription fact** + (released = existing `-=` / `.Stop()` matching, unchanged). + - resolved to a non-event (`ILocalSymbol`/`IFieldSymbol`/`IPropertySymbol`/ + `IParameterSymbol`…) → **skip** — this is what kills the numeric FPs. + - unresolved (`Symbol == null` / error-typed receiver) → emit an **unresolved + marker** fact (e.g. `resource: "unresolved-subscription"`) for the OWN050 + channel. **Do not** fall back to the old syntactic guess. +- **Accept:** on GTM all 105 numeric `+=` FPs disappear (`for(... i += step)`, + `sum += value`, `x.Qty += y.Qty`); in-project + BCL events + (`PropertyChanged`/`CollectionChanged`) still emit subscription facts; + WPF/DevExpress events emit unresolved markers (not leaks). + +### T3 — `--event-leaks` gate (default off) +- Parse a `--event-leaks` flag in Program.cs (default **off**). When off, suppress + emission of `subscription`/`timer`/`unresolved-subscription` facts; keep + `disposable`/`local-disposable`/`pool`. Tier A landing flips the documented + default to **on**. +- **Accept:** default GTM run emits zero subscription/OWN050 output; `--event-leaks` + turns them on. + +### T4 — OWN050 channel in the core (warning, bypasses the ERROR filter) +- `ownlang/ownir.py`: add `_unresolved_findings(facts)` mirroring `_di_findings` + (helper near :390-412), appended at :377 via `findings.extend(...)`. Build a + `Finding(code="OWN050", message="cannot verify '' — its declaring type is + an unresolved reference (build the project or pass references); leakage analysis + skipped", …)` rendered as **warning**. This path bypasses the `ERROR`-only filter + at check_facts (:326-329). +- `ownlang/diagnostics.py`: add `"OWN050": "declaring type unresolved — leakage + analysis skipped"` to `TITLES` (:33-71). +- **Accept:** a sample with an unresolved external event yields one OWN050 warning; + exit code unchanged (warnings never fail the build). + +### T5 — spec + prose (anti-drift) +- `spec/Diagnostics.md`: add the OWN050 row (the one normative addition — a + coverage note / warning). +- Supersede the 10 "syntax-only" sites listed under *Relationship to the spec & + docs*, **including the by-hand `ROADMAP.md:37-40` reword** (a grep for + "syntax-only" misses it). Remove the `frontend/roslyn/README.md:73-75` non-goal + "semantic event resolution". +- **Accept:** grep for "syntax-only" / "no compilation, no references" is clean; + ROADMAP philosophy reworded; spec/ core otherwise untouched. + +### T6 — tests / corpus validation +- Add corpus cases: numeric `+=` (silent), in-project event (subscription), + BCL `PropertyChanged` (subscription), unresolved external event (OWN050). Update + any test asserting the old syntactic behaviour. +- Re-run the GTM triage; record before/after (326 findings / 105 FP → event-FP ≈ 0). +- **Accept:** test suite green; GTM numeric-`+=` event FPs at zero. + +## Non-goals + +- **A full semantic C# frontend.** We resolve only what a *specific fact* needs + (the type of a `+=` LHS). No whole-program dataflow, no async/generics/LINQ + inference. Still **narrow, intraprocedural, fact-only** — `SemanticModel` is a + sharper chisel, not a mandate to "understand C#". +- **Parsing third-party source, or a decompiler.** Tier B reads compiled + metadata; that is the entire external-reference story. If it isn't in metadata, + it's unresolved → the fallback fires. +- **Deciding whether a *resolved* subscription is actually a leak.** That lifetime + judgment (is the owner long-lived? is a `Window`'s own-event `-=` moot?) stays + **P-004** — its mechanisms are `[OwnIgnore("source lifetime is shorter")]` + (P-004:60-61) and WPF005 firing OWN014 only on a *longer-lived* source + (P-004:35,45). P-014 removes the *gross* noise (arithmetic, unresolved + externals) so P-004's heuristic operates on real subscriptions only. +- **Requiring a green build for Tier A.** The compilation degrades gracefully: + partial resolution still types the locals/fields Tier A needs. +- **A new `info`/`note` severity level.** The unresolved channel is a `warning`. + Adding a third severity is a real (if small) core change and is out of scope. + +## Versioning + +An additive, optional informational channel does **not** require an +`OWNIR_VERSION` bump. `OWNIR_VERSION = 0`, bumped only "whenever the fact +vocabulary changes incompatibly" (ownir.py:77-81); the existing additive precedent +is explicit — "the resource/type fields are additive and optional, so they do NOT +bump `ownir_version`" (ownir.py:48-51). `load()` rejects only a mismatched integer +and treats an absent field as current (ownir.py:200-208). This discipline +originates in **P-001:93-97** and the `ownir.py` docstring — *not* P-013 (an +earlier draft mis-cited P-013, which contains no version rule; its governing +constraint is the ROADMAP "one checker" rule, P-013:19-21). + +In the recommended implementation the unresolved diagnostic is a **core-side +`Finding`** (the `_di_findings`-style path), so it introduces *no new OwnIR input +fact at all* — it is trivially version-neutral. Tier A/B change only *how* the +extractor decides to emit the existing subscription fact, not the fact's shape. + +## Relationship to the spec & docs (avoiding drift) + +The whole point of `spec/` vs `docs/` is that aspirational prose must not lie +about the code. Explicitly: + +- **Normative `spec/` is untouched.** A whole-word search over all 8 spec files + finds **zero** mentions of `Roslyn`, `SemanticModel`, whole-word `references`, + or `syntactic`; `syntax` only ever means OwnLang's own surface grammar + (Grammar.md:4), and `compilation` appears once meaning C# `[Conditional]` + symbols in codegen (BufferPolicies.md:57). The spec is **Roslyn-agnostic**, so + changing *how the C# front-end extracts a fact* moves no normative text. (To be + precise: `spec/` is not implementation-*neutral* — every file pins its source of + truth to the Python modules, e.g. Diagnostics.md:3-5 → `ownlang/diagnostics.py`. + But the front-end is not the spec's substrate, so the "zero normative drift" + conclusion holds for exactly that reason.) **Net normative drift: zero.** +- **The one normative addition** is the new diagnostic OWN050: one row in + `spec/Diagnostics.md` plus a matching `TITLES` entry in `ownlang/diagnostics.py`. + The `SemanticModel` work itself adds no code. +- **The "syntax-only" prose must be superseded on build**, or it becomes the lie + this repo guards against. Each flips to *"type-aware (project-local + `SemanticModel`), still fact-only and intraprocedural"*: + - `frontend/roslyn/OwnSharp.Extractor/Program.cs:3` — header "syntax only — no + compilation, no references". + - `Program.cs:117-118` — "syntax-only heuristic — no semantic model" (disposable + type matching; becomes symbol-based when P-005's increment lands). + - `Program.cs:293` — "ownership transfer is ambiguous syntactically" (soften: + resolvable via project-local binding where available, still conservative). + - `OwnSharp.Extractor.csproj:24` — "Syntax-only Roslyn… no compilation/references". + - `frontend/roslyn/README.md:13` — "Syntax-only (no compilation, no references)". + - `frontend/roslyn/README.md:73-75` — **removes** the non-goal "semantic event + resolution" (P-014 delivers exactly that); the "one checker, not two" line + (README:16) stays true. + - `docs/howto-visual-studio.md:153-155` — drop "syntax-only"; keep the + no-interprocedural / no-async / honest-skip sentences verbatim. + - `README.md:142` (Russian) — the `(frontend/roslyn/, syntax-only)` tag. + - `docs/proposals/P-001-csharp-extractor.md:12-13` and `:34` — the "C#, + syntax-only" / "syntactic/local pattern extractor" tags; cross-reference P-014 + as the successor increment. + - **`docs/ROADMAP.md:37-40`** — the load-bearing one: *"The frontend's job is not + to 'understand C#' (SemanticModel hides async, generics, LINQ…)"*. This is not + a literal "syntax-only" string (a grep-and-replace would miss it) and is the + single most direct philosophical contradiction. Reword by hand to: *"uses a + project-local `SemanticModel` for binding/type resolution — not whole-language + understanding (still no async/generics/LINQ/whole-program reasoning)."* +- **It fulfils the ROADMAP philosophy, not contradicts it.** "The frontend's job + is to extract facts" still holds — we resolve a type to get *one fact* right. + And the unresolved-fallback *is* "honestly skipping beats confidently lying". + The "deliberately does NOT touch yet" list (ROADMAP.md:44-48: async, generics, + LINQ, closures, interprocedural, whole-program…) is untouched — P-014 adds + binding, not any of those. + +## Resolved decisions + +- **Tier A reference set.** Framework assemblies via the extractor runtime's + `TRUSTED_PLATFORM_ASSEMBLIES` (zero-config); **one** `CSharpCompilation` over all + input trees (cross-project resolution for free — no project-to-project refs, no + MSBuild); do not chase the target's exact TFM (WPF/DevExpress stay unresolved → + fallback / Tier B). Killing the numeric FPs needs no references at all (symbol + *kind* comes from the declaration); the framework set only promotes BCL events + (`PropertyChanged`/`CollectionChanged`) from "unresolved" to a clean fact. +- **Interim behaviour.** No tightened-but-still-lying heuristic. Ship a + default-**off** gate on the event-subscription rule now (disposable/pool stay + on); Tier A graduates the default off→on. This gate is the *first instance* of a + general "select which checks run" surface — see Open questions Q2. +- **Diagnostic code.** **OWN050**, severity `warning`, emitted via the + filter-bypassing `_di_findings`-style path (not the `ERROR`-only `check_facts` + pipeline). + +## Open questions + +1. **Compilation scoping in a large monorepo.** The default is one compilation + over everything passed in. In a monorepo of many projects where the user wants + to analyse *one*, we need a way to scope it — e.g. accept a `.csproj`/`.sln` + filter, a per-project compilation mode, or path-based scoping. **Deferred — not + built in the first cut**, recorded here so the one-compilation default is a + conscious choice, not a ceiling. +2. **General check-selection / configuration surface.** The interim gate-off is a + special case of a broader need: a way to enable/disable check *categories* + (subscription / disposable / pool / local-disposable / …) and set their + severities — ideally a linter-style **config file** (`.ownrc` / `own.toml`, à la + `ruff.toml` / `.editorconfig`) rather than an ever-growing flag list. This is + large enough to warrant **its own proposal (candidate P-015)**; P-014 ships only + the single `--event-leaks` gate as instance #1 and forward-references it. +3. **Tier B mechanism / default.** `MSBuildWorkspace` (robust, needs SDK + + restore, heavier) vs `bin/**/*.dll` `MetadataReference`s (lighter, needs a prior + build). Leaning **bin-DLL `MetadataReference`** as the opt-in default + (CI-friendly, no SDK coupling — relevant given P-001's "sandbox has no local SDK" + note), with `MSBuildWorkspace` as the zero-config-from-`.csproj` alternative. +4. **`[OwnIgnore("reason")]` interplay.** Does a suppressed member also suppress + its OWN050 "unchecked" note, or is "unchecked" orthogonal to "ignored"? +5. **Performance / model shift.** The per-file scan becomes a single per-run + compilation (symbols must resolve across files). Cache it; acceptable for + CI/local, but it is a real architecture change to the extractor's loop + (Program.cs:131-155), not a comment edit. diff --git a/docs/proposals/P-015-configuration-surface.md b/docs/proposals/P-015-configuration-surface.md new file mode 100644 index 00000000..d3b43c23 --- /dev/null +++ b/docs/proposals/P-015-configuration-surface.md @@ -0,0 +1,113 @@ +# P-015 — Configuration surface: check selection & per-category severity + +- **Status:** draft (**stub** — to be fleshed out; P2, quality-of-life, not blocking) +- **Origin:** [P-014](P-014-semantic-resolution.md) Open Question 2. P-014 ships a + single `--event-leaks` gate as *instance #1* of the need this proposal generalises. +- **Depends on / reconciles with:** + - [P-013](P-013-distribution-surface.md) — the existing surface (`--format`, + `--severity`) and the **"one checker"** discipline: the Python core is the + single source of truth, wrappers stay thin, exactly one place decides what a + finding says (P-013:19-21, 45-46). Config must not become a second decider. + - [P-004](P-004-wpf-lifetime-profile.md) — `[OwnIgnore("reason")]` (P-004:60-61): + inline, per-site suppression. This proposal is its project-wide counterpart; + the two must compose, not collide. + - `spec/CLI.md` (the CLI contract), `ownlang/ownir.py` (renderer + the + `Severity` model, diagnostics.py:28-30). + +## Motivation + +A flag per check does not scale. P-014 already needs `--event-leaks`; the +disposable/pool/local-disposable detectors will each want the same on/off and +severity control; a real codebase wants *"treat subscriptions as warnings, keep +disposables as errors, skip pool checks in tests/"*. Every linter solved this with +a **config file** (`.eslintrc`, `ruff.toml`, `.editorconfig`, `rustfmt.toml`), not +a growing flag list. Own.NET should follow the convention rather than accreting +one `--no-X` flag per check. + +The honest-skip philosophy (ROADMAP.md:49-51) makes this load-bearing, not +cosmetic: until a check is trustworthy (cf. P-014's event rule before Tier A), the +*right* state is "off" — and that state should be expressible per-project, in one +place, under version control, not buried in CI invocation strings. + +## Scope (draft) + +A discovered config file — working name `.ownrc` / `own.toml` (format TBD, see +Open questions) — that controls, per **check category**: + +- **enabled / disabled** (the generalised `--event-leaks` gate); +- **severity** (`error` / `warning` / `off`), per category, overriding the global + `--severity` default; +- optionally **per-path overrides** (globs, e.g. relax a category under `tests/`), + à la `.editorconfig` / ruff `[per-file-ignores]`. + +Check categories map to the resource kinds the extractor already emits — +`subscription` / `subscribe` / `timer` / `disposable` / `local-disposable` / +`pool` — plus, later, the core OWN0NN families. The category vocabulary should be a +small, documented, stable set. + +Discovery: nearest config walking up from the scanned path (the +`.editorconfig`/ruff model), with CLI flags overriding the file. + +## Where it is enforced (the "one checker" constraint) + +Per P-013, the verdict authority is the **core**. So config that changes a verdict +(severity, on/off) is consumed in the core (`ownlang`), in one place — *not* split +across the wrappers, and *not* a second checker. Precedence (draft): + +``` +CLI flag > inline [OwnIgnore] > config file > built-in default +``` + +The extractor *may* skip emitting facts for a disabled category as an optimisation, +but the authoritative selection is core-side so there is one place that decides. + +This collides with one known plumbing fact (from P-014): `check_facts()` drops +every diagnostic whose `severity != Severity.ERROR` (ownir.py:326-329), and the +`Severity` enum is only `ERROR`/`WARNING` (diagnostics.py:28-30). So: + +- `off` → don't emit the finding; +- `warning` → emit but advisory — needs the filter-bypassing path P-014 already + introduces for OWN050 (`_di_findings`-style, ownir.py:377), or a widening of the + `check_facts` filter; +- `error` → the existing path. + +Whether to widen the filter or route everything advisory through the bypass path +is the central implementation question. + +## Non-goals (draft) + +- **Per-rule fine-grained config** beyond the category level (initially). Start + with categories; finer granularity is bug-driven later. +- **A query/policy language.** It is a settings file, not a DSL. +- **Editor integration / live config** — that is [P-011](P-011-editor-tooling.md). +- **Changing the OwnIR fact contract.** Config is a driver/presentation concern; + it adds no fact and needs no `ownir_version` bump. + +## Relationship to the spec & docs (anti-drift) + +- **Normative `spec/` core is untouched** — config selects/relabels findings, it + does not change the ownership semantics or the OwnIR vocabulary. The one doc that + grows is `spec/CLI.md` (the CLI/driver contract), plus a new section or doc + describing the config schema *when built* (spec follows code). +- The `Severity` model may gain an explicit `off`/advisory handling; if a third + severity tier is ever introduced it lands in `diagnostics.py` + `spec/Diagnostics.md` + — but the draft above avoids that by treating `off` as non-emission and `warning` + via the existing bypass path. + +## Open questions + +1. **File format.** TOML (`own.toml`, typed, ecosystem-standard) vs an + `.editorconfig`-style INI vs JSON. Leaning TOML. +2. **Discovery & precedence.** Nearest-file-up vs explicit `--config PATH`; + exact precedence vs CLI and vs `[OwnIgnore]` (draft order above). +3. **Enforcement point.** Core-side only (clean "one checker") vs extractor-side + skip for disabled categories (cheaper) — likely both, with the core + authoritative. +4. **Severity plumbing.** Widen the `check_facts` `ERROR`-only filter, or route all + advisory findings through the OWN050/`_di_findings` bypass path? (Shared + question with P-014.) +5. **Category vocabulary.** Lock the stable category names (resource kinds today; + how do core OWN0NN families map in?). +6. **Overlap with P-013.** `--severity` is a *global* presentation default; + per-category severity here supersedes it. Confirm the two compose cleanly and + document the single precedence story. diff --git a/docs/proposals/README.md b/docs/proposals/README.md index 5f7fb70b..5b4d94da 100644 --- a/docs/proposals/README.md +++ b/docs/proposals/README.md @@ -34,9 +34,12 @@ proposal is marked `done` with a pointer. | [P-010](P-010-type-disciplines.md) | Richer type disciplines (`Own.Types`) | draft | | [P-011](P-011-editor-tooling.md) | Editor tooling & syntax highlighting | draft | | [P-012](P-012-bug-corpus-mining.md) | Real-world bug corpus & mining | draft | +| [P-013](P-013-distribution-surface.md) | Distribution surface (how people run Own.NET) | v0 built (CI/Action + dotnet tool) | +| [P-014](P-014-semantic-resolution.md) | Project-local semantic resolution (kills `+=` false positives) | draft (P0) | +| [P-015](P-015-configuration-surface.md) | Configuration surface (check selection & per-category severity) | draft (stub) | > For priorities, milestones, the framing, and the design philosophy across all -> of these, see the strategy hub: [`docs/ROADMAP.md`](../ROADMAP.md). P-004 … P-012 +> of these, see the strategy hub: [`docs/ROADMAP.md`](../ROADMAP.md). P-004 … P-015 > capture ideas raised in design discussion — they are *on the record for > consideration*, drafts, not commitments.