diff --git a/Directory.Build.props b/Directory.Build.props index 7365dbf02..4aacd0ebe 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -69,4 +69,26 @@ + + + <_FalloutOuterApiAssemblies>;Fallout.Common;Fallout.Build;Fallout.Build.Shared;Fallout.Components;Fallout.Core;Fallout.Tooling;Fallout.ProjectModel;Fallout.Utilities;Fallout.Utilities.IO.Compression;Fallout.Utilities.IO.Globbing;Fallout.Utilities.Net;Fallout.Utilities.Text.Json;Fallout.Utilities.Text.Yaml;Fallout.Solution;Nuke.Common;Nuke.Build;Nuke.Components; + true + + + + + + + + diff --git a/docs/adr/0011-reintroduce-publicapi-ide-hint.md b/docs/adr/0011-reintroduce-publicapi-ide-hint.md new file mode 100644 index 000000000..409f9c0d8 --- /dev/null +++ b/docs/adr/0011-reintroduce-publicapi-ide-hint.md @@ -0,0 +1,97 @@ +# ADR-0011: Reintroduce JetBrains `[PublicAPI]` as an IDE authoring hint + +## Status + +Proposed (2026-07-24). Tracks [#539](https://github.com/Fallout-build/Fallout/issues/539). Partially reverses [PR #76](https://github.com/Fallout-build/Fallout/pull/76) (the wholesale `JetBrains.Annotations` removal), but only the `[PublicAPI]` concern and via a different mechanism — not a straight revert. + +## Context + +[PR #76](https://github.com/Fallout-build/Fallout/pull/76) removed `JetBrains.Annotations` wholesale — ~1450 `[PublicAPI]` attributes plus `[CanBeNull]`, `[Pure]`, `[UsedImplicitly]`, `[ContractAnnotation]`, and others. The stated rationale held: most of those attributes carried no weight for *this* repo's own build, and hand-maintaining ~1450 call sites is a genuine cost. + +One consequence has proven to hurt day-to-day contribution: **Rider/ReSharper now flags Fallout's public build-authoring surface as unused.** That surface is unused *in-repo by design* — external consumers call `DotNetTasks`, subclass `GitHubActionsAttribute`, implement `IComponent`, and so on; the framework itself does not. Without a signal that this is intentional API, the IDE's unused-symbol inspection lights up exactly the code we ship, drowning real findings in false positives. + +`[PublicAPI]` is precisely that signal: it tells Rider/ReSharper "this is intentional API, consumed externally — do not flag it as unused." It is an **IDE authoring hint only** — no runtime behaviour, no semantic change, no effect on compilation beyond an attribute in metadata. + +**This is a distinct concern from public-API break detection.** Two things get conflated because both say "public API": + +- **This ADR — an IDE authoring hint.** Silences a false "unused" inspection. Cares only that a symbol *is* API, not whether it changed. +- **API-surface break detection** — [#410](https://github.com/Fallout-build/Fallout/issues/410) / [PR #530](https://github.com/Fallout-build/Fallout/pull/530), the Roslyn `PublicApiAnalyzers` (`PublicAPI.Shipped.txt` / `.Unshipped.txt`) track, plus the `PublicApiGenerator` snapshot work. Guards the shipped surface against accidental change and feeds the changelog. + +They are complementary and independent. This ADR does not touch, block, or depend on that track. + +The prior re-introduction attempt lived on an unmerged fork branch (`feature/jetbrains-publicapi-annotations`) with no issue and no ADR — exactly the "did it without recording why" pattern we want to avoid. This ADR exists so the decision is reviewable in the open. + +## Decision + +**Reintroduce `JetBrains.Annotations` for the `[PublicAPI]` concern only, applied as a per-assembly blanket on the consumer-facing layer, with a selective per-type carve-out for extensibility surface in inner assemblies (the "hybrid" split).** + +1. **Outer (consumer-facing) assemblies get a blanket `[assembly: PublicAPI]`.** Stamping it at assembly scope declares the assembly's *entire* public surface as intentional API — which, for a build framework, is true: outer assemblies exist to be consumed. This avoids reintroducing ~1450 hand-placed attributes and the maintenance tax of keeping them in sync as new API lands. + +2. **The outer layer is a single MSBuild allowlist** — `_FalloutOuterApiAssemblies` in `Directory.Build.props` — the one source of truth for "what is consumer-facing." Membership at time of writing: + + `Fallout.Common`, `Fallout.Build`, `Fallout.Build.Shared`, `Fallout.Components`, `Fallout.Core`, `Fallout.Tooling`, `Fallout.ProjectModel`, `Fallout.Utilities`, `Fallout.Utilities.IO.Compression`, `Fallout.Utilities.IO.Globbing`, `Fallout.Utilities.Net`, `Fallout.Utilities.Text.Json`, `Fallout.Utilities.Text.Yaml`, `Fallout.Solution`, and the `Nuke.*` transition shims (`Nuke.Common`, `Nuke.Build`, `Nuke.Components`). + +3. **Inner assemblies get no blanket.** `Fallout.Cli`, `Fallout.Migrate`, `Fallout.Migrate.Analyzers`, `Fallout.MSBuildTasks`, `Fallout.SourceGenerators`, `Fallout.Tooling.Generator`, and `Fallout.Persistence.Solution` are build-time tooling, codegen, the CLI host, and a vendored parser — not build-authoring API. Their `public` types are implementation details behind a facade or an exe boundary. + +4. **Genuine extensibility surface *inside* an inner assembly is annotated per-type in source** — a `[PublicAPI]` on the specific type, not a blanket on the assembly. This is the "hybrid" half: it keeps the blanket honest (an inner assembly's blanket would wrongly bless its implementation details as API) while still silencing the false-unused inspection on the rare inner type an external consumer really does touch. Each such carve-out is recorded in [docs/api-encapsulation-audit.md](../api-encapsulation-audit.md). + +5. **The reference flows to consumers (no `PrivateAssets`).** `[PublicAPI]` is only useful to a consumer's IDE if the attribute is present in the metadata the consumer resolves; that requires `JetBrains.Annotations` to be a normal (non-private) dependency of the outer packages, exactly as upstream NUKE shipped it. A consumer's Rider/ReSharper then sees our surface as API too. + +6. **A project opts out** with `false` set before the import. Central Package Management is off for `build/_build.csproj`, so the wiring is additionally guarded on `'$(ManagePackageVersionsCentrally)' != 'false'`. + +Only `[PublicAPI]` is reintroduced. The other removed attributes (`[CanBeNull]`, `[Pure]`, `[ContractAnnotation]`, …) stay gone — they carried the maintenance cost PR #76 objected to without addressing the unused-symbol problem this ADR is about. Reintroducing them, if ever wanted, is a separate decision. + +## Consequences + +### Positive + +- **The false "unused" inspection on shipped API is silenced** — for contributors here and for consumers whose IDE resolves our packages. +- **Low churn, one source of truth.** The consumer-facing layer is one allowlist, not ~1450 attributes drifting out of sync. New API in an outer assembly is covered automatically. +- **The blanket stays honest.** By keeping inner assemblies off the blanket and carving out only real extensibility types, the annotation continues to *mean* "intentional API" rather than "happens to be public." +- **Reversible and inert.** It is an attribute in metadata; removing the allowlist entry or the package reference fully reverts it, and it never affects runtime. + +### Negative + +- **Outer packages gain a dependency on `JetBrains.Annotations`.** Because the reference is non-private (it must be, to reach consumers), consumers acquire a transitive dependency on the annotations package. It is small and runtime-inert, and it is what NUKE shipped, but it is a real addition to the dependency graph — the main trade-off this decision makes. (An internal, self-defined copy of the attribute avoids the dependency but does not flow to consumers; see Alternatives.) +- **`[assembly: PublicAPI]` is coarse.** It blesses an outer assembly's *entire* public surface, including anything that is `public` by accident. That is the risk the encapsulation audit exists to manage: public-by-accident types in outer assemblies are tracked and narrowed via the normal breaking-change flow, not left silently blessed forever. +- **The allowlist is a curated judgement call**, not mechanically derived — packability is SDK-default-true for non-test projects here, so it cannot stand in for "consumer-facing." Membership needs a human decision at review time and maintenance as the assembly graph evolves (notably the in-progress onion/layering work). + +### Neutral + +- No versioning, channel, or release-pipeline impact (ADR-0004/0007/0008 untouched). Adding or removing `[PublicAPI]` is not a breaking change. +- Independent of the API-break-detection track (#410 / #530); neither blocks the other. +- The other JetBrains attributes remain removed. + +## Alternatives considered + +### A. Restore per-type `[PublicAPI]` on every public member (the pre-#76 state) + +Reinstate all ~1450 attributes, hand-maintained. + +**Rejected** — it re-incurs exactly the maintenance cost PR #76 removed, and every new public API needs a remembered attribute. The assembly-level blanket achieves the same IDE outcome for the outer layer at a fraction of the surface area. + +### B. Define an internal `[PublicAPI]` attribute in our own source (no package dependency) + +Declare the attribute ourselves under a `JETBRAINS_ANNOTATIONS`-style guard, avoiding the `JetBrains.Annotations` package. + +**Rejected** — an internally-defined attribute silences the inspection in *our* IDE but does not flow to *consumers'* IDEs (they would need the same attribute type by full name in resolvable metadata). Since a stated goal is that consumers' Rider sees our surface as API, the real package is required. Worth revisiting only if the dependency proves objectionable and consumer-side hinting is dropped as a goal. + +### C. Do nothing; suppress the inspection via `.editorconfig` / DotSettings + +Turn down the unused-symbol severity for the relevant assemblies instead of annotating. + +**Rejected** — a blunt severity change hides *genuine* unused-symbol findings too (real dead code in the same assemblies), and it is per-checkout tooling config that does nothing for consumers. `[PublicAPI]` is the targeted signal: "unused because it's API," not "stop checking for unused." + +### D. Ship annotations as an external XML file alongside the assembly + +JetBrains supports `.ExternalAnnotations.xml`. + +**Rejected** — higher-friction to author and keep in sync than a compiled attribute, and packaging/resolving the XML for consumers is more fragile than the attribute-in-metadata path. The blanket assembly attribute is simpler and travels with the assembly. + +## References + +- [#539](https://github.com/Fallout-build/Fallout/issues/539) — the tracking issue for this decision. +- [PR #76](https://github.com/Fallout-build/Fallout/pull/76) — the wholesale `JetBrains.Annotations` removal this partially reverses. +- [#410](https://github.com/Fallout-build/Fallout/issues/410) / [PR #530](https://github.com/Fallout-build/Fallout/pull/530) — public-API break detection (Roslyn `PublicApiAnalyzers`); the separate, complementary concern. +- [docs/api-encapsulation-audit.md](../api-encapsulation-audit.md) — the public-by-accident audit + per-type carve-out registry that accompanies this pass. +- `Directory.Build.props` — `_FalloutOuterApiAssemblies` (the allowlist) and the `FalloutPublicApi` wiring. diff --git a/docs/adr/README.md b/docs/adr/README.md index a932561f3..586f8c477 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -40,3 +40,4 @@ If you change a decision, do NOT silently rewrite the old ADR — add a new one | [0004](0004-calendar-versioning-and-dual-pace-channels.md) | Calendar versioning + dual-pace channels (edge/stable) + experimental APIs | Accepted (§3 amended by 0007; channel ladder §2 superseded by 0008) | | [0007](0007-cut-release-branch-on-demand.md) | Cut `release/YYYY` on demand, not preemptively | Accepted | | [0008](0008-collapse-experimental-into-main.md) | Collapse `experimental` into `main`; `main` is the sole prerelease lane | Accepted | +| [0011](0011-reintroduce-publicapi-ide-hint.md) | Reintroduce JetBrains `[PublicAPI]` as an IDE authoring hint | Proposed | diff --git a/docs/api-encapsulation-audit.md b/docs/api-encapsulation-audit.md new file mode 100644 index 000000000..867a96a91 --- /dev/null +++ b/docs/api-encapsulation-audit.md @@ -0,0 +1,50 @@ +# API encapsulation audit + +Companion to the `[PublicAPI]` annotation pass — [issue #539](https://github.com/Fallout-build/Fallout/issues/539), [ADR-0011](adr/0011-reintroduce-publicapi-ide-hint.md). Stamping `[assembly: PublicAPI]` on an outer assembly declares its **entire** public surface as intentional API, so this doc records the outer/inner split, the hybrid per-type carve-outs, and any `public`-by-accident surface the pass exposes. + +> **Caveat that drives every call here:** repo-only grep is insufficient for a framework. A type with zero in-repo references can be internal plumbing **or** an extensibility surface only external consumers touch. Verify intent (NUKE history, `public`/`protected virtual` reachability, whether it appears in an outer assembly's public signatures) before flipping anything. + +## Outer layer — blanket `[assembly: PublicAPI]` + +Consumer-facing assemblies, listed in `_FalloutOuterApiAssemblies` in `Directory.Build.props` (single source of truth). Each gets a blanket assembly attribute: + +`Fallout.Common`, `Fallout.Build`, `Fallout.Build.Shared`, `Fallout.Components`, `Fallout.Core`, `Fallout.Tooling`, `Fallout.ProjectModel`, `Fallout.Utilities(.IO.Compression|.IO.Globbing|.Net|.Text.Json|.Text.Yaml)`, `Fallout.Solution`, and the `Nuke.*` transition shims (`Nuke.Common`, `Nuke.Build`, `Nuke.Components`). + +The blanket is coarse — it blesses anything `public` in these assemblies, including public-by-accident types. See **Public-by-accident (outer) — follow-up** below. + +## Inner layer — no blanket + +Build-time tooling, codegen, the CLI host, the migration tool, and the vendored parser. Their `public` types are implementation details behind a facade, an exe boundary, or a Roslyn/MSBuild plugin contract — not build-authoring API. Audited (2026-07-24), genuine consumer-facing surface per assembly: + +| Assembly | Genuine consumer extensibility? | Note | +|---|---|---| +| `Fallout.Cli` | None | Almost entirely `internal`; `Program` is an entry point; the one `public` `Configuration` is a non-compiled scaffolding template. | +| `Fallout.Migrate` | None | Only `public static class Program` (tool entry point). | +| `Fallout.Migrate.Analyzers` | None | `DiagnosticAnalyzer` / `CodeFixProvider` — Roslyn plugin endpoints, loaded by reflection, not consumer-referenced. | +| `Fallout.MSBuildTasks` | None (1 borderline) | All public types are MSBuild `Task` endpoints consumed via ``. `ContextAwareTask` is an abstract base but is subclassed **only in-assembly** — see borderline note. | +| `Fallout.SourceGenerators` | None | `ISourceGenerator` / `IIncrementalGenerator` plugin endpoints + one static helper. | +| `Fallout.Tooling.Generator` | None | Referenced only by `Fallout.MSBuildTasks` (inner→inner codegen). Its interfaces/models read like an API but no outer assembly or consumer reaches them. | +| `Fallout.Persistence.Solution` | **Yes** — see hybrid carve-out | Vendored parser; already encapsulated (`internal`-dominated). A small model whitelist leaks through `Fallout.Solution`'s public API. | + +## Hybrid carve-out — per-type `[PublicAPI]` in `Fallout.Persistence.Solution` + +`Fallout.Persistence.Solution` stays **off** the outer allowlist deliberately: its csproj keeps most types `internal`, exposing only a small whitelist. A blanket would be fragile — a future accidental `public` in vendored code would be silently blessed. Instead, the model types that appear directly in `Fallout.Solution`'s public constructors / `GetModel()` returns carry a per-type `[PublicAPI]` in source: + +| Type | File | Surfaces via | +|---|---|---| +| `SolutionModel` | `Model/SolutionModel.cs` | `Solution(SolutionModel, …)` ctor + `Solution.GetModel()` | +| `SolutionItemModel` | `Model/SolutionItemModel.cs` | `SolutionItem(SolutionItemModel, …)` ctor (abstract base of the two below) | +| `SolutionProjectModel` | `Model/SolutionProjectModel.cs` | `Project(SolutionProjectModel, …)` ctor + `Project.GetModel()` | +| `SolutionFolderModel` | `Model/SolutionFolderModel.cs` | `SolutionFolder(SolutionFolderModel, …)` ctor + `SolutionFolder.GetModel()` | + +The project takes a non-private `JetBrains.Annotations` reference so the annotation flows to consumers of the (packed) `Fallout.Persistence.Solution` package. + +**Line drawn at "appears in an outer assembly's public signature."** Types reachable only by *navigation* from the four above — `PropertyContainerModel` (their base), `SolutionPropertyBag` + `PropertiesScope`, `ProjectType`, `ConfigurationRule` + `BuildDimension`, `StringTable` — are **not** annotated. They're candidates if reviewers want the deeper model graph covered, or if the facade later exposes them directly. The serializer surface (`ISolutionSerializer*`, `ISerializerModelExtension*`, `SolutionSerializers`, the `Solution*Exception` types) is a vendored-library extension point Fallout does not expose through its facade — left unannotated. + +### Borderline + +- `ContextAwareTask` (`Fallout.MSBuildTasks`) — an abstract MSBuild-task base with `protected abstract`/`virtual` members, but subclassed only within its own assembly. Not annotated; revisit if it becomes a supported consumer base. + +## Public-by-accident (outer) — follow-up + +The blanket on outer assemblies blesses their *entire* public surface. A full sweep for types that are `public` by accident (implementation detail that should be `internal`) is **tracked follow-up**, not done in this pass — narrowing any of them is a breaking change and follows the breaking-change flow (gate behind `[Experimental("FALLOUT0xx")]` or hold for the year cut; `CHANGELOG` + `breaking-change` label). Do not silently internalize an outer public type on the strength of in-repo grep alone. diff --git a/src/Persistence/Fallout.Persistence.Solution/Fallout.Persistence.Solution.csproj b/src/Persistence/Fallout.Persistence.Solution/Fallout.Persistence.Solution.csproj index d9a7e8c6b..5501fd147 100644 --- a/src/Persistence/Fallout.Persistence.Solution/Fallout.Persistence.Solution.csproj +++ b/src/Persistence/Fallout.Persistence.Solution/Fallout.Persistence.Solution.csproj @@ -28,6 +28,14 @@ + + + + + ResXFileCodeGenerator diff --git a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionFolderModel.cs b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionFolderModel.cs index 09c899e75..8b518c7d7 100644 --- a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionFolderModel.cs +++ b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionFolderModel.cs @@ -8,6 +8,8 @@ namespace Fallout.Persistence.Solution.Model; /// /// Represents a solution folder in the solution model. /// +// [PublicAPI]: surfaces through Fallout.Solution's public API (SolutionFolder ctor + GetModel) — IDE authoring hint, see docs/adr/0011-reintroduce-publicapi-ide-hint.md +[JetBrains.Annotations.PublicAPI] public sealed class SolutionFolderModel : SolutionItemModel { private const string CycleBreaker = "***"; // to ensure no cycles diff --git a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionItemModel.cs b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionItemModel.cs index d8ae245b3..f374828cb 100644 --- a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionItemModel.cs +++ b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionItemModel.cs @@ -6,6 +6,8 @@ namespace Fallout.Persistence.Solution.Model; /// /// Represents an item in the solution model, either a project or a solution folder. /// +// [PublicAPI]: surfaces through Fallout.Solution's public API (SolutionItem ctor) — IDE authoring hint, see docs/adr/0011-reintroduce-publicapi-ide-hint.md +[JetBrains.Annotations.PublicAPI] public abstract class SolutionItemModel : PropertyContainerModel { private Guid? id; diff --git a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionModel.cs b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionModel.cs index 5f8855229..6918cd538 100644 --- a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionModel.cs +++ b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionModel.cs @@ -11,6 +11,8 @@ namespace Fallout.Persistence.Solution.Model; /// This contains a list of projects and folders and the information /// required to build the solution in different configurations. /// +// [PublicAPI]: surfaces through Fallout.Solution's public API (Solution ctor + GetModel) — IDE authoring hint, see docs/adr/0011-reintroduce-publicapi-ide-hint.md +[JetBrains.Annotations.PublicAPI] public sealed class SolutionModel : PropertyContainerModel { #if NETFRAMEWORK || NETSTANDARD diff --git a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionProjectModel.cs b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionProjectModel.cs index 08f615dd5..d9d577a7e 100644 --- a/src/Persistence/Fallout.Persistence.Solution/Model/SolutionProjectModel.cs +++ b/src/Persistence/Fallout.Persistence.Solution/Model/SolutionProjectModel.cs @@ -8,6 +8,8 @@ namespace Fallout.Persistence.Solution.Model; /// /// Represents a project in the solution model. /// +// [PublicAPI]: surfaces through Fallout.Solution's public API (Project ctor + GetModel) — IDE authoring hint, see docs/adr/0011-reintroduce-publicapi-ide-hint.md +[JetBrains.Annotations.PublicAPI] public sealed class SolutionProjectModel : SolutionItemModel { private Guid typeId;