Add @AckType discriminated union extension generation#82
Conversation
|
To view this pull requests documentation preview, visit the following URL: Documentation is deployed and generated using docs.page. |
There was a problem hiding this comment.
Pull request overview
This PR adds code-generator support for @AckType schemas defined with Ack.discriminated(...). It produces a discriminated union extension type for the base schema and separate subtype extension types for each branch, with full parse/safeParse/copyWith/args-getter support.
Changes:
- Schema AST analysis (
schema_ast_analyzer.dart): adds_parseDiscriminatedSchemawhich validates and extracts discriminated schema structure (inline branch rejection, missing@AckType, non-object shapes, nullability, duplicate values, same-library constraint, and nested unions) - Model linking & code generation (
generator.dart,type_builder.dart,model_info.dart,model_analyzer.dart): replaces theMap<String, ClassElement2>subtype map with a string-keyedsubtypeNamesmap; adds_linkSchemaVariableDiscriminatedModelslinking pass andbuildDiscriminatedExtensionBaseto generate the base extension type - Test assets & integration tests: adds
DiscriminatedSchema.nullable()to the mock asset, a comprehensive success test plus 8 failure-mode tests, and example-folder golden-file assertions
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/ack_generator/lib/src/models/model_info.dart |
Replaces Map<String, ClassElement2> subtypes with Map<String, String> subtypeNames; adds discriminatedBaseClassName |
packages/ack_generator/lib/src/analyzer/schema_ast_analyzer.dart |
Adds _parseDiscriminatedSchema, alias propagation, sourceDeclaration/sourceLibraryUri in _ResolvedSchemaReference |
packages/ack_generator/lib/src/analyzer/model_analyzer.dart |
Updates class-based discriminator relationship building to use string-based subtype names |
packages/ack_generator/lib/src/generator.dart |
Adds _linkSchemaVariableDiscriminatedModels second pass; routes schema-variable discriminated bases to buildDiscriminatedExtensionBase |
packages/ack_generator/lib/src/builders/type_builder.dart |
Adds buildDiscriminatedExtensionBase; extends buildDiscriminatedSubtype with parse factories, args getter, and copyWith with fixed discriminator |
packages/ack_generator/lib/src/builders/schema_builder.dart |
Updates _buildDiscriminatedSchema to use string subtype names |
packages/ack_generator/test/test_utils/test_assets.dart |
Adds nullable() method to mock DiscriminatedSchema |
packages/ack_generator/test/integration/ack_type_discriminated_test.dart |
New integration tests for success and all 8 failure modes |
packages/ack_generator/test/integration/example_folder_build_test.dart |
Adds golden-file assertions for the new discriminated example |
example/lib/schema_types_discriminated.dart |
New example source file with cat/dog/pet discriminated schemas |
example/lib/schema_types_discriminated.g.dart |
Pre-committed generated output for the example file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| discriminatedContent, | ||
| contains('implements PetType, Map<String, Object?>'), | ||
| reason: | ||
| 'schema_types_discriminated.g.dart should include discriminated subtype extension types', |
There was a problem hiding this comment.
The reason text for the third expect call is identical to the one immediately before it ('schema_types_discriminated.g.dart should include discriminated subtype extension types'). Since it is testing a different property (the implements PetType, Map<String, Object?> clause), the reason should describe what is actually being checked to make test failures easier to diagnose.
| 'schema_types_discriminated.g.dart should include discriminated subtype extension types', | |
| 'schema_types_discriminated.g.dart discriminated subtypes should implement PetType and Map<String, Object?>', |
|
|
||
| /// Default representation type for object schemas | ||
| const String _kMapType = 'Map<String, Object?>'; | ||
|
|
There was a problem hiding this comment.
An extra blank line was introduced between the _log declaration and the typedef _SchemaReference declaration. This results in two consecutive blank lines, which is inconsistent with the rest of the file's style.
| final resolvedSubtypeNames = <String, String>{}; | ||
| for (final entry in subtypeNames.entries) { | ||
| final branchModel = allModels.firstWhere( | ||
| (m) => m.schemaClassName == entry.value, |
There was a problem hiding this comment.
The firstWhere call in buildDiscriminatedExtensionBase has no orElse guard. If allModels somehow does not contain a model with the expected schemaClassName (e.g., due to an ordering issue or a bug in model linking), it will throw a bare StateError with no context about which base or branch caused the failure. Adding an orElse that throws an InvalidGenerationSourceError (or StateError) with a descriptive message would make failures much easier to diagnose.
| (m) => m.schemaClassName == entry.value, | |
| (m) => m.schemaClassName == entry.value, | |
| orElse: () => throw StateError( | |
| 'Failed to resolve discriminated subtype "${entry.value}" ' | |
| '(discriminator "${entry.key}") for base ' | |
| '"${model.schemaClassName}" while building ' | |
| 'extension type "$typeName".', | |
| ), |
| /// Field name for discrimination (only for base classes) | ||
| final String? discriminatorKey; | ||
|
|
||
| /// This class's discriminator value (only for subtypes) | ||
| final String? discriminatorValue; | ||
|
|
||
| /// Map of discriminator values to class elements (only for base classes) | ||
| final Map<String, ClassElement2>? subtypes; | ||
| /// Map of discriminator values to subtype identifiers (only for base classes). | ||
| /// For @AckModel: discriminator value → className (e.g., 'cat' → 'Cat') | ||
| /// For @AckType: discriminator value → schemaClassName (e.g., 'cat' → 'catSchema') | ||
| final Map<String, String>? subtypeNames; | ||
|
|
||
| /// Parent discriminated base class name for subtypes. | ||
| final String? discriminatedBaseClassName; | ||
|
|
||
| /// Computed property: Whether this class is a discriminated base class (has discriminatedKey) | ||
| bool get isDiscriminatedBase => discriminatorKey != null; |
There was a problem hiding this comment.
After _linkSchemaVariableDiscriminatedModels, branch models (schema-variable discriminated subtypes) have both discriminatorKey and discriminatorValue set, which makes isDiscriminatedBase return true for them, even though they are not discriminated bases. This semantic inconsistency means the isDiscriminatedBase property is now an unreliable signal when isFromSchemaVariable is true.
For example, if code checks model.isDiscriminatedBase without also verifying model.subtypeNames != null, it would incorrectly treat a branch model as a base. The current generator code happens to work because buildDiscriminatedExtensionBase returns null when subtypeNames is null, but this is a fragile invariant.
Consider updating the comment on discriminatorKey to clarify this dual-use, or introducing a separate computed property (e.g., isDiscriminatedSchemaVariableBase) that checks both isDiscriminatedBase && subtypeNames != null.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Builds an extension type for the given model | ||
| /// | ||
| /// Returns null if the model should not generate an extension type: | ||
| /// - Discriminated base classes (use sealed classes instead) | ||
| /// - Discriminated base classes (generated separately via | ||
| /// [buildDiscriminatedExtensionBase] or [buildSealedClass]) | ||
| /// - Nullable schema variables (representation is non-nullable) | ||
| ExtensionType? buildExtensionType( |
There was a problem hiding this comment.
The doc comment for buildExtensionType was updated to mention the two separate functions buildDiscriminatedExtensionBase and buildSealedClass, but the guard inside buildExtensionType still uses model.isDiscriminatedBase (i.e. discriminatorKey != null), which is now also true for schema-variable subtypes after the linking pass. Using model.isDiscriminatedBaseDefinition (or additionally checking !model.isDiscriminatedSubtype) would be more precise and prevent silent null returns for subtypes if the routing logic ever changes.
| ModelInfo model, | ||
| List<ModelInfo> allModels, | ||
| ) { | ||
| if (!model.isDiscriminatedBase) { |
There was a problem hiding this comment.
The guard in buildDiscriminatedExtensionBase uses model.isDiscriminatedBase (which now means discriminatorKey != null) rather than model.isDiscriminatedBaseDefinition (which means discriminatorKey != null && subtypeNames != null). After the linking pass, schema-variable subtypes also have discriminatorKey != null (i.e., isDiscriminatedBase == true), so isDiscriminatedBase no longer unambiguously identifies declared discriminated bases. Using isDiscriminatedBaseDefinition here would be more precise and consistent with the intent — the subtypeNames null-check at line 213 currently compensates, but the guard at line 205 is misleading and inconsistent with the rest of the updated codebase which uses isDiscriminatedBaseDefinition.
| if (!model.isDiscriminatedBase) { | |
| if (!model.isDiscriminatedBaseDefinition) { |
| /// Computed property: Whether this model declares subtype mappings. | ||
| bool get hasDiscriminatedSubtypeMappings => subtypeNames != null; | ||
|
|
There was a problem hiding this comment.
The hasDiscriminatedSubtypeMappings computed property is declared but never used anywhere in the codebase. It is functionally equivalent to isDiscriminatedBaseDefinition (both rely on subtypeNames != null). Consider removing this unused property to keep the API surface clean.
| /// Computed property: Whether this model declares subtype mappings. | |
| bool get hasDiscriminatedSubtypeMappings => subtypeNames != null; |
This adds generator support for @AckType schemas defined with Ack.discriminated(...), including base/subtype linkage and generated extension types for both the base union and each branch. It updates discriminator metadata from element-backed subtype maps to string-based subtype names so class-based and schema-variable flows can share handling. Schema AST analysis now supports discriminated schemas and alias references with validation for branch references, @AckType usage, object shape, nullability, duplicate discriminator values, and same-library constraints. It also adds integration coverage for success and failure paths, updates test assets, and includes an example discriminated schema with generated output checks.