Produce analyzer errors for invalid generic arguments in converters and validators

Closes #103

CliFx.Analyzers.Tests/OptionMustHaveValidConverterAnalyzerSpecs.cs

@@ -33,7 +33,31 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_the_specified_option_converter_derives_from_BindingConverter()
+    public void Analyzer_reports_an_error_if_the_specified_option_converter_does_not_derive_from_a_compatible_BindingConverter()
+    {
+        // Arrange
+        // language=cs
+        const string code = @"
+public class MyConverter : BindingConverter<int>
+{
+    public override int Convert(string rawValue) => 42;
+}
+
+[Command]
+public class MyCommand : ICommand
+{
+    [CommandOption(""foo"", Converter = typeof(MyConverter))]
+    public string Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+        // Act & assert
+        Analyzer.Should().ProduceDiagnostics(code);
+    }
+
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_the_specified_option_converter_derives_from_a_compatible_BindingConverter()
     {
         // Arrange
         // language=cs

CliFx.Analyzers.Tests/OptionMustHaveValidValidatorsAnalyzerSpecs.cs

@@ -33,7 +33,31 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_all_specified_option_validators_derive_from_BindingValidator()
+    public void Analyzer_reports_an_error_if_one_of_the_specified_option_validators_does_not_derive_from_a_compatible_BindingValidator()
+    {
+        // Arrange
+        // language=cs
+        const string code = @"
+public class MyValidator : BindingValidator<int>
+{
+    public override BindingValidationError Validate(int value) => Ok();
+}
+
+[Command]
+public class MyCommand : ICommand
+{
+    [CommandOption(""foo"", Validators = new[] {typeof(MyValidator)})]
+    public string Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+        // Act & assert
+        Analyzer.Should().ProduceDiagnostics(code);
+    }
+
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_each_specified_option_validator_derives_from_a_compatible_BindingValidator()
     {
         // Arrange
         // language=cs

CliFx.Analyzers.Tests/ParameterMustHaveValidConverterAnalyzerSpecs.cs

@@ -33,7 +33,32 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_the_specified_parameter_converter_derives_from_BindingConverter()
+    public void Analyzer_reports_an_error_if_the_specified_parameter_converter_does_not_derive_from_a_compatible_BindingConverter()
+    {
+        // Arrange
+        // language=cs
+        const string code = @"
+public class MyConverter : BindingConverter<int>
+{
+    public override int Convert(string rawValue) => 42;
+}
+
+[Command]
+public class MyCommand : ICommand
+{
+    [CommandParameter(0, Converter = typeof(MyConverter))]
+    public string Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+
+        // Act & assert
+        Analyzer.Should().ProduceDiagnostics(code);
+    }
+
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_the_specified_parameter_converter_derives_from_a_compatible_BindingConverter()
     {
         // Arrange
         // language=cs

CliFx.Analyzers.Tests/ParameterMustHaveValidValidatorsAnalyzerSpecs.cs

@@ -33,7 +33,31 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_all_specified_parameter_validators_derive_from_BindingValidator()
+    public void Analyzer_reports_an_error_if_one_of_the_specified_parameter_validators_does_not_derive_from_a_compatible_BindingValidator()
+    {
+        // Arrange
+        // language=cs
+        const string code = @"
+public class MyValidator : BindingValidator<int>
+{
+    public override BindingValidationError Validate(int value) => Ok();
+}
+
+[Command]
+public class MyCommand : ICommand
+{
+    [CommandParameter(0, Validators = new[] {typeof(MyValidator)})]
+    public string Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+        // Act & assert
+        Analyzer.Should().ProduceDiagnostics(code);
+    }
+
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_each_specified_parameter_validator_derives_from_a_compatible_BindingValidator()
     {
         // Arrange
         // language=cs

CliFx.Analyzers.Tests/Utils/AnalyzerAssertions.cs

@@ -57,7 +57,7 @@ internal class AnalyzerAssertions : ReferenceTypeAssertions<DiagnosticAnalyzer,
         // Compile the code to IL
         var compilation = CSharpCompilation.Create(
             "CliFxTests_DynamicAssembly_" + Guid.NewGuid(),
-            new[] {ast},
+            new[] { ast },
             ReferenceAssemblies.Net50
                 .Append(MetadataReference.CreateFromFile(typeof(ICommand).Assembly.Location)),
             // DLL to avoid having to define the Main() method
@@ -101,11 +101,11 @@ internal class AnalyzerAssertions : ReferenceTypeAssertions<DiagnosticAnalyzer,
         var expectedDiagnosticIds = expectedDiagnostics.Select(d => d.Id).Distinct().ToArray();
         var producedDiagnosticIds = producedDiagnostics.Select(d => d.Id).Distinct().ToArray();
 
-        var result =
+        var isSuccessfulAssertion =
             expectedDiagnosticIds.Intersect(producedDiagnosticIds).Count() ==
             expectedDiagnosticIds.Length;
 
-        Execute.Assertion.ForCondition(result).FailWith(() =>
+        Execute.Assertion.ForCondition(isSuccessfulAssertion).FailWith(() =>
         {
             var buffer = new StringBuilder();
 
@@ -125,11 +125,18 @@ internal class AnalyzerAssertions : ReferenceTypeAssertions<DiagnosticAnalyzer,
 
             buffer.AppendLine("Produced diagnostics:");
 
+            if (producedDiagnostics.Any())
+            {
                 foreach (var producedDiagnostic in producedDiagnostics)
                 {
                     buffer.Append("  - ");
                     buffer.Append(producedDiagnostic);
                 }
+            }
+            else
+            {
+                buffer.AppendLine("  < none >");
+            }
 
             return new FailReason(buffer.ToString());
         });
@@ -138,10 +145,9 @@ internal class AnalyzerAssertions : ReferenceTypeAssertions<DiagnosticAnalyzer,
     public void NotProduceDiagnostics(string sourceCode)
     {
         var producedDiagnostics = GetProducedDiagnostics(sourceCode);
+        var isSuccessfulAssertion = !producedDiagnostics.Any();
 
-        var result = !producedDiagnostics.Any();
+        Execute.Assertion.ForCondition(isSuccessfulAssertion).FailWith(() =>
-
-        Execute.Assertion.ForCondition(result).FailWith(() =>
         {
             var buffer = new StringBuilder();
 

CliFx.Analyzers/CommandMustBeAnnotatedAnalyzer.cs

@@ -30,12 +30,12 @@ public class CommandMustBeAnnotatedAnalyzer : AnalyzerBase
 
         var implementsCommandInterface = type
             .AllInterfaces
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
+            .Any(i => i.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
 
         var hasCommandAttribute = type
             .GetAttributes()
             .Select(a => a.AttributeClass)
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxCommandAttribute));
+            .Any(c => c.DisplayNameMatches(SymbolNames.CliFxCommandAttribute));
 
         // If the interface is implemented, but the attribute is missing,
         // then it's very likely a user error.

CliFx.Analyzers/CommandMustImplementInterfaceAnalyzer.cs

@@ -25,11 +25,11 @@ public class CommandMustImplementInterfaceAnalyzer : AnalyzerBase
         var hasCommandAttribute = type
             .GetAttributes()
             .Select(a => a.AttributeClass)
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxCommandAttribute));
+            .Any(c => c.DisplayNameMatches(SymbolNames.CliFxCommandAttribute));
 
         var implementsCommandInterface = type
             .AllInterfaces
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
+            .Any(i => i.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
 
         // If the attribute is present, but the interface is not implemented,
         // it's very likely a user error.

CliFx.Analyzers/ObjectModel/CommandOptionSymbol.cs

@@ -1,7 +1,7 @@
 using System.Collections.Generic;
-using Microsoft.CodeAnalysis;
 using System.Linq;
 using CliFx.Analyzers.Utils.Extensions;
+using Microsoft.CodeAnalysis;
 
 namespace CliFx.Analyzers.ObjectModel;
 

CliFx.Analyzers/ObjectModel/SymbolNames.cs

@@ -7,8 +7,6 @@ internal static class SymbolNames
     public const string CliFxCommandParameterAttribute = "CliFx.Attributes.CommandParameterAttribute";
     public const string CliFxCommandOptionAttribute = "CliFx.Attributes.CommandOptionAttribute";
     public const string CliFxConsoleInterface = "CliFx.Infrastructure.IConsole";
-    public const string CliFxBindingConverterInterface = "CliFx.Extensibility.IBindingConverter";
     public const string CliFxBindingConverterClass = "CliFx.Extensibility.BindingConverter<T>";
-    public const string CliFxBindingValidatorInterface = "CliFx.Extensibility.IBindingValidator";
     public const string CliFxBindingValidatorClass = "CliFx.Extensibility.BindingValidator<T>";
 }

CliFx.Analyzers/OptionMustBeInsideCommandAnalyzer.cs

@@ -34,7 +34,7 @@ public class OptionMustBeInsideCommandAnalyzer : AnalyzerBase
         var isInsideCommand = property
             .ContainingType
             .AllInterfaces
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
+            .Any(i => i.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
 
         if (!isInsideCommand)
         {

CliFx.Analyzers/OptionMustHaveValidConverterAnalyzer.cs

@@ -13,7 +13,7 @@ public class OptionMustHaveValidConverterAnalyzer : AnalyzerBase
     public OptionMustHaveValidConverterAnalyzer()
         : base(
             $"Option converters must derive from `{SymbolNames.CliFxBindingConverterClass}`",
-            $"Converter specified for this option must derive from `{SymbolNames.CliFxBindingConverterClass}`.")
+            $"Converter specified for this option must derive from a compatible `{SymbolNames.CliFxBindingConverterClass}`.")
     {
     }
 
@@ -29,13 +29,15 @@ public class OptionMustHaveValidConverterAnalyzer : AnalyzerBase
         if (option.ConverterType is null)
             return;
 
-        // We check against an internal interface because checking against a generic class is a pain
+        var converterValueType = option
-        var converterImplementsInterface = option
             .ConverterType
-            .AllInterfaces
+            .GetBaseTypes()
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxBindingConverterInterface));
+            .FirstOrDefault(t => t.ConstructedFrom.DisplayNameMatches(SymbolNames.CliFxBindingConverterClass))?
+            .TypeArguments
+            .FirstOrDefault();
 
-        if (!converterImplementsInterface)
+        // Value returned by the converter must be assignable to the property type
+        if (converterValueType is null || !property.Type.IsAssignableFrom(converterValueType))
         {
             context.ReportDiagnostic(CreateDiagnostic(propertyDeclaration.GetLocation()));
         }

CliFx.Analyzers/OptionMustHaveValidValidatorsAnalyzer.cs

@@ -13,7 +13,7 @@ public class OptionMustHaveValidValidatorsAnalyzer : AnalyzerBase
     public OptionMustHaveValidValidatorsAnalyzer()
         : base(
             $"Option validators must derive from `{SymbolNames.CliFxBindingValidatorClass}`",
-            $"All validators specified for this option must derive from `{SymbolNames.CliFxBindingValidatorClass}`.")
+            $"Each validator specified for this option must derive from a compatible `{SymbolNames.CliFxBindingValidatorClass}`.")
     {
     }
 
@@ -28,12 +28,14 @@ public class OptionMustHaveValidValidatorsAnalyzer : AnalyzerBase
 
         foreach (var validatorType in option.ValidatorTypes)
         {
-            // We check against an internal interface because checking against a generic class is a pain
+            var validatorValueType = validatorType
-            var validatorImplementsInterface = validatorType
+                .GetBaseTypes()
-                .AllInterfaces
+                .FirstOrDefault(t => t.ConstructedFrom.DisplayNameMatches(SymbolNames.CliFxBindingValidatorClass))?
-                .Any(s => s.DisplayNameMatches(SymbolNames.CliFxBindingValidatorInterface));
+                .TypeArguments
+                .FirstOrDefault();
 
-            if (!validatorImplementsInterface)
+            // Value passed to the validator must be assignable from the property type
+            if (validatorValueType is null || !validatorValueType.IsAssignableFrom(property.Type))
             {
                 context.ReportDiagnostic(CreateDiagnostic(propertyDeclaration.GetLocation()));
 

CliFx.Analyzers/ParameterMustBeInsideCommandAnalyzer.cs

@@ -34,7 +34,7 @@ public class ParameterMustBeInsideCommandAnalyzer : AnalyzerBase
         var isInsideCommand = property
             .ContainingType
             .AllInterfaces
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
+            .Any(i => i.DisplayNameMatches(SymbolNames.CliFxCommandInterface));
 
         if (!isInsideCommand)
         {

CliFx.Analyzers/ParameterMustBeLastIfNonScalarAnalyzer.cs

@@ -22,7 +22,7 @@ public class ParameterMustBeLastIfNonScalarAnalyzer : AnalyzerBase
         type.DisplayNameMatches("System.String") ||
         !type.AllInterfaces
             .Select(i => i.ConstructedFrom)
-            .Any(s => s.DisplayNameMatches("System.Collections.Generic.IEnumerable<T>"));
+            .Any(t => t.DisplayNameMatches("System.Collections.Generic.IEnumerable<T>"));
 
     private void Analyze(
         SyntaxNodeAnalysisContext context,

CliFx.Analyzers/ParameterMustBeSingleIfNonScalarAnalyzer.cs

@@ -22,7 +22,7 @@ public class ParameterMustBeSingleIfNonScalarAnalyzer : AnalyzerBase
         type.DisplayNameMatches("System.String") ||
         !type.AllInterfaces
             .Select(i => i.ConstructedFrom)
-            .Any(s => s.DisplayNameMatches("System.Collections.Generic.IEnumerable<T>"));
+            .Any(t => t.DisplayNameMatches("System.Collections.Generic.IEnumerable<T>"));
 
     private void Analyze(
         SyntaxNodeAnalysisContext context,

CliFx.Analyzers/ParameterMustHaveValidConverterAnalyzer.cs

@@ -13,7 +13,7 @@ public class ParameterMustHaveValidConverterAnalyzer : AnalyzerBase
     public ParameterMustHaveValidConverterAnalyzer()
         : base(
             $"Parameter converters must derive from `{SymbolNames.CliFxBindingConverterClass}`",
-            $"Converter specified for this parameter must derive from `{SymbolNames.CliFxBindingConverterClass}`.")
+            $"Converter specified for this parameter must derive from a compatible `{SymbolNames.CliFxBindingConverterClass}`.")
     {
     }
 
@@ -29,13 +29,15 @@ public class ParameterMustHaveValidConverterAnalyzer : AnalyzerBase
         if (parameter.ConverterType is null)
             return;
 
-        // We check against an internal interface because checking against a generic class is a pain
+        var converterValueType = parameter
-        var converterImplementsInterface = parameter
             .ConverterType
-            .AllInterfaces
+            .GetBaseTypes()
-            .Any(s => s.DisplayNameMatches(SymbolNames.CliFxBindingConverterInterface));
+            .FirstOrDefault(t => t.ConstructedFrom.DisplayNameMatches(SymbolNames.CliFxBindingConverterClass))?
+            .TypeArguments
+            .FirstOrDefault();
 
-        if (!converterImplementsInterface)
+        // Value returned by the converter must be assignable to the property type
+        if (converterValueType is null || !property.Type.IsAssignableFrom(converterValueType))
         {
             context.ReportDiagnostic(CreateDiagnostic(propertyDeclaration.GetLocation()));
         }

CliFx.Analyzers/ParameterMustHaveValidValidatorsAnalyzer.cs

@@ -13,7 +13,7 @@ public class ParameterMustHaveValidValidatorsAnalyzer : AnalyzerBase
     public ParameterMustHaveValidValidatorsAnalyzer()
         : base(
             $"Parameter validators must derive from `{SymbolNames.CliFxBindingValidatorClass}`",
-            $"All validators specified for this parameter must derive from `{SymbolNames.CliFxBindingValidatorClass}`.")
+            $"Each validator specified for this parameter must derive from a compatible `{SymbolNames.CliFxBindingValidatorClass}`.")
     {
     }
 
@@ -28,12 +28,14 @@ public class ParameterMustHaveValidValidatorsAnalyzer : AnalyzerBase
 
         foreach (var validatorType in parameter.ValidatorTypes)
         {
-            // We check against an internal interface because checking against a generic class is a pain
+            var validatorValueType = validatorType
-            var validatorImplementsInterface = validatorType
+                .GetBaseTypes()
-                .AllInterfaces
+                .FirstOrDefault(t => t.ConstructedFrom.DisplayNameMatches(SymbolNames.CliFxBindingValidatorClass))?
-                .Any(s => s.DisplayNameMatches(SymbolNames.CliFxBindingValidatorInterface));
+                .TypeArguments
+                .FirstOrDefault();
 
-            if (!validatorImplementsInterface)
+            // Value passed to the validator must be assignable from the property type
+            if (validatorValueType is null || !validatorValueType.IsAssignableFrom(property.Type))
             {
                 context.ReportDiagnostic(CreateDiagnostic(propertyDeclaration.GetLocation()));
 

CliFx.Analyzers/Utils/Extensions/RoslynExtensions.cs

@@ -1,4 +1,6 @@
 using System;
+using System.Collections.Generic;
+using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
@@ -16,6 +18,22 @@ internal static class RoslynExtensions
             StringComparison.Ordinal
         );
 
+    public static IEnumerable<INamedTypeSymbol> GetBaseTypes(this ITypeSymbol type)
+    {
+        var current = type.BaseType;
+
+        while (current is not null)
+        {
+            yield return current;
+            current = current.BaseType;
+        }
+    }
+
+    public static bool IsAssignableFrom(this ITypeSymbol target, ITypeSymbol source) =>
+        SymbolEqualityComparer.Default.Equals(target, source) ||
+        source.GetBaseTypes().Contains(target, SymbolEqualityComparer.Default) ||
+        source.AllInterfaces.Contains(target, SymbolEqualityComparer.Default);
+
     public static void HandleClassDeclaration(
         this AnalysisContext analysisContext,
         Action<SyntaxNodeAnalysisContext, ClassDeclarationSyntax, ITypeSymbol> analyze)

CliFx/CliApplication.cs

@@ -112,7 +112,7 @@ public class CliApplication
         // Activate command instance
         var commandInstance = commandSchema == FallbackDefaultCommand.Schema
             ? new FallbackDefaultCommand() // bypass activator
-            : (ICommand) _typeActivator.CreateInstance(commandSchema.Type);
+            : (ICommand)_typeActivator.CreateInstance(commandSchema.Type);
 
         // Assemble help context
         var helpContext = new HelpContext(
@@ -176,7 +176,7 @@ public class CliApplication
     {
         try
         {
-            // Console colors may have already been overriden by the parent process,
+            // Console colors may have already been overridden by the parent process,
             // so we need to reset it to make sure that everything we write looks properly.
             _console.ResetColor();
 

CliFx/CommandBinder.cs

@@ -27,7 +27,7 @@ internal class CommandBinder
         // Custom converter
         if (memberSchema.ConverterType is not null)
         {
-            var converter = (IBindingConverter) _typeActivator.CreateInstance(memberSchema.ConverterType);
+            var converter = (IBindingConverter)_typeActivator.CreateInstance(memberSchema.ConverterType);
             return converter.Convert(rawValue);
         }
 
@@ -78,24 +78,24 @@ internal class CommandBinder
         }
 
         // String-constructible (FileInfo, etc)
-        var stringConstructor = targetType.GetConstructor(new[] {typeof(string)});
+        var stringConstructor = targetType.GetConstructor(new[] { typeof(string) });
         if (stringConstructor is not null)
         {
-            return stringConstructor.Invoke(new object?[] {rawValue});
+            return stringConstructor.Invoke(new object?[] { rawValue });
         }
 
         // String-parseable (with IFormatProvider)
         var parseMethodWithFormatProvider = targetType.TryGetStaticParseMethod(true);
         if (parseMethodWithFormatProvider is not null)
         {
-            return parseMethodWithFormatProvider.Invoke(null, new object?[] {rawValue, _formatProvider});
+            return parseMethodWithFormatProvider.Invoke(null, new object?[] { rawValue, _formatProvider });
         }
 
         // String-parseable (without IFormatProvider)
         var parseMethod = targetType.TryGetStaticParseMethod();
         if (parseMethod is not null)
         {
-            return parseMethod.Invoke(null, new object?[] {rawValue});
+            return parseMethod.Invoke(null, new object?[] { rawValue });
         }
 
         throw CliFxException.InternalError(
@@ -126,10 +126,10 @@ internal class CommandBinder
         }
 
         // Array-constructible (List<T>, HashSet<T>, etc)
-        var arrayConstructor = targetEnumerableType.GetConstructor(new[] {arrayType});
+        var arrayConstructor = targetEnumerableType.GetConstructor(new[] { arrayType });
         if (arrayConstructor is not null)
         {
-            return arrayConstructor.Invoke(new object?[] {array});
+            return arrayConstructor.Invoke(new object?[] { array });
         }
 
         throw CliFxException.InternalError(
@@ -192,7 +192,7 @@ internal class CommandBinder
 
         foreach (var validatorType in memberSchema.ValidatorTypes)
         {
-            var validator = (IBindingValidator) _typeActivator.CreateInstance(validatorType);
+            var validator = (IBindingValidator)_typeActivator.CreateInstance(validatorType);
             var error = validator.Validate(convertedValue);
 
             if (error is not null)
@@ -238,7 +238,7 @@ internal class CommandBinder
             {
                 var parameterInput = commandInput.Parameters[position];
 
-                var rawValues = new[] {parameterInput.Value};
+                var rawValues = new[] { parameterInput.Value };
                 BindMember(parameterSchema, commandInstance, rawValues);
 
                 position++;
@@ -278,7 +278,7 @@ internal class CommandBinder
                 "Missing required parameter(s):" +
                 Environment.NewLine +
                 remainingRequiredParameterSchemas
-                    .Select(o => o.GetFormattedIdentifier())
+                    .Select(p => p.GetFormattedIdentifier())
                     .JoinToString(" ")
             );
         }
@@ -316,7 +316,7 @@ internal class CommandBinder
             else if (environmentVariableInput is not null)
             {
                 var rawValues = optionSchema.Property.IsScalar()
-                    ? new[] {environmentVariableInput.Value}
+                    ? new[] { environmentVariableInput.Value }
                     : environmentVariableInput.SplitValues();
 
                 BindMember(optionSchema, commandInstance, rawValues);