Read InternalsVisibleTo grants from the compiled assembly manifest #85
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/internals-visible-to-test"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes task #145.
Review found the original
.csproj/Roslyn pair still missed two forms confirmed against built code: anInternalsVisibleToitem inDirectory.Build.props(grants every project at once) and anAssemblyAttributeitem namingInternalsVisibleToAttribute(a second MSBuild mechanism, invisible to both the item reader and the source sweep). Rather than extend the pair a third and fourth time — the same instalment plan ADR-0070's History paid for a different rule —CompiledAssembliesnow reads every project's compiled assembly manifest withMetadataLoadContext. Every expression of the grant converges on the sameInternalsVisibleToAttributemanifest entry, so the manifest is downstream of all of them at once. The item-form.csprojreader and the Roslyn source-attribute sweep are deleted, not kept alongside it — both are fully subsumed.ADR-0074 records the decision, including why it inverts ADR-0030's own reasoning rather than contradicting it (ADR-0030's objection to compiled output was that it under-reports a
ProjectReference; anInternalsVisibleTogrant has no effect unless it reaches the manifest, so there's nothing to under-report), and states the manifest reader's own limits: it can't say a grant is still needed rather than merely declared, and a grant behind an MSBuildConditionthat never evaluates true for the built configuration won't be found. ADR-0030 cross-references ADR-0074 in both directions.All four forms — the MSBuild item, a hand-written
[assembly: InternalsVisibleTo(...)]attribute, anAssemblyAttributeitem in a.csproj, and an item inDirectory.Build.props— were verified reddening the test by mutation, then reverted.Verdict: changes needed
Two confirmed, unstated evasions — both verified by building the mutation and watching every architecture test stay green:
InternalsVisibleToinDirectory.Build.props.<ItemGroup><InternalsVisibleTo Include="X" /></ItemGroup>there grantsXvisibility into every project in the solution (confirmed via-getItem:InternalsVisibleTo,DefiningProjectFullPath=Directory.Build.props).Solution.AllProjects()/Read()only opens each project's own.csproj, soEveryOtherProject_InternalsVisibleTo_IsNonenever sees it. It's also not caught by the existingBuildFileTests.BuildFiles_AcrossTheRepository_DeclareNothingThatLeadsElsewhere, becauseSolution.DeclarationsForbiddenInchecksDependencyItemNames(ProjectReference+ the external-reference items) andInternalsVisibleToItemNamewas deliberately kept out of that list. This is worse than a single-project miss — it's a solution-wide grant, invisible to both the new tests and the pre-existing build-file guard whose whole purpose is exactly this class of hole for every other item type.AssemblyAttributeitem in a project's own.csproj.<AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleToAttribute"><_Parameter1>X</_Parameter1></AssemblyAttribute>compiles to a real[assembly: InternalsVisibleToAttribute("X")](confirmed in the generatedAssemblyInfo.cs). Invisible to the item-form reader (only matches theInternalsVisibleToitem name) and to the Roslyn sweep (only parses.csfiles; this is.csprojXML).Neither gap is disclosed. The class remarks on
InternalsVisibleToTestscarefully name theusing-alias gap and the declared-vs-still-used gap, but say nothing about either of these, despiteDirectory.Build.propsbeing singled out elsewhere in this same test project as exactly the place a reference arrives from unseen. Close at least theDirectory.Build.propsone (addInternalsVisibleToItemNameto whatBuildFileTestsforbids there, or an equivalent explicit check) — it's a solution-wide bypass of a test whose entire job is catching a widened grant. If either is left open, say so in the remarks the same way the alias gap is.Everything else checks out: widened
PlaceMark.Apigrant, an untracked new.csprojwith a grant, and the fully-qualified attribute form (System.Runtime.CompilerServices.InternalsVisibleTo(...)) all correctly redden; PublicKeyToken/wildcard qualification isn't a real vector sinceInternalsVisibleTodoesn't support wildcards and any qualifier changes the string, which fails the exact-matchShouldBe. No ADR is a reasonable call — this applies ADR-0030's XML reading and ADR-0070's Roslyn precedent to their respective questions rather than deciding anything new. README paragraph matches what's implemented and doesn't overclaim. 0073 is free (highest ADR on this branch and on main is 0072).Assert the exact InternalsVisibleTo grants in the architecture teststo Read InternalsVisibleTo grants from the compiled assembly manifestVerdict: mergeable
Both prior evasions (
Directory.Build.propsitem,AssemblyAttributeitem) verified closed by mutation, alongside the two forms already caught (widened item, item on a clean project) — all four reddened as claimed, then reverted cleanly. Also tried an MSBuild-target-generated[assembly: InternalsVisibleTo](a fifth mechanism, not in the author's list) — caught correctly, confirming the manifest-is-downstream-of-everything claim rather than just the four enumerated forms.Deletion confirmed genuine:
Solution.AllProjects(),InternalsVisibleToItemName,ProjectFile.InternalsVisibleTo, and the Roslyn source sweep are gone with no remnants (grep -rn "AllProjects\|InternalsVisibleToItemName"acrosstests/andsrc/is empty).The three stated limits all verified true: a
Condition="'$(Configuration)'=='Debug'"grant on a project this reader considers clean is invisible when built/read in Release, exactly as disclosed; a missing compiled assembly throws the namedInvalidOperationExceptionrather than passing vacuously (confirmed by building only the test project, which leaves ~7 unreferenced projects unbuilt).One real limit isn't named alongside the other three. The reader opens every project but
PlaceMark.Apiby file path with noProjectReference, so nothing forces a rebuild before the read. Building/testingtests/PlaceMark.Architecture.Testsin isolation transitively rebuildsPlaceMark.Apiand its dependency chain (confirmed — those DLLs are freshly written), but notPlaceMark.WebUI,PlaceMark.Database, or the sibling*.Testsprojects. MutatingPlaceMark.WebUI's source to add a realInternalsVisibleTo("Evil")grant, without rebuilding it, then runningdotnet test tests/PlaceMark.Architecture.Tests -c Release— with or without--no-build— passes green: the reader silently opens the stale, pre-mutationPlaceMark.WebUI.dlland finds nothing. No exception, unlike the missing-assembly case. CI is unaffected (PlaceMark.slnxis always built and tested whole, per.forgejo/workflows/ci.yml), but the class remarks otherwise commit to naming every honest limit here (the alias gap, the declared-vs-needed gap, the Condition gap) and this one — present specifically because noProjectReferenceis taken to the other ~7 projects — isn't among them. Worth a line inCompiledAssemblies' orInternalsVisibleToTests' remarks alongside the other three.Cross-reference reasoning between ADR-0030 and ADR-0074 is sound as written and present in both directions: ADR-0030's objection to compiled output is under-reporting a declared dependency (an unused
ProjectReferencethat hasn't hit the manifest yet is still a real, if latent, coupling risk); anInternalsVisibleTogrant that never reaches the manifest has no effect at all, so there's nothing to under-report. Title, PR body, README and ADR README index all match the manifest-reading approach; ADR-0074 is genuinely the next free number (0073 landed onmainsince the last review).CI green on
fb3cf14(run #400). Merges cleanly againstmain(git merge-treereports no conflicts, matching the PR's ownmergeable: true). Full solution builds and all test projects pass under the pinned10.0.100SDK.