Repository navigation
test(arch): compute module edges from bound symbols, not import scope - #1078
Conversation
ModuleLedgerScanner now binds each module-owned file against a per-project compilation built with CompatibilityExceptionScanner's parse options, implicit usings and references. An edge is a type a node binds to, or the type declaring the member it binds to; using directives are skipped. Deletes six rows no code realises and declares three real references the syntax walk missed (FarmModule -> DiscountCeiling, IIdentityProvider -> EggUnit, AccessModule -> UserRoleAssignment).
|
Review by Codex (gpt-6-astra) at fdba5dc Verdict: request changes. One P2 semantic miss and one P3 stale-metadata finding.
Required mutations reproduced against head and its parent,
Twenty isolated syntax/semantic probes caught extension calls, simple Missing Domain metadata failed with 2,006 diagnostics. Raising the file floor to 10,000 failed both real-tree tests. Three fresh parallel scans produced identical serialized reports: 544 files, floor 400, 73 edges, no errors or stale rows. Per-file lists and ordered merging avoid shared mutation. Compilation helpers and real preprocessor symbols are reused; four real-tree scans became one shared All six removed rows are unbacked by direct references; all three added rows are real. Final restored-head checks passed, one class per process: |
|
Both findings are fixed at 3f610d3. P2:
Your repro ( P3: Compiling against source compilations was not the simpler fix, so I added a freshness check. Doc: decision 514's paragraph saying file-level imports are charged to every type is rewritten. I reran the full mutation table at 3f610d3; it is in the PR body. Every expected red is red, and the unused- |
|
Re-review by Codex (gpt-6-astra) at 3f610d3 Verdict: approve the reviewed fixes. No new P1/P2/P3 finding in the incremental diff. The original P2 repro now fails for the right reason: adding the I found no normal-build false red from the timestamp check:
The check compares the implementation DLL in the test project's bin directory, via Current Decision 514 is included in the three-file incremental diff. The obsolete import-scope paragraph is corrected. Both requested quality-review passes found no maintainability issue in the incremental changes. Final restored-head checks passed: The disclosed inferred-generic, deconstruction, awaiter and query-pattern gaps remain outside these fixes. Timestamp freshness also remains a modification-time check rather than proof of content identity. |
Closes #1071
TL;DR.
ModuleLedgerScannernow builds edges from the semantic model, not fromusingscope. An edge is a type that a node in a module-owned file binds to, or the type that declares the member the node binds to. That includes calls the compiler adds without a name in the source: user-defined conversions, collection-initializerAddand theforeachenumerator pattern. The edge is charged to the node's top-level type. Using directives are skipped. If a sibling project's built assembly is older than that project's source, the walk fails. As a result, the ledger loses six rows that no code realises and gains three real references that the syntax walk missed.Start here:
ModuleLedgerScanner.cs(Scan's project loop,RecordEdges,ReferencedTypes), thenRealModuleLedger.Edges.cs. Everything else follows from those two files.Change map
Architecture/ModuleLedgerScanner.csScanFilekeeps the syntax checks (declarations, unowned namespaces,global using) and returns the file's attributions. A newRecordEdgesbinds eachExpressionSyntaxin module-owned files. A member resolves to its declaring type. A type includes its type arguments, element types and pointer targets. A type claimed by #1023 is charged to its claimant, which is howIIdentityProviderbecomes an Access type. A compile error in a module-owned file fails the walk, except CS8795, the stub of a generated[GeneratedRegex]partial. Files owned only by Platform are never bound, soCluckwork.Api's unresolved package references (826 errors) are skipped at no cost.BoundSymbolsaddsGetConversion(...).MethodSymbol,GetCollectionInitializerSymbolInfoandGetForEachStatementInfo(GetEnumerator, MoveNext, Current, Dispose and the element conversion) toGetSymbolInfo(review round 1, P2).AddStaleAssembliesfails the walk whenbin/<Project>.dllis older than the newest.csinsrc/<Project>. Stale metadata can bind a member to the wrong type without any diagnostic, so this is checked explicitly (review round 1, P3). Binding runs per file in parallel. The syntax walk's "referenced unowned namespace" check is gone: a referenced src type is always declared somewhere, and that declaration is already checked.Architecture/CompatibilityExceptionScanner.csCompile,References,ImplicitUsingsand a newParsebecomeinternal.Referencestakes the assembly to exclude. The ledger reuses this machinery instead of building a second compile path. Compat's behaviour is unchanged.Architecture/RealModuleLedger.Edges.csFirstRunProvisioningLatch(Access→Farm),UnassignFlockHandler(Access→FlockManagement),{Adjust,Submit,Void}DailyEntryResponse(EggOperations→FlockManagement) andIFarmModule(Farm→Commerce). Added rows:IIdentityProvider(Access→Commerce),FarmModule(Farm→Commerce) andAccessModule(Access→Farm, because it readsUserRoleAssignment.Id/FlockId, a third real reference the issue did not list). The reasons are updated to match.Architecture/ModuleLedgerRealTreeTests.cs,CouplingMatrixRealTreeTests.csLazyreal-tree report, using the patternCompatibilityExceptionRealTreeTestsalready follows. Four scans become one.Architecture/ModuleLedgerTests.csusingalone was an edge. They now use the type they import. Removed tests: block-scoped using (the scope logic is gone) and referenced-unowned (replaced by the compile-error test). Added tests:UsingWithNoUse_IsGreen,MemberAccessOnly_IsAnEdge,ClaimedType_ChargesItsReferencesToItsClaimant,UnboundName_InAModuleType_MakesTheWalkUntrusted_ButNotInAPlatformType,CompilerChosenCall_IsAnEdge(3 cases: implicit conversion, inherited collection-initializerAdd,foreach) andSiblingAssemblyOlderThanItsSource_MakesTheWalkUntrusted.Architecture/Data/coupling-matrix.mdCLUCKWORK_REGENERATE_MATRIX=1, not edited by hand. Access→FlockManagement R (4→3), Access→Commerce W (5→6), EggOperations→FlockManagement W (7→4).docs/decisions/514-module-ledger.md,src/AGENTS.mdusingcounts as a dependency". It also said "a file-levelusingis attributed to every top-level type". All three statements are now false and have been rewritten. The record now names the compiler-chosen calls, the inferred-generics gap and the stale-assembly check.Note on the issue text: its seventh false row (
UserFlockAssignment→ Farm) was already removed by #1070.Mutations
All mutations were rerun at 3f610d3. A row marked base ran on origin/main f9bcdb0. Filter:
ModuleLedgerRealTreeTests, plusModuleLedgerTestsfor the scanner mutations.DiscountCeilinggainsimplicit operator decimal, andCreateExpenseHandleraddsdecimal? percent = account.MaxDiscount;undeclared … Finance -> Commerce … CreateExpenseHandlerGetConversionCompilerChosenCall_IsAnEdge(conversion)AddCompilerChosenCall_IsAnEdge(initializer)foreachpatternCompilerChosenCall_IsAnEdge(foreach)SiblingAssemblyOlderThanItsSource_…toucha Domain source, then run with--no-buildCluckwork.Domain.dll is older than src/Cluckwork.Domain/Sales/DiscountCeiling.cs; green after a normal buildCreateExpenseHandleradds_ = account.MaxDiscount?.Percent;(member access only, Commerce is never named)undeclared cross-owner edge Finance -> Commerce … CreateExpenseHandlerCreateExpenseHandleradds an unusedusing Cluckwork.Domain.Sales;undeclared … Finance -> CommerceCluckwork.Domain.Sales.DiscountCeilingundeclared cross-owner edge Finance -> Commerce(also red on base)SetStepperUnitHandlerfrom cell 1 (the first symbol at base)undeclared cross-owner edge Access -> Commerce(also red on base)IIdentityProvider, cell 1's first symbol at headundeclared … Access -> Commerce from …IIdentityProviderSetLanguageValidator.cs:24: CS8795ClaimedType_…and staleAccess -> Commerce :: IIdentityProviderMemberAccessOnly_IsAnEdgeand staleFarm -> Commerce :: FarmModuleUsingWithNoUse_IsGreen(the first fixture survived; I tightened it until this went red)UnboundName_…CompoundUsingAlias_IsAnEdgeMeasured
Cluckwork.Application.Testslisted testsArchitecture.*ModuleLedgerTestsModuleLedgerRealTreeTestswall time (12-core host, 3 runs)CouplingMatrixRealTreeTestswall timeCompatibilityExceptionRealTreeTestswall timeThe file-count floor (400) and the build's preprocessor symbols (#1056/#1061) are unchanged: the parse still goes through
ModuleLedgerScanner.ParseOptions. Other suites run on this branch, all green:CompatibilityExceptionTests,CouplingMatrixTests,SourcePreprocessorTests,PeerContractRealTreeTests,AdapterReachRealTreeTestsand the remainingArchitecture.*classes one by one, plusImagePin/SchemaDocsTests,RenameAccountDocsTestsandTenancyDocsFreshnessTests.Known limits: inferred generic arguments are not charged unless a name in the source binds to them. Other calls the compiler adds implicitly, such as
Deconstruct,GetAwaiterand query-pattern methods, are also not charged; the review's probes found no others. Each per-project compilation references the bin'sCluckwork.*.dllfor every sibling project. Stale metadata can bind silently with zero diagnostics (Astra's fixture showed this), so the walk compares timestamps and fails when an assembly is older than its project's newest source. It does not compare content.