From 4c1036a9a4a9389b1327d396fb4e303a7b57de56 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:40:24 +0200 Subject: [PATCH 01/21] Import events a referenced package declares instead of leaving them dangling A reactor observing what a sibling bounded context publishes refers to an event a package declares. The document had nothing to say about it beyond SP0021, and then referred to a name it never introduced - which the official Screenplay compiler reports as an unknown event. Screenplay already has the construct for this: the compiler reads the last segment of an 'import' as the name of an event that is known. Every undeclared name is now looked for in the assemblies the compilation references, and the one that is found is imported rather than reported. SP0021 is left for what is really unresolvable - a name nothing at all declares an event under, where importing it would state that a package declares something it does not. Co-Authored-By: Claude Opus 5 --- Source/DotNET/Screenplay.Specs/Analyzed.cs | 73 +++++++++- .../an_event_a_referenced_package_declares.cs | 53 +++++++ .../an_event_nothing_declares.cs | 52 +++++++ .../an_application_importing_an_event.cs | 55 +++++++ .../Analysis/ApplicationModelAnalyzer.cs | 57 +------- .../Analysis/Events/ExternalEvents.cs | 135 ++++++++++++++++++ .../Emission/ApplicationSyntaxBuilder.cs | 28 +++- .../Screenplay/Model/ApplicationModel.cs | 9 ++ .../Screenplay/ScreenplayDiagnosticCodes.cs | 8 +- 9 files changed, 413 insertions(+), 57 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_a_referenced_package_declares.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_nothing_declares.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/an_application_importing_an_event.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Events/ExternalEvents.cs diff --git a/Source/DotNET/Screenplay.Specs/Analyzed.cs b/Source/DotNET/Screenplay.Specs/Analyzed.cs index 60ca2afa2..f60b08383 100644 --- a/Source/DotNET/Screenplay.Specs/Analyzed.cs +++ b/Source/DotNET/Screenplay.Specs/Analyzed.cs @@ -54,6 +54,46 @@ public static class Analyzed public static ApplicationModelAnalysis Source(params (string Path, string Text)[] sources) => Source(DeclaredUserInterfaceFiles.None, sources); + /// + /// Compiles source referencing a package and recovers the model it describes. + /// + /// The package the source references. + /// The source files, keyed by the path each one is compiled as. + /// The . + public static ApplicationModelAnalysis SourceReferencing(MetadataReference package, params (string Path, string Text)[] sources) => + new ApplicationModelAnalyzer(DeclaredUserInterfaceFiles.None) + .Analyze(Compile([package], sources), new ScreenplayOptions().WithDefaults(AssemblyName)); + + /// + /// Compiles source into the assembly image a referenced package really is. + /// + /// The name of the assembly. + /// The source. + /// The . + /// Thrown when the source of the package does not compile. + /// + /// The image is emitted rather than referenced as a compilation, because a package the application depends on is + /// metadata with no syntax tree behind it - which is the whole reason nothing in the compilation declares what it + /// holds. + /// + public static MetadataReference Package(string name, string text) + { + var compilation = CSharpCompilation.Create( + name, + [CSharpSyntaxTree.ParseText(text, new CSharpParseOptions(documentationMode: DocumentationMode.Parse))], + _references, + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary, nullableContextOptions: NullableContextOptions.Enable)); + + using var stream = new MemoryStream(); + var emitted = compilation.Emit(stream); + if (!emitted.Success) + { + throw new InvalidOperationException($"The package '{name}' did not compile - {emitted.Diagnostics.First(_ => _.Severity == DiagnosticSeverity.Error)}"); + } + + return MetadataReference.CreateFromImage(stream.ToArray()); + } + /// /// Compiles source and recovers the model it describes. /// @@ -87,14 +127,22 @@ public static ApplicationModelAnalysis Source(IUserInterfaceFiles files, params /// /// The source files, keyed by the path each one is compiled as. /// The . - public static Compilation Compile(params (string Path, string Text)[] sources) => + public static Compilation Compile(params (string Path, string Text)[] sources) => Compile([], sources); + + /// + /// Compiles source referencing further packages into a compilation. + /// + /// The packages the source references beyond the platform. + /// The source files, keyed by the path each one is compiled as. + /// The . + public static Compilation Compile(IEnumerable packages, params (string Path, string Text)[] sources) => CSharpCompilation.Create( AssemblyName, sources.Append(Root).Select(_ => CSharpSyntaxTree.ParseText( _.Text, new CSharpParseOptions(documentationMode: DocumentationMode.Parse), path: _.Path)), - _references, + _references.Concat(packages), new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary, nullableContextOptions: NullableContextOptions.Enable)); /// @@ -102,8 +150,25 @@ public static Compilation Compile(params (string Path, string Text)[] sources) = /// /// The source files, keyed by the path each one is compiled as. /// The errors, empty when the source compiles. - public static IEnumerable ErrorsIn(params (string Path, string Text)[] sources) => - Compile(sources) + public static IEnumerable ErrorsIn(params (string Path, string Text)[] sources) => ErrorsIn([], sources); + + /// + /// Gets everything the compiler itself reported for source referencing a package. + /// + /// The package the source references. + /// The source files, keyed by the path each one is compiled as. + /// The errors, empty when the source compiles. + public static IEnumerable ErrorsIn(MetadataReference package, params (string Path, string Text)[] sources) => + ErrorsIn([package], sources); + + /// + /// Gets everything the compiler itself reported for source referencing further packages. + /// + /// The packages the source references beyond the platform. + /// The source files, keyed by the path each one is compiled as. + /// The errors, empty when the source compiles. + public static IEnumerable ErrorsIn(IEnumerable packages, params (string Path, string Text)[] sources) => + Compile(packages, sources) .GetDiagnostics() .Where(_ => _.Severity == DiagnosticSeverity.Error) .Select(_ => _.ToString()); diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_a_referenced_package_declares.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_a_referenced_package_declares.cs new file mode 100644 index 000000000..7d863611b --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_a_referenced_package_declares.cs @@ -0,0 +1,53 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A reactor observing what a sibling bounded context publishes refers to an event a package declares. The event is +/// real, so a document saying nothing about it refers to a name it never introduces - and Screenplay has the +/// construct for exactly this, which is what an import is for. +/// +public class an_event_a_referenced_package_declares : Specification +{ + const string Contracts = """ + using Cratis.Chronicle.Events; + + namespace Partners.Contracts; + + [EventType] + public record InvitationToJoinAdaAccepted(string Email); + """; + + const string Slice = """ + using System.Threading.Tasks; + using Cratis.Chronicle.Events; + using Cratis.Chronicle.Reactors; + using Partners.Contracts; + + namespace Library.Admin.Invitations; + + public class AcceptedInvitationProvisioner : IReactor + { + public Task Provision(InvitationToJoinAdaAccepted @event, EventContext context) => Task.CompletedTask; + } + """; + + static readonly (string Path, string Text)[] _sources = [("Library/Admin/Invitations/Provision.cs", Slice)]; + + MetadataReference _package; + ApplicationModelAnalysis _analysis; + + void Establish() => _package = Analyzed.Package("Partners.Contracts", Contracts); + + void Because() => _analysis = Analyzed.SourceReferencing(_package, _sources); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(_package, _sources).ShouldBeEmpty(); + [Fact] void should_import_the_event_by_its_qualified_name() => _analysis.Model.Imports.ShouldContainOnly(["Partners.Contracts.InvitationToJoinAdaAccepted"]); + [Fact] void should_still_observe_it_from_the_reactor() => _analysis.Slice().Reactors.Single().ObservedEvents.ShouldContainOnly(["InvitationToJoinAdaAccepted"]); + [Fact] void should_not_report_it_as_undeclared() => _analysis.Diagnostics.Any(_ => _.Code == ScreenplayDiagnosticCodes.EventDeclaredOutsideCompilation).ShouldBeFalse(); + [Fact] void should_report_nothing_at_all() => _analysis.Diagnostics.ShouldBeEmpty(); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_nothing_declares.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_nothing_declares.cs new file mode 100644 index 000000000..2a0dbb732 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_nothing_declares.cs @@ -0,0 +1,52 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A constraint can name any type at all, and a type nothing declares as an event is a name no import can introduce - +/// importing it would state that a package declares an event it does not. This is what is left for the report to say. +/// +public class an_event_nothing_declares : Specification +{ + const string Contracts = """ + namespace Partners.Contracts; + + public record CustomerRegistered(string Name); + """; + + const string Slice = """ + using Cratis.Chronicle.Events.Constraints; + using Partners.Contracts; + + namespace Library.Customers.Registration; + + public class UniqueCustomerConstraint : IConstraint + { + public void Define(IConstraintBuilder builder) => builder.Unique(); + } + """; + + static readonly (string Path, string Text)[] _sources = [("Library/Customers/Registration/Registration.cs", Slice)]; + + MetadataReference _package; + ApplicationModelAnalysis _analysis; + ScreenplayDiagnostic _reported; + + void Establish() => _package = Analyzed.Package("Partners.Contracts", Contracts); + + void Because() + { + _analysis = Analyzed.SourceReferencing(_package, _sources); + _reported = _analysis.Diagnostics.Single(_ => _.Code == ScreenplayDiagnosticCodes.EventDeclaredOutsideCompilation); + } + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(_package, _sources).ShouldBeEmpty(); + [Fact] void should_import_nothing() => _analysis.Model.Imports.ShouldBeEmpty(); + [Fact] void should_name_the_event_in_the_report() => _reported.Message.Contains("CustomerRegistered", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_locate_the_report_at_the_slice() => _reported.Location.ShouldEqual("Library.Customers.Registration"); + [Fact] void should_report_it_as_a_loss() => _reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Warning); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/an_application_importing_an_event.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/an_application_importing_an_event.cs new file mode 100644 index 000000000..c8ca10fd3 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/an_application_importing_an_event.cs @@ -0,0 +1,55 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Emission; +using Cratis.Arc.Screenplay.Library; +using Cratis.Arc.Screenplay.Model; +using Cratis.Arc.Screenplay.Verification; + +namespace Cratis.Arc.Screenplay.for_ScreenplayEmitter.when_emitting; + +/// +/// The Screenplay compiler reads the last segment of an import as the name of an event that is known, which is +/// exactly what a reactor observing an event of another bounded context needs. Compiling the document back is what +/// proves it - without the import the language reports the trigger as an event nothing declares. +/// +public class an_application_importing_an_event : given.an_emitter +{ + ApplicationModel _model; + ScreenplayEmission _emission; + RoundTripResult _roundTrip; + + void Establish() => + _model = LibraryApplication.Build() with + { + Imports = ["Partners.Contracts.InvitationToJoinAdaAccepted"], + Slices = + [ + .. LibraryApplication.Slices(), + SliceModel.Empty("Library.Admin.Invitations", "Invitations", SliceKind.Automation) with + { + Reactors = + [ + new ReactorModel( + "AcceptedInvitationProvisioner", + ["InvitationToJoinAdaAccepted"], + false, + "Admin/Invitations/Provision.cs") + ] + } + ] + }; + + void Because() + { + _emission = _emitter.Emit(_model, _options); + _roundTrip = RoundTrip.For(_emission.Application); + } + + [Fact] void should_declare_the_import() => _emission.Source.Contains("import Partners.Contracts.InvitationToJoinAdaAccepted", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_still_observe_it_from_the_reactor() => _emission.Source.Contains("on InvitationToJoinAdaAccepted", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_compile_without_errors() => _roundTrip.Errors.ShouldBeEmpty(); + [Fact] void should_leave_the_language_nothing_to_warn_about() => _roundTrip.Diagnostics.ShouldBeEmpty(); + [Fact] void should_print_the_same_text_on_a_second_pass() => _roundTrip.Reprinted.ShouldEqual(_roundTrip.Printed); + [Fact] void should_report_nothing_as_unmappable() => _emission.Diagnostics.ShouldBeEmpty(); +} diff --git a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs index b3bdf0f82..d33d66f6a 100644 --- a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs +++ b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs @@ -1,6 +1,7 @@ // Copyright (c) Cratis. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using Cratis.Arc.Screenplay.Analysis.Events; using Cratis.Arc.Screenplay.Analysis.Policies; using Cratis.Arc.Screenplay.Analysis.Screens; using Cratis.Arc.Screenplay.Analysis.Slices; @@ -56,7 +57,7 @@ public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptio ConceptValidations.Link(catalog, readers); readers.AggregateRoots.Report(diagnostics); ReportTypesTheDocumentCannotName(readers, diagnostics, compilation.AssemblyName); - ReportEventsFromOutside(slices, diagnostics); + var imports = ExternalEvents.Resolve(compilation, slices, diagnostics); ReportNamespacesWithoutStructure(slices, diagnostics, options.SegmentsToSkip ?? 0); if (slices.Count == 0 && !failedToCompile) @@ -73,7 +74,10 @@ public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptio options.Module ?? options.Domain ?? ScreenplayOptions.DefaultName, readers.Types.Concepts, new PolicyCatalog(compilation, diagnostics).Declare(slices.SelectMany(AuthorizationsIn)), - slices), + slices) + { + Imports = imports + }, diagnostics.All); } @@ -144,31 +148,6 @@ static void ReportTypesTheDocumentCannotName(ArtifactReaders readers, Screenplay } } - /// - /// Reports every event the application refers to but does not declare. - /// - /// The slices to check. - /// The diagnostics to report to. - /// - /// An event living in a referenced package is real, but nothing in the compilation declares it, so the document - /// would refer to something it never introduces. Saying so is better than inventing a declaration for it. - /// - static void ReportEventsFromOutside(IReadOnlyList slices, ScreenplayDiagnostics diagnostics) - { - var declared = slices.SelectMany(_ => _.Events).Select(_ => _.Name).ToHashSet(StringComparer.Ordinal); - - foreach (var slice in slices) - { - foreach (var name in ReferencedEvents(slice).Where(_ => !declared.Contains(_)).Order(StringComparer.Ordinal)) - { - diagnostics.Warning( - ScreenplayDiagnosticCodes.EventDeclaredOutsideCompilation, - $"'{name}' is referred to but declared outside the compilation, so the document refers to an event it never introduces", - slice.Namespace); - } - } - } - /// /// Reports a namespace that carries nothing to arrange the document by. /// @@ -203,28 +182,4 @@ static void ReportNamespacesWithoutStructure( /// The number of segments. static int Segments(string @namespace, int segmentsToSkip) => @namespace.Split('.', StringSplitOptions.RemoveEmptyEntries).Length - segmentsToSkip; - - /// - /// Gets the names of every event a slice refers to. - /// - /// The slice to read. - /// The names, distinct. - static IEnumerable ReferencedEvents(SliceModel slice) => - slice.Commands.SelectMany(_ => _.Produces).Select(_ => _.EventName) - .Concat(slice.Reactors.SelectMany(_ => _.ObservedEvents)) - .Concat(slice.Constraints.SelectMany(EventsOf)) - .Concat(ProjectionEvents.In(slice.Projection)) - .Distinct(StringComparer.Ordinal); - - /// - /// Gets the names of the events a constraint refers to. - /// - /// The constraint to read. - /// The names. - static IEnumerable EventsOf(ConstraintModel constraint) => constraint switch - { - UniquePropertyConstraintModel unique => [unique.EventName], - UniqueEventConstraintModel unique => [unique.EventName], - _ => [] - }; } diff --git a/Source/DotNET/Screenplay/Analysis/Events/ExternalEvents.cs b/Source/DotNET/Screenplay/Analysis/Events/ExternalEvents.cs new file mode 100644 index 000000000..5618d27ca --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Events/ExternalEvents.cs @@ -0,0 +1,135 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Model; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.Analysis.Events; + +/// +/// Resolves the events an application refers to but does not declare itself. +/// +/// +/// A reactor observing what a sibling bounded context publishes refers to an event living in a referenced package. +/// The event is real and the document has to name it, but nothing in the compilation declares it, so a document +/// stating only the reference refers to something it never introduces. Screenplay has the construct for exactly this +/// situation - an import states the dependency outright and registers the name as an event that is known - so +/// the one that can be found is imported and only the one that cannot is reported. +/// +public static class ExternalEvents +{ + /// + /// Resolves the events the application refers to without declaring them, reporting every one nothing declares. + /// + /// The compilation being analyzed. + /// The slices to read. + /// The diagnostics to report to. + /// The fully qualified name of every event to import, ordered. + public static IReadOnlyList Resolve( + Compilation compilation, + IReadOnlyList slices, + ScreenplayDiagnostics diagnostics) + { + var declared = slices.SelectMany(_ => _.Events).Select(_ => _.Name).ToHashSet(StringComparer.Ordinal); + var undeclared = slices.SelectMany(ReferredToBy).Where(_ => !declared.Contains(_)).ToHashSet(StringComparer.Ordinal); + var imported = DeclaredByAReference(compilation, undeclared); + + foreach (var slice in slices) + { + foreach (var name in ReferredToBy(slice) + .Where(_ => !declared.Contains(_) && !imported.ContainsKey(_)) + .Order(StringComparer.Ordinal)) + { + diagnostics.Warning( + ScreenplayDiagnosticCodes.EventDeclaredOutsideCompilation, + $"'{name}' is referred to but nothing declares it, so the document refers to an event it never introduces", + slice.Namespace); + } + } + + return [.. imported.Values.Order(StringComparer.Ordinal)]; + } + + /// + /// Gets the names of every event a slice refers to. + /// + /// The slice to read. + /// The names, distinct. + public static IEnumerable ReferredToBy(SliceModel slice) => + slice.Commands.SelectMany(_ => _.Produces).Select(_ => _.EventName) + .Concat(slice.Reactors.SelectMany(_ => _.ObservedEvents)) + .Concat(slice.Constraints.SelectMany(EventsOf)) + .Concat(ProjectionEvents.In(slice.Projection)) + .Distinct(StringComparer.Ordinal); + + /// + /// Gets the names of the events a constraint refers to. + /// + /// The constraint to read. + /// The names. + static IEnumerable EventsOf(ConstraintModel constraint) => constraint switch + { + UniquePropertyConstraintModel unique => [unique.EventName], + UniqueEventConstraintModel unique => [unique.EventName], + _ => [] + }; + + /// + /// Finds the event a referenced assembly declares under each of a set of names. + /// + /// The compilation being analyzed. + /// The names to look for. + /// The fully qualified name each one was found under, keyed by the name it is referred to by. + /// + /// Every assembly says which type names it holds without any of them being read, so the search only ever opens + /// the few that could answer. Assemblies and namespaces are both walked in name order and the first declaration + /// of a name wins, because two packages declaring an event under one name is a document that would otherwise + /// depend on the order the compiler happened to hand its references over. + /// + static Dictionary DeclaredByAReference(Compilation compilation, HashSet names) + { + var found = new Dictionary(StringComparer.Ordinal); + if (names.Count == 0) + { + return found; + } + + foreach (var assembly in compilation.SourceModule.ReferencedAssemblySymbols + .OrderBy(_ => _.Identity.GetDisplayName(), StringComparer.Ordinal)) + { + if (names.Any(assembly.TypeNames.Contains)) + { + Collect(assembly.GlobalNamespace, names, found); + } + } + + return found; + } + + /// + /// Collects every event declared under one of a set of names within a namespace and those nested in it. + /// + /// The namespace to walk. + /// The names to look for. + /// The declarations found so far. + static void Collect(INamespaceSymbol @namespace, HashSet names, Dictionary found) + { + foreach (var name in names.Where(_ => !found.ContainsKey(_)).Order(StringComparer.Ordinal)) + { + var declaration = @namespace.GetTypeMembers(name) + .Where(EventReader.IsEvent) + .OrderBy(_ => _.ToDisplayString(), StringComparer.Ordinal) + .FirstOrDefault(); + + if (declaration is not null) + { + found[name] = declaration.ToDisplayString(); + } + } + + foreach (var nested in @namespace.GetNamespaceMembers().OrderBy(_ => _.Name, StringComparer.Ordinal)) + { + Collect(nested, names, found); + } + } +} diff --git a/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs b/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs index a85102ec3..320f6a52c 100644 --- a/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs +++ b/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs @@ -54,7 +54,7 @@ public ApplicationSyntax Build(ApplicationModel model, ScreenplayOptions options var policies = new PolicySyntaxBuilder(naming).Build(model.Policies, _authorize.Referenced); return new( - [], + [.. BuildImports(model)], [.. concepts], [.. policies], [.. modules], @@ -62,6 +62,32 @@ public ApplicationSyntax Build(ApplicationModel model, ScreenplayOptions options new DomainSyntax(domain, SourceLocation.Start)); } + /// + /// Builds the imports naming every event the application refers to without declaring it. + /// + /// The model to build from. + /// The imports, ordered. + /// + /// The Screenplay compiler reads the last segment of an import as the name of an event that is known, so the + /// segment naming the event is written exactly as every reference to it is written - through the same conversion + /// - or the document would import one name and refer to another. + /// + IEnumerable BuildImports(ApplicationModel model) => + model.Imports + .Select(ToQualifiedName) + .Where(_ => _.Length > 0) + .Distinct(StringComparer.Ordinal) + .Order(StringComparer.Ordinal) + .Select(_ => new ImportSyntax(_, SourceLocation.Start)); + + /// + /// Sanitizes every segment of a dotted name. + /// + /// The name to sanitize. + /// The sanitized name. + string ToQualifiedName(string name) => + string.Join('.', name.Split('.', StringSplitOptions.RemoveEmptyEntries).Select(naming.ToDeclarationName)); + /// /// Builds the modules holding every slice that declares something. /// diff --git a/Source/DotNET/Screenplay/Model/ApplicationModel.cs b/Source/DotNET/Screenplay/Model/ApplicationModel.cs index 7b7f4d8f9..146ad63a2 100644 --- a/Source/DotNET/Screenplay/Model/ApplicationModel.cs +++ b/Source/DotNET/Screenplay/Model/ApplicationModel.cs @@ -22,4 +22,13 @@ public record ApplicationModel( /// Represents an application that declares nothing at all. /// public static readonly ApplicationModel Empty = new(string.Empty, string.Empty, [], [], []); + + /// + /// Gets the fully qualified name of every event the application refers to that something it references declares. + /// + /// + /// An event a sibling bounded context publishes is real, but nothing here declares it, so the document states the + /// dependency outright rather than referring to a name it never introduces. + /// + public IEnumerable Imports { get; init; } = []; } diff --git a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs index f7dc7eddc..aeeb42c0b 100644 --- a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs +++ b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs @@ -112,8 +112,14 @@ public static class ScreenplayDiagnosticCodes public const string ReducerWithoutCounterpart = "SP0020"; /// - /// An event referenced by the application is declared outside the compilation being analyzed. + /// An event the application refers to is declared neither by it nor by anything it references. /// + /// + /// An event a referenced package declares is real and can be stated - an import names it and the compiler + /// then reads it as an event that is known - so that case is written rather than reported. This is what is left: + /// a name nothing at all resolves to, where inventing a declaration would describe an event the application does + /// not have and staying silent would leave a document referring to something it never introduces. + /// public const string EventDeclaredOutsideCompilation = "SP0021"; /// From 2a23238f6d44863ad99d0b50d00caae6a55ab479 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:43:40 +0200 Subject: [PATCH 02/21] Leave source a build wrote out of where a slice is written A compilation loaded from a project carries what every source generator emitted to the intermediate folder. Those files declare real members of the slice, so the folders they sit in were counted among the folders the slice is written across - which reported a slice sitting in one folder as spread over two, claimed the folder was shared with the next slice to be handed the same generator output, and widened the screen scan to a build folder. Generated source now answers neither where a slice is written nor where the project is. Where a build wrote every last file the whole set is kept, since answering with nothing writes every path against the machine that generated it. Co-Authored-By: Claude Opus 5 --- ...slice_a_source_generator_contributed_to.cs | 64 ++++++++++++++++++ .../and_a_build_wrote_all_of_it.cs | 26 ++++++++ .../and_a_person_wrote_some_of_it.cs | 24 +++++++ .../when_recognizing_a_path.cs | 41 ++++++++++++ .../Screenplay/Analysis/GeneratedSource.cs | 66 +++++++++++++++++++ .../Analysis/Screens/SliceDirectories.cs | 10 ++- .../DotNET/Screenplay/Analysis/SourcePaths.cs | 7 +- 7 files changed, 235 insertions(+), 3 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_slice_a_source_generator_contributed_to.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_build_wrote_all_of_it.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_person_wrote_some_of_it.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_recognizing_a_path.cs create mode 100644 Source/DotNET/Screenplay/Analysis/GeneratedSource.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_slice_a_source_generator_contributed_to.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_slice_a_source_generator_contributed_to.cs new file mode 100644 index 000000000..637888ebd --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_slice_a_source_generator_contributed_to.cs @@ -0,0 +1,64 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A compilation loaded from a project carries what every source generator emitted to the intermediate folder of the +/// build. Those files declare real members of the slice and say nothing at all about where it is written, so counting +/// them reports a slice sitting in one folder as spread over two and looks for its screens in a build folder. +/// +public class a_slice_a_source_generator_contributed_to : Specification +{ + const string Written = """ + using System.Threading.Tasks; + using Cratis.Chronicle.Events; + using Cratis.Chronicle.Reactors; + + namespace Library.Onboarding.Registry; + + [EventType] + public record CompanyRegistered(string Name); + + public partial class RegistryNotifier : IReactor + { + public Task CompanyRegistered(CompanyRegistered @event, EventContext context) => Task.CompletedTask; + } + """; + + const string Emitted = """ + namespace Library.Onboarding.Registry; + + public partial class RegistryNotifier + { + public string Describe() => nameof(RegistryNotifier); + } + """; + + static readonly (string Path, string Text)[] _sources = + [ + ("Library/Onboarding/Registry/Registry.cs", Written), + ("Library/obj/Debug/net10.0/Microsoft.Gen.Logging/Microsoft.Gen.Logging.LoggingGenerator/Registry.Logging.g.cs", Emitted) + ]; + + static readonly DeclaredUserInterfaceFiles _files = new( + "Library/Onboarding/Registry/RegistryPage.tsx", + "Library/obj/Debug/net10.0/Microsoft.Gen.Logging/Microsoft.Gen.Logging.LoggingGenerator/Emitted.tsx"); + + ApplicationModelAnalysis _analysis; + IEnumerable _screens; + + void Establish() + { + _analysis = Analyzed.Source(_files, _sources); + _screens = _analysis.Slice().Screens; + } + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(_sources).ShouldBeEmpty(); + [Fact] void should_not_report_the_slice_as_spread_over_folders() => _analysis.Diagnostics.Any(_ => _.Code == ScreenplayDiagnosticCodes.AmbiguousScreenFile).ShouldBeFalse(); + [Fact] void should_take_its_screens_only_from_the_folder_it_is_written_in() => _screens.Select(_ => _.Name).ShouldContainOnly(["RegistryPage"]); + [Fact] void should_report_nothing_beyond_what_no_screen_states() => _analysis.Diagnostics.Select(_ => _.Code).Distinct().ShouldContainOnly([ScreenplayDiagnosticCodes.ScreenStructureNotInferred]); +} diff --git a/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_build_wrote_all_of_it.cs b/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_build_wrote_all_of_it.cs new file mode 100644 index 000000000..5b7598155 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_build_wrote_all_of_it.cs @@ -0,0 +1,26 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_GeneratedSource.when_excluding_generated_source; + +/// +/// Where a build wrote every last file, leaving nothing is not an improvement on leaving all of it - the question of +/// where the application sits still has to be answered, and answering it with nothing writes every path in the +/// document against the machine that generated it. +/// +public class and_a_build_wrote_all_of_it : Specification +{ + static readonly string[] _paths = + [ + "Library/obj/Debug/net10.0/Some.Generator/Registration.g.cs", + "Library/obj/Debug/net10.0/Some.Generator/Listing.g.cs" + ]; + + IReadOnlyList _result; + + void Because() => _result = GeneratedSource.Excluded(_paths); + + [Fact] void should_fall_back_to_all_of_it() => _result.ShouldContainOnly(_paths); +} diff --git a/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_person_wrote_some_of_it.cs b/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_person_wrote_some_of_it.cs new file mode 100644 index 000000000..9befe37ab --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_excluding_generated_source/and_a_person_wrote_some_of_it.cs @@ -0,0 +1,24 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_GeneratedSource.when_excluding_generated_source; + +/// +/// Where a person wrote some of the source, that is the whole of the answer to where the application is written. +/// +public class and_a_person_wrote_some_of_it : Specification +{ + IReadOnlyList _result; + + void Because() => _result = GeneratedSource.Excluded( + [ + "Library/Authors/Registration/Registration.cs", + "Library/obj/Debug/net10.0/Some.Generator/Registration.g.cs", + null, + " " + ]); + + [Fact] void should_keep_only_what_a_person_wrote() => _result.ShouldContainOnly(["Library/Authors/Registration/Registration.cs"]); +} diff --git a/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_recognizing_a_path.cs b/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_recognizing_a_path.cs new file mode 100644 index 000000000..7733907ba --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_GeneratedSource/when_recognizing_a_path.cs @@ -0,0 +1,41 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_GeneratedSource; + +/// +/// Source a build wrote is told from source a person wrote by where it sits and by what it is called, because those +/// are the only two things a path says. Both halves matter - a generator emits under the intermediate folder, and a +/// designer writes a companion file next to the source it belongs to. +/// +public class when_recognizing_a_path : Specification +{ + bool _underTheIntermediateFolder; + bool _underTheOutputFolder; + bool _windowsSeparated; + bool _namedAsGenerated; + bool _writtenByAPerson; + bool _namedAfterTheOutputFolder; + bool _nothingAtAll; + + void Because() + { + _underTheIntermediateFolder = GeneratedSource.Is("Library/obj/Debug/net10.0/Some.Generator/Slice.g.cs"); + _underTheOutputFolder = GeneratedSource.Is("Library/bin/Release/net10.0/Slice.cs"); + _windowsSeparated = GeneratedSource.Is(@"C:\Work\Library\obj\Debug\Slice.cs"); + _namedAsGenerated = GeneratedSource.Is("Library/Authors/Registration/Registration.generated.cs"); + _writtenByAPerson = GeneratedSource.Is("Library/Authors/Registration/Registration.cs"); + _namedAfterTheOutputFolder = GeneratedSource.Is("Library/Authors/obj.cs"); + _nothingAtAll = GeneratedSource.Is(null); + } + + [Fact] void should_recognize_source_under_the_intermediate_folder() => _underTheIntermediateFolder.ShouldBeTrue(); + [Fact] void should_recognize_source_under_the_output_folder() => _underTheOutputFolder.ShouldBeTrue(); + [Fact] void should_recognize_a_path_separated_the_way_windows_separates_one() => _windowsSeparated.ShouldBeTrue(); + [Fact] void should_recognize_a_file_named_as_generated() => _namedAsGenerated.ShouldBeTrue(); + [Fact] void should_leave_source_a_person_wrote_alone() => _writtenByAPerson.ShouldBeFalse(); + [Fact] void should_not_read_a_file_name_as_a_folder() => _namedAfterTheOutputFolder.ShouldBeFalse(); + [Fact] void should_answer_for_a_path_that_is_not_there() => _nothingAtAll.ShouldBeFalse(); +} diff --git a/Source/DotNET/Screenplay/Analysis/GeneratedSource.cs b/Source/DotNET/Screenplay/Analysis/GeneratedSource.cs new file mode 100644 index 000000000..7e51b047d --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/GeneratedSource.cs @@ -0,0 +1,66 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.Analysis; + +/// +/// Recognizes the source a build wrote rather than a person. +/// +/// +/// A compilation loaded from a project carries the output of every source generator that ran, emitted to disk under +/// the intermediate folder. Those files declare real members of real types, so they belong in the model - but they +/// say nothing whatsoever about where the application is written, and letting them answer that question puts a build +/// folder among the folders a slice is said to live in. What follows from that is a slice reported as spread over +/// two folders when it sits in one, and a screen scan widened to a tree no screen was ever written in. +/// +public static class GeneratedSource +{ + /// + /// The folders a build writes its output to. + /// + public static readonly string[] OutputFolders = ["obj", "bin"]; + + /// + /// The suffixes a generated file is conventionally named with. + /// + public static readonly string[] Suffixes = [".g.cs", ".g.i.cs", ".generated.cs"]; + + /// + /// Determines whether a path names source a build wrote. + /// + /// The path to check. + /// True when the path names generated source. + public static bool Is(string? path) + { + if (string.IsNullOrWhiteSpace(path)) + { + return false; + } + + var normalized = path.Replace('\\', '/'); + + return Array.Exists(Suffixes, _ => normalized.EndsWith(_, StringComparison.OrdinalIgnoreCase)) || + normalized + .Split('/', StringSplitOptions.RemoveEmptyEntries) + .SkipLast(1) + .Any(segment => Array.Exists(OutputFolders, _ => string.Equals(segment, _, StringComparison.OrdinalIgnoreCase))); + } + + /// + /// Removes the paths naming source a build wrote, keeping every one of them when none is left otherwise. + /// + /// The paths to filter. + /// The paths a person wrote. + /// + /// Falling back to the whole set is what keeps a compilation of nothing but generated source answering at all. + /// Where a person wrote none of it there is no better answer available, and an empty one would leave every path + /// in the document written against the machine that generated it. + /// + public static IReadOnlyList Excluded(IEnumerable paths) + { + var all = paths.Where(_ => !string.IsNullOrWhiteSpace(_)).Select(_ => _!).ToList(); + var written = all.FindAll(_ => !Is(_)); + + return written.Count > 0 ? written : all; + } +} diff --git a/Source/DotNET/Screenplay/Analysis/Screens/SliceDirectories.cs b/Source/DotNET/Screenplay/Analysis/Screens/SliceDirectories.cs index 2d999286e..1fce9e9e6 100644 --- a/Source/DotNET/Screenplay/Analysis/Screens/SliceDirectories.cs +++ b/Source/DotNET/Screenplay/Analysis/Screens/SliceDirectories.cs @@ -12,11 +12,17 @@ namespace Cratis.Arc.Screenplay.Analysis.Screens; /// This is what a syntax tree buys that metadata never could - a real path - and it is the whole reason a screen can /// be recovered at all. The vertical slice convention puts the file realizing a screen next to the source of the /// slice it belongs to, so where the source lives is where to look. +/// +/// What a source generator emitted is not where the source lives. A logging generator writing partial members into +/// the intermediate folder of the build contributes symbols to the slice and nothing at all to the question of where +/// its screens are, so a slice sitting in one folder would otherwise be reported as spread over two and its screens +/// looked for in a build folder. +/// /// public static class SliceDirectories { /// - /// Gets the directories a set of types is declared across. + /// Gets the directories a set of types is written across. /// /// The types to locate. /// The directories, distinct and ordered. @@ -25,7 +31,7 @@ public static IReadOnlyList Of(IEnumerable types) => .. types .SelectMany(_ => _.DeclaringSyntaxReferences) .Select(_ => _.SyntaxTree.FilePath) - .Where(_ => !string.IsNullOrWhiteSpace(_)) + .Where(_ => !string.IsNullOrWhiteSpace(_) && !GeneratedSource.Is(_)) .Select(ScreenFiles.DirectoryOf) .Where(_ => _.Length > 0) .Distinct(StringComparer.Ordinal) diff --git a/Source/DotNET/Screenplay/Analysis/SourcePaths.cs b/Source/DotNET/Screenplay/Analysis/SourcePaths.cs index 284e94565..774887918 100644 --- a/Source/DotNET/Screenplay/Analysis/SourcePaths.cs +++ b/Source/DotNET/Screenplay/Analysis/SourcePaths.cs @@ -34,10 +34,15 @@ public class SourcePaths(string root) /// belongs to the project too and widens the root; a file sharing nothing with them but the file system root /// belongs to somebody else and is left out of the question entirely. /// + /// + /// What a source generator emitted is left out of that vote. It is compiled from the intermediate folder of the + /// build rather than from anywhere the application is written, so counting it moves the answer towards a folder + /// nobody committed a line to. + /// /// public static SourcePaths For(Compilation compilation, ArtifactCatalog catalog) { - var declared = DirectoriesOf(catalog.Types.Select(_ => _.SourceFilePath())); + var declared = DirectoriesOf(GeneratedSource.Excluded(catalog.Types.Select(_ => _.SourceFilePath()))); var anchor = DeepestSharedBy(declared); var project = Rooted(declared, anchor); var root = Rooted(DirectoriesOf(compilation.SyntaxTrees.Select(_ => _.FilePath)), project); From fd22de878067883f8f8685f3ab8c04ddb419ef49 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:46:37 +0200 Subject: [PATCH 03/21] Read a separator in a name as the word boundary it is Kebab case is idiomatic for the runtime name of a Chronicle constraint and a Screenplay identifier cannot carry a hyphen, so a transformation is forced. Deleting the separators was the least readable one available - every one of Ada's thirty one constraints came out as a run-together lower case word, and the word boundaries the source stated were thrown away with them. Each character an identifier cannot hold now ends a word instead, and the words are joined in Pascal case: 'unique-timesheet-start' reads as 'UniqueTimesheetStart'. This is every identifier position rather than only a constraint. A name carrying no separator is left exactly as it is, so nothing the application already writes as one word changes. Co-Authored-By: Claude Opus 5 --- ...raint_named_the_way_chronicle_names_one.cs | 45 +++++++++++++ .../from_names_carrying_separators.cs | 45 +++++++++++++ .../from_names_carrying_separators.cs | 33 ++++++++++ .../Emission/Naming/ScreenplayNaming.cs | 65 +++++++++++++++++-- 4 files changed, 181 insertions(+), 7 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_constraint_named_the_way_chronicle_names_one.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_carrying_separators.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_property_name/from_names_carrying_separators.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_constraint_named_the_way_chronicle_names_one.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_constraint_named_the_way_chronicle_names_one.cs new file mode 100644 index 000000000..491b07653 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_constraint_named_the_way_chronicle_names_one.cs @@ -0,0 +1,45 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Emission; +using Cratis.Arc.Screenplay.Library; +using Cratis.Arc.Screenplay.Model; +using Cratis.Arc.Screenplay.Verification; + +namespace Cratis.Arc.Screenplay.for_ScreenplayEmitter.when_emitting; + +/// +/// Kebab case is idiomatic for the runtime name of a Chronicle constraint, and a Screenplay identifier cannot carry a +/// hyphen. Deleting the hyphens leaves one run-together word that reads as nothing, so the words either side of each +/// one are joined the way a reader would have written them. +/// +public class a_constraint_named_the_way_chronicle_names_one : given.an_emitter +{ + ApplicationModel _model; + ScreenplayEmission _emission; + RoundTripResult _roundTrip; + + void Establish() => + _model = LibraryApplication.Build() with + { + Slices = + [ + .. LibraryApplication.Slices(), + SliceModel.Empty("Library.Lending.Reserving.Rules", "Rules", SliceKind.StateChange) with + { + Constraints = [new UniqueEventConstraintModel("unique-book-reservation", "BookReserved")] + } + ] + }; + + void Because() + { + _emission = _emitter.Emit(_model, _options); + _roundTrip = RoundTrip.For(_emission.Application); + } + + [Fact] void should_name_the_constraint_by_the_words_the_source_stated() => _emission.Source.Contains("constraint UniqueBookReservation", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_not_run_the_words_together() => _emission.Source.Contains("uniquebookreservation", StringComparison.Ordinal).ShouldBeFalse(); + [Fact] void should_compile_without_errors() => _roundTrip.Errors.ShouldBeEmpty(); + [Fact] void should_print_the_same_text_on_a_second_pass() => _roundTrip.Reprinted.ShouldEqual(_roundTrip.Printed); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_carrying_separators.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_carrying_separators.cs new file mode 100644 index 000000000..84393c044 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_carrying_separators.cs @@ -0,0 +1,45 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.for_ScreenplayNaming.when_making_a_declaration_name; + +/// +/// A runtime name is routinely written with separators - kebab case is idiomatic for a Chronicle constraint - and the +/// Screenplay identifier rules force some transformation of it. Deleting the separators throws away the word +/// boundaries the source stated, so each one is read as the boundary it is instead. +/// +public class from_names_carrying_separators : given.a_naming +{ + string _kebabCased; + string _snakeCased; + string _spaced; + string _dotted; + string _mixed; + string _leadingSeparator; + string _withoutASeparator; + string _alreadyCamelCased; + string _nothingButSeparators; + + void Because() + { + _kebabCased = _naming.ToDeclarationName("unique-timesheet-start"); + _snakeCased = _naming.ToDeclarationName("unique_invitation_email"); + _spaced = _naming.ToDeclarationName("Only one retirement"); + _dotted = _naming.ToDeclarationName("library.authors.registered"); + _mixed = _naming.ToDeclarationName("unique invitation-email.address"); + _leadingSeparator = _naming.ToDeclarationName("-overdue"); + _withoutASeparator = _naming.ToDeclarationName("AuthorRegistered"); + _alreadyCamelCased = _naming.ToDeclarationName("authorRegistered"); + _nothingButSeparators = _naming.ToDeclarationName("---"); + } + + [Fact] void should_pascal_case_a_kebab_cased_name() => _kebabCased.ShouldEqual("UniqueTimesheetStart"); + [Fact] void should_pascal_case_a_snake_cased_name() => _snakeCased.ShouldEqual("UniqueInvitationEmail"); + [Fact] void should_pascal_case_a_name_written_as_words() => _spaced.ShouldEqual("OnlyOneRetirement"); + [Fact] void should_pascal_case_a_dotted_name() => _dotted.ShouldEqual("LibraryAuthorsRegistered"); + [Fact] void should_read_every_kind_of_separator_in_one_name() => _mixed.ShouldEqual("UniqueInvitationEmailAddress"); + [Fact] void should_leave_the_casing_of_one_word_alone() => _leadingSeparator.ShouldEqual("overdue"); + [Fact] void should_leave_a_name_carrying_no_separator_exactly_as_it_is() => _withoutASeparator.ShouldEqual("AuthorRegistered"); + [Fact] void should_not_raise_a_name_that_carries_no_separator() => _alreadyCamelCased.ShouldEqual("authorRegistered"); + [Fact] void should_fall_back_for_a_name_with_no_word_in_it() => _nothingButSeparators.ShouldEqual("_"); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_property_name/from_names_carrying_separators.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_property_name/from_names_carrying_separators.cs new file mode 100644 index 000000000..ea68ac6ed --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_property_name/from_names_carrying_separators.cs @@ -0,0 +1,33 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.for_ScreenplayNaming.when_making_a_property_name; + +/// +/// Reading separators as word boundaries is what every identifier position gets, not only the ones a constraint is +/// named in. A property line is lower camel cased on top of that, which is the same transformation a name written +/// with words would be given by hand. +/// +public class from_names_carrying_separators : given.a_naming +{ + string _kebabCased; + string _snakeCased; + string _leadingUnderscore; + string _startingWithADigitAfterASeparator; + string _nothingButSeparators; + + void Because() + { + _kebabCased = _naming.ToPropertyName("unique-timesheet-start"); + _snakeCased = _naming.ToPropertyName("first_name"); + _leadingUnderscore = _naming.ToPropertyName("_isbn"); + _startingWithADigitAfterASeparator = _naming.ToPropertyName("first-1st"); + _nothingButSeparators = _naming.ToPropertyName("---"); + } + + [Fact] void should_lower_camel_a_kebab_cased_name() => _kebabCased.ShouldEqual("uniqueTimesheetStart"); + [Fact] void should_lower_camel_a_snake_cased_name() => _snakeCased.ShouldEqual("firstName"); + [Fact] void should_drop_a_leading_underscore() => _leadingUnderscore.ShouldEqual("isbn"); + [Fact] void should_join_a_word_starting_with_a_digit() => _startingWithADigitAfterASeparator.ShouldEqual("first1st"); + [Fact] void should_fall_back_for_a_name_with_no_word_in_it() => _nothingButSeparators.ShouldEqual("value"); +} diff --git a/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs b/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs index f46ecbd06..91f574481 100644 --- a/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs +++ b/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs @@ -127,10 +127,19 @@ static string OnOneLine(string value) } /// - /// Strips everything that is not a valid identifier character, including generic type arity suffixes. + /// Reduces a name to the identifier the Screenplay grammar accepts, including generic type arity suffixes. /// /// The name to sanitize. /// The sanitized name. + /// + /// An identifier is [A-Za-z_]\w*, so a name carrying separators has to be transformed - and a runtime name + /// is routinely written with them, kebab case being idiomatic for a Chronicle constraint. Deleting them is the + /// least readable answer available: unique-timesheet-start becomes one run-together word and the word + /// boundaries the source stated are thrown away. Each separator is therefore read as the boundary it is and the + /// words either side of it are joined in Pascal case, which is what the grammar accepts and what a reader would + /// have written by hand. A name carrying no separator at all is left exactly as it is, because its casing is + /// already whatever the application chose. + /// static string Sanitize(string name) { if (string.IsNullOrWhiteSpace(name)) @@ -139,17 +148,59 @@ static string Sanitize(string name) } var backTick = name.IndexOf('`', StringComparison.Ordinal); - var candidate = backTick > 0 ? name[..backTick] : name; - var builder = new StringBuilder(candidate.Length); + var words = WordsIn(backTick > 0 ? name[..backTick] : name); + + var identifier = words.Count switch + { + 0 => string.Empty, + 1 => words[0], + _ => string.Concat(words.Select(Capitalized)) + }; + + return identifier.Normalize(NormalizationForm.FormC); + } + + /// + /// Splits a name into the words the characters that cannot be written in an identifier separate. + /// + /// The name to split. + /// The words, in order, none of them empty. + /// + /// An underscore separates words as surely as a hyphen does, and is treated as one even though the grammar would + /// accept it, so that every way of writing a name apart comes out the same way. + /// + static List WordsIn(string name) + { + var words = new List(); + var word = new StringBuilder(name.Length); - foreach (var character in candidate) + foreach (var character in name) { - if (char.IsLetterOrDigit(character) || character == '_') + if (char.IsLetterOrDigit(character)) + { + word.Append(character); + continue; + } + + if (word.Length > 0) { - builder.Append(character); + words.Add(word.ToString()); + word.Clear(); } } - return builder.ToString().Normalize(NormalizationForm.FormC); + if (word.Length > 0) + { + words.Add(word.ToString()); + } + + return words; } + + /// + /// Raises the first character of a word, leaving the rest of it as the application wrote it. + /// + /// The word to raise. + /// The raised word. + static string Capitalized(string word) => $"{char.ToUpperInvariant(word[0])}{word[1..]}"; } From be8440b25b13f521903509d376422d211e9655d7 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:49:24 +0200 Subject: [PATCH 04/21] Mark personal data under the name the value is referenced by Resolving a type strips the collection it sits in and then strips the optionality of the element, while marking one as personal data stripped only the collection. A collection of an optional value therefore registered the mark under 'Nullable' - a name no concept is declared with - so the document said the value was not sensitive while the runtime encrypted it. Both now strip the same wrappers through one path. Co-Authored-By: Claude Opus 5 --- ...ion_of_optional_values_as_personal_data.cs | 50 +++++++++++++++++++ .../Screenplay/Analysis/Types/TypeRegistry.cs | 41 +++++++++++---- 2 files changed, 81 insertions(+), 10 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_marking_a_collection_of_optional_values_as_personal_data.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_marking_a_collection_of_optional_values_as_personal_data.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_marking_a_collection_of_optional_values_as_personal_data.cs new file mode 100644 index 000000000..043895dcc --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/an_event_marking_a_collection_of_optional_values_as_personal_data.cs @@ -0,0 +1,50 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A concept is declared once and referenced by name, so a value marked as personal data has to be marked under the +/// name it is referenced under. A collection of an optional value says one thing about the value and two about how +/// many there are and whether it may be absent - strip fewer of them than the reference does and the mark lands on +/// the name of the wrapper, leaving a document that says a value is not sensitive while the runtime encrypts it. +/// +public class an_event_marking_a_collection_of_optional_values_as_personal_data : Specification +{ + const string Source = """ + using System.Collections.Generic; + using Cratis.Chronicle.Compliance.GDPR; + using Cratis.Chronicle.Events; + + namespace Library.Authors.Registration; + + public enum AuthorStanding + { + Member, + Honorary + } + + public enum AuthorTier + { + Standard, + Premium + } + + [EventType] + public record AuthorRegistered([PII] IEnumerable Standings, IEnumerable Tiers); + """; + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(Source); + + bool IsPii(string name) => _analysis.Model.Concepts.Single(_ => _.Name == name).IsPii; + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Feature/Slice/Slice.cs", Source)).ShouldBeEmpty(); + [Fact] void should_mark_the_value_the_collection_holds() => IsPii("AuthorStanding").ShouldBeTrue(); + [Fact] void should_leave_the_unmarked_value_alone() => IsPii("AuthorTier").ShouldBeFalse(); + [Fact] void should_declare_a_concept_for_nothing_the_reference_stripped() => _analysis.Model.Concepts.Select(_ => _.Name).ShouldContainOnly(["AuthorStanding", "AuthorTier"]); + [Fact] void should_report_nothing() => _analysis.Diagnostics.ShouldBeEmpty(); +} diff --git a/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs b/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs index 7c64c2bbe..d2ac2f324 100644 --- a/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs +++ b/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs @@ -55,27 +55,27 @@ .. _concepts.Values public TypeReferenceModel Resolve(ITypeSymbol type) { var optional = false; - var current = Unwrap(type, ref optional); - var collection = CollectionElements.ElementOf(current); - if (collection is not null) - { - current = Unwrap(collection, ref optional); - } + var collection = false; - return new(NameOf(current), collection is not null, optional); + return new(NameOf(Underlying(type, ref optional, ref collection)), collection, optional); } /// /// Records that a value of a concept carries personally identifiable information. /// /// The type of the value. + /// + /// The concept a value is marked under has to be the one it is referenced under, or the mark lands on a name no + /// concept is declared with and the document says a value is not sensitive while the runtime encrypts it. Both + /// therefore strip the same wrappers - a collection of an optional concept says one thing about the value and + /// three things about how many there are and whether it may be absent. + /// public void MarkAsPii(ITypeSymbol type) { var optional = false; - var current = Unwrap(type, ref optional); - var collection = CollectionElements.ElementOf(current); + var collection = false; - _pii.Add((collection ?? current).Name); + _pii.Add(Underlying(type, ref optional, ref collection).Name); } /// @@ -94,6 +94,27 @@ public void AddValidations(string conceptName, IEnumerable declared.AddRange(rules); } + /// + /// Strips everything a value is wrapped in that says how many there are or whether it may be absent. + /// + /// The type to strip. + /// Set when a wrapper said the value may be absent. + /// Set when the value is a collection of what is left. + /// The type of the value itself. + static ITypeSymbol Underlying(ITypeSymbol type, ref bool optional, ref bool collection) + { + var current = Unwrap(type, ref optional); + var element = CollectionElements.ElementOf(current); + if (element is null) + { + return current; + } + + collection = true; + + return Unwrap(element, ref optional); + } + /// /// Strips the wrappers that only say whether a value may be absent. /// From a3321cac8917077770450f9390573662a16e70e8 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:50:56 +0200 Subject: [PATCH 05/21] Read a commented out import as the import a component does not make The statement pattern anchors on a line starting with 'import', which a commented out one still does, so a component carrying the leftovers of a change bound its screen to queries it never calls. Comments are now removed before statements are read, and the line breaks a block comment spanned are kept - joining the line after one to the line before would hide the real import that follows it. Co-Authored-By: Claude Opus 5 --- ..._screen_whose_imports_are_commented_out.cs | 61 +++++++++++++++++++ .../and_some_of_them_are_commented_out.cs | 31 ++++++++++ .../Analysis/Screens/ScreenImports.cs | 23 ++++++- 3 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_whose_imports_are_commented_out.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenImports/when_reading_imports/and_some_of_them_are_commented_out.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_whose_imports_are_commented_out.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_whose_imports_are_commented_out.cs new file mode 100644 index 000000000..7ec98ca3b --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_whose_imports_are_commented_out.cs @@ -0,0 +1,61 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// An import that has been commented out is an import the component does not make, so a binding recovered from one +/// says the screen reads through a query it never calls. A commented import is also the most ordinary thing to find +/// in a component under change, which makes it exactly the case the reader has to be right about. +/// +public class a_screen_whose_imports_are_commented_out : Specification +{ + const string Source = """ + using System.Collections.Generic; + using Cratis.Arc.Queries.ModelBound; + + namespace Library.Authors.Listing; + + [ReadModel] + public record Author + { + public string Id { get; init; } = string.Empty; + + public static IEnumerable AllAuthors() => []; + + public static IEnumerable RetiredAuthors() => []; + + public static IEnumerable HonoraryAuthors() => []; + } + """; + + const string Component = """ + import { DataTable } from 'primereact/datatable'; + // import { RetiredAuthors } from './RetiredAuthors'; + /* + import { HonoraryAuthors } from './HonoraryAuthors'; + */ + import { AllAuthors } from './AllAuthors'; + + export const AuthorList = () => ; + """; + + static readonly DeclaredUserInterfaceFiles _files = DeclaredUserInterfaceFiles.Holding( + ("Library/Authors/Listing/AuthorList.tsx", Component)); + + ApplicationModelAnalysis _analysis; + IEnumerable _data; + + void Establish() + { + _analysis = Analyzed.Source(_files, ("Library/Authors/Listing/Listing.cs", Source)); + _data = _analysis.Slice().Screens.Single().Data; + } + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Authors/Listing/Listing.cs", Source)).ShouldBeEmpty(); + [Fact] void should_bind_only_the_query_the_component_really_imports() => _data.Select(_ => _.Query).ShouldContainOnly(["AllAuthors"]); + [Fact] void should_report_only_what_no_screen_states() => _analysis.Diagnostics.Select(_ => _.Code).ShouldContainOnly([ScreenplayDiagnosticCodes.ScreenStructureNotInferred]); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenImports/when_reading_imports/and_some_of_them_are_commented_out.cs b/Source/DotNET/Screenplay.Specs/for_ScreenImports/when_reading_imports/and_some_of_them_are_commented_out.cs new file mode 100644 index 000000000..fc14c3832 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenImports/when_reading_imports/and_some_of_them_are_commented_out.cs @@ -0,0 +1,31 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis.Screens; + +namespace Cratis.Arc.Screenplay.for_ScreenImports.when_reading_imports; + +/// +/// A statement is recognized by starting a line, which a commented one still does. Removing what a comment holds is +/// what tells them apart, and the line breaks the comment spanned have to survive it - joining the line after a block +/// comment to the line before would hide the real import that follows one. +/// +public class and_some_of_them_are_commented_out : Specification +{ + const string Component = """ + // import { CommentedOnItsOwnLine } from './One'; + //import { IndentedAndCommented } from './Two'; + import { Kept } from './Three'; // import { AfterAStatement } from './Four'; + /* import { CommentedInABlock } from './Five'; */ + /* + import { CommentedOverSeveralLines } from './Six'; + */ + import { KeptAfterABlock } from './Seven'; + """; + + IReadOnlyCollection _names; + + void Because() => _names = ScreenImports.In(Component); + + [Fact] void should_keep_every_import_the_file_really_makes() => _names.ShouldContainOnly(["Kept", "KeptAfterABlock"]); +} diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs index bb0c9b9c5..84f6718d8 100644 --- a/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs @@ -35,7 +35,7 @@ public static IReadOnlyCollection In(string? text) return names; } - foreach (var statement in StatementRegex().Matches(text).Cast()) + foreach (var statement in StatementRegex().Matches(WithoutComments(text)).Cast()) { var clause = statement.Groups["clause"].Value; if (!IsRelative(statement.Groups["module"].Value) || IsTypeOnly(clause)) @@ -52,6 +52,20 @@ public static IReadOnlyCollection In(string? text) return names; } + /// + /// Removes everything a comment holds, leaving the lines around it where they were. + /// + /// The text of the file. + /// The text with nothing commented out left in it. + /// + /// An import that has been commented out is an import the file does not make, and binding a screen to a query it + /// no longer calls is a plain untruth of exactly the kind this reader exists to avoid. The line breaks a comment + /// spans are kept, because a statement is recognized by starting a line and joining it to the line before would + /// hide a real import rather than a commented one. + /// + static string WithoutComments(string text) => + CommentRegex().Replace(text, match => new string('\n', match.Value.Count(_ => _ == '\n'))); + /// /// Determines whether a module specifier names a file sitting alongside the one importing it. /// @@ -114,6 +128,13 @@ static IEnumerable NamedIn(string clause) return exported is not null && IdentifierRegex().IsMatch(exported) ? exported : null; } + /// + /// Gets the pattern a comment has to match. + /// + /// The compiled regular expression. + [GeneratedRegex(@"/\*[\s\S]*?\*/|//[^\r\n]*", RegexOptions.None, matchTimeoutMilliseconds: 1000)] + private static partial Regex CommentRegex(); + /// /// Gets the pattern an import statement naming a module has to match. /// From e2f1f257fc2bb03575252b26890bb8557b12ad8e Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:53:08 +0200 Subject: [PATCH 06/21] Leave what the host fills in out of what a query asks a caller for Only an interface parameter was read as a collaborator rather than as input, which is not the whole of what a host hands a query. A cancellation token, the page asked for and the order asked for are all filled in from the request and none of them is an interface, so each was stated as caller input - a parameter no caller sends, typed by a name the document never declares. They are named explicitly, because there is no property of a type that tells infrastructure from a value a caller really sends. Co-Authored-By: Claude Opus 5 --- ...uery_the_host_hands_more_than_arguments.cs | 52 +++++++++++++++++++ .../Analysis/Queries/QueryReader.cs | 20 ++++++- .../Screenplay/Analysis/WellKnownTypeNames.cs | 12 +++++ 3 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_query_the_host_hands_more_than_arguments.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_query_the_host_hands_more_than_arguments.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_query_the_host_hands_more_than_arguments.cs new file mode 100644 index 000000000..b8a1f9e57 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_query_the_host_hands_more_than_arguments.cs @@ -0,0 +1,52 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// What a caller sends is what the document describes a query by. A cancellation token, the page asked for and the +/// order asked for are all filled in by the host from the request rather than sent as arguments, and none of them is +/// an interface - so stating them as caller input puts parameters in the document that no caller sends, typed by +/// names the document never declares. +/// +public class a_query_the_host_hands_more_than_arguments : Specification +{ + const string Source = """ + using System.Collections.Generic; + using System.Threading; + using Cratis.Arc.Queries; + using Cratis.Arc.Queries.ModelBound; + + namespace Library.Authors.Listing; + + [ReadModel] + public record Author + { + public string Id { get; init; } = string.Empty; + + public static IEnumerable AuthorsByName( + string name, + CancellationToken cancellationToken, + Paging paging, + Sorting sorting, + QueryContext context) => []; + } + """; + + ApplicationModelAnalysis _analysis; + QueryModel _query; + + void Establish() + { + _analysis = Analyzed.Source(("Library/Authors/Listing/Listing.cs", Source)); + _query = _analysis.Slice().Queries.Single(); + } + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Authors/Listing/Listing.cs", Source)).ShouldBeEmpty(); + [Fact] void should_key_the_query_by_what_the_caller_sends() => _query.By!.Name.ShouldEqual("name"); + [Fact] void should_leave_out_everything_the_host_fills_in() => _query.Filters.ShouldBeEmpty(); + [Fact] void should_report_nothing() => _analysis.Diagnostics.ShouldBeEmpty(); +} diff --git a/Source/DotNET/Screenplay/Analysis/Queries/QueryReader.cs b/Source/DotNET/Screenplay/Analysis/Queries/QueryReader.cs index cd562c1ef..3ad7ec40d 100644 --- a/Source/DotNET/Screenplay/Analysis/Queries/QueryReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Queries/QueryReader.cs @@ -18,6 +18,17 @@ namespace Cratis.Arc.Screenplay.Analysis.Queries; /// public class QueryReader(TypeRegistry types, ScreenplayDiagnostics diagnostics) { + /// + /// The types a query is handed by the host it runs in rather than by the caller it answers. + /// + public static readonly string[] InfrastructureTypes = + [ + WellKnownTypeNames.CancellationToken, + WellKnownTypeNames.QueryContext, + WellKnownTypeNames.Paging, + WellKnownTypeNames.Sorting + ]; + /// /// Determines whether a type is a model-bound read model. /// @@ -77,9 +88,14 @@ [.. parameters.Where(_ => !SymbolEqualityComparer.Default.Equals(_, required)).S /// True when the parameter is input rather than infrastructure. /// /// An interface parameter is a collaborator the host injects, never something a caller can send, so it is not - /// part of the query's shape. + /// part of the query's shape. Being an interface is not the whole of that though: a cancellation token, the page + /// asked for and the order asked for are all filled in by the host from the request rather than sent as + /// arguments, and all three are values rather than interfaces. Stating one as caller input puts a parameter in + /// the document that no caller sends, typed by a name the document never declares. /// - static bool IsInput(IParameterSymbol parameter) => parameter.Type.TypeKind != TypeKind.Interface; + static bool IsInput(IParameterSymbol parameter) => + parameter.Type.TypeKind != TypeKind.Interface && + !Array.Exists(InfrastructureTypes, parameter.Type.Is); /// /// Determines whether a method returns the read model declaring it. diff --git a/Source/DotNET/Screenplay/Analysis/WellKnownTypeNames.cs b/Source/DotNET/Screenplay/Analysis/WellKnownTypeNames.cs index 4c66b0be2..62fc50fb4 100644 --- a/Source/DotNET/Screenplay/Analysis/WellKnownTypeNames.cs +++ b/Source/DotNET/Screenplay/Analysis/WellKnownTypeNames.cs @@ -116,4 +116,16 @@ public static class WellKnownTypeNames /// The transport level result carrying the value a controller method really returns. public const string ActionResultOfT = "Microsoft.AspNetCore.Mvc.ActionResult`1"; + + /// The token a query is handed to observe cancellation through. + public const string CancellationToken = "System.Threading.CancellationToken"; + + /// The everything a query is performed with, which the host fills in. + public const string QueryContext = "Cratis.Arc.Queries.QueryContext"; + + /// The page of a result a query is performed for, which the host fills in from the request. + public const string Paging = "Cratis.Arc.Queries.Paging"; + + /// The order a result is returned in, which the host fills in from the request. + public const string Sorting = "Cratis.Arc.Queries.Sorting"; } From 748dc3743654e54f9462a72dc373a6e590af0681 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:53:24 +0200 Subject: [PATCH 07/21] Reserve SP0029 so nothing is ever declared with it The sequence jumped from SP0028 to SP0030 with nothing saying why. A code is what a consumer suppresses and groups on, so a number handed to something else later would silently change what an existing suppression means. The gap is now stated as deliberate. Co-Authored-By: Claude Opus 5 --- Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs index aeeb42c0b..f206ea2ab 100644 --- a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs +++ b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs @@ -180,6 +180,11 @@ public static class ScreenplayDiagnosticCodes /// public const string ScreenStructureNotInferred = "SP0028"; + // SP0029 is deliberately unused. It was assigned to a code that was retired before the first release, and the + // sequence is left with the gap rather than closed up: a code is what a consumer suppresses and groups on, so + // handing this number to something else would silently change what an existing suppression means. Nothing is to + // be declared with it. + /// /// A type is referred to by a name that does not say what it is, because Screenplay cannot express it. /// From f30c1b646b1d60e59b5376c4d58eb557da5f06c5 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:55:05 +0200 Subject: [PATCH 08/21] Compose a name before stripping what an identifier cannot hold An accented letter can be written as one character or as a letter followed by a combining mark, and Unicode calls the two canonically equal. A combining mark is not a letter, so stripping first kept the accent of one spelling and quietly removed it from the other - two documents for one application, decided by how an editor happened to save the file. Co-Authored-By: Claude Opus 5 --- ...es_written_two_ways_that_mean_one_thing.cs | 30 +++++++++++++++++++ .../Emission/Naming/ScreenplayNaming.cs | 15 ++++++---- 2 files changed, 40 insertions(+), 5 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_written_two_ways_that_mean_one_thing.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_written_two_ways_that_mean_one_thing.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_written_two_ways_that_mean_one_thing.cs new file mode 100644 index 000000000..7b7876b6b --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayNaming/when_making_a_declaration_name/from_names_written_two_ways_that_mean_one_thing.cs @@ -0,0 +1,30 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.for_ScreenplayNaming.when_making_a_declaration_name; + +/// +/// An accented letter can be written as one character or as a letter followed by a combining mark, and Unicode calls +/// the two canonically equal - a compiler does too, and an editor is free to save either. A combining mark is not a +/// letter though, so composing the name has to happen before anything is stripped from it or one spelling keeps its +/// accent and the other quietly loses it, which is two documents for one application. +/// +public class from_names_written_two_ways_that_mean_one_thing : given.a_naming +{ + const string Composed = "Andr\u00E9Registered"; + const string Decomposed = "Andre\u0301Registered"; + + string _fromTheComposedName; + string _fromTheDecomposedName; + + void Because() + { + _fromTheComposedName = _naming.ToDeclarationName(Composed); + _fromTheDecomposedName = _naming.ToDeclarationName(Decomposed); + } + + [Fact] void should_spell_the_two_names_apart_to_begin_with() => Decomposed.ShouldNotEqual(Composed); + [Fact] void should_read_the_two_spellings_as_one_name() => _fromTheDecomposedName.ShouldEqual(_fromTheComposedName); + [Fact] void should_keep_the_accent_of_the_composed_spelling() => _fromTheComposedName.ShouldEqual(Composed); + [Fact] void should_keep_the_accent_of_the_decomposed_spelling() => _fromTheDecomposedName.ShouldEqual(Composed); +} diff --git a/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs b/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs index 91f574481..d856dad01 100644 --- a/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs +++ b/Source/DotNET/Screenplay/Emission/Naming/ScreenplayNaming.cs @@ -139,6 +139,12 @@ static string OnOneLine(string value) /// words either side of it are joined in Pascal case, which is what the grammar accepts and what a reader would /// have written by hand. A name carrying no separator at all is left exactly as it is, because its casing is /// already whatever the application chose. + /// + /// Composing the name is the first thing done to it rather than the last. An accented letter can be written as + /// one character or as a letter followed by a combining mark, and the two are canonically the same name - but a + /// combining mark is not a letter, so stripping before composing keeps one of them and quietly unaccents the + /// other. Two source files saying the same thing would then produce two different documents. + /// /// static string Sanitize(string name) { @@ -147,17 +153,16 @@ static string Sanitize(string name) return string.Empty; } - var backTick = name.IndexOf('`', StringComparison.Ordinal); - var words = WordsIn(backTick > 0 ? name[..backTick] : name); + var composed = name.Normalize(NormalizationForm.FormC); + var backTick = composed.IndexOf('`', StringComparison.Ordinal); + var words = WordsIn(backTick > 0 ? composed[..backTick] : composed); - var identifier = words.Count switch + return words.Count switch { 0 => string.Empty, 1 => words[0], _ => string.Concat(words.Select(Capitalized)) }; - - return identifier.Normalize(NormalizationForm.FormC); } /// From da2b5711b821d5166e7848d1556d2d86fe343d87 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 20:57:20 +0200 Subject: [PATCH 09/21] Specify that a value which may be absent stays that way in the document C# says a value may be absent in two ways - a nullable annotation on a reference type and a Nullable wrapper on a value type - and Screenplay says it in one, a trailing question mark. Unwrapping distinguished the two all along and nothing asserted that either survived as far as the printed text, where getting it wrong describes a shape the application does not have or names a type the document never declares. The specification is end to end and covers a command property, an event property, an enumeration, a collection of optional values and the parameter of a query. Co-Authored-By: Claude Opus 5 --- ...rce_declaring_values_that_may_be_absent.cs | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/from_source_declaring_values_that_may_be_absent.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/from_source_declaring_values_that_may_be_absent.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/from_source_declaring_values_that_may_be_absent.cs new file mode 100644 index 000000000..3166baf49 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/from_source_declaring_values_that_may_be_absent.cs @@ -0,0 +1,81 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Screenplay; + +namespace Cratis.Arc.Screenplay.for_ScreenplayGenerator.when_generating; + +/// +/// Whether a value may be absent is written in two different ways in C# - a reference type annotated as nullable and +/// a value type wrapped in Nullable - and in one way in Screenplay, a trailing question mark. Stripping the +/// wrapper without carrying what it said across leaves a document claiming every value is always there, which is a +/// shape the application does not have; carrying it across but naming the wrapper leaves a type nothing declares. +/// This asks the whole way through, from source to printed text, in every position a type reference is written in. +/// +public class from_source_declaring_values_that_may_be_absent : Specification +{ + const string Invoicing = """ + using System; + using System.Collections.Generic; + using Cratis.Arc.Commands.ModelBound; + using Cratis.Arc.Queries.ModelBound; + using Cratis.Chronicle.Events; + using Cratis.Concepts; + + namespace Library.Invoicing.Grouping; + + public record InvoiceGroupKey(string Value) : ConceptAs(Value); + + public record InvoiceNumber(int Value) : ConceptAs(Value); + + public enum InvoiceStanding + { + Draft, + Issued + } + + [EventType] + public record InvoiceGrouped(InvoiceGroupKey? Grouping, InvoiceStanding? Standing, IEnumerable Numbers); + + [Command] + public record GroupInvoice(InvoiceGroupKey? Reference, InvoiceStanding? Standing, IEnumerable Numbers) + { + public InvoiceGrouped Handle() => new(Reference, Standing, Numbers); + } + + [ReadModel] + public record InvoiceGroup + { + public string Id { get; init; } = string.Empty; + + public static IEnumerable GroupsByKey(InvoiceGroupKey? groupKey) => []; + } + """; + + static readonly (string Path, string Text)[] _sources = [("Library/Invoicing/Grouping/Grouping.cs", Invoicing)]; + + ScreenplayGenerationResult _result; + CompilationResult _compiled; + string _reprinted; + + void Because() + { + _result = new ScreenplayGenerator().Generate(Analyzed.Compile(_sources), new ScreenplayOptions()); + _compiled = new ScreenplayCompiler().Compile(_result.Source); + _reprinted = _compiled.Value is null ? string.Empty : new Cratis.Screenplay.Printing.ScreenplayPrinter().Print(_compiled.Value); + } + + bool Says(string text) => _result.Source.Contains(text, StringComparison.Ordinal); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(_sources).ShouldBeEmpty(); + [Fact] void should_produce_a_document_that_compiles() => _compiled.Success.ShouldBeTrue(); + [Fact] void should_produce_a_document_the_compiler_says_nothing_about() => _compiled.Diagnostics.ShouldBeEmpty(); + [Fact] void should_print_the_same_text_on_a_second_pass() => _reprinted.ShouldEqual(_result.Source); + [Fact] void should_mark_an_optional_concept_on_a_command() => Says("reference InvoiceGroupKey?").ShouldBeTrue(); + [Fact] void should_mark_an_optional_concept_on_an_event() => Says("grouping InvoiceGroupKey?").ShouldBeTrue(); + [Fact] void should_mark_an_optional_enumeration() => Says("standing InvoiceStanding?").ShouldBeTrue(); + [Fact] void should_mark_a_collection_of_optional_values() => Says("numbers InvoiceNumber[]?").ShouldBeTrue(); + [Fact] void should_mark_an_optional_parameter_of_a_query() => Says("by groupKey InvoiceGroupKey?").ShouldBeTrue(); + [Fact] void should_never_name_the_wrapper_a_value_may_be_absent_behind() => Says("Nullable").ShouldBeFalse(); + [Fact] void should_be_successful() => _result.IsSuccess.ShouldBeTrue(); +} From c5e1a574b5eb22878a917cee8a6d137f343f1184 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 21:00:23 +0200 Subject: [PATCH 10/21] Resolve the options of an emission in one place The generator resolved them against the assembly it was reading and the syntax builder resolved them again against the domain of the model, so one generation ran two resolutions with two different fallbacks and let whichever came last decide. They agree today, which is the only reason nothing showed. The emitter is now the single place the emission half resolves, and the builder is handed options that are already resolved. Co-Authored-By: Claude Opus 5 --- ...el_with_nothing_configured_alongside_it.cs | 26 +++++++++++++++++++ .../and_they_are_already_resolved.cs | 26 +++++++++++++++++++ .../Emission/ApplicationSyntaxBuilder.cs | 15 +++++++---- .../Screenplay/Emission/ScreenplayEmitter.cs | 8 +++++- 4 files changed, 69 insertions(+), 6 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_model_with_nothing_configured_alongside_it.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayOptions/when_resolving_defaults/and_they_are_already_resolved.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_model_with_nothing_configured_alongside_it.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_model_with_nothing_configured_alongside_it.cs new file mode 100644 index 000000000..12b016e42 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayEmitter/when_emitting/a_model_with_nothing_configured_alongside_it.cs @@ -0,0 +1,26 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Emission; +using Cratis.Arc.Screenplay.Library; +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.for_ScreenplayEmitter.when_emitting; + +/// +/// A host emitting a model it already has configures nothing, and there is no compilation to name the document +/// after. The domain the model carries is the only answer available, so the emitter is where the options an emission +/// runs with are resolved - moving that any deeper meant it happened twice on the way through a generation. +/// +public class a_model_with_nothing_configured_alongside_it : given.an_emitter +{ + ApplicationModel _model; + ScreenplayEmission _emission; + + void Establish() => _model = LibraryApplication.Build() with { Domain = "Lending", Module = string.Empty }; + + void Because() => _emission = _emitter.Emit(_model, new ScreenplayOptions()); + + [Fact] void should_name_the_document_after_the_domain_the_model_carries() => _emission.Source.StartsWith("domain Lending", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_fall_back_to_the_domain_for_the_module() => _emission.Source.Contains("module Lending", StringComparison.Ordinal).ShouldBeTrue(); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayOptions/when_resolving_defaults/and_they_are_already_resolved.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayOptions/when_resolving_defaults/and_they_are_already_resolved.cs new file mode 100644 index 000000000..53859f95b --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayOptions/when_resolving_defaults/and_they_are_already_resolved.cs @@ -0,0 +1,26 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.for_ScreenplayOptions.when_resolving_defaults; + +/// +/// A generation resolves for the analysis half against the assembly it is reading, and the emission half resolves +/// against the domain of the model it is handed. Both run on the way through one generation, so resolving what is +/// already resolved has to answer the same thing however a second fallback would have answered - otherwise which +/// entry point was used decides what the document is called. +/// +public class and_they_are_already_resolved : Specification +{ + ScreenplayOptions _resolved; + ScreenplayOptions _resolvedAgain; + + void Because() + { + _resolved = new ScreenplayOptions().WithDefaults("Library"); + _resolvedAgain = _resolved.WithDefaults("SomethingElse"); + } + + [Fact] void should_answer_the_same_thing_a_second_time() => _resolvedAgain.ShouldEqual(_resolved); + [Fact] void should_not_take_the_second_fallback_for_the_domain() => _resolvedAgain.Domain.ShouldEqual("Library"); + [Fact] void should_not_take_the_second_fallback_for_the_module() => _resolvedAgain.Module.ShouldEqual("Library"); +} diff --git a/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs b/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs index 320f6a52c..403ca5534 100644 --- a/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs +++ b/Source/DotNET/Screenplay/Emission/ApplicationSyntaxBuilder.cs @@ -41,15 +41,20 @@ public class ApplicationSyntaxBuilder(IScreenplayNaming naming, ScreenplayDiagno /// Builds the document. /// /// The model to build from. - /// The options to build with. + /// The options to build with, already resolved. /// The . + /// + /// The options arrive resolved rather than being resolved here. What a name falls back to depends on how the + /// document was asked for - the assembly being analyzed when a generation asked for it, the domain of the model + /// when a host emitted one it already had - so resolving where neither of those is known meant resolving a + /// second time against a different answer and letting one of them quietly win. + /// public ApplicationSyntax Build(ApplicationModel model, ScreenplayOptions options) { - var resolved = options.WithDefaults(model.Domain); - var domain = ToName(model.Domain, resolved.Domain); - var module = ToName(model.Module, resolved.Module); + var domain = ToName(model.Domain, options.Domain); + var module = ToName(model.Module, options.Module); - var modules = BuildModules(model, module, resolved.SegmentsToSkip ?? 0); + var modules = BuildModules(model, module, options.SegmentsToSkip ?? 0); var concepts = new ConceptSyntaxBuilder(naming, _validations, diagnostics, _names).Build(model.Concepts); var policies = new PolicySyntaxBuilder(naming).Build(model.Policies, _authorize.Referenced); diff --git a/Source/DotNET/Screenplay/Emission/ScreenplayEmitter.cs b/Source/DotNET/Screenplay/Emission/ScreenplayEmitter.cs index 7b2691c12..0c2c85ed0 100644 --- a/Source/DotNET/Screenplay/Emission/ScreenplayEmitter.cs +++ b/Source/DotNET/Screenplay/Emission/ScreenplayEmitter.cs @@ -23,10 +23,16 @@ public ScreenplayEmitter() } /// + /// + /// This is where the options an emission runs with are resolved, and the only place. What a name falls back to + /// is a question only an entry point can answer - a host that emits a model it already holds has nothing to fall + /// back on but the domain of that model - so resolving deeper down meant resolving twice on the way through a + /// generation, against two answers that only happen to agree. + /// public ScreenplayEmission Emit(ApplicationModel model, ScreenplayOptions options) { var diagnostics = new ScreenplayDiagnostics(); - var application = new ApplicationSyntaxBuilder(naming, diagnostics).Build(model, options); + var application = new ApplicationSyntaxBuilder(naming, diagnostics).Build(model, options.WithDefaults(model.Domain)); return new(printer.Print(application), application, diagnostics.All); } From 5d75a6c55aedb544f46d725c3607ce3265f36b86 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 21:07:47 +0200 Subject: [PATCH 11/21] Hold the document of a real application to the compiler in CI Every Screenplay specification builds its compilation from source strings, which is what makes them hermetic and what puts a class of defect out of their reach by construction: a compilation built that way has no intermediate folder, no source generator output on disk and no referenced package declaring an event, so a generator that mis-attributes one of those produces a wrong document that every specification still passes. Two shipped that way. A console project reads a real project through MSBuild, generates from it and reads the document back with the compiler the language ships. It has to come back with nothing to say, warnings included - a warning there is the document referring to something it never introduces, which is this generator being wrong rather than the application. Co-Authored-By: Claude Opus 5 --- .github/workflows/dotnet-build.yml | 51 ++++++++++++ Arc.slnx | 1 + Directory.Packages.props | 6 ++ Source/DotNET/Screenplay.EndToEnd/Program.cs | 80 +++++++++++++++++++ .../Screenplay.EndToEnd/ProjectCompilation.cs | 61 ++++++++++++++ .../Screenplay.EndToEnd.csproj | 28 +++++++ 6 files changed, 227 insertions(+) create mode 100644 Source/DotNET/Screenplay.EndToEnd/Program.cs create mode 100644 Source/DotNET/Screenplay.EndToEnd/ProjectCompilation.cs create mode 100644 Source/DotNET/Screenplay.EndToEnd/Screenplay.EndToEnd.csproj diff --git a/.github/workflows/dotnet-build.yml b/.github/workflows/dotnet-build.yml index 9db62aeaa..763239ee4 100644 --- a/.github/workflows/dotnet-build.yml +++ b/.github/workflows/dotnet-build.yml @@ -176,6 +176,57 @@ jobs: - name: Test (${{ matrix.project }}) run: dotnet test ${{ matrix.project }} --no-build --configuration Release --framework net10.0 + # Every Screenplay specification builds its compilation from source strings, which is what makes them hermetic - + # and what puts a whole class of defect out of their reach by construction. A compilation built that way has no + # intermediate folder, no source generator output on disk and no referenced package declaring an event, so a + # generator that mis-attributes one of those produces a wrong document that every specification still passes. Two + # shipped that way. This reads a real project through MSBuild and holds the document it generates to the compiler + # the language ships - it has to come back with nothing to say, warnings included. + screenplay-end-to-end: + needs: [dotnet-build] + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + with: + ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.sha || github.sha }} + + - name: Setup .Net + uses: actions/setup-dotnet@v4 + with: + dotnet-version: | + ${{ env.DOTNET8_VERSION }} + ${{ env.DOTNET9_VERSION }} + ${{ env.DOTNET10_VERSION }} + + - name: Cache NuGet packages + uses: actions/cache@v6 + with: + path: ~/.nuget/packages + key: nuget-${{ runner.os }}-${{ hashFiles('**/*.csproj', '**/Directory.*.props', '**/Directory.*.targets') }} + restore-keys: | + nuget-${{ runner.os }}- + + - name: Download build output + uses: actions/download-artifact@v4 + with: + name: build-output + + - name: Extract build output + run: tar -xzf build-output.tar.gz + + # The build output artifact carries bin folders only, and reading a project through MSBuild needs the assets + # file that restore writes to obj. + - name: Restore the application to generate from + run: dotnet restore TestApps/AspNetCore/AspNetCore.csproj + + - name: Generate and read back the document of a real application + run: | + dotnet Source/DotNET/Screenplay.EndToEnd/bin/Release/net10.0/Cratis.Arc.Screenplay.EndToEnd.dll \ + TestApps/AspNetCore/AspNetCore.csproj \ + "${RUNNER_TEMP}/AspNetCore.play" + proxy-specs: needs: [dotnet-build] runs-on: ubuntu-latest diff --git a/Arc.slnx b/Arc.slnx index 67d6a64e7..5bdfa91d1 100644 --- a/Arc.slnx +++ b/Arc.slnx @@ -31,6 +31,7 @@ + diff --git a/Directory.Packages.props b/Directory.Packages.props index b62c03507..690fc5ebb 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -67,6 +67,12 @@ + + + + diff --git a/Source/DotNET/Screenplay.EndToEnd/Program.cs b/Source/DotNET/Screenplay.EndToEnd/Program.cs new file mode 100644 index 000000000..d28378d94 --- /dev/null +++ b/Source/DotNET/Screenplay.EndToEnd/Program.cs @@ -0,0 +1,80 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay; +using Cratis.Arc.Screenplay.EndToEnd; +using Cratis.Screenplay; + +if (args.Length < 2) +{ + Console.WriteLine("Usage:"); + Console.WriteLine(" Cratis.Arc.Screenplay.EndToEnd "); + + return 2; +} + +var project = Path.GetFullPath(args[0]); +var output = Path.GetFullPath(args[1]); + +Console.WriteLine($"Generating the Screenplay document of '{project}'"); + +var failures = new List(); +var compilation = await ProjectCompilation.Of(project, failures); + +foreach (var failure in failures) +{ + Console.WriteLine($" workspace: {failure}"); +} + +if (compilation is null) +{ + Console.WriteLine($"'{project}' yielded no compilation, so there is nothing to generate from"); + + return 1; +} + +var generated = new ScreenplayGenerator().Generate(compilation, new ScreenplayOptions()); + +Directory.CreateDirectory(Path.GetDirectoryName(output)!); +await File.WriteAllTextAsync(output, generated.Source); + +Console.WriteLine($"Wrote {generated.Source.Split('\n').Length} lines to '{output}'"); + +foreach (var group in generated.Diagnostics.GroupBy(_ => _.Code).OrderBy(_ => _.Key, StringComparer.Ordinal)) +{ + Console.WriteLine($" {group.Key} x{group.Count()}"); +} + +var errors = generated.Diagnostics.Where(_ => _.Severity == ScreenplayDiagnosticSeverity.Error).ToList(); +foreach (var error in errors) +{ + Console.WriteLine($" generation error {error.Code}: {error.Message}"); +} + +var compiled = new ScreenplayCompiler().Compile(generated.Source); +var rejected = compiled.Diagnostics.ToList(); +foreach (var diagnostic in rejected) +{ + Console.WriteLine($" document {diagnostic.Severity} on line {diagnostic.Location.Line}: {diagnostic.Message}"); +} + +// The document has to compile clean, warnings included. A warning the language reports is the generator writing a +// document that refers to something it never introduces, which is a defect here rather than in the application - and +// it is precisely the class of defect no specification built from source strings can reach. +if (rejected.Count > 0) +{ + Console.WriteLine($"The generated document did not read back clean - {rejected.Count} diagnostic(s)"); + + return 1; +} + +if (errors.Count > 0) +{ + Console.WriteLine($"Generation reported {errors.Count} error(s)"); + + return 1; +} + +Console.WriteLine("The generated document reads back clean"); + +return 0; diff --git a/Source/DotNET/Screenplay.EndToEnd/ProjectCompilation.cs b/Source/DotNET/Screenplay.EndToEnd/ProjectCompilation.cs new file mode 100644 index 000000000..115cf78f0 --- /dev/null +++ b/Source/DotNET/Screenplay.EndToEnd/ProjectCompilation.cs @@ -0,0 +1,61 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Runtime.CompilerServices; +using Microsoft.Build.Locator; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.MSBuild; + +namespace Cratis.Arc.Screenplay.EndToEnd; + +/// +/// Reads a real project file into the compilation the generator is handed. +/// +/// +/// The generator itself never loads a workspace - it takes a and nothing else - which is +/// what lets every specification build one from source strings. That seam is exactly what this check exists to get +/// behind: a compilation built from strings has no intermediate folder, no source generator output on disk and no +/// referenced package that declares an event, so a defect that only shows in one of those is invisible to a +/// specification by construction. Two shipped that way. +/// +public static class ProjectCompilation +{ + /// + /// Loads a project and everything it references. + /// + /// The full path of the project file. + /// Everything the workspace reported while loading, in the order it reported them. + /// The , or when the project yielded none. + /// + /// Registering the SDK has to happen before any MSBuild type is touched, which is why the member touching one is + /// marked as not inlinable - the JIT would otherwise resolve those types while registration is still running. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + public static async Task Of(string path, ICollection failures) + { + if (!MSBuildLocator.IsRegistered) + { + MSBuildLocator.RegisterDefaults(); + } + + return await Load(path, failures); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static async Task Load(string path, ICollection failures) + { + var reported = new Lock(); + using var workspace = MSBuildWorkspace.Create(); + using var subscription = workspace.RegisterWorkspaceFailedHandler(args => + { + lock (reported) + { + failures.Add(args.Diagnostic.Message); + } + }); + + var project = await workspace.OpenProjectAsync(path); + + return await project.GetCompilationAsync(); + } +} diff --git a/Source/DotNET/Screenplay.EndToEnd/Screenplay.EndToEnd.csproj b/Source/DotNET/Screenplay.EndToEnd/Screenplay.EndToEnd.csproj new file mode 100644 index 000000000..b4a640c85 --- /dev/null +++ b/Source/DotNET/Screenplay.EndToEnd/Screenplay.EndToEnd.csproj @@ -0,0 +1,28 @@ + + + + Exe + Cratis.Arc.Screenplay.EndToEnd + Cratis.Arc.Screenplay.EndToEnd + + net10.0 + false + + + + + + + + + + + + + + + + + From 7b88843c4f23a1ef93054706889ceffc2ad709b0 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 21:39:03 +0200 Subject: [PATCH 12/21] Report source that did not compile for what survived it Any compilation error at all made SP0024 an error saying "nothing recovered from it describes the application reliably". That claim is false whenever recovery succeeded, and it usually did: a host handing over a compilation assembled without the compile items a build generates leaves every reference to a generated type unresolved, and the errors then sit in code declaring no artifact while every command, event and reactor is read exactly as written. One real application produced 607 such errors alongside a complete and accurate document, which the host then discarded on the error alone. The severity now follows how many artifacts were recovered from a declaration no compilation error sits inside. None - nothing recovered at all, or every declaration something came out of being one the compiler could not make sense of - stays an error, because no part of that document can be trusted. Any at all is a warning stating how many errors there were, how many artifacts came through anyway and what usually causes it, so the result is successful and the document is kept. A count is used rather than a proportion because any threshold would make the same recovery pass for a large application and fail for a small one. Zero is the only number that means recovery was prevented rather than dented. Suppressing "declares nothing" is unchanged and, in the warning branch, can never apply - a warning is only reached when something was recovered. Co-Authored-By: Claude Opus 5 --- ...error_sits_in_a_partial_the_build_wrote.cs | 57 ++++++++++++ ..._artifact_it_recovered_carries_an_error.cs | 40 ++++++++ .../and_nothing_at_all_was_recovered.cs} | 31 +++---- ...e_of_what_it_recovered_carries_an_error.cs | 44 +++++++++ ...he_errors_sit_outside_what_it_recovered.cs | 65 +++++++++++++ .../Analysis/ApplicationModelAnalyzer.cs | 40 ++------ .../Analysis/ArtifactDeclaration.cs | 53 +++++++++++ .../Screenplay/Analysis/CompilationErrors.cs | 93 +++++++++++++++++++ .../Screenplay/Analysis/RecoveredArtifacts.cs | 53 +++++++++++ .../Analysis/Slices/SliceArtifactReader.cs | 22 ++++- .../Analysis/Slices/SliceContents.cs | 23 +++-- .../Screenplay/Analysis/Slices/SliceReader.cs | 9 +- .../Screenplay/ScreenplayDiagnosticCodes.cs | 25 ++++- 13 files changed, 495 insertions(+), 60 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_an_error_sits_in_a_partial_the_build_wrote.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_every_artifact_it_recovered_carries_an_error.cs rename Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/{source_that_did_not_compile.cs => source_that_did_not_compile/and_nothing_at_all_was_recovered.cs} (53%) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_only_some_of_what_it_recovered_carries_an_error.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_the_errors_sit_outside_what_it_recovered.cs create mode 100644 Source/DotNET/Screenplay/Analysis/ArtifactDeclaration.cs create mode 100644 Source/DotNET/Screenplay/Analysis/CompilationErrors.cs create mode 100644 Source/DotNET/Screenplay/Analysis/RecoveredArtifacts.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_an_error_sits_in_a_partial_the_build_wrote.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_an_error_sits_in_a_partial_the_build_wrote.cs new file mode 100644 index 000000000..e70e26486 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_an_error_sits_in_a_partial_the_build_wrote.cs @@ -0,0 +1,57 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing.source_that_did_not_compile; + +/// +/// A type is written in as many places as it has partial declarations, and the Cratis house style puts one of them in +/// the intermediate folder of the build. An error there is an error inside the artifact just as much as one in the +/// file a person wrote, so every place a type is written counts - reading only the first would call a declaration +/// clean on the strength of the half that happens to be listed first. +/// +public class and_an_error_sits_in_a_partial_the_build_wrote : Specification +{ + const string Written = """ + using System.Threading.Tasks; + using Cratis.Chronicle.Events; + using Cratis.Chronicle.Reactors; + + namespace Library.Onboarding.Registry; + + [EventType] + public record CompanyRegistered(string Name); + + public partial class RegistryNotifier : IReactor + { + public Task CompanyRegistered(CompanyRegistered @event, EventContext context) => Task.CompletedTask; + } + """; + + const string Emitted = """ + namespace Library.Onboarding.Registry; + + public partial class RegistryNotifier + { + public string Describe() => RegistryMessages.Describe; + } + """; + + static readonly (string Path, string Text)[] _sources = + [ + ("Library/Onboarding/Registry/Registry.cs", Written), + ("Library/obj/Debug/net10.0/Generated/Registry.Logging.g.cs", Emitted) + ]; + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(_sources); + + ScreenplayDiagnostic Reported => _analysis.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); + + [Fact] void should_be_analyzing_source_that_really_does_not_compile() => Analyzed.ErrorsIn(_sources).ShouldNotBeEmpty(); + [Fact] void should_report_it_as_a_warning() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Warning); + [Fact] void should_not_count_the_reactor_the_error_is_written_inside() => Reported.Message.Contains("2 artifact(s) were recovered anyway, 1 of them", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_still_return_the_reactor() => _analysis.Slice().Reactors.Single().Name.ShouldEqual("RegistryNotifier"); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_every_artifact_it_recovered_carries_an_error.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_every_artifact_it_recovered_carries_an_error.cs new file mode 100644 index 000000000..04996a40a --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_every_artifact_it_recovered_carries_an_error.cs @@ -0,0 +1,40 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing.source_that_did_not_compile; + +/// +/// Something was recovered, but every declaration it came out of is one the compiler could not make sense of - so +/// what the document says about each of them is read from source that never held together. That is the other way +/// recovery is genuinely prevented, and it is an error for the same reason as recovering nothing at all: there is +/// no part of the document a reader could trust. +/// +public class and_every_artifact_it_recovered_carries_an_error : Specification +{ + const string Source = """ + using Cratis.Arc.Commands.ModelBound; + + namespace Library.Authors.Registration; + + [Command] + public record RegisterAuthor(string Name) + { + public void Handle() => ThisDoesNotExist(); + } + """; + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(Source); + + ScreenplayDiagnostic Reported => _analysis.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); + + [Fact] void should_be_analyzing_source_that_really_does_not_compile() => Analyzed.ErrorsIn((Analyzed.SlicePath, Source)).ShouldNotBeEmpty(); + [Fact] void should_report_it_as_an_error() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Error); + [Fact] void should_say_how_much_it_recovered() => Reported.Message.Contains("1 artifact(s) were recovered", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_say_the_errors_sit_inside_what_it_recovered() => Reported.Message.Contains("every declaration they were read from is one an error sits inside", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_say_the_document_cannot_be_relied_on() => Reported.Message.Contains("describes the application reliably", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_still_return_whatever_it_recovered() => _analysis.Slice().Commands.Single().Name.ShouldEqual("RegisterAuthor"); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_nothing_at_all_was_recovered.cs similarity index 53% rename from Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile.cs rename to Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_nothing_at_all_was_recovered.cs index b5cb660f1..bf521480c 100644 --- a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile.cs +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_nothing_at_all_was_recovered.cs @@ -3,29 +3,22 @@ using Cratis.Arc.Screenplay.Analysis; -namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing.source_that_did_not_compile; /// -/// Source that does not build still yields symbols, and analyzing them produces a document that looks like an -/// answer while describing an application that does not exist. A nearly empty document handed back with nothing -/// wrong reported is the one outcome nobody can act on, so this is an error - which is what makes a host exit non -/// zero - while whatever was recovered is still returned for whoever wants to look at it. +/// A build that produced nothing an artifact could be read out of is the case the claim was always true for - a +/// document describing an empty application handed back with nothing wrong reported is the one outcome nobody can +/// act on, so this is an error and a host exits non zero on it. It is also the case that must not be mistaken for +/// an application which simply declares no artifact yet, so "declares nothing" stays suppressed. /// -public class source_that_did_not_compile : Specification +public class and_nothing_at_all_was_recovered : Specification { const string Source = """ - using Cratis.Arc.Commands.ModelBound; - using Cratis.Chronicle.Events; + namespace Library.Plumbing; - namespace Library.Authors.Registration; - - [EventType] - public record AuthorRegistered(string Name); - - [Command] - public record RegisterAuthor(string Name) + public class NotAnArtifact { - public AuthorRegistered Handle() => new(ThisDoesNotExist); + public string Name() => ThisDoesNotExist; } """; @@ -35,11 +28,13 @@ public record RegisterAuthor(string Name) ScreenplayDiagnostic Reported => _analysis.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); - [Fact] void should_be_analyzing_source_that_really_does_not_compile() => Analyzed.ErrorsIn(("Library/Feature/Slice/Slice.cs", Source)).ShouldNotBeEmpty(); + [Fact] void should_be_analyzing_source_that_really_does_not_compile() => Analyzed.ErrorsIn((Analyzed.SlicePath, Source)).ShouldNotBeEmpty(); [Fact] void should_report_that_the_source_did_not_compile() => _analysis.Diagnostics.Select(_ => _.Code).ShouldContain(ScreenplayDiagnosticCodes.SourceDidNotCompile); [Fact] void should_report_it_as_an_error() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Error); + [Fact] void should_say_nothing_at_all_came_out_of_it() => Reported.Message.Contains("Nothing at all was recovered from it", StringComparison.Ordinal).ShouldBeTrue(); [Fact] void should_say_the_document_cannot_be_relied_on() => Reported.Message.Contains("describes the application reliably", StringComparison.Ordinal).ShouldBeTrue(); [Fact] void should_quote_the_first_thing_the_compiler_said() => Reported.Message.Contains("ThisDoesNotExist", StringComparison.Ordinal).ShouldBeTrue(); - [Fact] void should_still_return_whatever_it_recovered() => _analysis.Slice().Commands.Single().Name.ShouldEqual("RegisterAuthor"); + [Fact] void should_not_also_say_the_application_declares_nothing() => _analysis.Diagnostics.Select(_ => _.Code).ShouldNotContain(ScreenplayDiagnosticCodes.AnalysisUnavailable); + [Fact] void should_recover_no_slice() => _analysis.Model.Slices.ShouldBeEmpty(); [Fact] void should_not_throw_instead() => _analysis.Model.ShouldNotBeNull(); } diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_only_some_of_what_it_recovered_carries_an_error.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_only_some_of_what_it_recovered_carries_an_error.cs new file mode 100644 index 000000000..5054f2cbb --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_only_some_of_what_it_recovered_carries_an_error.cs @@ -0,0 +1,44 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing.source_that_did_not_compile; + +/// +/// This is the boundary between the two severities. One artifact was read from a declaration an error sits inside +/// and one was not, and the one that was not is described exactly as its source states it - so "nothing recovered +/// describes the application reliably" is already false with a single clean declaration. The severity therefore +/// turns on whether any survived rather than on whether any was hit, and the count says which is which. +/// +public class and_only_some_of_what_it_recovered_carries_an_error : Specification +{ + const string Source = """ + using Cratis.Arc.Commands.ModelBound; + using Cratis.Chronicle.Events; + + namespace Library.Authors.Registration; + + [EventType] + public record AuthorRegistered(string Name); + + [Command] + public record RegisterAuthor(string Name) + { + public AuthorRegistered Handle() => new(ThisDoesNotExist); + } + """; + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(Source); + + ScreenplayDiagnostic Reported => _analysis.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); + + [Fact] void should_be_analyzing_source_that_really_does_not_compile() => Analyzed.ErrorsIn((Analyzed.SlicePath, Source)).ShouldNotBeEmpty(); + [Fact] void should_report_it_as_a_warning() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Warning); + [Fact] void should_report_nothing_at_all_as_an_error() => _analysis.Diagnostics.Any(_ => _.Severity == ScreenplayDiagnosticSeverity.Error).ShouldBeFalse(); + [Fact] void should_count_only_the_one_no_error_sits_inside() => Reported.Message.Contains("2 artifact(s) were recovered anyway, 1 of them", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_quote_the_first_thing_the_compiler_said() => Reported.Message.Contains("ThisDoesNotExist", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_still_return_whatever_it_recovered() => _analysis.Slice().Commands.Single().Name.ShouldEqual("RegisterAuthor"); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_the_errors_sit_outside_what_it_recovered.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_the_errors_sit_outside_what_it_recovered.cs new file mode 100644 index 000000000..83e8793f2 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/source_that_did_not_compile/and_the_errors_sit_outside_what_it_recovered.cs @@ -0,0 +1,65 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing.source_that_did_not_compile; + +/// +/// This is the case that made the claim a lie. A host handing over a compilation assembled without the compile items +/// a build generates leaves every reference to a strongly typed resource class unresolved, and the errors then sit in +/// source that declares no artifact at all while every command and event is read exactly as written. Calling that an +/// error says something untrue about a document that is entirely correct and makes the host throw it away, so it is a +/// warning that states both facts - what failed, and how much came through anyway. +/// +public class and_the_errors_sit_outside_what_it_recovered : Specification +{ + const string Slice = """ + using Cratis.Arc.Commands.ModelBound; + using Cratis.Chronicle.Events; + + namespace Library.Authors.Registration; + + [EventType] + public record AuthorRegistered(string Name); + + [Command] + public record RegisterAuthor(string Name) + { + public AuthorRegistered Handle() => new(Name); + } + """; + + const string Wording = """ + namespace Library.Authors; + + public static class Wording + { + public static string NameIsRequired() => AuthorsMessages.NameIsRequired; + } + """; + + static readonly (string Path, string Text)[] _sources = + [ + ("Library/Authors/Registration/Registration.cs", Slice), + ("Library/Authors/Wording.cs", Wording) + ]; + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(_sources); + + ScreenplayDiagnostic Reported => _analysis.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); + + [Fact] void should_be_analyzing_source_that_really_does_not_compile() => Analyzed.ErrorsIn(_sources).ShouldNotBeEmpty(); + [Fact] void should_report_that_the_source_did_not_compile() => _analysis.Diagnostics.Select(_ => _.Code).ShouldContain(ScreenplayDiagnosticCodes.SourceDidNotCompile); + [Fact] void should_report_it_as_a_warning() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Warning); + [Fact] void should_report_nothing_at_all_as_an_error() => _analysis.Diagnostics.Any(_ => _.Severity == ScreenplayDiagnosticSeverity.Error).ShouldBeFalse(); + [Fact] void should_say_how_many_errors_there_were() => Reported.Message.Contains("1 error(s)", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_quote_the_first_thing_the_compiler_said() => Reported.Message.Contains("AuthorsMessages", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_say_how_much_it_recovered_anyway() => Reported.Message.Contains("2 artifact(s) were recovered anyway, 2 of them", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_hint_at_what_the_host_left_out() => Reported.Message.Contains("without the compile items a build generates", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_not_claim_nothing_describes_the_application_reliably() => Reported.Message.Contains("describes the application reliably", StringComparison.Ordinal).ShouldBeFalse(); + [Fact] void should_recover_the_command() => _analysis.Slice().Commands.Single().Name.ShouldEqual("RegisterAuthor"); + [Fact] void should_recover_the_event() => _analysis.Slice().Events.Single().Name.ShouldEqual("AuthorRegistered"); +} diff --git a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs index d33d66f6a..ecf4b44b1 100644 --- a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs +++ b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs @@ -38,7 +38,6 @@ public ApplicationModelAnalyzer() public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptions options) { var diagnostics = new ScreenplayDiagnostics(); - var failedToCompile = ReportCompilationErrors(compilation, diagnostics); var catalog = ArtifactCatalog.From(compilation); var readers = ArtifactReaders.For(compilation, catalog, diagnostics); var screens = new ScreenReader( @@ -47,7 +46,8 @@ public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptio new(diagnostics), new(userInterfaceFiles, diagnostics)); - var reader = new SliceReader(readers, diagnostics, screens); + var recovered = new RecoveredArtifacts(); + var reader = new SliceReader(readers, diagnostics, screens, recovered); var slices = catalog.Namespaces .Select(@namespace => reader.Read(@namespace, catalog.In(@namespace))) @@ -60,6 +60,13 @@ public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptio var imports = ExternalEvents.Resolve(compilation, slices, diagnostics); ReportNamespacesWithoutStructure(slices, diagnostics, options.SegmentsToSkip ?? 0); + // How serious source that did not compile is depends on how much survived it, so it is reported once the + // slices are in rather than on the way in. Source that did not compile still suppresses "declares nothing" - + // an empty document from a broken build is the broken build, not an application with nothing in it - and + // when it is reported as a warning the suppression can never apply, because a warning is only ever reached + // when something was recovered and something recovered is a slice. + var failedToCompile = CompilationErrors.Report(compilation, recovered, diagnostics); + if (slices.Count == 0 && !failedToCompile) { diagnostics.Information( @@ -81,35 +88,6 @@ public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptio diagnostics.All); } - /// - /// Reports source that did not compile, which nothing recovered from it can be relied on past. - /// - /// The compilation to check. - /// The diagnostics to report to. - /// True when the source did not compile. - /// - /// A compilation that does not build still yields symbols, and analyzing them produces a document that looks - /// like an answer while describing an application that does not exist. Reporting it as an error rather than a - /// warning is what makes a host exit non zero, because a nearly empty document and a success code is the one - /// outcome nobody can act on. The model is still returned - what was recovered is worth seeing, as long as - /// nobody is told it is trustworthy. - /// - static bool ReportCompilationErrors(Compilation compilation, ScreenplayDiagnostics diagnostics) - { - var errors = compilation.GetDiagnostics().Where(_ => _.Severity == DiagnosticSeverity.Error).ToList(); - if (errors.Count == 0) - { - return false; - } - - diagnostics.Error( - ScreenplayDiagnosticCodes.SourceDidNotCompile, - $"The source did not compile - {errors.Count} error(s), the first being '{errors[0].GetMessage(System.Globalization.CultureInfo.InvariantCulture)}'. Nothing recovered from it describes the application reliably", - compilation.AssemblyName); - - return true; - } - /// /// Gets everything within a slice that requires something of the caller. /// diff --git a/Source/DotNET/Screenplay/Analysis/ArtifactDeclaration.cs b/Source/DotNET/Screenplay/Analysis/ArtifactDeclaration.cs new file mode 100644 index 000000000..81dfab5f7 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/ArtifactDeclaration.cs @@ -0,0 +1,53 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Text; + +namespace Cratis.Arc.Screenplay.Analysis; + +/// +/// Holds where the declaration that artifacts were recovered from is written. +/// +/// +/// A type is written in as many places as it has partial declarations, and a source generator contributing one of +/// them is exactly the case this exists to tell apart, so every place it is written is held rather than the first. +/// +public class ArtifactDeclaration +{ + readonly (SyntaxTree Tree, TextSpan Span)[] _written; + + ArtifactDeclaration(int artifacts, (SyntaxTree Tree, TextSpan Span)[] written) + { + Artifacts = artifacts; + _written = written; + } + + /// + /// Gets the number of artifacts recovered from the declaration. + /// + public int Artifacts { get; } + + /// + /// Holds where the type artifacts were recovered from is written. + /// + /// The type they were recovered from. + /// The number of artifacts recovered from it. + /// The . + public static ArtifactDeclaration For(INamedTypeSymbol type, int artifacts) => + new(artifacts, [.. type.DeclaringSyntaxReferences.Select(_ => (_.SyntaxTree, _.Span))]); + + /// + /// Answers whether something the compiler reported sits within the declaration. + /// + /// The diagnostic the compiler reported. + /// True when it sits within the declaration. + /// + /// An error somewhere else in the compilation says nothing about what was read here, while one written inside + /// the declaration itself is the compiler saying it could not make sense of the very source the artifact was + /// recovered from. Telling the two apart is the whole reason the spans are kept. + /// + public bool Encloses(Diagnostic diagnostic) => + diagnostic.Location.SourceTree is { } tree && + _written.Any(_ => ReferenceEquals(_.Tree, tree) && _.Span.Contains(diagnostic.Location.SourceSpan)); +} diff --git a/Source/DotNET/Screenplay/Analysis/CompilationErrors.cs b/Source/DotNET/Screenplay/Analysis/CompilationErrors.cs new file mode 100644 index 000000000..6cdf80f46 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/CompilationErrors.cs @@ -0,0 +1,93 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Globalization; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.Analysis; + +/// +/// Reports source that did not compile, at the severity what was recovered from it earns. +/// +/// +/// A compilation that does not build still yields symbols, and analyzing them can produce a document that looks like +/// an answer while describing an application that does not exist - which is why this is reported at all. What it is +/// not is a single outcome. A host that hands over a compilation assembled without the compile items a build +/// generates leaves every reference to a strongly typed resource class unresolved, and hundreds of errors then sit +/// in source that declares no artifact at all while every command, event and reactor is read exactly as written. +/// Calling that an error says something untrue about thousands of correct lines and makes the host discard them. +/// +/// So the severity follows one number: how many artifacts were recovered from a declaration no compilation error +/// sits inside. When that number is zero - because nothing was recovered, or because every declaration something was +/// recovered from is one the compiler could not make sense of - nothing in the document is worth trusting and this is +/// an error, which is what makes a host exit non zero. Otherwise it is a warning stating how much was recovered +/// anyway, because the artifacts read from source the compiler accepted are described exactly as that source states +/// them and a document holding them is worth having. +/// +/// +/// A count is used rather than a proportion deliberately. Any threshold - a tenth, a half - would make the same +/// recovery pass for a large application and fail for a small one, and no number is defensible. Zero is the only one +/// that means recovery was prevented rather than merely dented. +/// +/// +public static class CompilationErrors +{ + /// + /// Reports the errors a compilation carries, if it carries any. + /// + /// The compilation that was analyzed. + /// What was recovered from it, and where each of it was written. + /// The diagnostics to report to. + /// True when the source did not compile, whatever was recovered from it. + public static bool Report(Compilation compilation, RecoveredArtifacts recovered, ScreenplayDiagnostics diagnostics) + { + var errors = compilation.GetDiagnostics().Where(_ => _.Severity == DiagnosticSeverity.Error).ToList(); + if (errors.Count == 0) + { + return false; + } + + var first = errors[0].GetMessage(CultureInfo.InvariantCulture); + var accepted = recovered.RecoveredFromAcceptedSource(errors); + + if (accepted == 0) + { + diagnostics.Error( + ScreenplayDiagnosticCodes.SourceDidNotCompile, + PreventedRecovery(errors.Count, first, recovered.Count), + compilation.AssemblyName); + } + else + { + diagnostics.Warning( + ScreenplayDiagnosticCodes.SourceDidNotCompile, + SurvivedRecovery(errors.Count, first, recovered.Count, accepted), + compilation.AssemblyName); + } + + return true; + } + + /// + /// Says that the errors left nothing behind worth trusting. + /// + /// The number of errors the compiler reported. + /// The first thing the compiler said. + /// The number of artifacts recovered in all. + /// The message. + static string PreventedRecovery(int errors, string first, int recovered) => + recovered == 0 + ? $"The source did not compile - {errors} error(s), the first being '{first}'. Nothing at all was recovered from it, so nothing in the document describes the application reliably" + : $"The source did not compile - {errors} error(s), the first being '{first}'. {recovered} artifact(s) were recovered, and every declaration they were read from is one an error sits inside, so nothing recovered describes the application reliably"; + + /// + /// Says how much came through the errors intact. + /// + /// The number of errors the compiler reported. + /// The first thing the compiler said. + /// The number of artifacts recovered in all. + /// The number of them read from a declaration no error sits inside. + /// The message. + static string SurvivedRecovery(int errors, string first, int recovered, int accepted) => + $"The source did not compile - {errors} error(s), the first being '{first}'. {recovered} artifact(s) were recovered anyway, {accepted} of them from a declaration no error sits inside, so the document describes those exactly as the source states them - a missing type named like 'SomethingMessages' or a designer class usually means the compilation was handed over without the compile items a build generates"; +} diff --git a/Source/DotNET/Screenplay/Analysis/RecoveredArtifacts.cs b/Source/DotNET/Screenplay/Analysis/RecoveredArtifacts.cs new file mode 100644 index 000000000..03edc2d08 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/RecoveredArtifacts.cs @@ -0,0 +1,53 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.Analysis; + +/// +/// Counts what analysis recovered from a compilation, and remembers where each of it was written. +/// +/// +/// "Nothing recovered from it describes the application reliably" is a claim about a compilation that did not +/// compile, and for most of them it is false - a build handed over without the compile items it generates leaves +/// hundreds of errors behind while every artifact is read exactly as written. This is what turns that claim into +/// one that can be checked: how much was recovered, and how much of it came from source the compiler accepted. +/// +public class RecoveredArtifacts +{ + readonly List _declarations = []; + + /// + /// Gets the number of artifacts recovered in all. + /// + public int Count => _declarations.Sum(_ => _.Artifacts); + + /// + /// Holds what one type contributed, ignoring a type that contributed nothing. + /// + /// The type that was read. + /// The number of artifacts recovered from it. + public void Declare(INamedTypeSymbol type, int artifacts) + { + if (artifacts <= 0) + { + return; + } + + _declarations.Add(ArtifactDeclaration.For(type, artifacts)); + } + + /// + /// Counts the artifacts recovered from a declaration that none of the compilation errors sit inside. + /// + /// The errors the compiler reported. + /// The number of artifacts. + /// + /// This is the number that decides how serious a compilation that did not compile is. An artifact read from a + /// declaration the compiler accepted is described exactly as the source states it, whatever went wrong + /// elsewhere, so as long as there is one of them the document is not worthless. + /// + public int RecoveredFromAcceptedSource(IReadOnlyList errors) => + _declarations.Where(declaration => !errors.Any(declaration.Encloses)).Sum(_ => _.Artifacts); +} diff --git a/Source/DotNET/Screenplay/Analysis/Slices/SliceArtifactReader.cs b/Source/DotNET/Screenplay/Analysis/Slices/SliceArtifactReader.cs index 0b17f0409..73b6dc1ec 100644 --- a/Source/DotNET/Screenplay/Analysis/Slices/SliceArtifactReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Slices/SliceArtifactReader.cs @@ -19,11 +19,12 @@ namespace Cratis.Arc.Screenplay.Analysis.Slices; /// /// The reading each kind of artifact. /// The anything unmappable is reported to. +/// The holding what each declaration yielded. /// /// A type can be more than one thing at once - a read model with queries is also the declaration of a projection - /// so every recognizer is asked in turn rather than the first match winning. /// -public class SliceArtifactReader(ArtifactReaders readers, ScreenplayDiagnostics diagnostics) +public class SliceArtifactReader(ArtifactReaders readers, ScreenplayDiagnostics diagnostics, RecoveredArtifacts recovered) { /// /// Reads everything one type contributes. @@ -31,7 +32,26 @@ public class SliceArtifactReader(ArtifactReaders readers, ScreenplayDiagnostics /// The type to read. /// The namespace the slice lives in. /// The content collected so far. + /// + /// What the type yielded is taken as the difference it made to the content rather than recorded by each + /// recognizer, so a recognizer added later is tied to its declaration without anyone remembering to say so. + /// public void Read(INamedTypeSymbol type, string @namespace, SliceContents content) + { + var before = content.Count; + + ReadInto(type, @namespace, content); + + recovered.Declare(type, content.Count - before); + } + + /// + /// Asks every recognizer what the type is. + /// + /// The type to read. + /// The namespace the slice lives in. + /// The content collected so far. + void ReadInto(INamedTypeSymbol type, string @namespace, SliceContents content) { if (EventReader.IsEvent(type)) { diff --git a/Source/DotNET/Screenplay/Analysis/Slices/SliceContents.cs b/Source/DotNET/Screenplay/Analysis/Slices/SliceContents.cs index 4360669d3..f27e22e9c 100644 --- a/Source/DotNET/Screenplay/Analysis/Slices/SliceContents.cs +++ b/Source/DotNET/Screenplay/Analysis/Slices/SliceContents.cs @@ -49,14 +49,23 @@ public class SliceContents /// public bool HasAggregateRoot { get; set; } + /// + /// Gets the number of artifacts collected so far. + /// + /// + /// What one type contributed is the difference between this before it was read and after, which is how a + /// declaration is tied to what came out of it without every recognizer having to say so itself. + /// + public int Count => + Commands.Count + + Events.Count + + Queries.Count + + Reactors.Count + + Constraints.Count + + (Projection is null ? 0 : 1); + /// /// Gets a value indicating whether the slice declares nothing at all. /// - public bool IsEmpty => - Commands.Count == 0 && - Events.Count == 0 && - Queries.Count == 0 && - Reactors.Count == 0 && - Constraints.Count == 0 && - Projection is null; + public bool IsEmpty => Count == 0; } diff --git a/Source/DotNET/Screenplay/Analysis/Slices/SliceReader.cs b/Source/DotNET/Screenplay/Analysis/Slices/SliceReader.cs index 12a78e7fe..e8c164c01 100644 --- a/Source/DotNET/Screenplay/Analysis/Slices/SliceReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Slices/SliceReader.cs @@ -13,14 +13,19 @@ namespace Cratis.Arc.Screenplay.Analysis.Slices; /// The reading each kind of artifact. /// The anything unmappable is reported to. /// The reading what the slice ends in. +/// The holding what each declaration yielded. /// /// A namespace is the unit a slice is recovered from, because that is what the vertical slice convention makes it - /// everything belonging to one behavior lives together, and nothing else does. The same convention puts the file /// realizing a screen in that folder too, which is why screens are read here rather than discovered separately. /// -public class SliceReader(ArtifactReaders readers, ScreenplayDiagnostics diagnostics, ScreenReader screens) +public class SliceReader( + ArtifactReaders readers, + ScreenplayDiagnostics diagnostics, + ScreenReader screens, + RecoveredArtifacts recovered) { - readonly SliceArtifactReader _artifacts = new(readers, diagnostics); + readonly SliceArtifactReader _artifacts = new(readers, diagnostics, recovered); /// /// Reads the slice a namespace declares. diff --git a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs index f206ea2ab..06286a787 100644 --- a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs +++ b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs @@ -133,8 +133,31 @@ public static class ScreenplayDiagnosticCodes public const string NamespaceWithoutStructure = "SP0023"; /// - /// The source did not compile, so nothing recovered from it can be relied on. + /// The source did not compile, and how far what was recovered from it can be relied on is what the severity says. /// + /// + /// This is the one code whose severity is decided rather than fixed, because "the source did not compile" covers + /// two outcomes that could not be further apart. A host handing over a compilation assembled without the compile + /// items a build generates leaves every reference to a generated type unresolved - hundreds of errors, none of + /// them anywhere near an artifact - while every command, event and reactor is read exactly as written. Calling + /// that an error says something untrue about a document that is entirely correct, and makes the host throw it + /// away. + /// + /// So the severity follows how many artifacts were recovered from a declaration no compilation error sits inside. + /// None - because nothing was recovered at all, or because every declaration something came out of is one the + /// compiler could not make sense of - is an error, and nothing in the document is worth trusting. Any at all is a + /// warning saying how many came through, because an artifact read from source the compiler accepted describes + /// what that source states regardless of what failed elsewhere. + /// + /// + /// As an error it suppresses and , both for + /// the same reason: a model recovered from symbols the compiler never accepted describes an application that does + /// not exist, so an empty document and a rejected one are consequences of the broken build rather than defects of + /// their own. As a warning it suppresses neither - it can never coincide with the first, since a warning is only + /// reached when something was recovered, and suppressing the second would hand back a document the language + /// rejects with nothing wrong reported. + /// + /// public const string SourceDidNotCompile = "SP0024"; /// From 7a3d89320927f25afc36145dbfce84b5cbf3233e Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 21:39:18 +0200 Subject: [PATCH 13/21] Read the document back when what was recovered still stands SP0024 suppressed the document check on the code alone, which was right while it was always an error: a model recovered from symbols the compiler never accepted describes an application that does not exist, so a document made from it being poor is the broken build rather than a second defect. Now that it can be a warning, that reasoning inverts. A warning says what was recovered stands, and a document built from a model that stands is exactly what the check exists for. Suppressing it there would hand back a document the language rejects with nothing wrong reported and a successful result, which is the one outcome the check was added to make impossible. The suppression therefore follows the severity rather than the code. Co-Authored-By: Claude Opus 5 --- .../RecoveringSource.cs | 56 +++++++++++++++++++ .../and_nothing_was_recovered_from_it.cs} | 15 +++-- ...the_document_it_printed_is_rejected_too.cs | 38 +++++++++++++ .../and_what_it_recovered_still_stands.cs | 44 +++++++++++++++ .../DotNET/Screenplay/ScreenplayGenerator.cs | 9 ++- 5 files changed, 155 insertions(+), 7 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/RecoveringSource.cs rename Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/{and_the_source_it_read_did_not_compile.cs => and_the_source_it_read_did_not_compile/and_nothing_was_recovered_from_it.cs} (67%) create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_the_document_it_printed_is_rejected_too.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_what_it_recovered_still_stands.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/RecoveringSource.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/RecoveringSource.cs new file mode 100644 index 000000000..ba1949517 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/RecoveringSource.cs @@ -0,0 +1,56 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.for_ScreenplayGenerator.when_generating.and_the_source_it_read_did_not_compile; + +/// +/// Holds source that does not compile and yet gives up everything it declares, which is what a host handing over a +/// compilation assembled without the compile items a build generates really produces. +/// +/// +/// The unresolved name sits in a helper class that declares no artifact, so the errors are real while the command and +/// the event are read from declarations the compiler accepted - the shape the whole recalibration turns on. +/// +public static class RecoveringSource +{ + /// + /// The slice, which compiles on its own. + /// + public const string Slice = """ + using Cratis.Arc.Commands.ModelBound; + using Cratis.Chronicle.Events; + + namespace Library.Authors.Registration; + + [EventType] + public record AuthorRegistered(string Name); + + [Command] + public record RegisterAuthor(string Name) + { + public AuthorRegistered Handle() => new(Name); + } + """; + + /// + /// A helper reaching for the strongly typed resource class a build would have generated. + /// + public const string Wording = """ + namespace Library.Authors; + + public static class Wording + { + public static string NameIsRequired() => AuthorsMessages.NameIsRequired; + } + """; + + /// + /// Gets the source files, keyed by the path each one is compiled as. + /// + /// The source files. + public static (string Path, string Text)[] Files() => + [ + ("Library/Authors/Registration/Registration.cs", Slice), + ("Library/Authors/Wording.cs", Wording) + ]; +} diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_nothing_was_recovered_from_it.cs similarity index 67% rename from Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile.cs rename to Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_nothing_was_recovered_from_it.cs index 19845d8bc..20ae761eb 100644 --- a/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile.cs +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_nothing_was_recovered_from_it.cs @@ -5,15 +5,15 @@ using Cratis.Screenplay; using Microsoft.CodeAnalysis; -namespace Cratis.Arc.Screenplay.for_ScreenplayGenerator.when_generating; +namespace Cratis.Arc.Screenplay.for_ScreenplayGenerator.when_generating.and_the_source_it_read_did_not_compile; /// -/// Source the compiler never accepted still yields symbols, and a model recovered from them describes an application -/// that does not exist - so a document made from it being poor is the consequence already reported rather than a -/// second defect. Saying both would send whoever reads the diagnostics hunting for a bug in the generator when the -/// thing to fix is the build, which is why the same suppression the empty document already gets applies here. +/// Source the compiler never accepted that yielded no artifact leaves a model describing an application that does not +/// exist - so a document made from it being poor is the consequence already reported rather than a second defect. +/// Saying both would send whoever reads the diagnostics hunting for a bug in the generator when the thing to fix is +/// the build, which is why the same suppression the empty document already gets applies here. /// -public class and_the_source_it_read_did_not_compile : given.a_document_the_language_rejects +public class and_nothing_was_recovered_from_it : given.a_document_the_language_rejects { const string Source = """ namespace Library.Authors.Registration; @@ -36,9 +36,12 @@ void Establish() void Because() => _result = _generator.Generate(_compilation, new ScreenplayOptions()); + ScreenplayDiagnostic Reported => _result.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); + [Fact] void should_be_generating_from_source_that_really_does_not_compile() => Analyzed.ErrorsIn((Analyzed.SlicePath, Source)).ShouldNotBeEmpty(); [Fact] void should_be_printing_a_document_the_language_really_rejects() => new ScreenplayCompiler().Compile(Rejected).Success.ShouldBeFalse(); [Fact] void should_report_that_the_source_did_not_compile() => _result.Diagnostics.Select(_ => _.Code).ShouldContain(ScreenplayDiagnosticCodes.SourceDidNotCompile); + [Fact] void should_report_it_as_an_error() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Error); [Fact] void should_not_report_the_document_on_top_of_it() => _result.Diagnostics.Select(_ => _.Code).ShouldNotContain(ScreenplayDiagnosticCodes.DocumentDidNotCompile); [Fact] void should_still_return_the_document() => _result.Source.ShouldEqual(Rejected); [Fact] void should_not_be_successful() => _result.IsSuccess.ShouldBeFalse(); diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_the_document_it_printed_is_rejected_too.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_the_document_it_printed_is_rejected_too.cs new file mode 100644 index 000000000..afacb0ffd --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_the_document_it_printed_is_rejected_too.cs @@ -0,0 +1,38 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Cratis.Screenplay; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.for_ScreenplayGenerator.when_generating.and_the_source_it_read_did_not_compile; + +/// +/// The suppression of the document check follows the severity rather than the code. Source that did not compile but +/// gave up everything it declares says the model stands, and a document built from a model that stands is exactly +/// what the check exists for - suppressing it here would hand back a document the language rejects with nothing +/// wrong reported, which is the one outcome nobody can act on. +/// +public class and_the_document_it_printed_is_rejected_too : given.a_document_the_language_rejects +{ + Compilation _compilation; + ScreenplayGenerator _generator; + ScreenplayGenerationResult _result; + + void Establish() + { + _compilation = Analyzed.Compile(RecoveringSource.Files()); + _generator = new(new ApplicationModelAnalyzer(), _emitter); + } + + void Because() => _result = _generator.Generate(_compilation, new ScreenplayOptions()); + + ScreenplayDiagnostic Reported => _result.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); + + [Fact] void should_be_generating_from_source_that_really_does_not_compile() => Analyzed.ErrorsIn(RecoveringSource.Files()).ShouldNotBeEmpty(); + [Fact] void should_be_printing_a_document_the_language_really_rejects() => new ScreenplayCompiler().Compile(Rejected).Success.ShouldBeFalse(); + [Fact] void should_report_the_source_it_read_only_as_a_warning() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Warning); + [Fact] void should_report_the_document_on_top_of_it() => _result.Diagnostics.Select(_ => _.Code).ShouldContain(ScreenplayDiagnosticCodes.DocumentDidNotCompile); + [Fact] void should_still_return_the_document() => _result.Source.ShouldEqual(Rejected); + [Fact] void should_not_be_successful() => _result.IsSuccess.ShouldBeFalse(); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_what_it_recovered_still_stands.cs b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_what_it_recovered_still_stands.cs new file mode 100644 index 000000000..952c6b3ba --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ScreenplayGenerator/when_generating/and_the_source_it_read_did_not_compile/and_what_it_recovered_still_stands.cs @@ -0,0 +1,44 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Screenplay; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.for_ScreenplayGenerator.when_generating.and_the_source_it_read_did_not_compile; + +/// +/// This is the point of the whole thing. A host that follows the documented contract discards the document on +/// anything reported as an error, so a compilation that lost only its generated symbols used to cost the caller a +/// complete and correct document. The errors are still reported - nothing is hidden - but the result is successful, +/// because what was recovered is described exactly as the source states it. +/// +public class and_what_it_recovered_still_stands : Specification +{ + Compilation _compilation; + ScreenplayGenerator _generator; + ScreenplayGenerationResult _result; + CompilationResult _compiled; + + void Establish() + { + _compilation = Analyzed.Compile(RecoveringSource.Files()); + _generator = new(); + } + + void Because() + { + _result = _generator.Generate(_compilation, new ScreenplayOptions()); + _compiled = new ScreenplayCompiler().Compile(_result.Source); + } + + ScreenplayDiagnostic Reported => _result.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile); + + [Fact] void should_be_generating_from_source_that_really_does_not_compile() => Analyzed.ErrorsIn(RecoveringSource.Files()).ShouldNotBeEmpty(); + [Fact] void should_report_that_the_source_did_not_compile() => _result.Diagnostics.Select(_ => _.Code).ShouldContain(ScreenplayDiagnosticCodes.SourceDidNotCompile); + [Fact] void should_report_it_as_a_warning() => Reported.Severity.ShouldEqual(ScreenplayDiagnosticSeverity.Warning); + [Fact] void should_be_successful() => _result.IsSuccess.ShouldBeTrue(); + [Fact] void should_report_nothing_at_all_as_an_error() => _result.Diagnostics.Any(_ => _.Severity == ScreenplayDiagnosticSeverity.Error).ShouldBeFalse(); + [Fact] void should_describe_the_command_the_source_declares() => _result.Source.Contains("command RegisterAuthor", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_describe_the_event_the_source_declares() => _result.Source.Contains("event AuthorRegistered", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_produce_a_document_that_compiles() => _compiled.Success.ShouldBeTrue(); +} diff --git a/Source/DotNET/Screenplay/ScreenplayGenerator.cs b/Source/DotNET/Screenplay/ScreenplayGenerator.cs index 2140740c1..08b6b6a70 100644 --- a/Source/DotNET/Screenplay/ScreenplayGenerator.cs +++ b/Source/DotNET/Screenplay/ScreenplayGenerator.cs @@ -68,6 +68,13 @@ public ScreenplayGenerationResult Generate(Compilation compilation, ScreenplayOp /// way it suppresses : a model recovered from symbols /// the compiler never accepted describes an application that does not exist, so a poor document made from it is /// the consequence already reported rather than a second defect. + /// + /// The suppression follows the severity that code was reported at rather than the code alone. As an error it + /// says nothing recovered can be trusted, which is the whole reason a document built from it says nothing + /// either. As a warning it says the opposite - that what was recovered stands - and a document built from a + /// model that stands is exactly what this check is for. Suppressing it there would hand back a document the + /// language rejects with nothing wrong reported, which is the one outcome this exists to make impossible. + /// /// void ReportDocumentThatDoesNotCompile( string source, @@ -75,7 +82,7 @@ void ReportDocumentThatDoesNotCompile( ScreenplayDiagnostics diagnostics, string? location) { - if (analyzed.Any(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile)) + if (analyzed.Any(_ => _.Code == ScreenplayDiagnosticCodes.SourceDidNotCompile && _.Severity == ScreenplayDiagnosticSeverity.Error)) { return; } From 2a7edb496419eb6c1c41438291f9aff0b648df57 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 21:39:27 +0200 Subject: [PATCH 14/21] Say what a compilation the generator is handed has to hold The generator takes a Compilation and nothing else, so assembling one the way a real build would is the caller's job - and nothing said so. The part hosts get wrong is source generators: no workspace loading mode runs them, and an Arc application leans on generation heavily enough that a compilation loaded without them is missing every type they emit. Documents the contract, how to run the generators before handing the compilation over, and what the generator does when a host does not - which is now two different things depending on how much survived. Co-Authored-By: Claude Opus 5 --- Documentation/generating-a-screenplay.md | 50 ++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/Documentation/generating-a-screenplay.md b/Documentation/generating-a-screenplay.md index 021813dd3..8369deaf5 100644 --- a/Documentation/generating-a-screenplay.md +++ b/Documentation/generating-a-screenplay.md @@ -35,7 +35,7 @@ The generator is used through the Cratis CLI, which ships separately from Arc: cratis screenplay generate ./MyApp/MyApp.csproj --file MyApp.play ``` -Point it at a project or a solution. Anything the generator could not express is reported on the way out, and a run that reports an error exits non-zero — so a broken build never quietly produces a document that looks fine. +Point it at a project or a solution. Anything the generator could not express is reported on the way out, and a run that reports an error exits non-zero — so a document nothing trustworthy went into never quietly looks fine. What the CLI has to do before it hands the source over is covered in [what the generator expects of its host](#what-the-generator-expects-of-its-host). Because output is reproducible, a CI step can regenerate and fail when the committed `.play` no longer matches the source. @@ -48,7 +48,49 @@ Warning SP0019: The query 'Raw' returns 'IActionResult', which says how the resu transported rather than what it is, so the query was left out (Library.Messaging.Feed) ``` -Diagnostics come in three severities. **Information** means something is worth knowing but the document is complete. **Warning** means something was left out. **Error** means the document should not be trusted at all — the most common cause being source that did not compile. +Diagnostics come in three severities. **Information** means something is worth knowing but the document is complete. **Warning** means something was left out. **Error** means the document should not be trusted at all — either because the generator produced something the language rejects, or because nothing at all was recovered from source the compiler accepted. + +## What the generator expects of its host + +The generator takes a Roslyn `Compilation` and nothing else. It never opens a project file or loads a workspace, which is what lets it be driven from a CLI, from a specification, or from an editor. The other side of that bargain is that **assembling the compilation the way a real build would is the caller's job** — the generator reads what it is handed and cannot tell a missing type from a type that was never written. + +The part hosts get wrong is source generators. `MSBuildWorkspace.GetCompilationAsync()` does not run them, and neither does any loading mode that stops at the compile items the project file lists. An Arc application leans on generation heavily — `[LoggerMessage]` partial classes, strongly-typed resource designer classes, the proxy and metrics generators — so a compilation loaded that way is missing every type those emit, and every reference to one becomes an unresolved-symbol error. This is the common case rather than an edge case. + +A host should run the project's generators before handing the compilation over: + +```csharp +var driver = CSharpGeneratorDriver.Create( + generators: project.AnalyzerReferences + .SelectMany(reference => reference.GetGenerators(LanguageNames.CSharp)) + .Select(GeneratorExtensions.AsSourceGenerator), + parseOptions: (CSharpParseOptions)project.ParseOptions!); + +driver.RunGeneratorsAndUpdateCompilation(compilation, out var generated, out _); +``` + +Then generate from `generated` rather than from `compilation`. + +### What happens when it does not + +Nothing is hidden, and nothing correct is thrown away. A compilation carrying errors is still analyzed, and `SP0024` says what happened — how many errors there were, the first of them, and how many artifacts came through anyway: + +```text +Warning SP0024: The source did not compile - 607 error(s), the first being 'The name +'AccountsMessages' does not exist in the current context'. 341 artifact(s) were recovered +anyway, 341 of them from a declaration no error sits inside, so the document describes those +exactly as the source states them - a missing type named like 'SomethingMessages' or a +designer class usually means the compilation was handed over without the compile items a +build generates (Accounts) +``` + +Its **severity is decided rather than fixed**, because "the source did not compile" covers two outcomes that could not be further apart: + +- **Warning** when at least one artifact was recovered from a declaration no compilation error sits inside. Those artifacts are described exactly as their source states them whatever failed elsewhere, so the run is successful and the document is worth keeping. A compilation missing its generated symbols lands here — the errors sit in code that declares no artifact, and the model is unaffected. +- **Error** when none were, either because nothing was recovered at all or because every declaration something came out of is one the compiler could not make sense of. There is then no part of the document a reader could trust, and a host following the contract exits non-zero. + +A count is used rather than a proportion deliberately: any threshold would make the same recovery pass for a large application and fail for a small one, and zero is the only number that means recovery was *prevented* rather than merely dented. + +Either way the document is written out, so what was recovered can be read. ## The generator checks its own output @@ -65,7 +107,9 @@ and the document is returned as it stands so the line can be read (Library) The document is still written out, so you can open it at the reported line and see what happened. If you hit this, it is a bug worth [reporting](https://github.com/Cratis/Arc/issues) — include the line, and the C# declaration it came from. -Source that did not compile (`SP0024`) suppresses `SP0034`. A model recovered from symbols the compiler never accepted describes an application that does not exist, so a poor document made from it is a consequence of the broken build rather than a second, separate defect. Fix the build and generate again. +Source that did not compile (`SP0024`) suppresses `SP0034` **when it is reported as an error** — a model recovered from symbols the compiler never accepted describes an application that does not exist, so a poor document made from it is a consequence of the broken build rather than a second, separate defect. Fix the build and generate again. + +As a warning it suppresses nothing. That severity says the model stands, and a document built from a model that stands is exactly what the check exists for — suppressing it there would hand back a `.play` the language rejects with nothing wrong reported. ## Screens From 2b052e56856c70cfe38e03452cdf6fce0b7d281d Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 22:00:22 +0200 Subject: [PATCH 15/21] Take a validation message from the constant a lambda names A message reaches a rule through a lambda wherever an application declares its messages once and names them from every validator rather than repeating the text. The lambda computes nothing there - it names a constant the compiler already substituted - but only the direct form was read, so those applications ended up with a document carrying no message on any rule at all. The lambda that really does build its text while the request runs stays reported, and the report now names the expression it could not write down rather than only saying that there was one. Co-Authored-By: Claude Opus 5 --- ...ator_taking_its_messages_from_constants.cs | 71 +++++++++++++++++++ .../Validation/ValidationChainReader.cs | 18 ++++- .../Analysis/Validation/ValidationMessages.cs | 65 +++++++++++++++++ 3 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_taking_its_messages_from_constants.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Validation/ValidationMessages.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_taking_its_messages_from_constants.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_taking_its_messages_from_constants.cs new file mode 100644 index 000000000..32fdc7bb0 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_taking_its_messages_from_constants.cs @@ -0,0 +1,71 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// Wherever messages are declared once and named from every validator rather than repeated, the message reaches the +/// rule through a lambda - and the lambda computes nothing, it names a constant the compiler already substituted. +/// Reading only the direct form left a document with no message on any rule at all in exactly the applications that +/// took the most care over them. The lambda that really does build its text while the request runs is the one shape +/// there is nothing to write down for, and it stays reported. +/// +public class a_validator_taking_its_messages_from_constants : Specification +{ + const string Source = """ + using Cratis.Arc.Commands; + using Cratis.Arc.Commands.ModelBound; + using FluentValidation; + + namespace Library.Authors.Registration; + + public static class AuthorMessages + { + public const string NameRequired = "An author must have a name"; + public static readonly string EmailRequired = "An author must have an email address"; + } + + [Command] + public record RegisterAuthor(string Name, string Pen, string Email, string Nickname) + { + public void Handle() + { + } + } + + public class RegisterAuthorValidator : CommandValidator + { + public RegisterAuthorValidator() + { + RuleFor(_ => _.Name).NotEmpty().WithMessage(_ => AuthorMessages.NameRequired); + RuleFor(_ => _.Pen).NotEmpty().WithMessage(_ => "An author must have a pen name"); + RuleFor(_ => _.Email).NotEmpty().WithMessage(_ => AuthorMessages.EmailRequired); + RuleFor(_ => _.Nickname).NotEmpty().WithMessage(command => $"'{command.Nickname}' will not do"); + } + } + """; + + ApplicationModelAnalysis _analysis; + IEnumerable _rules; + + void Establish() + { + _analysis = Analyzed.Source(Source); + _rules = _analysis.Slice().Commands.First().Validations; + } + + ValidationRuleModel Rule(string property) => _rules.First(_ => _.Property == property); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Feature/Slice/Slice.cs", Source)).ShouldBeEmpty(); + [Fact] void should_state_every_rule_a_message_was_written_after() => _rules.Select(_ => _.Property).ShouldContainOnly(["Name", "Pen", "Email", "Nickname"]); + [Fact] void should_recover_a_message_a_constant_holds() => Rule("Name").Message.ShouldEqual("An author must have a name"); + [Fact] void should_recover_a_message_written_into_the_lambda() => Rule("Pen").Message.ShouldEqual("An author must have a pen name"); + [Fact] void should_leave_a_message_only_the_running_application_holds_off_its_rule() => Rule("Email").Message.ShouldBeNull(); + [Fact] void should_leave_a_message_built_from_the_value_off_its_rule() => Rule("Nickname").Message.ShouldBeNull(); + [Fact] void should_report_only_the_messages_it_could_not_write_down() => _analysis.Diagnostics.Select(_ => _.Code).Distinct(StringComparer.Ordinal).ShouldContainOnly([ScreenplayDiagnosticCodes.UnmappableValidationRule]); + [Fact] void should_report_one_for_each_of_them() => _analysis.Diagnostics.Count.ShouldEqual(2); + [Fact] void should_say_which_message_it_was() => _analysis.Diagnostics.Any(_ => _.Message.Contains("AuthorMessages.EmailRequired", StringComparison.Ordinal)).ShouldBeTrue(); +} diff --git a/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs b/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs index 60568f7a7..bde394236 100644 --- a/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs @@ -195,12 +195,24 @@ void ApplyMessage( int preceding, string location) { - var message = InvocationChain.ArgumentOf(call) is { } argument ? semanticModel.GetConstantValue(argument).Value as string : null; - if (message is null || preceding == 0) + var argument = InvocationChain.ArgumentOf(call); + var message = ValidationMessages.Read(argument, semanticModel); + + if (message is null) + { + diagnostics.Warning( + ScreenplayDiagnosticCodes.UnmappableValidationRule, + $"The message '{argument}' is put together while the request runs rather than written down, so the rule it belongs to states none", + location); + + return; + } + + if (preceding == 0) { diagnostics.Warning( ScreenplayDiagnosticCodes.UnmappableValidationRule, - "A message was declared without a constant value or without a rule preceding it, and was left out", + $"The message '{message}' follows nothing the document states a rule for, so there is nothing to attach it to and it was left out", location); return; diff --git a/Source/DotNET/Screenplay/Analysis/Validation/ValidationMessages.cs b/Source/DotNET/Screenplay/Analysis/Validation/ValidationMessages.cs new file mode 100644 index 000000000..6a59856d2 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Validation/ValidationMessages.cs @@ -0,0 +1,65 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace Cratis.Arc.Screenplay.Analysis.Validation; + +/// +/// Reads the text a WithMessage call carries. +/// +/// +/// A message is written in two shapes that say the same thing. Given directly it is text the compiler already holds. +/// Given as a lambda it is usually a reference to a class the application declares its messages in once and names from +/// every validator, which is how a message is written wherever messages are shared rather than repeated - and the +/// lambda there computes nothing at all, so refusing it left the document without a single message it could have +/// stated. +/// +/// The lambda form exists so that a message can be built from the value being validated, and a message built while +/// the request runs has no text to write down - text put together from the value, or looked up in a resource by a +/// key resolved against the culture of the caller. What the compiler hands over is exactly the line between the two, +/// which is why it is what is asked rather than the shape of the expression. +/// +/// +public static class ValidationMessages +{ + /// + /// Reads the message an argument carries. + /// + /// The argument to read. + /// The semantic model of the tree the argument lives in. + /// The message, or when the argument carries no text the compiler holds. + public static string? Read(ExpressionSyntax? argument, SemanticModel semanticModel) => + argument is null ? null : TextOf(argument, semanticModel) ?? TextOf(BodyOf(argument), semanticModel); + + /// + /// Gets the expression a lambda returns. + /// + /// The argument to read. + /// The expression, or when the argument is not a lambda returning one. + /// + /// A lambda with a block body runs statements to arrive at its message, which is the definition of a message + /// there is no text for. + /// + static ExpressionSyntax? BodyOf(ExpressionSyntax argument) => argument switch + { + SimpleLambdaExpressionSyntax simple => simple.ExpressionBody, + ParenthesizedLambdaExpressionSyntax parenthesized => parenthesized.ExpressionBody, + _ => null + }; + + /// + /// Gets the text an expression holds, when the compiler holds it. + /// + /// The expression to read. + /// The semantic model of the tree the expression lives in. + /// The text, or when the expression is not text known while compiling. + /// + /// A reference to something declared as a constant is answered here just as a literal is, which is what makes the + /// shared message class work - the compiler substituted the text at the point of use before anything was asked of + /// it. A field holding text it was only assigned once is not that, and neither is a property returning one. + /// + static string? TextOf(ExpressionSyntax? expression, SemanticModel semanticModel) => + expression is null ? null : semanticModel.GetConstantValue(expression).Value as string; +} From 0c2f94d9a913a7142fd62e1b994e4c2b33a45589 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 22:03:25 +0200 Subject: [PATCH 16/21] Compare a property against another property the rule names A rule holding an end date to the start date it was sent with names that start date with a lambda, which is the same shape a chain names the property it is declared on. Only a constant was read, so every one of those rules looked like a comparison given nothing to compare against and was dropped - taking the layer a domain is really written in with it. A rule operand is a host expression, so the path is written as one. A rule held to a condition is stated as though it always holds, because a rule carries no condition of its own yet (Cratis/Screenplay#32). The report now carries the condition, so a reader can tell a rule that hardly ever applies from one that nearly always does. Co-Authored-By: Claude Opus 5 --- ..._comparing_one_property_against_another.cs | 60 ++++++++++++ ..._validator_holding_rules_to_a_condition.cs | 51 ++++++++++ ...rule_comparing_against_another_property.cs | 30 ++++++ .../Validation/ValidationChainReader.cs | 97 ++++++++----------- .../Analysis/Validation/ValidationOperands.cs | 91 +++++++++++++++++ .../Validation/ValidationSyntaxBuilder.cs | 10 ++ .../Screenplay/Model/ValidationRuleModel.cs | 5 + 7 files changed, 286 insertions(+), 58 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_comparing_one_property_against_another.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_holding_rules_to_a_condition.cs create mode 100644 Source/DotNET/Screenplay.Specs/for_ValidationSyntaxBuilder/when_building/a_rule_comparing_against_another_property.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Validation/ValidationOperands.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_comparing_one_property_against_another.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_comparing_one_property_against_another.cs new file mode 100644 index 000000000..1a1fa0409 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_comparing_one_property_against_another.cs @@ -0,0 +1,60 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A rule comparing one property of a command against another names the second one with a lambda, which is the same +/// shape a rule chain names the property it is declared on. Reading only constants left every such rule looking like a +/// comparison that had been given nothing to compare against, and dropped the whole of the layer a domain is really +/// written in. An operand that is genuinely worked out while the request runs is a different thing and stays unread. +/// +public class a_validator_comparing_one_property_against_another : Specification +{ + const string Source = """ + using System; + using Cratis.Arc.Commands; + using Cratis.Arc.Commands.ModelBound; + using FluentValidation; + + namespace Library.Lending.Reserving; + + [Command] + public record ReserveBook(DateOnly StartDate, DateOnly EndDate, DateOnly Deadline) + { + public void Handle() + { + } + } + + public class ReserveBookValidator : CommandValidator + { + public ReserveBookValidator() + { + RuleFor(_ => _.EndDate).GreaterThanOrEqualTo(_ => _.StartDate); + RuleFor(_ => _.Deadline).LessThanOrEqualTo(command => command.StartDate); + RuleFor(_ => _.StartDate).GreaterThan(DateOnly.FromDateTime(DateTime.UtcNow)); + } + } + """; + + ApplicationModelAnalysis _analysis; + IEnumerable _rules; + + void Establish() + { + _analysis = Analyzed.Source(Source); + _rules = _analysis.Slice().Commands.First().Validations; + } + + ValidationRuleModel Rule(string property) => _rules.First(_ => _.Property == property); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Feature/Slice/Slice.cs", Source)).ShouldBeEmpty(); + [Fact] void should_read_the_operand_as_the_property_it_names() => Rule("EndDate").Value.ShouldEqual(new PropertyPathSource("StartDate")); + [Fact] void should_read_it_whatever_the_lambda_calls_its_parameter() => Rule("Deadline").Value.ShouldEqual(new PropertyPathSource("StartDate")); + [Fact] void should_leave_an_operand_only_the_running_application_knows_unread() => Rule("StartDate").Value.ShouldBeNull(); + [Fact] void should_report_nothing_about_what_it_read() => _analysis.Diagnostics.ShouldBeEmpty(); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_holding_rules_to_a_condition.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_holding_rules_to_a_condition.cs new file mode 100644 index 000000000..6dd2e13b7 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_validator_holding_rules_to_a_condition.cs @@ -0,0 +1,51 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A rule carries no condition of its own, so a rule held to one is stated as though it always holds. That is a real +/// difference between the document and the application rather than a rule that could not be read, and a report saying +/// only that a call had no counterpart leaves a reader unable to tell which. Writing the condition into it is what +/// makes the difference something they can weigh. +/// +public class a_validator_holding_rules_to_a_condition : Specification +{ + const string Source = """ + using Cratis.Arc.Commands; + using Cratis.Arc.Commands.ModelBound; + using FluentValidation; + + namespace Library.Lending.Reserving; + + [Command] + public record ReserveBook(string Isbn, string Note, bool IsRush) + { + public void Handle() + { + } + } + + public class ReserveBookValidator : CommandValidator + { + public ReserveBookValidator() + { + RuleFor(_ => _.Isbn).NotEmpty().When(command => command.IsRush); + RuleFor(_ => _.Note).MaximumLength(50).Unless(command => command.IsRush); + } + } + """; + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(Source); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Feature/Slice/Slice.cs", Source)).ShouldBeEmpty(); + [Fact] void should_still_state_the_rules_the_condition_held() => _analysis.Slice().Commands.First().Validations.Select(_ => _.Property).ShouldContainOnly(["Isbn", "Note"]); + [Fact] void should_report_one_condition_for_each_chain() => _analysis.Diagnostics.Count.ShouldEqual(2); + [Fact] void should_report_them_as_rules_it_could_not_express() => _analysis.Diagnostics.Select(_ => _.Code).Distinct(StringComparer.Ordinal).ShouldContainOnly([ScreenplayDiagnosticCodes.UnmappableValidationRule]); + [Fact] void should_say_what_the_rules_were_held_to() => _analysis.Diagnostics.Any(_ => _.Message.Contains("When(command => command.IsRush)", StringComparison.Ordinal)).ShouldBeTrue(); + [Fact] void should_say_the_same_of_a_condition_written_the_other_way_round() => _analysis.Diagnostics.Any(_ => _.Message.Contains("Unless(command => command.IsRush)", StringComparison.Ordinal)).ShouldBeTrue(); +} diff --git a/Source/DotNET/Screenplay.Specs/for_ValidationSyntaxBuilder/when_building/a_rule_comparing_against_another_property.cs b/Source/DotNET/Screenplay.Specs/for_ValidationSyntaxBuilder/when_building/a_rule_comparing_against_another_property.cs new file mode 100644 index 000000000..ffba486ba --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ValidationSyntaxBuilder/when_building/a_rule_comparing_against_another_property.cs @@ -0,0 +1,30 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Model; +using Cratis.Screenplay.Syntax; +using ModelRuleKind = Cratis.Arc.Screenplay.Model.ValidationRuleKind; + +namespace Cratis.Arc.Screenplay.for_ValidationSyntaxBuilder.when_building; + +/// +/// A rule operand is a host expression, so a comparison against another property is written as the path naming it - +/// endDate >= startDate. Written as a literal instead it would read as a comparison against the word +/// startDate, which is a document stating something the application does not do. +/// +public class a_rule_comparing_against_another_property : given.a_validation_syntax_builder +{ + IEnumerable _rules; + + void Because() => _rules = _builder + .Build( + [new("EndDate", ModelRuleKind.GreaterThanOrEqual, new PropertyPathSource("StartDate"), null)], + "Library.Lending.Reserving") + .OfType() + .SelectMany(_ => _.Rules); + + [Fact] void should_state_the_rule() => _rules.Count().ShouldEqual(1); + [Fact] void should_write_the_operand_as_a_path() => _rules.Single().Value.ShouldBeOfExactType(); + [Fact] void should_write_it_the_way_a_property_is_named() => ((PathExpressionSyntax)_rules.Single().Value!).Path.ShouldEqual("startDate"); + [Fact] void should_report_nothing() => _diagnostics.All.ShouldBeEmpty(); +} diff --git a/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs b/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs index bde394236..15294baaa 100644 --- a/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Validation/ValidationChainReader.cs @@ -28,9 +28,16 @@ public class ValidationChainReader(ScreenplayDiagnostics diagnostics) public const string Length = "Length"; /// - /// The call constraining a value to look like an email address. + /// The call holding the rules before it to a condition. /// - public const string EmailAddress = "EmailAddress"; + public const string When = "When"; + + /// + /// The call holding the rules before it to a condition being false. + /// + public const string Unless = "Unless"; + + readonly ValidationOperands _operands = new(diagnostics); /// /// Reads one rule chain. @@ -70,59 +77,6 @@ public void Read( } } - /// - /// Reads the operand a rule compares against. - /// - /// The call declaring the rule. - /// The name of the rule builder. - /// The semantic model of the tree the call lives in. - /// Where the validator lives, for use in diagnostics. - /// The operand, or when the rule takes none. - object? OperandOf(InvocationExpressionSyntax call, string name, SemanticModel semanticModel, string location) => - string.Equals(name, EmailAddress, StringComparison.Ordinal) - ? ValidationRuleKinds.EmailPattern - : Constant(call, 0, semanticModel, location); - - /// - /// Reads the constant value of an argument. - /// - /// The call to read. - /// The position of the argument. - /// The semantic model of the tree the call lives in. - /// Where the validator lives, for use in diagnostics. - /// The value, or when the argument is not a constant. - /// - /// A rule comparing against a member of an enumeration is given the number behind the member, which would have - /// the document compare a concept declaring names against a number. Naming the member is what keeps the rule - /// readable against the concept it is written about. - /// - object? Constant(InvocationExpressionSyntax call, int index, SemanticModel semanticModel, string location) - { - var argument = InvocationChain.ArgumentOf(call, index); - if (argument is null) - { - return null; - } - - var value = semanticModel.GetConstantValue(argument).Value; - if (EnumConstants.EnumerationOf(argument, semanticModel) is not { } enumeration) - { - return value; - } - - if (EnumConstants.TryResolve(enumeration, value, out var member)) - { - return member; - } - - diagnostics.Warning( - ScreenplayDiagnosticCodes.UnnamedEnumerationValue, - $"'{enumeration.Name}' declares no member with the value '{value}', so it is written as that number rather than as a name the concept declares", - location); - - return value; - } - /// /// Reads one call of a rule chain. /// @@ -152,10 +106,17 @@ int ReadCall( return 0; } + if (string.Equals(name, When, StringComparison.Ordinal) || string.Equals(name, Unless, StringComparison.Ordinal)) + { + ReportCondition(call, name, property, location); + + return 0; + } + if (!forEach && string.Equals(name, Length, StringComparison.Ordinal) && call.ArgumentList.Arguments.Count == 2) { - rules.Add(new(property, ValidationRuleKind.Min, Constant(call, 0, semanticModel, location), null)); - rules.Add(new(property, ValidationRuleKind.Max, Constant(call, 1, semanticModel, location), null)); + rules.Add(new(property, ValidationRuleKind.Min, _operands.Constant(call, 0, semanticModel, location), null)); + rules.Add(new(property, ValidationRuleKind.Max, _operands.Constant(call, 1, semanticModel, location), null)); return 2; } @@ -170,11 +131,31 @@ int ReadCall( return 0; } - rules.Add(new(property, kind, OperandOf(call, name, semanticModel, location), null)); + rules.Add(new(property, kind, _operands.Read(call, name, semanticModel, location), null)); return 1; } + /// + /// Reports the condition the rules declared before it are held to. + /// + /// The call carrying the condition. + /// The name of the call. + /// The property the chain declares rules for. + /// Where the validator lives, for use in diagnostics. + /// + /// A rule that only holds sometimes is stated as though it always holds, because a rule carries no condition of + /// its own yet (Cratis/Screenplay#32). That is a real difference between the document and the application, so the + /// condition is written into the report rather than only its absence - a reader who can see what the rule was + /// held to can tell one that hardly ever applies from one that nearly always does, and a report naming only the + /// call says neither. + /// + void ReportCondition(InvocationExpressionSyntax call, string name, string property, string location) => + diagnostics.Warning( + ScreenplayDiagnosticCodes.UnmappableValidationRule, + $"The rules on '{property}' are held to '{name}{call.ArgumentList}', and a rule carries no condition, so they are stated as though nothing held them", + location); + /// /// Applies a message to every rule the call before it declared. /// diff --git a/Source/DotNET/Screenplay/Analysis/Validation/ValidationOperands.cs b/Source/DotNET/Screenplay/Analysis/Validation/ValidationOperands.cs new file mode 100644 index 000000000..77fd4a9d5 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Validation/ValidationOperands.cs @@ -0,0 +1,91 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Model; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace Cratis.Arc.Screenplay.Analysis.Validation; + +/// +/// Reads the value a validation rule compares against. +/// +/// The anything unmappable is reported to. +/// +/// A rule compares against one of two things, and both are written down in full. A value the compiler holds is the +/// obvious one. The other is another property of the same command - an end date is on or after the start date it was +/// sent with - which is a lambda naming that property and nothing more, and which a rule operand carries as the path +/// it names. +/// +public class ValidationOperands(ScreenplayDiagnostics diagnostics) +{ + /// + /// The call constraining a value to look like an email address. + /// + public const string EmailAddress = "EmailAddress"; + + /// + /// Reads the operand a rule compares against. + /// + /// The call declaring the rule. + /// The name of the rule builder. + /// The semantic model of the tree the call lives in. + /// Where the validator lives, for use in diagnostics. + /// The operand, or when the rule takes none that can be read. + /// + /// A lambda is asked about before a constant is, because a lambda holds no constant and the question would only + /// ever be answered with nothing. The other way round would drop every comparison written against a sibling + /// property as though the rule had been given no operand at all. + /// + public object? Read(InvocationExpressionSyntax call, string name, SemanticModel semanticModel, string location) + { + if (string.Equals(name, EmailAddress, StringComparison.Ordinal)) + { + return ValidationRuleKinds.EmailPattern; + } + + return LambdaPaths.Read(InvocationChain.ArgumentOf(call)) is { } path + ? new PropertyPathSource(path) + : Constant(call, 0, semanticModel, location); + } + + /// + /// Reads the constant value of an argument. + /// + /// The call to read. + /// The position of the argument. + /// The semantic model of the tree the call lives in. + /// Where the validator lives, for use in diagnostics. + /// The value, or when the argument is not a constant. + /// + /// A rule comparing against a member of an enumeration is given the number behind the member, which would have + /// the document compare a concept declaring names against a number. Naming the member is what keeps the rule + /// readable against the concept it is written about. + /// + public object? Constant(InvocationExpressionSyntax call, int index, SemanticModel semanticModel, string location) + { + var argument = InvocationChain.ArgumentOf(call, index); + if (argument is null) + { + return null; + } + + var value = semanticModel.GetConstantValue(argument).Value; + if (EnumConstants.EnumerationOf(argument, semanticModel) is not { } enumeration) + { + return value; + } + + if (EnumConstants.TryResolve(enumeration, value, out var member)) + { + return member; + } + + diagnostics.Warning( + ScreenplayDiagnosticCodes.UnnamedEnumerationValue, + $"'{enumeration.Name}' declares no member with the value '{value}', so it is written as that number rather than as a name the concept declares", + location); + + return value; + } +} diff --git a/Source/DotNET/Screenplay/Emission/Validation/ValidationSyntaxBuilder.cs b/Source/DotNET/Screenplay/Emission/Validation/ValidationSyntaxBuilder.cs index 5bd113de5..b59f0b8d4 100644 --- a/Source/DotNET/Screenplay/Emission/Validation/ValidationSyntaxBuilder.cs +++ b/Source/DotNET/Screenplay/Emission/Validation/ValidationSyntaxBuilder.cs @@ -86,6 +86,11 @@ public IEnumerable Build(IEnumerable rules, /// /// A pattern that is a bare word - email is the one the grammar knows - is written unquoted, because that /// is how the named patterns are referenced. Everything else is a string literal. + /// + /// A rule comparing against another property of the same command carries the path it names rather than a value, + /// and a rule operand is a host expression, so the path is written as one - endDate >= startDate reads + /// as what the developer wrote, where a literal would read as a comparison against the word. + /// /// ExpressionSyntax? ToOperand(ValidationRuleModel rule) { @@ -94,6 +99,11 @@ public IEnumerable Build(IEnumerable rules, return null; } + if (rule.Value is PropertyPathSource property) + { + return new PathExpressionSyntax(naming.ToPropertyPath(property.Path), SourceLocation.Start); + } + if (rule.Kind == ModelRuleKind.Matches && rule.Value is string pattern && ScreenplayIdentifier.IsBareIdentifier(pattern)) { return new PathExpressionSyntax(pattern, SourceLocation.Start); diff --git a/Source/DotNET/Screenplay/Model/ValidationRuleModel.cs b/Source/DotNET/Screenplay/Model/ValidationRuleModel.cs index 2a3561fa2..4de1095fc 100644 --- a/Source/DotNET/Screenplay/Model/ValidationRuleModel.cs +++ b/Source/DotNET/Screenplay/Model/ValidationRuleModel.cs @@ -12,5 +12,10 @@ namespace Cratis.Arc.Screenplay.Model; /// The message shown when the rule is broken, or for the default. /// /// A message starting with $strings. is a localization key and is emitted unquoted. +/// +/// An operand is a value the compiler held, except when the rule compares against another property of the same +/// command - an end date on or after the start date it was sent with - where it is a +/// naming that property and is written as the path rather than as text. +/// /// public record ValidationRuleModel(string Property, ValidationRuleKind Kind, object? Value, string? Message); From dce68b5e06761fa6ab9732b5e38a3eeeaaf70578 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 22:07:09 +0200 Subject: [PATCH 17/21] Declare a concept a record carries however far down it is carried A record is referred to by name and never declared, so nothing inside one was ever named on its own and every concept reached only through a child record or a read model stayed out of the document. Where such a concept is marked as personal data, the document then understated what the application holds about people - which is the opposite of what declaring concepts is for. The walk is cycle safe and ordered by name, so a record referring to itself ends and the same source always yields the same document. The shape of the record itself still has nowhere to go (Cratis/Screenplay#29), so a property carrying one now says so rather than leaving the reader to notice. Co-Authored-By: Claude Opus 5 --- .../concepts_carried_only_inside_a_record.cs | 75 ++++++++++++ .../Analysis/ApplicationModelAnalyzer.cs | 8 ++ .../Screenplay/Analysis/PropertyReader.cs | 2 +- .../Screenplay/Analysis/Types/CarriedTypes.cs | 80 +++++++++++++ .../Screenplay/Analysis/Types/TypeRegistry.cs | 111 ++++++++++-------- .../Analysis/Types/UnderlyingTypes.cs | 76 ++++++++++++ .../Screenplay/ScreenplayDiagnosticCodes.cs | 14 +++ 7 files changed, 313 insertions(+), 53 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/concepts_carried_only_inside_a_record.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Types/CarriedTypes.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Types/UnderlyingTypes.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/concepts_carried_only_inside_a_record.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/concepts_carried_only_inside_a_record.cs new file mode 100644 index 000000000..18f5127f8 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/concepts_carried_only_inside_a_record.cs @@ -0,0 +1,75 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A record is referred to by name and never declared, so nothing inside it is ever named on its own. Collecting only +/// the types written straight onto an artifact therefore lost every concept reached through one - and a concept marked +/// as personal data lost that way leaves a document understating what the application holds about people, which is the +/// one thing declaring concepts is most for. A concept can be declared wherever it was reached from, so it is. +/// +public class concepts_carried_only_inside_a_record : Specification +{ + const string Source = """ + using System; + using System.Collections.Generic; + using Cratis.Arc.Queries.ModelBound; + using Cratis.Chronicle.Compliance.GDPR; + using Cratis.Chronicle.Events; + using Cratis.Concepts; + + namespace Library.Authors.Registration; + + public record AuthorId(Guid Value) : ConceptAs(Value); + + [PII("The name of a person")] + public record FirstName(string Value) : ConceptAs(Value); + + public record MentorNote(string Value) : ConceptAs(Value); + + public record ShelfCode(string Value) : ConceptAs(Value); + + public enum ContactPreference { Email, Phone } + + public record PersonalDetails(FirstName First, ContactPreference Preference); + + public record Mentorship(MentorNote Note, Mentorship? Next); + + [EventType] + public record AuthorRegistered(AuthorId Id, PersonalDetails Details, IEnumerable Mentors); + + [ReadModel] + public record Author + { + public string Id { get; init; } = string.Empty; + + public ShelfCode Shelf { get; init; } = new(string.Empty); + + public static IEnumerable All() => []; + } + """; + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(Source); + + ConceptModel Concept(string name) => _analysis.Model.Concepts.First(_ => _.Name == name); + + IEnumerable Shapes => + _analysis.Diagnostics.Where(_ => _.Code == ScreenplayDiagnosticCodes.UndeclarableShape); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Feature/Slice/Slice.cs", Source)).ShouldBeEmpty(); + [Fact] void should_declare_a_concept_carried_inside_a_record() => Concept("FirstName").Primitive.ShouldEqual(ScreenplayPrimitive.String); + [Fact] void should_keep_what_that_concept_says_about_personal_data() => Concept("FirstName").IsPii.ShouldBeTrue(); + [Fact] void should_declare_an_enumeration_carried_inside_a_record() => Concept("ContactPreference").EnumValues.ShouldContainOnly(["Email", "Phone"]); + [Fact] void should_declare_a_concept_carried_inside_a_record_a_collection_holds() => Concept("MentorNote").Primitive.ShouldEqual(ScreenplayPrimitive.String); + [Fact] void should_declare_a_concept_only_a_read_model_carries() => Concept("ShelfCode").Primitive.ShouldEqual(ScreenplayPrimitive.String); + [Fact] void should_walk_a_record_referring_to_itself_only_once() => _analysis.Model.Concepts.Select(_ => _.Name).ShouldContainOnly(["AuthorId", "ContactPreference", "FirstName", "MentorNote", "ShelfCode"]); + [Fact] void should_say_the_shape_of_a_record_a_property_carries_is_not_declared() => Shapes.Count().ShouldEqual(2); + [Fact] void should_name_the_records_it_could_not_declare() => Shapes.All(_ => _.Message.Contains("PersonalDetails", StringComparison.Ordinal) || _.Message.Contains("Mentorship", StringComparison.Ordinal)).ShouldBeTrue(); + [Fact] void should_not_say_it_of_a_read_model_the_slice_describes() => Shapes.Any(_ => _.Message.Contains("Author'", StringComparison.Ordinal)).ShouldBeFalse(); +} diff --git a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs index ecf4b44b1..0a3731122 100644 --- a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs +++ b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs @@ -124,6 +124,14 @@ static void ReportTypesTheDocumentCannotName(ArtifactReaders readers, Screenplay $"'{type}' shares its simple name with a concept the document already declares, so what it is is described by the first one instead", location); } + + foreach (var shape in readers.Types.Shapes) + { + diagnostics.Information( + ScreenplayDiagnosticCodes.UndeclarableShape, + $"'{shape}' is a record an artifact carries, and there is no way to declare what a record holds, so the document names it without saying what is in it - the concepts it carries are declared, the shape itself is not", + location); + } } /// diff --git a/Source/DotNET/Screenplay/Analysis/PropertyReader.cs b/Source/DotNET/Screenplay/Analysis/PropertyReader.cs index 9d393d2a5..8e1f0b39e 100644 --- a/Source/DotNET/Screenplay/Analysis/PropertyReader.cs +++ b/Source/DotNET/Screenplay/Analysis/PropertyReader.cs @@ -39,7 +39,7 @@ public IEnumerable Read(ITypeSymbol type) types.MarkAsPii(property.Type); } - properties.Add(new(property.Name, types.Resolve(property.Type))); + properties.Add(new(property.Name, types.ResolveCarried(property.Type))); } return properties; diff --git a/Source/DotNET/Screenplay/Analysis/Types/CarriedTypes.cs b/Source/DotNET/Screenplay/Analysis/Types/CarriedTypes.cs new file mode 100644 index 000000000..23cfe8b47 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Types/CarriedTypes.cs @@ -0,0 +1,80 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Emission.Types; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.Analysis.Types; + +/// +/// Finds every type a record carries, however far down it is carried. +/// +/// +/// A concept is declared once at the top of a document and referred to by name, and a concept nothing refers to has no +/// reason to be declared - which is why only the types artifacts really name are collected. Reaching only the outermost +/// position took that too far: a name carried inside a line of an approved timesheet is referred to by the application +/// just as much as one written straight onto an event, and a document leaving it out understates what the application +/// holds. Where that value is marked as personal data, the document then understates something the reader is answerable +/// for, which is the opposite of what declaring concepts is for. +/// +public static class CarriedTypes +{ + /// + /// Determines whether a type is a record carrying values rather than being one. + /// + /// The type to check. + /// True when the type is a record whose members are worth walking. + /// + /// A record is what a value carrying several values is written as, and stopping at that is what keeps the walk + /// finite and about the application. Following every class a property mentions would descend into the framework + /// types the application merely touches, and a constructed generic is a type the document cannot name at all. + /// + public static bool IsRecord(ITypeSymbol type) => + type is INamedTypeSymbol { IsRecord: true, TypeArguments.Length: 0 } record && + !ScreenplayPrimitiveTypes.TryResolve(record.FullMetadataName(), out _) && + record.FindBase(WellKnownTypeNames.ConceptAs) is null; + + /// + /// Gets every type reachable through the members of a record. + /// + /// The type to walk. + /// The types, ordered so that the same source always reads the same way. + /// + /// A record referring to itself, directly or around a loop, is walked once - the second time round would say + /// nothing new and never end. What comes back is ordered by name rather than by the order the walk happened to + /// reach it, so that two records naming the same concept differently still leave the same document. + /// + public static IReadOnlyList Within(ITypeSymbol type) + { + var found = new Dictionary(StringComparer.Ordinal); + + if (IsRecord(type)) + { + Walk(type, found, new HashSet(StringComparer.Ordinal) { type.ToDisplayString() }); + } + + return [.. found.OrderBy(_ => _.Key, StringComparer.Ordinal).Select(_ => _.Value)]; + } + + /// + /// Collects the types the members of one record carry, descending into the records among them. + /// + /// The record to walk. + /// Everything found so far, keyed by the name it is told apart by. + /// The records already walked. + static void Walk(ITypeSymbol type, Dictionary found, HashSet walked) + { + foreach (var property in type.DeclaredProperties()) + { + var carried = UnderlyingTypes.Of(property.Type); + var name = carried.ToDisplayString(); + + found.TryAdd(name, carried); + + if (IsRecord(carried) && walked.Add(name)) + { + Walk(carried, found, walked); + } + } + } +} diff --git a/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs b/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs index d2ac2f324..fa9091ae7 100644 --- a/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs +++ b/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs @@ -22,12 +22,18 @@ public class TypeRegistry readonly HashSet _pii = new(StringComparer.Ordinal); readonly HashSet _unmappable = new(StringComparer.Ordinal); readonly HashSet _ambiguous = new(StringComparer.Ordinal); + readonly HashSet _shapes = new(StringComparer.Ordinal); /// /// Gets the full name of every type that had to be referred to by a name that does not say what it is. /// public IEnumerable Unmappable => _unmappable.Order(StringComparer.Ordinal); + /// + /// Gets the full name of every record a property carries whose shape no declaration can hold. + /// + public IEnumerable Shapes => _shapes.Order(StringComparer.Ordinal); + /// /// Gets the full name of every type whose simple name a concept was already declared under. /// @@ -57,7 +63,32 @@ public TypeReferenceModel Resolve(ITypeSymbol type) var optional = false; var collection = false; - return new(NameOf(Underlying(type, ref optional, ref collection)), collection, optional); + return new(NameOf(UnderlyingTypes.Of(type, ref optional, ref collection)), collection, optional); + } + + /// + /// Resolves the Screenplay type reference of a value an artifact carries. + /// + /// The type to resolve. + /// The . + /// + /// A property is where a record the document has no way to declare is really referred to - the line carrying it + /// names a shape nothing in the document introduces. That is asked here rather than everywhere a type is resolved, + /// because a query returning a read model refers to something the slice around it already describes, while a + /// property carrying a record refers to a shape stated nowhere at all. + /// + public TypeReferenceModel ResolveCarried(ITypeSymbol type) + { + var optional = false; + var collection = false; + var carried = UnderlyingTypes.Of(type, ref optional, ref collection); + + if (CarriedTypes.IsRecord(carried)) + { + _shapes.Add(carried.ToDisplayString()); + } + + return new(NameOf(carried), collection, optional); } /// @@ -70,13 +101,7 @@ public TypeReferenceModel Resolve(ITypeSymbol type) /// therefore strip the same wrappers - a collection of an optional concept says one thing about the value and /// three things about how many there are and whether it may be absent. /// - public void MarkAsPii(ITypeSymbol type) - { - var optional = false; - var collection = false; - - _pii.Add(Underlying(type, ref optional, ref collection).Name); - } + public void MarkAsPii(ITypeSymbol type) => _pii.Add(UnderlyingTypes.Of(type).Name); /// /// Records the validation rules a concept declares for itself. @@ -94,50 +119,6 @@ public void AddValidations(string conceptName, IEnumerable declared.AddRange(rules); } - /// - /// Strips everything a value is wrapped in that says how many there are or whether it may be absent. - /// - /// The type to strip. - /// Set when a wrapper said the value may be absent. - /// Set when the value is a collection of what is left. - /// The type of the value itself. - static ITypeSymbol Underlying(ITypeSymbol type, ref bool optional, ref bool collection) - { - var current = Unwrap(type, ref optional); - var element = CollectionElements.ElementOf(current); - if (element is null) - { - return current; - } - - collection = true; - - return Unwrap(element, ref optional); - } - - /// - /// Strips the wrappers that only say whether a value may be absent. - /// - /// The type to strip. - /// Set when a wrapper said the value may be absent. - /// The wrapped type. - static ITypeSymbol Unwrap(ITypeSymbol type, ref bool optional) - { - if (type is INamedTypeSymbol { OriginalDefinition.SpecialType: SpecialType.System_Nullable_T } nullable) - { - optional = true; - - return nullable.TypeArguments[0]; - } - - if (type.NullableAnnotation == NullableAnnotation.Annotated && type.IsReferenceType) - { - optional = true; - } - - return type; - } - /// /// Gets the values of an enumeration, in declaration order. /// @@ -172,11 +153,37 @@ string NameOf(ITypeSymbol type) return type.Name; } + RegisterWhatItCarries(type); ReportWhatTheNameLoses(type); return type.Name; } + /// + /// Registers every concept a record carries, however far down it is carried. + /// + /// The type being named. + /// + /// A record is referred to by name and never declared, so nothing inside it is ever named on its own - which left + /// every concept reached only through a line of a timesheet or a property of a read model out of the document + /// entirely. A concept can be declared wherever it was reached from, so it is, and the shape carrying it waits on + /// the language. + /// + void RegisterWhatItCarries(ITypeSymbol type) + { + foreach (var carried in CarriedTypes.Within(type)) + { + if (carried.TypeKind == TypeKind.Enum) + { + Register(carried, new(carried.Name, ScreenplayPrimitive.Enum, false, ValuesOf(carried), [])); + } + else if (carried.FindBase(WellKnownTypeNames.ConceptAs) is { } concept) + { + Register(carried, ToConcept(carried, concept.TypeArguments[0])); + } + } + } + /// /// Records a type whose simple name says less than the type does. /// diff --git a/Source/DotNET/Screenplay/Analysis/Types/UnderlyingTypes.cs b/Source/DotNET/Screenplay/Analysis/Types/UnderlyingTypes.cs new file mode 100644 index 000000000..ba2fc65fa --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Types/UnderlyingTypes.cs @@ -0,0 +1,76 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.Analysis.Types; + +/// +/// Strips everything a value is wrapped in that says how many there are or whether it may be absent. +/// +/// +/// The value a type carries and the wrappers around it answer different questions, and every reader that asks either +/// one has to strip the same wrappers to get the same answer. A collection of an optional concept says one thing about +/// the value and three things about how many there are and whether it may be absent - so a reader marking that value +/// as personal data and a reader naming it have to arrive at the same type, or the mark lands on a name nothing is +/// declared with. +/// +public static class UnderlyingTypes +{ + /// + /// Strips a type down to the value it carries. + /// + /// The type to strip. + /// Set when a wrapper said the value may be absent. + /// Set when the value is a collection of what is left. + /// The type of the value itself. + public static ITypeSymbol Of(ITypeSymbol type, ref bool optional, ref bool collection) + { + var current = Unwrap(type, ref optional); + var element = CollectionElements.ElementOf(current); + if (element is null) + { + return current; + } + + collection = true; + + return Unwrap(element, ref optional); + } + + /// + /// Strips a type down to the value it carries, when nothing is asked about the wrappers. + /// + /// The type to strip. + /// The type of the value itself. + public static ITypeSymbol Of(ITypeSymbol type) + { + var optional = false; + var collection = false; + + return Of(type, ref optional, ref collection); + } + + /// + /// Strips the wrappers that only say whether a value may be absent. + /// + /// The type to strip. + /// Set when a wrapper said the value may be absent. + /// The wrapped type. + static ITypeSymbol Unwrap(ITypeSymbol type, ref bool optional) + { + if (type is INamedTypeSymbol { OriginalDefinition.SpecialType: SpecialType.System_Nullable_T } nullable) + { + optional = true; + + return nullable.TypeArguments[0]; + } + + if (type.NullableAnnotation == NullableAnnotation.Annotated && type.IsReferenceType) + { + optional = true; + } + + return type; + } +} diff --git a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs index 06286a787..15addf88c 100644 --- a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs +++ b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs @@ -258,4 +258,18 @@ public static class ScreenplayDiagnosticCodes /// as it stands so the line that was rejected can be read. /// public const string DocumentDidNotCompile = "SP0034"; + + /// + /// A value an artifact carries is a record, whose shape no declaration in the language can hold. + /// + /// + /// A concept is one value with a name, and every concept the application refers to is declared. A record carrying + /// several values is a different thing: an event property written as days ApprovedDayLine[] names a shape + /// the document has no construct to introduce, so what that line holds is stated nowhere - including anything + /// within it the application marks as personal data. The concepts inside it are recovered and declared, because a + /// concept can be declared wherever it was reached from; the shape itself waits on the language + /// (Cratis/Screenplay#29). This is reported rather than left unsaid because a reader counting what the document + /// declares against what the application holds otherwise has no way of knowing where the difference went. + /// + public const string UndeclarableShape = "SP0035"; } From d3be9af98e362bb59352db9875077555a2954780 Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 22:14:37 +0200 Subject: [PATCH 18/21] Report a query a screen reads through that another slice declares An import naming something a slice does not declare says nothing, which holds for a component, a command and a package and was taken to hold for everything. It does not hold for a query another slice declares - a screen aggregating several read models is what an Event Modeling screen routinely is, and those bindings were dropped without a word. Writing them down is what the language cannot do: a binding names a query by the bare name its own slice declares it under, and an application declares All once per read model (Cratis/Screenplay#28). Where the import was written does say which slice was meant, so the screen, the query and that slice are reported instead. Co-Authored-By: Claude Opus 5 --- ...mporting_a_query_another_slice_declares.cs | 76 ++++++++++++ .../Analysis/ApplicationModelAnalyzer.cs | 5 +- .../Analysis/Screens/CrossSliceQueries.cs | 111 ++++++++++++++++++ .../Analysis/Screens/ModulePaths.cs | 56 +++++++++ .../Analysis/Screens/ScreenDataReader.cs | 19 ++- .../Analysis/Screens/ScreenImport.cs | 17 +++ .../Analysis/Screens/ScreenImportSite.cs | 17 +++ .../Analysis/Screens/ScreenImports.cs | 29 +++-- .../Analysis/Screens/ScreenReader.cs | 6 +- .../Screenplay/ScreenplayDiagnosticCodes.cs | 13 ++ 10 files changed, 333 insertions(+), 16 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_importing_a_query_another_slice_declares.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Screens/CrossSliceQueries.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Screens/ModulePaths.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Screens/ScreenImport.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Screens/ScreenImportSite.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_importing_a_query_another_slice_declares.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_importing_a_query_another_slice_declares.cs new file mode 100644 index 000000000..ecd6a38d7 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_importing_a_query_another_slice_declares.cs @@ -0,0 +1,76 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// A screen aggregating several read models is what an Event Modeling screen routinely is, so an import naming a query +/// another slice declares is a binding rather than the noise every other unmatched import is. It cannot be written +/// down - a binding names a query by the bare name its own slice declares it under, and an application declares the +/// same name once per read model - so the screen, the query and the slice declaring it are reported instead of the +/// binding disappearing without a word. +/// +public class a_screen_importing_a_query_another_slice_declares : Specification +{ + const string Listing = """ + using System.Collections.Generic; + using Cratis.Arc.Queries.ModelBound; + + namespace Library.Authors.Listing; + + [ReadModel] + public record Author + { + public string Id { get; init; } = string.Empty; + + public static IEnumerable All() => []; + } + """; + + const string Lending = """ + using System.Collections.Generic; + using Cratis.Arc.Queries.ModelBound; + + namespace Library.Lending.Loans; + + [ReadModel] + public record Loan + { + public string Id { get; init; } = string.Empty; + + public static IEnumerable All() => []; + } + """; + + const string Component = """ + import { All } from './Loans'; + import { All as AllAuthors } from '../../Authors/Listing/Listing'; + import { All as AllCatalogs } from '../../Catalogs/Listing/Listing'; + import { DataTable } from 'primereact/datatable'; + + export const LoanBoard = () => ; + """; + + static readonly DeclaredUserInterfaceFiles _files = DeclaredUserInterfaceFiles.Holding( + ("Library/Lending/Loans/LoanBoard.tsx", Component)); + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source( + _files, + ("Library/Authors/Listing/Listing.cs", Listing), + ("Library/Lending/Loans/Loans.cs", Lending)); + + ScreenplayDiagnostic Reported => + _analysis.Diagnostics.First(_ => _.Code == ScreenplayDiagnosticCodes.CrossSliceQueryBinding); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Authors/Listing/Listing.cs", Listing), ("Library/Lending/Loans/Loans.cs", Lending)).ShouldBeEmpty(); + [Fact] void should_still_bind_the_query_its_own_slice_declares() => _analysis.Model.Slices.First(_ => _.Namespace == "Library.Lending.Loans").Screens.Single().Data.Select(_ => _.Query).ShouldContainOnly(["All"]); + [Fact] void should_report_the_binding_it_could_not_write_down() => _analysis.Diagnostics.Count(_ => _.Code == ScreenplayDiagnosticCodes.CrossSliceQueryBinding).ShouldEqual(1); + [Fact] void should_name_the_screen_reading_through_it() => Reported.Message.Contains("'LoanBoard'", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_name_the_slice_declaring_it() => Reported.Message.Contains("'Library.Authors.Listing'", StringComparison.Ordinal).ShouldBeTrue(); + [Fact] void should_locate_it_against_the_slice_the_screen_belongs_to() => Reported.Location.ShouldEqual("Library.Lending.Loans"); + [Fact] void should_say_nothing_of_an_import_resolving_to_no_slice_at_all() => _analysis.Diagnostics.Any(_ => _.Message.Contains("Catalogs", StringComparison.Ordinal)).ShouldBeFalse(); +} diff --git a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs index 0a3731122..5511d39eb 100644 --- a/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs +++ b/Source/DotNET/Screenplay/Analysis/ApplicationModelAnalyzer.cs @@ -40,11 +40,13 @@ public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptio var diagnostics = new ScreenplayDiagnostics(); var catalog = ArtifactCatalog.From(compilation); var readers = ArtifactReaders.For(compilation, catalog, diagnostics); + var elsewhere = new CrossSliceQueries(); var screens = new ScreenReader( userInterfaceFiles, SourcePaths.For(compilation, catalog), new(diagnostics), - new(userInterfaceFiles, diagnostics)); + new(userInterfaceFiles, diagnostics, elsewhere), + elsewhere); var recovered = new RecoveredArtifacts(); var reader = new SliceReader(readers, diagnostics, screens, recovered); @@ -56,6 +58,7 @@ public ApplicationModelAnalysis Analyze(Compilation compilation, ScreenplayOptio ConceptValidations.Link(catalog, readers); readers.AggregateRoots.Report(diagnostics); + elsewhere.Report(diagnostics); ReportTypesTheDocumentCannotName(readers, diagnostics, compilation.AssemblyName); var imports = ExternalEvents.Resolve(compilation, slices, diagnostics); ReportNamespacesWithoutStructure(slices, diagnostics, options.SegmentsToSkip ?? 0); diff --git a/Source/DotNET/Screenplay/Analysis/Screens/CrossSliceQueries.cs b/Source/DotNET/Screenplay/Analysis/Screens/CrossSliceQueries.cs new file mode 100644 index 000000000..2f25b02aa --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Screens/CrossSliceQueries.cs @@ -0,0 +1,111 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Model; + +namespace Cratis.Arc.Screenplay.Analysis.Screens; + +/// +/// Reports the queries a screen reads through that a different slice of the application declares. +/// +/// +/// A screen binds to what a slice declares, and an import naming something the slice does not declare says nothing - +/// which is true of a component, a command and a package, and was taken to be true of everything. It is not true of a +/// query another slice declares: a screen aggregating several read models is what Event Modeling screens routinely +/// do, and dropping those left the document describing screens as reading almost nothing. +/// +/// It still cannot be written down. A data directive names a query by the bare name its slice declares it +/// under, and a real application declares All once per read model, so a name reaching across slices would say +/// which query only by accident (Cratis/Screenplay#28). Where the import was written does say which one, and that is +/// what is reported - which turns a silent drop into something a reader can act on today and a binding the moment a +/// reference can carry the slice. +/// +/// +public class CrossSliceQueries +{ + readonly Dictionary _owners = new(StringComparer.Ordinal); + readonly Dictionary> _declared = new(StringComparer.Ordinal); + readonly List _sites = []; + + /// + /// Declares the queries a slice holds and the directories its source lives in. + /// + /// The namespace of the slice. + /// The directories the source of the slice lives in. + /// The queries the slice declares. + /// + /// A directory claimed by a second slice keeps the first, which is the same answer given wherever a folder holds + /// the source of more than one slice, and is already reported as the ambiguity it is. + /// + public void Declare(string @namespace, IReadOnlyList directories, IReadOnlyCollection queries) + { + _declared[@namespace] = [.. queries.Select(_ => _.Name)]; + + foreach (var directory in directories) + { + _owners.TryAdd(directory, @namespace); + } + } + + /// + /// Records every name a screen imports from a module sitting alongside it. + /// + /// The namespace of the slice the screen belongs to. + /// The name of the screen. + /// The path of the file realizing the screen. + /// The names it imported. + /// + /// Every import is handed over rather than only the ones the slice declares no query under, because the name a + /// slice declares a query under is the same word twenty other slices declare one under - so a screen importing + /// All from another slice would be filtered out by the very name that makes it worth reporting. + /// + public void Record(string @namespace, string screen, string path, IEnumerable imports) => + _sites.AddRange(imports.Select(_ => new ScreenImportSite(@namespace, screen, ScreenFiles.DirectoryOf(path), _))); + + /// + /// Reports every import that named a query of another slice. + /// + /// The to report to. + /// + /// Reports are ordered by the slice, the screen and the query rather than by the order the screens happened to be + /// read, so the same source always reports the same way. + /// + public void Report(ScreenplayDiagnostics diagnostics) + { + var ordered = _sites + .OrderBy(_ => _.Namespace, StringComparer.Ordinal) + .ThenBy(_ => _.Screen, StringComparer.Ordinal) + .ThenBy(_ => _.Import.Name, StringComparer.Ordinal) + .ThenBy(_ => _.Import.Module, StringComparer.Ordinal); + + foreach (var site in ordered) + { + if (DeclaringSlice(site) is not { } owner) + { + continue; + } + + diagnostics.Warning( + ScreenplayDiagnosticCodes.CrossSliceQueryBinding, + $"The screen '{site.Screen}' reads through the query '{site.Import.Name}' that '{owner}' declares, and a binding names a query by the bare name the screen's own slice declares it under, so it was left out", + site.Namespace); + } + } + + /// + /// Gets the slice an import really names a query of, when it is not the one that wrote it. + /// + /// The import to resolve. + /// The namespace of the slice, or when the import names no query of another one. + string? DeclaringSlice(ScreenImportSite site) + { + if (ModulePaths.Resolve(site.Directory, site.Import.Module) is not { } resolved || + !_owners.TryGetValue(ScreenFiles.DirectoryOf(resolved), out var owner) || + string.Equals(owner, site.Namespace, StringComparison.Ordinal)) + { + return null; + } + + return _declared.TryGetValue(owner, out var queries) && queries.Contains(site.Import.Name) ? owner : null; + } +} diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ModulePaths.cs b/Source/DotNET/Screenplay/Analysis/Screens/ModulePaths.cs new file mode 100644 index 000000000..08f810c3d --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Screens/ModulePaths.cs @@ -0,0 +1,56 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.Analysis.Screens; + +/// +/// Works out which file a relative module specifier names. +/// +/// +/// A module specifier is a path written from the file doing the importing, and the folder it lands in is what says +/// which slice the imported name belongs to. Resolving it is therefore the only way a name shared by twenty slices can +/// be tied to one of them. Nothing here touches a disk - the extension is never appended and no file is looked for, +/// because a slice is recognized by its folder rather than by the file within it. +/// +public static class ModulePaths +{ + /// + /// Resolves the path a module specifier names. + /// + /// The directory of the file writing the import. + /// The module specifier. + /// The path without an extension, or when the specifier names nothing within. + /// + /// A specifier climbing past the root of the paths a compilation was built from names something outside the + /// application, which no slice of it can be. + /// + public static string? Resolve(string directory, string module) + { + var normalized = directory.Replace('\\', '/'); + var rooted = normalized.StartsWith('/'); + var segments = new List(normalized.Split('/', StringSplitOptions.RemoveEmptyEntries)); + + foreach (var segment in module.Replace('\\', '/').Split('/')) + { + if (segment.Length == 0 || string.Equals(segment, ".", StringComparison.Ordinal)) + { + continue; + } + + if (!string.Equals(segment, "..", StringComparison.Ordinal)) + { + segments.Add(segment); + continue; + } + + if (segments.Count == 0) + { + return null; + } + + segments.RemoveAt(segments.Count - 1); + } + + return segments.Count == 0 ? null : (rooted ? "/" : string.Empty) + string.Join('/', segments); + } +} diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs index d8df0be59..58e028052 100644 --- a/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs @@ -10,18 +10,25 @@ namespace Cratis.Arc.Screenplay.Analysis.Screens; /// /// The the text of a component is asked of. /// The anything not inferred is reported to. +/// The a name matching no query of the slice is held by. /// /// Arc generates a proxy per query and a component imports it by name, so an import is a name the model can be held -/// against rather than a reading of a user interface. A name matching a query the slice declares is a binding; a -/// name matching nothing is dropped, whatever the component does with it. Nothing about the binding is taken from -/// the component beyond the name - the type and the key come from the query, which is C#. +/// against rather than a reading of a user interface. A name matching a query the slice declares is a binding. +/// Nothing about the binding is taken from the component beyond the name - the type and the key come from the query, +/// which is C#. +/// +/// A name matching nothing the slice declares is handed on rather than dropped, because a query another slice +/// declares is a binding too and the only reason it cannot be written down is that a reference has no way to say +/// which slice it belongs to. What that name means depends on every slice the application has, so answering it waits +/// until they have all been read. +/// /// /// Every screen also reports what stays out. The rest of the declarative form is JSX structure, and the cost of /// guessing it wrong is a document that states something about the application that is not so - which is worse than /// a document that says less. Saying so per screen is what turns that limit from a silence into an answer. /// /// -public class ScreenDataReader(IUserInterfaceFiles files, ScreenplayDiagnostics diagnostics) +public class ScreenDataReader(IUserInterfaceFiles files, ScreenplayDiagnostics diagnostics, CrossSliceQueries elsewhere) { /// /// Reads the bindings of one screen. @@ -37,9 +44,11 @@ public IEnumerable Read( string path, IReadOnlyCollection queries) { - var imported = ScreenImports.In(files.Contents(path)); + var imports = ScreenImports.Statements(files.Contents(path)); + var imported = new HashSet(imports.Select(_ => _.Name), StringComparer.Ordinal); ReportUninferredStructure(@namespace, name); + elsewhere.Record(@namespace, name, path, imports); return [ diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenImport.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImport.cs new file mode 100644 index 000000000..a8ff7aa3b --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImport.cs @@ -0,0 +1,17 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.Analysis.Screens; + +/// +/// Represents a name a user interface file imports, together with where it imported it from. +/// +/// The name as the module exports it, rather than as the importing file renames it. +/// The module specifier, exactly as it was written. +/// +/// The name alone answers what a screen binds while the query is one its own slice declares. It stops answering the +/// moment the query is declared elsewhere, because a real application declares All once per read model and the +/// name says nothing about which of them was meant. Where the module was written is what says it, so the two travel +/// together. +/// +public record ScreenImport(string Name, string Module); diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenImportSite.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImportSite.cs new file mode 100644 index 000000000..56b9e820c --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImportSite.cs @@ -0,0 +1,17 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.Analysis.Screens; + +/// +/// Represents an import a screen wrote, together with where it was written from. +/// +/// The namespace of the slice the screen belongs to. +/// The name of the screen. +/// The directory the file realizing the screen sits in, which the module is relative to. +/// The name it imported and the module it came from. +/// +/// Where the import was written is kept because a module specifier means nothing on its own - it is a path from the +/// file that wrote it, and only that file says where it starts. +/// +public record ScreenImportSite(string Namespace, string Screen, string Directory, ScreenImport Import); diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs index 84f6718d8..fc269034c 100644 --- a/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenImports.cs @@ -27,29 +27,40 @@ public static partial class ScreenImports /// slice, so that is the shape every real binding has, and the shapes that are left out - a default import, a /// namespace import, an import of a package - cannot be tied to an exported name with any certainty. /// - public static IReadOnlyCollection In(string? text) + public static IReadOnlyCollection In(string? text) => + new HashSet(Statements(text).Select(_ => _.Name), StringComparer.Ordinal); + + /// + /// Gets the names a file imports from a module sitting alongside it, together with the module each came from. + /// + /// The text of the file, or when it could not be read. + /// The imports, in the order the file writes them. + /// + /// Source order is kept rather than sorted, because it is the order the file was written in and nothing else is + /// any more meaningful - and keeping it is what makes the same file always read the same way. + /// + public static IReadOnlyList Statements(string? text) { - var names = new HashSet(StringComparer.Ordinal); + var imports = new List(); + var seen = new HashSet(); if (string.IsNullOrWhiteSpace(text)) { - return names; + return imports; } foreach (var statement in StatementRegex().Matches(WithoutComments(text)).Cast()) { var clause = statement.Groups["clause"].Value; - if (!IsRelative(statement.Groups["module"].Value) || IsTypeOnly(clause)) + var module = statement.Groups["module"].Value; + if (!IsRelative(module) || IsTypeOnly(clause)) { continue; } - foreach (var name in NamedIn(clause)) - { - names.Add(name); - } + imports.AddRange(NamedIn(clause).Select(_ => new ScreenImport(_, module)).Where(seen.Add)); } - return names; + return imports; } /// diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenReader.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenReader.cs index 9528927ca..2a3e22cb5 100644 --- a/Source/DotNET/Screenplay/Analysis/Screens/ScreenReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenReader.cs @@ -13,6 +13,7 @@ namespace Cratis.Arc.Screenplay.Analysis.Screens; /// The rewriting the path of each file. /// The anything uncertain is reported to. /// The reading which of the slice's queries a screen binds. +/// The told what each slice declares and where it lives. /// /// Two things about a screen are recovered and no more: the file realizing it, which is what a reader opens, and the /// queries it binds, which are names the model already holds and can be held against what the slice really declares. @@ -23,7 +24,8 @@ public class ScreenReader( IUserInterfaceFiles files, SourcePaths paths, AmbiguousScreens ambiguity, - ScreenDataReader data) + ScreenDataReader data, + CrossSliceQueries elsewhere) { /// /// Reads the screens of a slice. @@ -38,6 +40,8 @@ public IEnumerable Read( IReadOnlyCollection queries) { var directories = SliceDirectories.Of(types); + elsewhere.Declare(@namespace, directories, queries); + if (directories.Count == 0) { return []; diff --git a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs index 15addf88c..c4e01f22f 100644 --- a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs +++ b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs @@ -272,4 +272,17 @@ public static class ScreenplayDiagnosticCodes /// declares against what the application holds otherwise has no way of knowing where the difference went. /// public const string UndeclarableShape = "SP0035"; + + /// + /// A screen reads through a query a different slice of the application declares. + /// + /// + /// A screen aggregating several read models is what an Event Modeling screen routinely is, and an import naming a + /// query the model really holds is a binding rather than the noise every other unmatched import is. A data + /// directive names a query by the bare name its slice declares it under, though, and an application declares + /// All once per read model, so writing one down would say which query only by accident + /// (Cratis/Screenplay#28). Naming the screen, the query and the slice declaring it is what a reader needs to see + /// the binding the document is missing, and what turns it into one the moment a reference can carry the slice. + /// + public const string CrossSliceQueryBinding = "SP0036"; } From 48354e843f71d0f3301ce7b2e4b34915cd8edbad Mon Sep 17 00:00:00 2001 From: woksin Date: Tue, 28 Jul 2026 22:16:50 +0200 Subject: [PATCH 19/21] Follow a component one hop into the view model beside it Where components are written against a view model, the component imports the view model and the view model imports the query - so reading the component alone found the name of a file and nothing about what the screen reads, and those screens ended up bound to nothing at all. The view model is written down rather than guessed at: the component names the module, it sits in the slice's own folder, and what it imports is read exactly the way the component's own imports are. One hop and no further, and only within that folder, because a chain across folders would be inferring an architecture rather than reading one. Co-Authored-By: Claude Opus 5 --- ...naming_its_queries_through_a_view_model.cs | 65 +++++++++++++++++ .../Analysis/Screens/ScreenDataReader.cs | 8 +- .../Analysis/Screens/ScreenFiles.cs | 2 +- .../Analysis/Screens/ViewModelImports.cs | 73 +++++++++++++++++++ 4 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_naming_its_queries_through_a_view_model.cs create mode 100644 Source/DotNET/Screenplay/Analysis/Screens/ViewModelImports.cs diff --git a/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_naming_its_queries_through_a_view_model.cs b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_naming_its_queries_through_a_view_model.cs new file mode 100644 index 000000000..75291f626 --- /dev/null +++ b/Source/DotNET/Screenplay.Specs/for_ApplicationModelAnalyzer/when_analyzing/a_screen_naming_its_queries_through_a_view_model.cs @@ -0,0 +1,65 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Analysis; + +namespace Cratis.Arc.Screenplay.for_ApplicationModelAnalyzer.when_analyzing; + +/// +/// Where components are written against a view model, the component imports the view model and the view model imports +/// the query, so reading the component alone finds a file name and nothing about what the screen reads. The view model +/// belongs to the same slice and sits in the same folder, so what it imports is what the screen reads - and it is +/// followed exactly one hop, because a chain across folders would be inferring an architecture rather than reading one. +/// +public class a_screen_naming_its_queries_through_a_view_model : Specification +{ + const string Source = """ + using System.Collections.Generic; + using Cratis.Arc.Queries.ModelBound; + + namespace Library.Lending.Loans; + + [ReadModel] + public record Loan + { + public string Id { get; init; } = string.Empty; + + public static IEnumerable All() => []; + + public static IEnumerable Overdue() => []; + } + """; + + const string Component = """ + import { useLoanBoard } from './LoanBoardViewModel'; + import { DataTable } from 'primereact/datatable'; + + export const LoanBoard = () => ; + """; + + const string ViewModel = """ + import { All } from './Loans'; + + export const useLoanBoard = () => All.use()[0]; + """; + + const string Elsewhere = """ + import { Overdue } from './Loans'; + + export const useOverdue = () => Overdue.use()[0]; + """; + + static readonly DeclaredUserInterfaceFiles _files = DeclaredUserInterfaceFiles.Holding( + ("Library/Lending/Loans/LoanBoard.tsx", Component), + ("Library/Lending/Loans/LoanBoardViewModel.ts", ViewModel), + ("Library/Lending/Loans/LoanBoardHelpers.ts", Elsewhere)); + + ApplicationModelAnalysis _analysis; + + void Establish() => _analysis = Analyzed.Source(_files, ("Library/Lending/Loans/Loans.cs", Source)); + + [Fact] void should_compile_the_source_it_analyzed() => Analyzed.ErrorsIn(("Library/Lending/Loans/Loans.cs", Source)).ShouldBeEmpty(); + [Fact] void should_bind_the_query_the_view_model_reads_through() => _analysis.Slice().Screens.Single().Data.Select(_ => _.Query).ShouldContainOnly(["All"]); + [Fact] void should_leave_a_module_that_is_no_view_model_unread() => _analysis.Slice().Screens.Single().Data.Any(_ => _.Query == "Overdue").ShouldBeFalse(); + [Fact] void should_report_only_what_no_screen_states() => _analysis.Diagnostics.Select(_ => _.Code).ShouldContainOnly([ScreenplayDiagnosticCodes.ScreenStructureNotInferred]); +} diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs index 58e028052..ed306daea 100644 --- a/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenDataReader.cs @@ -23,6 +23,11 @@ namespace Cratis.Arc.Screenplay.Analysis.Screens; /// until they have all been read. /// /// +/// What a view model beside the component imports counts as what the component imports, because where components are +/// written that way the query is named there and the component names only the view model - see +/// for how far that is followed. +/// +/// /// Every screen also reports what stays out. The rest of the declarative form is JSX structure, and the cost of /// guessing it wrong is a document that states something about the application that is not so - which is worse than /// a document that says less. Saying so per screen is what turns that limit from a silence into an answer. @@ -44,7 +49,8 @@ public IEnumerable Read( string path, IReadOnlyCollection queries) { - var imports = ScreenImports.Statements(files.Contents(path)); + var written = ScreenImports.Statements(files.Contents(path)); + var imports = written.Concat(ViewModelImports.Of(path, written, files)).Distinct().ToList(); var imported = new HashSet(imports.Select(_ => _.Name), StringComparer.Ordinal); ReportUninferredStructure(@namespace, name); diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ScreenFiles.cs b/Source/DotNET/Screenplay/Analysis/Screens/ScreenFiles.cs index dd759fdcf..9f5f9a6ba 100644 --- a/Source/DotNET/Screenplay/Analysis/Screens/ScreenFiles.cs +++ b/Source/DotNET/Screenplay/Analysis/Screens/ScreenFiles.cs @@ -67,7 +67,7 @@ public static string DirectoryOf(string path) /// /// The path to read. /// The file name. - static string FileNameOf(string path) + public static string FileNameOf(string path) { var normalized = Normalize(path); diff --git a/Source/DotNET/Screenplay/Analysis/Screens/ViewModelImports.cs b/Source/DotNET/Screenplay/Analysis/Screens/ViewModelImports.cs new file mode 100644 index 000000000..dad8dd9a6 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Screens/ViewModelImports.cs @@ -0,0 +1,73 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Cratis.Arc.Screenplay.Analysis.Screens; + +/// +/// Reads the imports a view model sitting beside a component writes on its behalf. +/// +/// +/// Where components are written against a view model, the component imports the view model and the view model imports +/// the query - so reading the component alone finds the name of a file and nothing about what the screen reads. That +/// is not a variation worth guessing at in general, but this one is written down: the component names the module, the +/// module sits in the slice's own folder, and what it imports is read exactly the way the component's own imports are. +/// +/// One hop and no further, and only into the folder the screen itself sits in. A view model belongs to the slice that +/// declares it, so following it says what that slice's own screen reads; following a chain of files across folders +/// would be inferring an application's architecture, which is a guess rather than a reading. +/// +/// +public static class ViewModelImports +{ + /// + /// The ending that marks a module as the view model of a component. + /// + public const string Suffix = "ViewModel"; + + static readonly string[] _extensions = [".ts", ".tsx"]; + + /// + /// Reads the imports the view models a screen names write. + /// + /// The path of the file realizing the screen. + /// What the screen itself imports. + /// The the text of a module is asked of. + /// The imports, in the order the view models write them. + public static IEnumerable Of(string path, IEnumerable imports, IUserInterfaceFiles files) + { + var directory = ScreenFiles.DirectoryOf(path); + + return imports + .Select(_ => Beside(directory, _.Module)) + .OfType() + .SelectMany(_ => ScreenImports.Statements(TextOf(_, files))); + } + + /// + /// Gets the path of a view model a module specifier names in a directory. + /// + /// The directory the screen sits in. + /// The module specifier. + /// The path without an extension, or when the module is not one. + static string? Beside(string directory, string module) => + ModulePaths.Resolve(directory, module) is { } resolved && + string.Equals(ScreenFiles.DirectoryOf(resolved), directory, StringComparison.Ordinal) && + ScreenFiles.FileNameOf(resolved).EndsWith(Suffix, StringComparison.Ordinal) + ? resolved + : null; + + /// + /// Gets the text of a module, trying the endings a module is written with. + /// + /// The path of the module, without an extension. + /// The the text is asked of. + /// The text, or when nothing was found to read. + /// + /// A module specifier carries no extension, and a view model holding no markup is written as one file ending and + /// one holding some as another, so both are asked for in turn. Nothing found is not an error - it is a module + /// that is not a view model of this slice, or a file that cannot be read, and either way the screen is recovered + /// from what it says itself. + /// + static string? TextOf(string path, IUserInterfaceFiles files) => + _extensions.Select(_ => files.Contents(path + _)).FirstOrDefault(_ => _ is not null); +} From a8422af0b36a10e4c8052e7f2a30fff7b5f1c12c Mon Sep 17 00:00:00 2001 From: woksin Date: Wed, 29 Jul 2026 01:05:31 +0200 Subject: [PATCH 20/21] Name the language issue four diagnostics wait on SP0013, SP0015, SP0016 and SP0020 each report something the application really declares that the document cannot yet hold. Which of them is a permanent gap and which is waiting on a specific change to the language was carried nowhere, so a reader had no way to tell a report that will always stand from one that disappears the moment an issue is resolved. Each reference is written into the explanation rather than appended to it, so it reads as the reason the gap exists, matching how SP0035 and SP0036 already cite theirs. Co-Authored-By: Claude Opus 5 --- .../Screenplay/ScreenplayDiagnosticCodes.cs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs index c4e01f22f..7a910e53e 100644 --- a/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs +++ b/Source/DotNET/Screenplay/ScreenplayDiagnosticCodes.cs @@ -74,6 +74,11 @@ public static class ScreenplayDiagnosticCodes /// /// A command handler yields the identifier of the event source it appends to, which Screenplay cannot express. /// + /// + /// A produces line names the event and says nothing about where it lands, so a handler returning the event + /// source alongside it is stating exactly what that line cannot carry (Cratis/Screenplay#33). The production is + /// written as it stands, because what the handler produces is right even while where it produces it is unsaid. + /// public const string UnmappableEventSourceIdResult = "SP0013"; /// @@ -84,11 +89,26 @@ public static class ScreenplayDiagnosticCodes /// /// A projection declares something the projection definition language has no counterpart for. /// + /// + /// Most of what this reports is a construct the language has no word for. One case is not: a slice holding a + /// second projection has that projection turned away because a slice declares at most one, which drops a read + /// model the application really builds (Cratis/Screenplay#30). Reporting it stays the right thing to do until a + /// slice can hold more than one - the projection is left out either way, and a reader counting read models + /// against the application otherwise has no way of seeing which one went missing. + /// public const string UnmappableProjectionConstruct = "SP0015"; /// /// A validator declares a rule that could not be expressed declaratively. /// + /// + /// A rule living in code, a chain rooted in something that does not name a property, and a message either put + /// together while the request runs or following no rule to attach it to are all read as far as they go and then + /// left out. A rule held to a When or an Unless is different: it is written down, but as though + /// nothing held it, because a rule carries no condition of its own (Cratis/Screenplay#32). That is a difference + /// between the document and the application rather than an omission from it, which is why the report names the + /// condition the rule was held to rather than only the call carrying it. + /// public const string UnmappableValidationRule = "SP0016"; /// @@ -109,6 +129,12 @@ public static class ScreenplayDiagnosticCodes /// /// A reducer folds events into a read model, which Screenplay has no counterpart for. /// + /// + /// A projection says what each event does to the read model it builds. A reducer says the same thing as code, + /// and the language has no construct to fold one value into another (Cratis/Screenplay#39). The events it + /// observes are read from its signatures and are real, so the document states which events reach the read model + /// while leaving unsaid what they do to it. + /// public const string ReducerWithoutCounterpart = "SP0020"; /// From 0569cb8cae1193942c2c8d1fd0f35f15905acc73 Mon Sep 17 00:00:00 2001 From: woksin Date: Wed, 29 Jul 2026 01:05:43 +0200 Subject: [PATCH 21/21] Separate collecting concepts from resolving a type TypeRegistry had grown past the size a file is meant to stay under by answering two questions at once: what name to write for a type, and what the application's concepts are. The second is a registry with rules of its own - what counts as a concept, which declaration wins when two share a name, how a personal-data mark and a validation rule arriving later still find the concept they belong to - and none of that has anything to do with unwrapping a collection or reporting what a name loses. ConceptRegistry now owns the concepts, and TypeRegistry keeps type resolution and the two things that fall out of writing a name. The enumeration and ConceptAs branches that were repeated between naming a type and walking what it carries collapse into one TryRegister, which is the same decision stated once. Behavior is unchanged: the document generated for a real application is byte-identical before and after, and all 973 specs pass. Co-Authored-By: Claude Opus 5 --- .../Analysis/Types/ConceptRegistry.cs | 163 ++++++++++++++++++ .../Screenplay/Analysis/Types/TypeRegistry.cs | 125 ++------------ 2 files changed, 180 insertions(+), 108 deletions(-) create mode 100644 Source/DotNET/Screenplay/Analysis/Types/ConceptRegistry.cs diff --git a/Source/DotNET/Screenplay/Analysis/Types/ConceptRegistry.cs b/Source/DotNET/Screenplay/Analysis/Types/ConceptRegistry.cs new file mode 100644 index 000000000..27f668e42 --- /dev/null +++ b/Source/DotNET/Screenplay/Analysis/Types/ConceptRegistry.cs @@ -0,0 +1,163 @@ +// Copyright (c) Cratis. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using Cratis.Arc.Screenplay.Emission.Types; +using Cratis.Arc.Screenplay.Model; +using Microsoft.CodeAnalysis; + +namespace Cratis.Arc.Screenplay.Analysis.Types; + +/// +/// Collects the concepts an application refers to, keeping one declaration per name. +/// +/// +/// A concept is declared once at the top of the document and referenced by its simple name from there on, so which +/// concepts a document declares has nothing to do with which ones the application defines and everything to do with +/// which ones were reached while a type was being resolved. They are gathered as they are encountered rather than +/// found up front, which keeps the document to what the application actually uses. +/// +/// What a concept says about itself arrives from more than one place and at different moments - the values of an +/// enumeration come from the type, the mark saying it carries personal data comes from the property referring to it, +/// and the rules it holds its own value to come from a validator read later still. Only the declaration is kept as +/// the concept; the rest is kept beside it and folded in when the concepts are read back, so that a mark or a rule +/// arriving after the concept was first seen still lands on it. +/// +/// +public class ConceptRegistry +{ + readonly Dictionary _concepts = new(StringComparer.Ordinal); + readonly Dictionary> _validations = new(StringComparer.Ordinal); + readonly HashSet _pii = new(StringComparer.Ordinal); + readonly HashSet _ambiguous = new(StringComparer.Ordinal); + + /// + /// Gets the full name of every type whose simple name a concept was already declared under. + /// + public IEnumerable Ambiguous => _ambiguous.Order(StringComparer.Ordinal); + + /// + /// Gets every concept referenced by the application, ordered by name. + /// + public IEnumerable Concepts => + [ + .. _concepts.Values + .Select(_ => _ with + { + IsPii = _.IsPii || _pii.Contains(_.Name), + Validations = _validations.TryGetValue(_.Name, out var rules) ? rules : [] + }) + .OrderBy(_ => _.Name, StringComparer.Ordinal) + ]; + + /// + /// Registers a type as a concept when it is one. + /// + /// The type to register. + /// True when the type is a concept and was registered. + /// + /// An enumeration and a type backed by ConceptAs are both one value with a name, which is what a concept + /// is, and both are therefore declared. Anything else is a type referred to by name and never declared, which is + /// what the false answer says. + /// + public bool TryRegister(ITypeSymbol type) + { + if (type.TypeKind == TypeKind.Enum) + { + Register(type, new(type.Name, ScreenplayPrimitive.Enum, false, ValuesOf(type), [])); + + return true; + } + + if (type.FindBase(WellKnownTypeNames.ConceptAs) is { } concept) + { + Register(type, ToConcept(type, concept.TypeArguments[0])); + + return true; + } + + return false; + } + + /// + /// Records that a value of a concept carries personally identifiable information. + /// + /// The type of the value. + /// + /// The concept a value is marked under has to be the one it is referenced under, or the mark lands on a name no + /// concept is declared with and the document says a value is not sensitive while the runtime encrypts it. Both + /// therefore strip the same wrappers - a collection of an optional concept says one thing about the value and + /// three things about how many there are and whether it may be absent. + /// + public void MarkAsPii(ITypeSymbol type) => _pii.Add(UnderlyingTypes.Of(type).Name); + + /// + /// Records the validation rules a concept declares for itself. + /// + /// The name of the concept. + /// The rules to record. + public void AddValidations(string conceptName, IEnumerable rules) + { + if (!_validations.TryGetValue(conceptName, out var declared)) + { + declared = []; + _validations[conceptName] = declared; + } + + declared.AddRange(rules); + } + + /// + /// Gets the values of an enumeration, in declaration order. + /// + /// The enumeration to read. + /// The value names. + static IEnumerable ValuesOf(ITypeSymbol type) => + [.. type.GetMembers().OfType().Where(_ => _.HasConstantValue).Select(_ => _.Name)]; + + /// + /// Builds the concept a type backed by ConceptAs declares. + /// + /// The concept type. + /// The type the concept is backed by. + /// The . + ConceptModel ToConcept(ITypeSymbol type, ITypeSymbol backing) + { + var pii = type.HasAttribute(WellKnownTypeNames.PiiAttribute); + + if (backing.TypeKind == TypeKind.Enum) + { + return new(type.Name, ScreenplayPrimitive.Enum, pii, ValuesOf(backing), []); + } + + var resolved = backing is INamedTypeSymbol named && ScreenplayPrimitiveTypes.TryResolve(named.FullMetadataName(), out var primitive) + ? primitive + : ScreenplayPrimitive.String; + + return new(type.Name, resolved, pii, [], []); + } + + /// + /// Registers a concept, keeping the first declaration of a given name. + /// + /// The type the concept was read from. + /// The concept to register. + /// + /// A concept is declared once at the top of the document and referenced by its simple name, so two types sharing + /// that name cannot both be described. Keeping the first is the only choice left, and saying so is what stops the + /// document from quietly claiming the second one is something it is not. + /// + void Register(ITypeSymbol type, ConceptModel concept) + { + if (!_concepts.TryGetValue(concept.Name, out var existing)) + { + _concepts[concept.Name] = concept; + + return; + } + + if (existing.Primitive != concept.Primitive || !existing.EnumValues.SequenceEqual(concept.EnumValues, StringComparer.Ordinal)) + { + _ambiguous.Add(type.ToDisplayString()); + } + } +} diff --git a/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs b/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs index fa9091ae7..bd864d5e7 100644 --- a/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs +++ b/Source/DotNET/Screenplay/Analysis/Types/TypeRegistry.cs @@ -11,17 +11,20 @@ namespace Cratis.Arc.Screenplay.Analysis.Types; /// Resolves the type of a property, a parameter or a return value, collecting the concepts encountered along the way. /// /// -/// A concept is declared once at the top of the document and referenced by name from there on, so every concept an -/// artifact refers to has to be registered while its type is resolved. Only concepts that are actually referenced -/// are declared, which keeps the document to what the application uses. +/// Resolving a type is answering two questions at once. The first is what to write - a single identifier, whether +/// there is one of it or many, and whether it may be absent. The second is what naming it commits the document to, +/// because every concept reached on the way has to be declared before it can be referenced, and every name that says +/// less than the type does has to be reported rather than passed off as a description. +/// +/// The first question is answered here. The second is split: what a name loses and which shapes no declaration can +/// hold are kept here because they are consequences of writing the name, while the concepts themselves are kept by a +/// , which decides what a concept is and what happens when two of them share a name. +/// /// public class TypeRegistry { - readonly Dictionary _concepts = new(StringComparer.Ordinal); - readonly Dictionary> _validations = new(StringComparer.Ordinal); - readonly HashSet _pii = new(StringComparer.Ordinal); + readonly ConceptRegistry _concepts = new(); readonly HashSet _unmappable = new(StringComparer.Ordinal); - readonly HashSet _ambiguous = new(StringComparer.Ordinal); readonly HashSet _shapes = new(StringComparer.Ordinal); /// @@ -37,21 +40,12 @@ public class TypeRegistry /// /// Gets the full name of every type whose simple name a concept was already declared under. /// - public IEnumerable Ambiguous => _ambiguous.Order(StringComparer.Ordinal); + public IEnumerable Ambiguous => _concepts.Ambiguous; /// /// Gets every concept referenced by the application, ordered by name. /// - public IEnumerable Concepts => - [ - .. _concepts.Values - .Select(_ => _ with - { - IsPii = _.IsPii || _pii.Contains(_.Name), - Validations = _validations.TryGetValue(_.Name, out var rules) ? rules : [] - }) - .OrderBy(_ => _.Name, StringComparer.Ordinal) - ]; + public IEnumerable Concepts => _concepts.Concepts; /// /// Resolves the Screenplay type reference a symbol corresponds to. @@ -95,37 +89,15 @@ public TypeReferenceModel ResolveCarried(ITypeSymbol type) /// Records that a value of a concept carries personally identifiable information. /// /// The type of the value. - /// - /// The concept a value is marked under has to be the one it is referenced under, or the mark lands on a name no - /// concept is declared with and the document says a value is not sensitive while the runtime encrypts it. Both - /// therefore strip the same wrappers - a collection of an optional concept says one thing about the value and - /// three things about how many there are and whether it may be absent. - /// - public void MarkAsPii(ITypeSymbol type) => _pii.Add(UnderlyingTypes.Of(type).Name); + public void MarkAsPii(ITypeSymbol type) => _concepts.MarkAsPii(type); /// /// Records the validation rules a concept declares for itself. /// /// The name of the concept. /// The rules to record. - public void AddValidations(string conceptName, IEnumerable rules) - { - if (!_validations.TryGetValue(conceptName, out var declared)) - { - declared = []; - _validations[conceptName] = declared; - } - - declared.AddRange(rules); - } - - /// - /// Gets the values of an enumeration, in declaration order. - /// - /// The enumeration to read. - /// The value names. - static IEnumerable ValuesOf(ITypeSymbol type) => - [.. type.GetMembers().OfType().Where(_ => _.HasConstantValue).Select(_ => _.Name)]; + public void AddValidations(string conceptName, IEnumerable rules) => + _concepts.AddValidations(conceptName, rules); /// /// Resolves the name a type is referenced by, registering it as a concept when it is one. @@ -139,17 +111,8 @@ string NameOf(ITypeSymbol type) return ScreenplayPrimitiveTypes.GetName(primitive); } - if (type.TypeKind == TypeKind.Enum) + if (_concepts.TryRegister(type)) { - Register(type, new(type.Name, ScreenplayPrimitive.Enum, false, ValuesOf(type), [])); - - return type.Name; - } - - if (type.FindBase(WellKnownTypeNames.ConceptAs) is { } concept) - { - Register(type, ToConcept(type, concept.TypeArguments[0])); - return type.Name; } @@ -173,14 +136,7 @@ void RegisterWhatItCarries(ITypeSymbol type) { foreach (var carried in CarriedTypes.Within(type)) { - if (carried.TypeKind == TypeKind.Enum) - { - Register(carried, new(carried.Name, ScreenplayPrimitive.Enum, false, ValuesOf(carried), [])); - } - else if (carried.FindBase(WellKnownTypeNames.ConceptAs) is { } concept) - { - Register(carried, ToConcept(carried, concept.TypeArguments[0])); - } + _concepts.TryRegister(carried); } } @@ -201,51 +157,4 @@ void ReportWhatTheNameLoses(ITypeSymbol type) _unmappable.Add(type.ToDisplayString()); } } - - /// - /// Builds the concept a type backed by ConceptAs declares. - /// - /// The concept type. - /// The type the concept is backed by. - /// The . - ConceptModel ToConcept(ITypeSymbol type, ITypeSymbol backing) - { - var pii = type.HasAttribute(WellKnownTypeNames.PiiAttribute); - - if (backing.TypeKind == TypeKind.Enum) - { - return new(type.Name, ScreenplayPrimitive.Enum, pii, ValuesOf(backing), []); - } - - var resolved = backing is INamedTypeSymbol named && ScreenplayPrimitiveTypes.TryResolve(named.FullMetadataName(), out var primitive) - ? primitive - : ScreenplayPrimitive.String; - - return new(type.Name, resolved, pii, [], []); - } - - /// - /// Registers a concept, keeping the first declaration of a given name. - /// - /// The type the concept was read from. - /// The concept to register. - /// - /// A concept is declared once at the top of the document and referenced by its simple name, so two types sharing - /// that name cannot both be described. Keeping the first is the only choice left, and saying so is what stops the - /// document from quietly claiming the second one is something it is not. - /// - void Register(ITypeSymbol type, ConceptModel concept) - { - if (!_concepts.TryGetValue(concept.Name, out var existing)) - { - _concepts[concept.Name] = concept; - - return; - } - - if (existing.Primitive != concept.Primitive || !existing.EnumValues.SequenceEqual(concept.EnumValues, StringComparer.Ordinal)) - { - _ambiguous.Add(type.ToDisplayString()); - } - } }