Fix converter analyzer false positive when handling non-scalars or nullable types

CliFx.Analyzers.Tests/OptionMustHaveValidConverterAnalyzerSpecs.cs

@@ -9,7 +9,7 @@ public class OptionMustHaveValidConverterAnalyzerSpecs
     private static DiagnosticAnalyzer Analyzer { get; } = new OptionMustHaveValidConverterAnalyzer();
 
     [Fact]
-    public void Analyzer_reports_an_error_if_the_specified_option_converter_does_not_derive_from_BindingConverter()
+    public void Analyzer_reports_an_error_if_an_option_has_a_converter_that_does_not_derive_from_BindingConverter()
     {
         // Arrange
         // language=cs
@@ -33,7 +33,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_reports_an_error_if_the_specified_option_converter_does_not_derive_from_a_compatible_BindingConverter()
+    public void Analyzer_reports_an_error_if_an_option_has_a_converter_that_does_not_derive_from_a_compatible_BindingConverter()
     {
         // Arrange
         // language=cs
@@ -57,7 +57,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_the_specified_option_converter_derives_from_a_compatible_BindingConverter()
+    public void Analyzer_does_not_report_an_error_if_an_option_has_a_converter_that_derives_from_a_compatible_BindingConverter()
     {
         // Arrange
         // language=cs
@@ -80,6 +80,54 @@ public class MyCommand : ICommand
         Analyzer.Should().NotProduceDiagnostics(code);
     }
 
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_a_nullable_option_has_a_converter_that_derives_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 int? Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+        // Act & assert
+        Analyzer.Should().NotProduceDiagnostics(code);
+    }
+
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_a_non_scalar_option_has_a_converter_that_derives_from_a_compatible_BindingConverter()
+    {
+        // Arrange
+        // language=cs
+        const string code = @"
+public class MyConverter : BindingConverter<string>
+{
+    public override string Convert(string rawValue) => rawValue;
+}
+
+[Command]
+public class MyCommand : ICommand
+{
+    [CommandOption(""foo"", Converter = typeof(MyConverter))]
+    public IReadOnlyList<string> Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+        // Act & assert
+        Analyzer.Should().NotProduceDiagnostics(code);
+    }
+
     [Fact]
     public void Analyzer_does_not_report_an_error_if_an_option_does_not_have_a_converter()
     {

CliFx.Analyzers.Tests/OptionMustHaveValidValidatorsAnalyzerSpecs.cs

@@ -9,7 +9,7 @@ public class OptionMustHaveValidValidatorsAnalyzerSpecs
     private static DiagnosticAnalyzer Analyzer { get; } = new OptionMustHaveValidValidatorsAnalyzer();
 
     [Fact]
-    public void Analyzer_reports_an_error_if_one_of_the_specified_option_validators_does_not_derive_from_BindingValidator()
+    public void Analyzer_reports_an_error_if_an_option_has_a_validator_that_does_not_derive_from_BindingValidator()
     {
         // Arrange
         // language=cs
@@ -33,7 +33,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_reports_an_error_if_one_of_the_specified_option_validators_does_not_derive_from_a_compatible_BindingValidator()
+    public void Analyzer_reports_an_error_if_an_option_has_a_validator_that_does_not_derive_from_a_compatible_BindingValidator()
     {
         // Arrange
         // language=cs
@@ -57,7 +57,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_each_specified_option_validator_derives_from_a_compatible_BindingValidator()
+    public void Analyzer_does_not_report_an_error_if_an_option_has_validators_that_all_derive_from_compatible_BindingValidators()
     {
         // Arrange
         // language=cs

CliFx.Analyzers.Tests/ParameterMustHaveValidConverterAnalyzerSpecs.cs

@@ -9,7 +9,7 @@ public class ParameterMustHaveValidConverterAnalyzerSpecs
     private static DiagnosticAnalyzer Analyzer { get; } = new ParameterMustHaveValidConverterAnalyzer();
 
     [Fact]
-    public void Analyzer_reports_an_error_if_the_specified_parameter_converter_does_not_derive_from_BindingConverter()
+    public void Analyzer_reports_an_error_if_a_parameter_has_a_converter_that_does_not_derive_from_BindingConverter()
     {
         // Arrange
         // language=cs
@@ -33,7 +33,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_reports_an_error_if_the_specified_parameter_converter_does_not_derive_from_a_compatible_BindingConverter()
+    public void Analyzer_reports_an_error_if_a_parameter_has_a_converter_that_does_not_derive_from_a_compatible_BindingConverter()
     {
         // Arrange
         // language=cs
@@ -58,7 +58,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_the_specified_parameter_converter_derives_from_a_compatible_BindingConverter()
+    public void Analyzer_does_not_report_an_error_if_a_parameter_has_a_converter_that_derives_from_a_compatible_BindingConverter()
     {
         // Arrange
         // language=cs
@@ -81,6 +81,54 @@ public class MyCommand : ICommand
         Analyzer.Should().NotProduceDiagnostics(code);
     }
 
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_a_nullable_parameter_has_a_converter_that_derives_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 int? Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+        // Act & assert
+        Analyzer.Should().NotProduceDiagnostics(code);
+    }
+
+    [Fact]
+    public void Analyzer_does_not_report_an_error_if_a_non_scalar_parameter_has_a_converter_that_derives_from_a_compatible_BindingConverter()
+    {
+        // Arrange
+        // language=cs
+        const string code = @"
+public class MyConverter : BindingConverter<string>
+{
+    public override string Convert(string rawValue) => rawValue;
+}
+
+[Command]
+public class MyCommand : ICommand
+{
+    [CommandParameter(0, Converter = typeof(MyConverter))]
+    public IReadOnlyList<string> Foo { get; set; }
+
+    public ValueTask ExecuteAsync(IConsole console) => default;
+}";
+
+        // Act & assert
+        Analyzer.Should().NotProduceDiagnostics(code);
+    }
+
     [Fact]
     public void Analyzer_does_not_report_an_error_if_a_parameter_does_not_have_a_converter()
     {

CliFx.Analyzers.Tests/ParameterMustHaveValidValidatorsAnalyzerSpecs.cs

@@ -9,7 +9,7 @@ public class ParameterMustHaveValidValidatorsAnalyzerSpecs
     private static DiagnosticAnalyzer Analyzer { get; } = new ParameterMustHaveValidValidatorsAnalyzer();
 
     [Fact]
-    public void Analyzer_reports_an_error_if_one_of_the_specified_parameter_validators_does_not_derive_from_BindingValidator()
+    public void Analyzer_reports_an_error_a_parameter_has_a_validator_that_does_not_derive_from_BindingValidator()
     {
         // Arrange
         // language=cs
@@ -33,7 +33,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_reports_an_error_if_one_of_the_specified_parameter_validators_does_not_derive_from_a_compatible_BindingValidator()
+    public void Analyzer_reports_an_error_if_a_parameter_has_a_validator_that_does_not_derive_from_a_compatible_BindingValidator()
     {
         // Arrange
         // language=cs
@@ -57,7 +57,7 @@ public class MyCommand : ICommand
     }
 
     [Fact]
-    public void Analyzer_does_not_report_an_error_if_each_specified_parameter_validator_derives_from_a_compatible_BindingValidator()
+    public void Analyzer_does_not_report_an_error_if_a_parameter_has_validators_that_all_derive_from_compatible_BindingValidators()
     {
         // Arrange
         // language=cs

CliFx.Analyzers/ObjectModel/CommandOptionSymbol.cs

@@ -5,8 +5,10 @@ using Microsoft.CodeAnalysis;
 
 namespace CliFx.Analyzers.ObjectModel;
 
-internal partial class CommandOptionSymbol
+internal partial class CommandOptionSymbol : ICommandMemberSymbol
 {
+    public IPropertySymbol Property { get; }
+
     public string? Name { get; }
 
     public char? ShortName { get; }
@@ -16,11 +18,13 @@ internal partial class CommandOptionSymbol
     public IReadOnlyList<ITypeSymbol> ValidatorTypes { get; }
 
     public CommandOptionSymbol(
+        IPropertySymbol property,
         string? name,
         char? shortName,
         ITypeSymbol? converterType,
         IReadOnlyList<ITypeSymbol> validatorTypes)
     {
+        Property = property;
         Name = name;
         ShortName = shortName;
         ConverterType = converterType;
@@ -30,22 +34,25 @@ internal partial class CommandOptionSymbol
 
 internal partial class CommandOptionSymbol
 {
-    private static AttributeData? TryGetOptionAttribute(IPropertySymbol property) =>
+    private static AttributeData? TryGetOptionAttribute(IPropertySymbol property) => property
-        property
+        .GetAttributes()
-            .GetAttributes()
+        .FirstOrDefault(a => a.AttributeClass?.DisplayNameMatches(SymbolNames.CliFxCommandOptionAttribute) == true);
-            .FirstOrDefault(a => a.AttributeClass.DisplayNameMatches(SymbolNames.CliFxCommandOptionAttribute));
 
-    private static CommandOptionSymbol FromAttribute(AttributeData attribute)
+    public static CommandOptionSymbol? TryResolve(IPropertySymbol property)
     {
+        var attribute = TryGetOptionAttribute(property);
+        if (attribute is null)
+            return null;
+
         var name = attribute
             .ConstructorArguments
-            .Where(a => a.Type.DisplayNameMatches("string") || a.Type.DisplayNameMatches("System.String"))
+            .Where(a => a.Type?.SpecialType == SpecialType.System_String)
             .Select(a => a.Value)
             .FirstOrDefault() as string;
 
         var shortName = attribute
             .ConstructorArguments
-            .Where(a => a.Type.DisplayNameMatches("char") || a.Type.DisplayNameMatches("System.Char"))
+            .Where(a => a.Type?.SpecialType == SpecialType.System_Char)
             .Select(a => a.Value)
             .FirstOrDefault() as char?;
 
@@ -64,16 +71,7 @@ internal partial class CommandOptionSymbol
             .Cast<ITypeSymbol>()
             .ToArray();
 
-        return new CommandOptionSymbol(name, shortName, converter, validators);
+        return new CommandOptionSymbol(property, name, shortName, converter, validators);
-    }
-
-    public static CommandOptionSymbol? TryResolve(IPropertySymbol property)
-    {
-        var attribute = TryGetOptionAttribute(property);
-
-        return attribute is not null
-            ? FromAttribute(attribute)
-            : null;
     }
 
     public static bool IsOptionProperty(IPropertySymbol property) =>

CliFx.Analyzers/ObjectModel/CommandParameterSymbol.cs

@@ -5,8 +5,10 @@ using Microsoft.CodeAnalysis;
 
 namespace CliFx.Analyzers.ObjectModel;
 
-internal partial class CommandParameterSymbol
+internal partial class CommandParameterSymbol : ICommandMemberSymbol
 {
+    public IPropertySymbol Property { get; }
+
     public int Order { get; }
 
     public string? Name { get; }
@@ -18,12 +20,14 @@ internal partial class CommandParameterSymbol
     public IReadOnlyList<ITypeSymbol> ValidatorTypes { get; }
 
     public CommandParameterSymbol(
+        IPropertySymbol property,
         int order,
         string? name,
         bool? isRequired,
         ITypeSymbol? converterType,
         IReadOnlyList<ITypeSymbol> validatorTypes)
     {
+        Property = property;
         Order = order;
         Name = name;
         IsRequired = isRequired;
@@ -34,13 +38,16 @@ internal partial class CommandParameterSymbol
 
 internal partial class CommandParameterSymbol
 {
-    private static AttributeData? TryGetParameterAttribute(IPropertySymbol property) =>
+    private static AttributeData? TryGetParameterAttribute(IPropertySymbol property) => property
-        property
+        .GetAttributes()
-            .GetAttributes()
+        .FirstOrDefault(a => a.AttributeClass?.DisplayNameMatches(SymbolNames.CliFxCommandParameterAttribute) == true);
-            .FirstOrDefault(a => a.AttributeClass.DisplayNameMatches(SymbolNames.CliFxCommandParameterAttribute));
 
-    private static CommandParameterSymbol FromAttribute(AttributeData attribute)
+    public static CommandParameterSymbol? TryResolve(IPropertySymbol property)
     {
+        var attribute = TryGetParameterAttribute(property);
+        if (attribute is null)
+            return null;
+
         var order = (int)attribute
             .ConstructorArguments
             .Select(a => a.Value)
@@ -73,16 +80,7 @@ internal partial class CommandParameterSymbol
             .Cast<ITypeSymbol>()
             .ToArray();
 
-        return new CommandParameterSymbol(order, name, isRequired, converter, validators);
+        return new CommandParameterSymbol(property, order, name, isRequired, converter, validators);
-    }
-
-    public static CommandParameterSymbol? TryResolve(IPropertySymbol property)
-    {
-        var attribute = TryGetParameterAttribute(property);
-
-        return attribute is not null
-            ? FromAttribute(attribute)
-            : null;
     }
 
     public static bool IsParameterProperty(IPropertySymbol property) =>

CliFx.Analyzers/ObjectModel/ICommandMemberSymbol.cs

@@ -0,0 +1,21 @@
+using System.Collections.Generic;
+using CliFx.Analyzers.Utils.Extensions;
+using Microsoft.CodeAnalysis;
+
+namespace CliFx.Analyzers.ObjectModel;
+
+internal interface ICommandMemberSymbol
+{
+    IPropertySymbol Property { get; }
+
+    ITypeSymbol? ConverterType { get; }
+
+    IReadOnlyList<ITypeSymbol> ValidatorTypes { get; }
+}
+
+internal static class CommandMemberSymbolExtensions
+{
+    public static bool IsScalar(this ICommandMemberSymbol member) =>
+        member.Property.Type.SpecialType == SpecialType.System_String ||
+        member.Property.Type.TryGetEnumerableUnderlyingType() is null;
+}

CliFx.Analyzers/OptionMustHaveValidConverterAnalyzer.cs

@@ -37,7 +37,16 @@ public class OptionMustHaveValidConverterAnalyzer : AnalyzerBase
             .FirstOrDefault();
 
         // Value returned by the converter must be assignable to the property type
-        if (converterValueType is null || !property.Type.IsAssignableFrom(converterValueType))
+        var isCompatible =
+            converterValueType is not null && (option.IsScalar()
+                // Scalar
+                ? context.Compilation.IsAssignable(converterValueType, property.Type)
+                // Non-scalar (assume we can handle all IEnumerable types for simplicity)
+                : property.Type.TryGetEnumerableUnderlyingType() is { } enumerableUnderlyingType &&
+                  context.Compilation.IsAssignable(converterValueType, enumerableUnderlyingType)
+            );
+
+        if (!isCompatible)
         {
             context.ReportDiagnostic(
                 CreateDiagnostic(propertyDeclaration.Identifier.GetLocation())

CliFx.Analyzers/OptionMustHaveValidValidatorsAnalyzer.cs

@@ -35,7 +35,11 @@ public class OptionMustHaveValidValidatorsAnalyzer : AnalyzerBase
                 .FirstOrDefault();
 
             // Value passed to the validator must be assignable from the property type
-            if (validatorValueType is null || !validatorValueType.IsAssignableFrom(property.Type))
+            var isCompatible =
+                validatorValueType is not null &&
+                context.Compilation.IsAssignable(property.Type, validatorValueType);
+
+            if (!isCompatible)
             {
                 context.ReportDiagnostic(
                     CreateDiagnostic(propertyDeclaration.Identifier.GetLocation())

CliFx.Analyzers/ParameterMustBeLastIfNonScalarAnalyzer.cs

@@ -17,13 +17,6 @@ public class ParameterMustBeLastIfNonScalarAnalyzer : AnalyzerBase
     {
     }
 
-    private static bool IsScalar(ITypeSymbol type) =>
-        type.DisplayNameMatches("string") ||
-        type.DisplayNameMatches("System.String") ||
-        !type.AllInterfaces
-            .Select(i => i.ConstructedFrom)
-            .Any(t => t.DisplayNameMatches("System.Collections.Generic.IEnumerable<T>"));
-
     private void Analyze(
         SyntaxNodeAnalysisContext context,
         PropertyDeclarationSyntax propertyDeclaration,
@@ -32,13 +25,13 @@ public class ParameterMustBeLastIfNonScalarAnalyzer : AnalyzerBase
         if (property.ContainingType is null)
             return;
 
-        if (IsScalar(property.Type))
-            return;
-
         var parameter = CommandParameterSymbol.TryResolve(property);
         if (parameter is null)
             return;
 
+        if (parameter.IsScalar())
+            return;
+
         var otherProperties = property
             .ContainingType
             .GetMembers()

CliFx.Analyzers/ParameterMustBeSingleIfNonScalarAnalyzer.cs

@@ -17,13 +17,6 @@ public class ParameterMustBeSingleIfNonScalarAnalyzer : AnalyzerBase
     {
     }
 
-    private static bool IsScalar(ITypeSymbol type) =>
-        type.DisplayNameMatches("string") ||
-        type.DisplayNameMatches("System.String") ||
-        !type.AllInterfaces
-            .Select(i => i.ConstructedFrom)
-            .Any(t => t.DisplayNameMatches("System.Collections.Generic.IEnumerable<T>"));
-
     private void Analyze(
         SyntaxNodeAnalysisContext context,
         PropertyDeclarationSyntax propertyDeclaration,
@@ -32,10 +25,11 @@ public class ParameterMustBeSingleIfNonScalarAnalyzer : AnalyzerBase
         if (property.ContainingType is null)
             return;
 
-        if (!CommandParameterSymbol.IsParameterProperty(property))
+        var parameter = CommandParameterSymbol.TryResolve(property);
+        if (parameter is null)
             return;
 
-        if (IsScalar(property.Type))
+        if (parameter.IsScalar())
             return;
 
         var otherProperties = property
@@ -47,10 +41,11 @@ public class ParameterMustBeSingleIfNonScalarAnalyzer : AnalyzerBase
 
         foreach (var otherProperty in otherProperties)
         {
-            if (!CommandParameterSymbol.IsParameterProperty(otherProperty))
+            var otherParameter = CommandParameterSymbol.TryResolve(otherProperty);
+            if (otherParameter is null)
                 continue;
 
-            if (!IsScalar(otherProperty.Type))
+            if (!otherParameter.IsScalar())
             {
                 context.ReportDiagnostic(
                     CreateDiagnostic(propertyDeclaration.Identifier.GetLocation())

CliFx.Analyzers/ParameterMustHaveValidConverterAnalyzer.cs

@@ -37,7 +37,16 @@ public class ParameterMustHaveValidConverterAnalyzer : AnalyzerBase
             .FirstOrDefault();
 
         // Value returned by the converter must be assignable to the property type
-        if (converterValueType is null || !property.Type.IsAssignableFrom(converterValueType))
+        var isCompatible =
+            converterValueType is not null && (parameter.IsScalar()
+                // Scalar
+                ? context.Compilation.IsAssignable(converterValueType, property.Type)
+                // Non-scalar (assume we can handle all IEnumerable types for simplicity)
+                : property.Type.TryGetEnumerableUnderlyingType() is { } enumerableUnderlyingType &&
+                  context.Compilation.IsAssignable(converterValueType, enumerableUnderlyingType)
+            );
+
+        if (!isCompatible)
         {
             context.ReportDiagnostic(
                 CreateDiagnostic(propertyDeclaration.Identifier.GetLocation())

CliFx.Analyzers/ParameterMustHaveValidValidatorsAnalyzer.cs

@@ -35,7 +35,11 @@ public class ParameterMustHaveValidValidatorsAnalyzer : AnalyzerBase
                 .FirstOrDefault();
 
             // Value passed to the validator must be assignable from the property type
-            if (validatorValueType is null || !validatorValueType.IsAssignableFrom(property.Type))
+            var isCompatible =
+                validatorValueType is not null &&
+                context.Compilation.IsAssignable(property.Type, validatorValueType);
+
+            if (!isCompatible)
             {
                 context.ReportDiagnostic(
                     CreateDiagnostic(propertyDeclaration.Identifier.GetLocation())

CliFx.Analyzers/Utils/Extensions/RoslynExtensions.cs

@@ -29,10 +29,13 @@ internal static class RoslynExtensions
         }
     }
 
-    public static bool IsAssignableFrom(this ITypeSymbol target, ITypeSymbol source) =>
+    public static ITypeSymbol? TryGetEnumerableUnderlyingType(this ITypeSymbol type) => type
-        SymbolEqualityComparer.Default.Equals(target, source) ||
+        .AllInterfaces
-        source.GetBaseTypes().Contains(target, SymbolEqualityComparer.Default) ||
+        .FirstOrDefault(i => i.ConstructedFrom.SpecialType == SpecialType.System_Collections_Generic_IEnumerable_T)?
-        source.AllInterfaces.Contains(target, SymbolEqualityComparer.Default);
+        .TypeArguments[0];
+
+    public static bool IsAssignable(this Compilation compilation, ITypeSymbol source, ITypeSymbol destination) =>
+        compilation.ClassifyConversion(source, destination).Exists;
 
     public static void HandleClassDeclaration(
         this AnalysisContext analysisContext,