feat: Required dependencies with auto-registration (fixes #2164)

[thomhurst]

Summary

Makes [DependsOn<T>] dependencies required by default with auto-registration of missing dependencies.

This addresses issue #2164 where using category filters with dependencies in different categories would cause unexpected failures.

New Behavior

Required Dependencies (Default)

[DependsOn<Module1>]  // Required by default
public class Module2 : Module<string>
{
    protected override async Task<string?> ExecuteAsync(...)
    {
        // Safe to call - Module1 is guaranteed to be registered
        var result = await context.GetModule<Module1>();
        return result.Value;
    }
}

• Auto-registration: If Module1 is not explicitly registered, it will be automatically registered
• Handles transitive dependencies (if Module1 depends on Module0, both are auto-registered)
• Use GetModule<T>() to access - guaranteed to be present

Optional Dependencies

[DependsOn<Module1>(Optional = true)]  // Won't be auto-registered
public class Module2 : Module<string>
{
    protected override async Task<string?> ExecuteAsync(...)
    {
        // Use GetModuleIfRegistered for optional dependencies
        var module1 = context.GetModuleIfRegistered<Module1>();
        if (module1 != null)
        {
            var result = await module1;
            return result.Value;
        }
        return "Module1 not available";
    }
}

• Not auto-registered: Must be explicitly added if needed
• Use GetModuleIfRegistered<T>() to safely check if present
• Useful when using category filters where dependencies may be excluded

Key Changes

File	Change
DependsOnAttribute.cs	Optional defaults to false, added comprehensive XML docs
ModuleAutoRegistrar.cs	New - auto-registers missing required dependencies
PipelineBuilder.cs	Calls auto-registrar after user services are added
IgnoredModuleResultRegistrar.cs	Fixed type mismatch for value types
DependencyWaiter.cs	Updated error messages
ModuleDependencyValidator.cs	Updated error messages
RequiresDependencyAttribute.cs	Removed (no longer needed)
execution-and-dependencies.md	Updated docs with examples

Test Plan

• DependsOnTests - 11/11 passing
• CategoryFilterDependencyTests - 3/3 passing
• ModuleNotRegisteredExceptionTests - 3/3 passing
• SingleTypeParameterGetModuleTests - 6/6 passing
• All dependency tests - 128/128 passing
• All module tests - 163/163 passing

Fixes #2164

[thomhurst]

Summary

This PR makes [DependsOn] dependencies optional by default, adds [RequiresDependency] for strict dependencies, and fixes issue #2164 where filtered/skipped modules caused dependent modules to hang indefinitely.

Critical Issues

1. Breaking change without proper backwards compatibility handling

The PR changes the default behavior of [DependsOn] from required to optional (Optional = true). While it provides IgnoreIfNotRegistered for backwards compatibility (marked obsolete), this is a breaking API change that will silently change behavior for existing pipelines.

This could hide bugs in existing pipelines where dependencies are accidentally not registered. Users upgrading will not get compile-time or runtime warnings that their pipeline behavior has changed.

Recommendation: This should be a major version bump (v3.0), or the default should remain Optional = false with a new [DependsOnOptional] attribute instead.

2. Potential reflection-related AOT compatibility issues

The new CompletionSourceSetterCache uses reflection to access the internal CompletionSource property (line 218). This may cause issues with AOT/trimming if not properly annotated. While the code uses compiled expressions, it lacks [DynamicallyAccessedMembers] annotations to ensure the reflection targets are preserved during trimming.

Recommendation: Add appropriate annotations or avoid reflection by making CompletionSource accessible via an internal interface method.

Suggestions

1. Consider explicit API design - The new [RequiresDependency] vs [DependsOn] distinction may be confusing since DependsOn usually implies a requirement. A clearer API might be [DependsOn] (required by default) and [DependsOnOptional] (new).

2. Documentation for the fix - The fix for issue #2164 is well-implemented but could explain why it's necessary.

Verdict

⚠️ REQUEST CHANGES - Critical issue #1 (breaking change) needs to be addressed before merge.

[thomhurst]

Summary

This PR adds auto-registration for required dependencies and fixes issue 2164 where filtered modules caused dependent modules to hang.

Critical Issues

1. Misleading PR Title - The title says make dependencies optional by default but Optional = false on line 237 means they are REQUIRED by default. The actual change is adding auto-registration. Suggest renaming to: feat: Add auto-registration for required dependencies

2. Potential NullReferenceException - IgnoredModuleResultRegistrar.cs line 444 uses null-forgiving operator on GetProperty without validation. If CompletionSource property does not exist this will throw. Add null check.

3. Unused Code - TypeExtensions.MakeNullableIfValueType appears unused and should be removed.

Suggestions

• Error messages could explain WHY auto-registration failed
• Test duplication in CategoryFilterDependencyTests.cs
• Docs could warn that GetModule throws at runtime for optional deps if not registered

Verdict

REQUEST CHANGES

[thomhurst]

Summary

This PR makes [DependsOn] dependencies required by default with auto-registration, addressing issue #2164 where category filters with dependencies in different categories would cause failures.

Critical Issues

None found ✅

Suggestions

1. CompletionSourceSetterCache type safety (IgnoredModuleResultRegistrar.cs:466)

The expression-compiled setter in CompletionSourceSetterCache.CreateSetter accesses the CompletionSource property using reflection and makes assumptions about the generic type structure. While this appears to work correctly, consider adding validation after line 465 to catch potential type system issues early.

2. ModuleAutoRegistrar infinite loop protection

The AutoRegisterMissingDependencies method uses a do-while loop with an addedAny flag to handle transitive dependencies. While the logic looks correct, in a pathological case (e.g., a circular dependency that somehow bypasses validation), this could theoretically loop indefinitely. Consider adding a safety counter with a reasonable upper bound (e.g., 1000 iterations).

3. Error message clarity (DependencyWaiter.cs:360-368)

The new error message mentions 'could not be auto-registered' but does not explain why it could not be auto-registered. This could confuse users when a required dependency fails validation (e.g., abstract class, interface, generic type definition). Consider adding diagnostic info about why auto-registration failed (is abstract, is interface, is generic type definition, does not implement IModule, etc).

Verdict

✅ APPROVE - No critical issues. The implementation is solid with comprehensive test coverage (128/128 dependency tests passing). The suggestions above are optional improvements for robustness and diagnostics.

[thomhurst]

Summary

This PR makes [DependsOn] dependencies required by default with auto-registration of missing dependencies, addressing issue #2164.

Critical Issues

None found ✅

Previous review concerns have been addressed:

1. Breaking change concern: RESOLVED - Optional = false by default (required)
2. Misleading PR title: RESOLVED - Title updated to match behavior
3. CompletionSource reflection: Implementation is sound with proper type safety

Suggestions

1. Add safety counter to auto-registration loop to prevent infinite loops in pathological cases
2. Improve error messages to explain WHY auto-registration failed
3. Add more descriptive error message for CompletionSource property access failure

Previous Review Status

All three previous review comments have been addressed successfully.

Verdict

✅ APPROVE - No critical issues. Implementation correctly addresses #2164 with required dependencies by default, auto-registration of missing deps, comprehensive test coverage (128/128 passing), and clear documentation.