feat(scribe): sdk rework and first draft collection and extraction pi…#12
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “Shapes” DSL and cache-safe data flow for incremental generators/analyzers (plus early code-fix support via a separate Ink package), while also reworking the Scribe MSBuild SDK into phased imports and updating LocalDev to a per-producer sentinel model.
Changes:
- Add
Scribe.ShapesAPIs (Shape/ShapeBuilder/Relation) plus cache-safe primitives (EquatableArray,LocationInfo,DiagnosticInfo, interning, etc.). - Add
Scribe.Ink(code-fix provider dispatch + cache-correctness analyzer) andScribe.Scriptorium(meta-generator for Should*/Could*/ShouldNot* variants). - Rework
BulletsForHumanity.Scribe.Sdkinto phase-based targets/props and overhaul LocalDev sentinels + cleanup; remove SolutionAnalyzer infrastructure/docs.
Reviewed changes
Copilot reviewed 71 out of 72 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Scribe/Shapes/TypeKindFilter.cs | Adds coarse syntax-stage type kind filtering enum. |
| Scribe/Shapes/SquiggleLocator.cs | Resolves diagnostic anchors (SquiggleAt) to concrete Roslyn Locations. |
| Scribe/Shapes/SquiggleAt.cs | Defines public squiggle anchor enum for diagnostics. |
| Scribe/Shapes/ShapeProjectionContext.cs | Adds stack-only projection context for user model projection. |
| Scribe/Shapes/ShapedSymbol.cs | Introduces cache-safe per-symbol projection container. |
| Scribe/Shapes/ShapedPair.cs | Adds cache-safe joined pair type for Relation joins. |
| Scribe/Shapes/ShapeCheck.cs | Internal representation of declarative checks + fix metadata. |
| Scribe/Shapes/Shape.cs | Public entry-point factories for the Shape DSL. |
| Scribe/Shapes/Shape_T.ToAnalyzer.cs | Projects a Shape into a DiagnosticAnalyzer. |
| Scribe/Shapes/Shape_T.cs | Projects a Shape into an incremental provider (ToProvider). |
| Scribe/Shapes/SeverityVariants.cs | Helper to apply variant severities only when not overridden. |
| Scribe/Shapes/Relation.cs | Provides join/pairing + orphan diagnostics across shape streams. |
| Scribe/Shapes/FixSpec.cs | Declares fix hint types (FixSpec, FixKind). |
| Scribe/Shapes/DiagnosticSpec.cs | Adds per-check diagnostic override struct. |
| Scribe/Scribe.csproj | Adds InternalsVisibleTo for Ink; references Scriptorium as an analyzer. |
| Scribe/Cache/WellKnownTypes.cs | Adds per-compilation symbol lookup cache. |
| Scribe/Cache/LocationInfo.cs | Adds cache-safe Location stand-in. |
| Scribe/Cache/InternPool.cs | Adds process-wide string interning pool. |
| Scribe/Cache/EquatableArray.cs | Adds structural-equality array wrapper for incremental caching. |
| Scribe/Cache/DiagnosticInfo.cs | Adds cache-safe diagnostic record + materialization helper. |
| Scribe/build/Scribe.SolutionAnalyzer.targets | Removes solution-local analyzer MSBuild target file. |
| Scribe/build/Scribe.SolutionAnalyzer.props | Removes solution-local analyzer MSBuild props file. |
| Scribe/build/Scribe.LocalDev.targets | Updates override emission/cleanup behavior for new LocalDev model. |
| Scribe/build/Scribe.LocalDev.props | Updates sentinel detection + producer/consumer activation model. |
| Scribe/Attributes/AttributeSchema.cs | Adds typed, cache-safe attribute discovery + projection helpers. |
| Scribe/Attributes/AttributeReader.cs | Adds stack-only typed reader over AttributeData + diagnostics. |
| Scribe.Tests/Shapes/ShapeToProviderTests.cs | Tests Shape ToProvider behavior and projection plumbing. |
| Scribe.Tests/Shapes/ShapeSeverityVariantsTests.cs | Tests generated Should*/Could* severity variants via analyzer. |
| Scribe.Tests/Shapes/ShapeFixProviderTests.cs | Tests Ink code-fix provider dispatch and transformations. |
| Scribe.Tests/Shapes/ShapeBuilderTests.cs | Unit tests for ShapeBuilder primitives/predicates. |
| Scribe.Tests/Shapes/ShapeAnalyzerTests.cs | Tests analyzer projection (ToAnalyzer) output and properties. |
| Scribe.Tests/Shapes/RelationPairTests.cs | Tests Relation pairing + orphan diagnostics end-to-end. |
| Scribe.Tests/Shapes/RelationPairIncrementalityTests.cs | Tests incremental caching behavior using tracked steps. |
| Scribe.Tests/Shapes/CacheCorrectnessAnalyzerTests.cs | Tests Ink analyzer that forbids cache-hostile model members. |
| Scribe.Tests/Scribe.Tests.csproj | Adds Ink reference + Roslyn workspace packages for fixer tests. |
| Scribe.Tests/Cache/WellKnownTypesTests.cs | Tests WellKnownTypes caching and resolution behavior. |
| Scribe.Tests/Cache/LocationInfoTests.cs | Tests LocationInfo capture/materialization semantics. |
| Scribe.Tests/Cache/InternPoolTests.cs | Tests string interning pool behavior. |
| Scribe.Tests/Cache/EquatableArrayTests.cs | Tests value equality semantics for EquatableArray. |
| Scribe.Tests/Cache/DiagnosticInfoTests.cs | Tests materialization and equality for DiagnosticInfo. |
| Scribe.Tests/Attributes/AttributeSchemaTests.cs | Tests AttributeSchema/AttributeReader typed reads + errors. |
| Scribe.slnx | Adds Scribe.Ink + Scribe.Scriptorium projects to the solution. |
| Scribe.Sdk/Sdk/Sdk.targets | Replaces monolithic targets with phased imports + LocalDev targets. |
| Scribe.Sdk/Sdk/Sdk.props | Replaces monolithic props with Defaults phase + LocalDev props import. |
| Scribe.Sdk/Sdk/Phases/Validation.targets | Enforces required/valid ScribeSdkProjectType. |
| Scribe.Sdk/Sdk/Phases/Stubs.targets | Centralizes Stubs.cs injection into its own phase. |
| Scribe.Sdk/Sdk/Phases/Meta.targets | Adds in-solution Meta project packaging + override emission. |
| Scribe.Sdk/Sdk/Phases/Library.targets | Adds Library packaging + companion embedding logic. |
| Scribe.Sdk/Sdk/Phases/Defaults.props | Adds universal, overridable defaults for all project types. |
| Scribe.Sdk/Sdk/Phases/Companion.targets | Adds Companion packaging/harvesting targets (GetAnalyzerFiles, deps). |
| Scribe.Sdk/Sdk/Phases/Classification.targets | Classifies ScribeSdkProjectType into boolean flags. |
| Scribe.Sdk/Sdk/Phases/Analyzer.targets | Adds standalone analyzer packaging logic. |
| Scribe.Sdk/Scribe.Sdk.csproj | Updates package contents/description to match phased SDK; removes SolutionAnalyzer packing. |
| Scribe.Scriptorium/ShapeBuilderVariantsGenerator.cs | Adds meta-generator for severity variants on ShapeBuilder primitives. |
| Scribe.Scriptorium/Scribe.Scriptorium.csproj | Adds build-time-only generator project. |
| Scribe.Ink/Shapes/ShapeInkExtensions.cs | Adds ToFixProvider entry point for shapes (Ink side). |
| Scribe.Ink/Shapes/ShapeCodeFixProvider.cs | Implements fix dispatch based on diagnostic properties. |
| Scribe.Ink/Shapes/IShapeFix.cs | Defines code-fix contract per FixKind. |
| Scribe.Ink/Shapes/Fixes/AddSealedModifierFix.cs | Implements “add sealed modifier” fix. |
| Scribe.Ink/Shapes/Fixes/AddPartialModifierFix.cs | Implements “add partial modifier” fix. |
| Scribe.Ink/Shapes/Fixes/AddInterfaceToBaseListFix.cs | Implements “add interface” fix. |
| Scribe.Ink/Shapes/Fixes/AddAttributeFix.cs | Implements “add attribute” fix. |
| Scribe.Ink/Shapes/Fixes/AddAbstractModifierFix.cs | Implements “add abstract modifier” fix. |
| Scribe.Ink/Shapes/CacheCorrectnessAnalyzer.cs | Adds analyzer that flags cache-hostile projection model members. |
| Scribe.Ink/Scribe.Ink.csproj | Adds Ink analyzer/fixer package project. |
| docs/solution-local-analyzers.md | Removes Solution-Local Analyzers documentation. |
| docs/project-setup.md | Updates LocalDev docs to new per-producer sentinel model. |
| docs/architecture-infrastructure.md | Updates LocalDev infra documentation (but still references removed SolutionAnalyzer). |
| Directory.Build.targets | Updates local timestamp versioning to key off IsLocalProducer. |
| Directory.Build.props | Adjusts warnings suppression and LocalDev producer property usage. |
| .gitignore | Updates sentinel ignore patterns to *.user / *.local. |
| foreach (var attribute in symbol.GetAttributes()) | ||
| { | ||
| var fqn = attribute.AttributeClass?.ToDisplayString(); | ||
| if (fqn is null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // Generic attributes like [Foo<int>] produce a display string with type args; | ||
| // strip them for the match so callers can pass the bare name. | ||
| var fqnBase = StripTypeArgs(fqn); | ||
|
|
||
| if (string.Equals(fqn, attributeFqn, StringComparison.Ordinal) | ||
| || string.Equals(fqnBase, attributeFqn, StringComparison.Ordinal) | ||
| || string.Equals(Normalize(fqn), normalized, StringComparison.Ordinal) | ||
| || string.Equals(Normalize(fqnBase), normalized, StringComparison.Ordinal)) |
There was a problem hiding this comment.
Attribute matching is based on AttributeClass.ToDisplayString() with the default (minimally-qualified) format. This will fail to match a fully-qualified metadata name (e.g. My.Ns.FooAttribute) when the attribute is in scope via using, which breaks the MustHaveAttribute pipeline (selector succeeds via ForAttributeWithMetadataName, but AttributeSchema.For/Read may report Exists=false). Consider matching using a fully-qualified symbol display format (and stripping global:: / generic args) or comparing via metadata name instead of the default display string.
| // Enums are boxed as their underlying integral value. | ||
| if (value.Kind == TypedConstantKind.Enum && value.Value is not null) | ||
| { | ||
| try | ||
| { | ||
| return (T)value.Value; | ||
| } | ||
| catch (InvalidCastException) | ||
| { | ||
| // fall through to mismatch path | ||
| } | ||
| } |
There was a problem hiding this comment.
Enum typed-reads appear broken: Roslyn TypedConstantKind.Enum values are typically boxed as the underlying integral type, so (T)value.Value will throw for T being an enum and you’ll always hit the mismatch path. If you want Ctor<MyEnum>() to work, convert via Enum.ToObject(typeof(T), value.Value) (or equivalent) when typeof(T).IsEnum.
| private static bool HasAttribute(INamedTypeSymbol symbol, string metadataName) | ||
| { | ||
| foreach (var attribute in symbol.GetAttributes()) | ||
| { | ||
| var fqn = attribute.AttributeClass?.ToDisplayString(); | ||
| if (fqn is null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if (string.Equals(fqn, metadataName, StringComparison.Ordinal)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| var bracket = fqn.IndexOf('<'); | ||
| var bare = bracket > 0 ? fqn.Substring(0, bracket) : fqn; | ||
| if (string.Equals(bare, metadataName, StringComparison.Ordinal)) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
HasAttribute uses AttributeClass.ToDisplayString() without a fully-qualified format. If PrimaryAttributeName is a real metadata name (as required by ForAttributeWithMetadataName in the generator pipeline), this check can return false for namespaced attributes and cause the analyzer to silently skip reporting. Use a fully-qualified symbol display format (or metadata name comparison) consistent with the generator-side selector.
| /// <summary>Fully-qualified display name of the matched type.</summary> | ||
| public string Fqn => _symbol.ToDisplayString(); |
There was a problem hiding this comment.
Fqn is documented as fully-qualified, but INamedTypeSymbol.ToDisplayString() without an explicit SymbolDisplayFormat is typically minimally-qualified. This can cause collisions between same-named types in different namespaces and makes downstream diagnostics/joins ambiguous. Consider using a fully-qualified format (optionally stripping global::) to ensure Fqn is actually unique and matches the documentation.
| var location = LocationInfo.From(FirstLocation(symbol)); | ||
|
|
||
| return new ShapedSymbol<TModel>( | ||
| Fqn: InternPool.Intern(symbol.ToDisplayString()), | ||
| Model: model, | ||
| Location: location, |
There was a problem hiding this comment.
ShapedSymbol.Fqn is constructed from symbol.ToDisplayString() which is usually minimally-qualified. If Fqn is intended to be a stable, unique identifier (and the docs say “fully-qualified”), consider switching to a fully-qualified symbol display format to avoid collisions across namespaces (and keep it consistent with ShapeProjectionContext.Fqn).
| Producer responsibilities (when $(IsLocalProducer) is 'true'): | ||
| 1. Auto-pack via $(GeneratePackageOnBuild) set in Scribe.LocalDev.props. | ||
| 2. Emit a version override file $(ScribesName).Directory.Packages.targets | ||
| listing locally-built package IDs at the timestamped dev version. | ||
|
|
There was a problem hiding this comment.
The header comment says the override file is named $(ScribesName).Directory.Packages.targets, but the target actually writes $(PackageId).Directory.Packages.targets. Please update the comment to reflect the real behaviour (per-PackageId override files).
| <Delete | ||
| Files="$(_ScribeCleanupOverride)" | ||
| Condition="Exists('$(_ScribeCleanupOverride)')" | ||
| /> | ||
| <Message | ||
| Text="[Scribe] Cleanup → removed stale override $(_ScribeCleanupOverride)" | ||
| Importance="high" | ||
| Condition="Exists('$(_ScribeCleanupOverride)')" | ||
| /> |
There was a problem hiding this comment.
The cleanup message is conditioned on Exists('$(_ScribeCleanupOverride)'), but it runs after the <Delete> task, so the condition will usually be false and the message won’t log. If you want a log when deletion happened, capture the pre-delete existence in a property/item and condition the message on that instead.
| In the producer's `Directory.Build.props`, set the producer name and package IDs *before* importing Scribe's LocalDev files: | ||
|
|
||
| ```xml | ||
| <PropertyGroup> | ||
| <ScribeRoot>$(MSBuildThisFileDirectory)..</ScribeRoot> | ||
| <ScribeLocalDevTriggerProject>MyFramework</ScribeLocalDevTriggerProject> | ||
| <ScribesName>MyFramework</ScribesName> | ||
| <ScribeLocalDevPackageNames>MyFramework</ScribeLocalDevPackageNames> | ||
| </PropertyGroup> | ||
| ``` | ||
|
|
||
| `$(ScribesName)` drives three things: | ||
|
|
||
| - **Sentinel detection:** matches `.$(ScribesName).user` OR `.$(ScribesName).local` (lowercased, e.g. `.myframework.user` or `.myframework.local`) at `$(ScribeRoot)`. Either form activates — pick whichever you prefer. | ||
| - **Auto-pack trigger:** the MSBuild project whose name equals `$(ScribesName)` is the one that packs. | ||
| - **Override file name:** generated as `$(ScribesName).Directory.Packages.targets`. | ||
|
|
||
| `$(ScribeName)` (non-possessive) is accepted as a typo-tolerant alias. |
There was a problem hiding this comment.
This section describes $(ScribesName) as controlling a single “auto-pack trigger” and the override file name, but the current LocalDev implementation enables pack-on-build for all packable projects in producer mode and writes overrides keyed by $(PackageId) (not $(ScribesName)). Also, $(ScribeLocalDevPackageNames) no longer appears to be used by the targets. Please update the docs snippet and bullets to match the current behaviour.
| | Concern | LocalDev | Solution-Local Analyzer | | ||
| | --------- | ---------- | ------------------------ | | ||
| | Scope | Cross-repo | Intra-solution | | ||
| | Trigger | `.localscribe` sentinel | `ScribeSolutionAnalyzer=true` | | ||
| | Trigger | `.<producer>.user` or `.<producer>.local` sentinel (e.g. `.scribe.user`, `.hermetic.local`) | `ScribeSolutionAnalyzer=true` | | ||
| | Version | NBGV + timestamp suffix | Timestamp-only (`0.0.0-dev.yyyyMMddHHmmss`) | | ||
| | Override files | Generated `.Directory.Packages.targets` | Same — generated `.Directory.Packages.targets` | |
There was a problem hiding this comment.
This doc still refers to ScribeSolutionAnalyzer=true and Scribe.SolutionAnalyzer.props/targets, but those files are removed in this PR and the SDK no longer imports/packs them. Either update this section to the new ScribeSdkProjectType=Meta workflow (or whatever replaced SolutionAnalyzer), or remove the Solution-Local Analyzer references to avoid broken guidance.
| /// <summary> | ||
| /// Drives <see cref="Relation.Pair{TLeft, TRight}"/> through | ||
| /// <see cref="CSharpGeneratorDriver"/> and validates three flows: | ||
| /// matched pairs stream, orphan-left diagnostic, orphan-right diagnostic. | ||
| /// Grounding scenario is the Hermetic-style Command ↔ Event join: each command | ||
| /// declares the event metadata name it raises, each event declares its own name; | ||
| /// the pair joins on that shared key. | ||
| /// </summary> |
There was a problem hiding this comment.
The example/test description uses a repo/product-specific reference (“Hermetic-style”). If this is intended as library documentation/examples, consider rephrasing to a generic scenario so the tests/docs stay product-agnostic.
…peline