feat(csharp): DI binding edges, partial-class merge, and ASP.NET entry points - #374
Conversation
…y points Emit resolver-consumed EdgeProvides for MS.DI and Autofac registrations (generic, typeof, closed-generic args, factory lambdas), with a synthesizer expanding AsImplementedInterfaces() from resolved implements edges. Merge partial-class fragments onto one canonical node, keyed (repo, scope_ns, name). Stamp controllers, handlers and test methods as entry points, propagating aspnet:controller down the resolved inheritance chain so dead-code analysis sees the full set.
zzet
left a comment
There was a problem hiding this comment.
Thanks for this — it's a substantial contribution and the write-up is excellent. Validating end-to-end on a real 8.5k-file Autofac + EF solution and reporting the concrete numbers (837 provides edges, 177 merged partials with 0 wrong fusions across 6 same-named migration sets, 44/119 controllers previously missed) is exactly the kind of evidence that makes a change like this reviewable. The documented known-gaps block at the emitter is also much appreciated — silent partial coverage is worse than a stated boundary.
I've gone through it in detail. Most of the placement calls are right; I have two things I'd like sorted before merge, plus a handful of smaller items.
What's well placed
Worth saying explicitly, since these were the non-obvious calls and you got them right:
- The DI emit reuses the shared spine.
emitDotNetDIBindingsgoing throughemitDIProvides/diShortName/diNormalizeFQCNfrominternal/parser/languages/di_container.go— rather than a parallel C#-only path — is exactly what I'd have asked for. The canonicalEdgeProvides+binding:"useClass"+provides_forshape documented in that file's header is the contract, and C# now speaks it. AsImplementedInterfacesas a framework synthesizer is the right decomposition. The extractor genuinely cannot see the interface set, the resolver can, and the marker-edge handoff is the clean way to express that. Registered with thedotnetfamily gate and ordered after the implements-producing passes — correct on both counts.- The partial-class merge correctly is not a framework synthesizer.
partialis a language feature, not a framework convention, so sitting next torebindGoMethodReceiversin the attribution sweep is right. The(repo, scope_ns, name)key and the "a Go package == its directory, a C# namespace ≠ its directory" reasoning are spot on, as is excluding partial interfaces with theInferImplementsrationale. - The join conservatism throughout (unique same-repo lookups, method-set-inference excluded, unindexed framework interfaces left unbound, dedup against an explicit
.As<>()) is the right default. Test coverage on gates, idempotency and negative cases is above the bar.
Two things to change before merge
1. propagateAspNetControllerHierarchy — placement and scope
This is the one I'd most like to move. Two separate issues:
Placement. It's entry-point semantics living in internal/indexer, and it hardcodes "entry_point", "entry_point_kind" and "aspnet:controller" as string literals instead of entrypoints.MetaEntryPoint / entrypoints.MetaEntryKind. internal/indexer already imports internal/entrypoints (indexer.go:29), so there's no import-cycle reason for the duplication. If either constant is ever renamed, this pass silently stops stamping with no compile error — and controller_hierarchy_test.go would stay green, because it hardcodes the same literals. A test that can't fail when production breaks is the part that worries me.
It's also a language-generic mechanism wearing a C#-specific name. "A framework entry point conferred through a base class the per-file detector can't see" is not unique to ASP.NET; Java, Python and TS all have the same hole. I'd rather this were propagateEntryPointsDownHierarchy(kinds) in internal/entrypoints, with aspnet:controller as the first registered kind, so the next language gets it for free.
Scope — this is the blocking half. The pass opens with g.EdgesByKind(graph.EdgeExtends) over the whole graph, builds derivedByBase for every language, hydrates every node touching any extends edge via GetNodesByIDs, and only then filters n.Language == "csharp". On the sqlite backend EdgesByKind is SELECT … FROM edges WHERE kind = ? materialised into a slice before yielding, so in a 20-repo TypeScript/Java workspace with zero C# this does a full extends materialisation plus mass node hydration to produce zero stamps.
The part that makes it blocking is the incremental path. In RunIncrementalDerivedPasses this is the only unscoped whole-graph call — every sibling is scoped (InferImplementsScoped(typeFrontier), markTestSymbolsAndEmitEdgesScoped(scopedPrefixes, …), synthesizeCapabilityEdgesScoped(…), RunFrameworkSynthesizersScopedForFiles(…)). And DerivedInvalidatesDeclarations fires on any language's declaration-fingerprint change (derived_invalidation.go:215), plus unconditionally in the LegacyFallback branch just above it. So editing a Go struct in a Go repo triggers a whole-graph extends scan across all tracked repos. We spent real effort a while back getting single-file saves off whole-graph passes (that was roughly a 10s → 200ms move), and I'd like not to hand any of it back.
The fix is cheap and I think makes the pass better anyway: invert the walk. Seed from the already-stamped controllers, then follow incoming extends edges outward from the seed set. Cost becomes O(controller subtree) instead of O(all extends edges in the workspace), it's naturally a no-op when there are no seeds, and the language gate falls out for free. On the incremental path, take scopedPrefixes + merged.TypeIDs like the passes around it.
One related note: the multi.go comment justifies whole-graph scope with "a controller hierarchy can span repos (a shared base project)", but the partial merge and csharpDIResolveIface in the same PR both deliberately restrict themselves to same-repo. I'd like the risk posture consistent — if cross-repo hierarchies matter, the scope should be "changed repos plus what they depend on", not "everything".
2. mergeCSharpPartialTypes isn't actually language-gated
This one is genuinely our fault, not yours, and the code reads as if it should work.
The pass relies on r.nodesByKindLang(KindType, "csharp") pushing the language filter server-side. No store implements NodesByKindLang — the only occurrence in the repo is the optional-interface assertion at internal/resolver/language_gate.go:29. So it always takes the fallback: NodesByKind(KindType) plus an in-Go filter, i.e. a full type-node scan with gob Meta decode on every whole-graph attribution sweep, in every workspace, C# or not.
The obvious next move is to add the graphHasLanguage("csharp") guard the sibling passes carry (go_builtins_attribution.go:82, external_call_attribution.go:74, razor_using.go:25). Heads up before you do: that helper is vestigial in production today. The only HasLanguage implementation in the tree is a test double in batch_hotpaths_test.go, so it always hits the conservative return true — meaning the existing Go/Python/Razor/Lua gates are no-ops too.
So the real fix is a backend HasLanguage (and ideally NodesByKindLang) implementation, which gates this pass and un-breaks several existing ones. That's arguably its own PR, and I'm happy for it to be one — I just don't want to merge a new full-graph scan on the assumption a gate is working when it isn't. Either land the backend method here, or gate the pass on something that actually holds today.
Smaller items
- Dead guard.
sameKindinmergeCSharpPartialGroupscan never be false — every node in the group came fromnodesByKindLang(KindType, …), so they all shareKindType. The comment reads as though it distinguishes class/struct/record, but those aren't separate kinds. Either drop it or reword. - Meta keys crossing module boundaries as literals.
"partial_merged_into"is written incsharp_partial_merge.goand read incsharp_di_ifaces.go;"binding"/"useClass"/"asImplementedInterfaces"/"provides_for"/"impl_fqcn"spaninternal/parser/languages↔internal/resolver. These should be exported constants indi_container.go, next to the header that already documents the shape as the contract. Also worth a second look: the merge marker is documented as not SQLite-persisted, butcsharp_di_ifaces.gouses it as a correctness input to its ambiguity check — I'd like that dependency either removed or made durable. lineAtis quadratic.strings.Count(text[:off], "\n")inside sixFindAllStringSubmatchIndexloops is O(file_size × registrations); a real Autofac module with a few hundred registrations will feel it. Matches come back in ascending offset order, so a cursor (or one precomputed newline table) fixes it.- Six extra full-file regex scans per C# file, run on every C# file rather than composition roots. A cheap
strings.Contains(text, "RegisterType")/".Add"pre-filter before the alternations would pay for itself. - No comment/string stripping, so a registration inside a
//comment or a@"…"verbatim string emits a real binding edge. Pre-existing indiRegistrationRe, but this PR multiplies the surface — worth at least a note in the known-gaps block. - Double stamp in
Detect.detectASPNet(…) + detectDotNetFramework(…): in a minimal-APIProgram.cs, a[HttpGet]method gets stampedaspnet:hostand then overwritten toaspnet:handler, and counted twice. The count is only logged so it's cosmetic, but the sum isn't a count of stamped symbols.
Happy to take the fixes myself
If you'd rather not carry the rework, I'm glad to do it — just tick "Allow edits by maintainers" on the PR (or push me a collaborator invite on your fork) and I'll take items 1 and 2 directly on your branch so your authorship stays on the commits. Equally happy to review another round if you'd prefer to drive it.
Either way, thanks again — the DI binding work in particular is a real accuracy win for .NET codebases, and I'd like to get it in.
The resolver's language gates were production no-ops: no store
implemented either optional interface. Add both to the sqlite store
(recursive repo-prefix skip-scan probe; kind+language scan pushed into
SQL) and gate mergeCSharpPartialTypes on graphHasLanguage("csharp").
Now language-generic PropagateEntryPointsDownHierarchy: seeds from already-stamped entry points and follows incoming resolved extends edges, O(seed subtree) instead of a whole-graph extends scan. The incremental path takes the changed-type frontier and also climbs to stamped ancestors, so a new derived type in an unchanged hierarchy still stamps. Uses the MetaEntryPoint/MetaEntryKind constants.
Export the DI binding Meta keys from internal/graph (neutral home: both sides import it without pulling CGO into the resolver). Persist the partial_merged_into marker — a correctness input to the DI synthesizer. lineAt now uses one newline table behind a containment prefilter, with comment/string false positives documented. stamp() reports first-stamp so Detect counts each symbol once. Drop the dead sameKind guard.
964e2ea to
2d75e34
Compare
|
Thanks for the detailed review — all items addressed in three commits on the branch, left as deltas (no squash). 1. Controller pass relocated + inverted (ff8f12b). Now The incremental form ( That also settles the cross-repo posture question: seeds come from the changed frontier, the upward climb may land in a repository the changed one depends on (a shared base project), and propagation follows resolved edges wherever derived types live — "changed repos plus what they depend on", now stated in the doc comment. One behavioral fix that fell out of the rewrite: a descendant already stamped with a different kind (a derived xunit fixture) is left alone instead of being overwritten to controller. 2. Backend language gates are real now (55bdd78). The sqlite store implements both optional interfaces. Smaller items (2d75e34):
On the Windows dev box the only test failures are the known pre-existing ones (verified identical on a clean 88ac9df worktree, plan-lock suite included); CI should confirm the rest. |
|
@pbednarcik thank you for your contribution! |
Summary
Improves C#/.NET accuracy for DI-heavy codebases: container registrations become resolver-consumed binding edges, partial classes merge onto one canonical type node, and ASP.NET controllers/handlers/tests are stamped as entry points (with inheritance-chain propagation). Validated end-to-end on a production Autofac + EF solution (8,535 files, 131k nodes / 1.1M edges).
Changes
csharp_dotnet.go): explicit registrations emit theEdgeProvidesshapebuildProvidesForIndexconsumes — MS.DI two-arg generic, AutofacRegisterType<T>().As<I>()incl. fluent chains andtypeofpairs, closed-generic type args (As<IRepository<User>>, bound on the de-genericized name), and factory lambdas thatnewup the impl. Known gaps (assembly scanning, delegating factories, multiple.As) documented at the emitter.AsImplementedInterfaces()synthesizer (resolver/csharp_di_ifaces.go, registered ascsharp-di-implemented-interfaces,dotnetfamily gate): the interface set isn't in the call text, so the extractor emits a marker edge and the synthesizer joins it with the impl's resolvedimplementsset after the implements-producing passes. Conservative joins: exact same-repo unique lookups, method-set-inferred implements excluded, unindexed interfaces (IDisposableetc.) deliberately unbound.resolver/csharp_partial_merge.go): folds per-file fragments of apartialtype onto the lowest-ID node, keyed(repo, scope_ns, name). Runs last in the whole-graph attribution sweep + a per-file scoped variant on the incremental path; reindexes ridepersistAttributionReindexes. Classes/structs/records only — merging partial interfaces would corruptInferImplements.entrypoints.go, modeled ondetectJava):[ApiController]/[Controller]orController/ControllerBasebase →aspnet:controller;[HttpGet..Head]/[Route]→aspnet:handler;[Fact]/[Theory]/[Test]/[TestMethod]→ framework test kinds. Stamps symbols, not files.indexer/controller_hierarchy.go): per-file detection can't see an intermediate base's own base list (ClaimController : BaseController) — measured at 44 of 119 controllers missed on the reference codebase. A derived-graph pass walks resolvedextendsedges downward from every stamped controller and persists via the sameAddBatchupsert the test-symbol stamping uses.Real-world results on the reference solution: 837 provides edges, 177 partial types merged (0 wrong fusions across 6 same-named EF migration sets in different namespaces), 119/119 controllers + 1,116 handlers + ~7,200 test methods stamped.
Testing
go test -race ./...) — on Linux-equivalent scope; the only failures on the Windows dev box are pre-existing path-normalization tests, verified identical on a cleanmainworktreeChecklist
rebindGoMethodReceivers,ResolveCSharpInterfaceDispatchScoped,detectJava,markTestSymbolsAndEmitEdges)Meta["methods"]for interfaces (unchanged; the partial merge deliberately excludes interfaces because of it)EdgeMemberOfedges to their containing type (unchanged; the merge retargets them onto the canonical fragment)