diff --git a/CliFx.Analyzers.Tests/CliFx.Analyzers.Tests.csproj b/CliFx.Analyzers.Tests/CliFx.Analyzers.Tests.csproj index a3a08e2..f7287de 100644 --- a/CliFx.Analyzers.Tests/CliFx.Analyzers.Tests.csproj +++ b/CliFx.Analyzers.Tests/CliFx.Analyzers.Tests.csproj @@ -9,7 +9,7 @@ - + diff --git a/CliFx.Analyzers.Tests/OptionMustBeRequiredIfPropertyRequiredAnalyzerSpecs.cs b/CliFx.Analyzers.Tests/OptionMustBeRequiredIfPropertyRequiredAnalyzerSpecs.cs new file mode 100644 index 0000000..d97ef5e --- /dev/null +++ b/CliFx.Analyzers.Tests/OptionMustBeRequiredIfPropertyRequiredAnalyzerSpecs.cs @@ -0,0 +1,114 @@ +using CliFx.Analyzers.Tests.Utils; +using Microsoft.CodeAnalysis.Diagnostics; +using Xunit; + +namespace CliFx.Analyzers.Tests; + +public class OptionMustBeRequiredIfPropertyRequiredAnalyzerSpecs +{ + private static DiagnosticAnalyzer Analyzer { get; } = new OptionMustBeRequiredIfPropertyRequiredAnalyzer(); + + [Fact] + public void Analyzer_reports_an_error_if_a_non_required_option_is_bound_to_a_required_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandOption('f', IsRequired = false)] + public required 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_a_required_option_is_bound_to_a_required_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandOption('f', IsRequired = true)] + public required 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_non_required_option_is_bound_to_an_unannotated_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandOption('f', IsRequired = false)] + public 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_required_option_is_bound_to_an_unannotated_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandOption('f', IsRequired = true)] + public 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_on_a_property_that_is_not_an_option() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + public required string Foo { get; set; } + + public ValueTask ExecuteAsync(IConsole console) => default; + } + """; + + // Act & assert + Analyzer.Should().NotProduceDiagnostics(code); + } +} \ No newline at end of file diff --git a/CliFx.Analyzers.Tests/ParameterMustBeLastIfNonRequiredAnalyzerSpecs.cs b/CliFx.Analyzers.Tests/ParameterMustBeLastIfNonRequiredAnalyzerSpecs.cs index ff240df..388cb9e 100644 --- a/CliFx.Analyzers.Tests/ParameterMustBeLastIfNonRequiredAnalyzerSpecs.cs +++ b/CliFx.Analyzers.Tests/ParameterMustBeLastIfNonRequiredAnalyzerSpecs.cs @@ -18,10 +18,10 @@ public class ParameterMustBeLastIfNonRequiredAnalyzerSpecs [Command] public class MyCommand : ICommand { - [CommandParameter(0, Name = "foo", IsRequired = false)] + [CommandParameter(0, IsRequired = false)] public string Foo { get; set; } - [CommandParameter(1, Name = "bar")] + [CommandParameter(1)] public string Bar { get; set; } public ValueTask ExecuteAsync(IConsole console) => default; @@ -42,10 +42,10 @@ public class ParameterMustBeLastIfNonRequiredAnalyzerSpecs [Command] public class MyCommand : ICommand { - [CommandParameter(0, Name = "foo")] + [CommandParameter(0)] public string Foo { get; set; } - [CommandParameter(1, Name = "bar", IsRequired = false)] + [CommandParameter(1, IsRequired = false)] public string Bar { get; set; } public ValueTask ExecuteAsync(IConsole console) => default; @@ -66,10 +66,10 @@ public class ParameterMustBeLastIfNonRequiredAnalyzerSpecs [Command] public class MyCommand : ICommand { - [CommandParameter(0, Name = "foo")] + [CommandParameter(0)] public string Foo { get; set; } - [CommandParameter(1, Name = "bar", IsRequired = true)] + [CommandParameter(1, IsRequired = true)] public string Bar { get; set; } public ValueTask ExecuteAsync(IConsole console) => default; diff --git a/CliFx.Analyzers.Tests/ParameterMustBeRequiredIfPropertyRequiredAnalyzerSpecs.cs b/CliFx.Analyzers.Tests/ParameterMustBeRequiredIfPropertyRequiredAnalyzerSpecs.cs new file mode 100644 index 0000000..2f851b0 --- /dev/null +++ b/CliFx.Analyzers.Tests/ParameterMustBeRequiredIfPropertyRequiredAnalyzerSpecs.cs @@ -0,0 +1,114 @@ +using CliFx.Analyzers.Tests.Utils; +using Microsoft.CodeAnalysis.Diagnostics; +using Xunit; + +namespace CliFx.Analyzers.Tests; + +public class ParameterMustBeRequiredIfPropertyRequiredAnalyzerSpecs +{ + private static DiagnosticAnalyzer Analyzer { get; } = new ParameterMustBeRequiredIfPropertyRequiredAnalyzer(); + + [Fact] + public void Analyzer_reports_an_error_if_a_non_required_parameter_is_bound_to_a_required_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandParameter(0, IsRequired = false)] + public required 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_a_required_parameter_is_bound_to_a_required_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandParameter(0, IsRequired = true)] + public required 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_non_required_parameter_is_bound_to_an_unannotated_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandParameter(0, IsRequired = false)] + public 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_required_parameter_is_bound_to_an_unannotated_property() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + [CommandParameter(0, IsRequired = true)] + public 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_on_a_property_that_is_not_a_parameter() + { + // Arrange + // language=cs + const string code = + """ + [Command] + public class MyCommand : ICommand + { + public required string Foo { get; set; } + + public ValueTask ExecuteAsync(IConsole console) => default; + } + """; + + // Act & assert + Analyzer.Should().NotProduceDiagnostics(code); + } +} \ No newline at end of file diff --git a/CliFx.Analyzers.Tests/Utils/AnalyzerAssertions.cs b/CliFx.Analyzers.Tests/Utils/AnalyzerAssertions.cs index 76e67d0..83df884 100644 --- a/CliFx.Analyzers.Tests/Utils/AnalyzerAssertions.cs +++ b/CliFx.Analyzers.Tests/Utils/AnalyzerAssertions.cs @@ -58,8 +58,7 @@ internal class AnalyzerAssertions : ReferenceTypeAssertions ValidatorTypes { get; } @@ -21,12 +23,14 @@ internal partial class CommandOptionSymbol : ICommandMemberSymbol IPropertySymbol property, string? name, char? shortName, + bool? isRequired, ITypeSymbol? converterType, IReadOnlyList validatorTypes) { Property = property; Name = name; ShortName = shortName; + IsRequired = isRequired; ConverterType = converterType; ValidatorTypes = validatorTypes; } @@ -56,6 +60,12 @@ internal partial class CommandOptionSymbol .Select(a => a.Value) .FirstOrDefault() as char?; + var isRequired = attribute + .NamedArguments + .Where(a => a.Key == "IsRequired") + .Select(a => a.Value.Value) + .FirstOrDefault() as bool?; + var converter = attribute .NamedArguments .Where(a => a.Key == "Converter") @@ -71,7 +81,7 @@ internal partial class CommandOptionSymbol .Cast() .ToArray(); - return new CommandOptionSymbol(property, name, shortName, converter, validators); + return new CommandOptionSymbol(property, name, shortName, isRequired, converter, validators); } public static bool IsOptionProperty(IPropertySymbol property) => diff --git a/CliFx.Analyzers/OptionMustBeRequiredIfPropertyRequiredAnalyzer.cs b/CliFx.Analyzers/OptionMustBeRequiredIfPropertyRequiredAnalyzer.cs new file mode 100644 index 0000000..5971c58 --- /dev/null +++ b/CliFx.Analyzers/OptionMustBeRequiredIfPropertyRequiredAnalyzer.cs @@ -0,0 +1,49 @@ +using CliFx.Analyzers.ObjectModel; +using CliFx.Analyzers.Utils.Extensions; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace CliFx.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class OptionMustBeRequiredIfPropertyRequiredAnalyzer : AnalyzerBase +{ + public OptionMustBeRequiredIfPropertyRequiredAnalyzer() + : base( + "Options bound to required properties cannot be marked as non-required", + "This option cannot be marked as non-required because it's bound to a required property.") + { + } + + private void Analyze( + SyntaxNodeAnalysisContext context, + PropertyDeclarationSyntax propertyDeclaration, + IPropertySymbol property) + { + if (property.ContainingType is null) + return; + + if (!property.IsRequired) + return; + + var option = CommandOptionSymbol.TryResolve(property); + if (option is null) + return; + + if (option.IsRequired != false) + return; + + context.ReportDiagnostic( + CreateDiagnostic( + propertyDeclaration.Identifier.GetLocation() + ) + ); + } + + public override void Initialize(AnalysisContext context) + { + base.Initialize(context); + context.HandlePropertyDeclaration(Analyze); + } +} \ No newline at end of file diff --git a/CliFx.Analyzers/ParameterMustBeRequiredIfPropertyRequiredAnalyzer.cs b/CliFx.Analyzers/ParameterMustBeRequiredIfPropertyRequiredAnalyzer.cs new file mode 100644 index 0000000..3fc6311 --- /dev/null +++ b/CliFx.Analyzers/ParameterMustBeRequiredIfPropertyRequiredAnalyzer.cs @@ -0,0 +1,49 @@ +using CliFx.Analyzers.ObjectModel; +using CliFx.Analyzers.Utils.Extensions; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace CliFx.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class ParameterMustBeRequiredIfPropertyRequiredAnalyzer : AnalyzerBase +{ + public ParameterMustBeRequiredIfPropertyRequiredAnalyzer() + : base( + "Parameters bound to required properties cannot be marked as non-required", + "This parameter cannot be marked as non-required because it's bound to a required property.") + { + } + + private void Analyze( + SyntaxNodeAnalysisContext context, + PropertyDeclarationSyntax propertyDeclaration, + IPropertySymbol property) + { + if (property.ContainingType is null) + return; + + if (!property.IsRequired) + return; + + var parameter = CommandParameterSymbol.TryResolve(property); + if (parameter is null) + return; + + if (parameter.IsRequired != false) + return; + + context.ReportDiagnostic( + CreateDiagnostic( + propertyDeclaration.Identifier.GetLocation() + ) + ); + } + + public override void Initialize(AnalysisContext context) + { + base.Initialize(context); + context.HandlePropertyDeclaration(Analyze); + } +} \ No newline at end of file diff --git a/CliFx.Demo/Readme.md b/CliFx.Demo/Readme.md index a75638a..6241f7d 100644 --- a/CliFx.Demo/Readme.md +++ b/CliFx.Demo/Readme.md @@ -2,4 +2,4 @@ Sample command line interface for managing a library of books. -This demo project showcases basic CliFx functionality such as command routing, argument parsing, autogenerated help text. \ No newline at end of file +This demo project showcases basic CliFx functionality such as command routing, argument parsing, and autogenerated help text. \ No newline at end of file diff --git a/CliFx.Tests/CliFx.Tests.csproj b/CliFx.Tests/CliFx.Tests.csproj index 4cf9fe4..c6ab55b 100644 --- a/CliFx.Tests/CliFx.Tests.csproj +++ b/CliFx.Tests/CliFx.Tests.csproj @@ -9,7 +9,7 @@ - + diff --git a/CliFx.Tests/OptionBindingSpecs.cs b/CliFx.Tests/OptionBindingSpecs.cs index 0a51d9c..0d1b630 100644 --- a/CliFx.Tests/OptionBindingSpecs.cs +++ b/CliFx.Tests/OptionBindingSpecs.cs @@ -816,4 +816,40 @@ public class OptionBindingSpecs : SpecsBase exitCode.Should().NotBe(0); stdErr.Should().Contain("expects a single argument, but provided with multiple"); } + + [Fact] + public async Task Option_binding_fails_if_a_required_property_option_has_not_been_provided() + { + // Arrange + var commandType = DynamicCommandBuilder.Compile( + // language=cs + """ + [Command] + public class Command : ICommand + { + [CommandOption("foo")] + public required string Foo { get; set; } + + public ValueTask ExecuteAsync(IConsole console) => default; + } + """ + ); + + var application = new CliApplicationBuilder() + .AddCommand(commandType) + .UseConsole(FakeConsole) + .Build(); + + // Act + var exitCode = await application.RunAsync( + Array.Empty(), + new Dictionary() + ); + + var stdErr = FakeConsole.ReadErrorString(); + + // Assert + exitCode.Should().NotBe(0); + stdErr.Should().Contain("Missing required option(s)"); + } } \ No newline at end of file diff --git a/CliFx.Tests/Utils/DynamicCommandBuilder.cs b/CliFx.Tests/Utils/DynamicCommandBuilder.cs index a67cfbc..5d1591d 100644 --- a/CliFx.Tests/Utils/DynamicCommandBuilder.cs +++ b/CliFx.Tests/Utils/DynamicCommandBuilder.cs @@ -61,8 +61,7 @@ internal static class DynamicCommandBuilder var compilation = CSharpCompilation.Create( "CliFxTests_DynamicAssembly_" + Guid.NewGuid(), new[] {ast}, - ReferenceAssemblies - .Net60 + Net70.References.All .Append(MetadataReference.CreateFromFile(typeof(ICommand).Assembly.Location)) .Append(MetadataReference.CreateFromFile(typeof(DynamicCommandBuilder).Assembly.Location)), // DLL to avoid having to define the Main() method diff --git a/CliFx/Attributes/CommandOptionAttribute.cs b/CliFx/Attributes/CommandOptionAttribute.cs index 2337ae0..862bf91 100644 --- a/CliFx/Attributes/CommandOptionAttribute.cs +++ b/CliFx/Attributes/CommandOptionAttribute.cs @@ -32,6 +32,10 @@ public sealed class CommandOptionAttribute : Attribute /// Whether this option is required (default: false). /// If an option is required, the user will get an error if they don't set it. /// + /// + /// You can use the required keyword on the property (introduced in C# 11) to implicitly + /// set to true. + /// public bool IsRequired { get; set; } /// diff --git a/CliFx/Schema/OptionSchema.cs b/CliFx/Schema/OptionSchema.cs index 34e4f7b..1818dda 100644 --- a/CliFx/Schema/OptionSchema.cs +++ b/CliFx/Schema/OptionSchema.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Reflection; using System.Text; using CliFx.Attributes; +using CliFx.Utils.Extensions; namespace CliFx.Schema; @@ -101,6 +102,7 @@ internal partial class OptionSchema // The user may mistakenly specify dashes, thinking it's required, so trim them var name = attribute.Name?.TrimStart('-').Trim(); var environmentVariable = attribute.EnvironmentVariable?.Trim(); + var isRequired = attribute.IsRequired || property.IsRequired(); var description = attribute.Description?.Trim(); return new OptionSchema( @@ -108,7 +110,7 @@ internal partial class OptionSchema name, attribute.ShortName, environmentVariable, - attribute.IsRequired, + isRequired, description, attribute.Converter, attribute.Validators diff --git a/CliFx/Schema/ParameterSchema.cs b/CliFx/Schema/ParameterSchema.cs index b4f8a94..f7f2988 100644 --- a/CliFx/Schema/ParameterSchema.cs +++ b/CliFx/Schema/ParameterSchema.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Reflection; using CliFx.Attributes; +using CliFx.Utils.Extensions; namespace CliFx.Schema; @@ -53,13 +54,14 @@ internal partial class ParameterSchema return null; var name = attribute.Name?.Trim() ?? property.Name.ToLowerInvariant(); + var isRequired = attribute.IsRequired || property.IsRequired(); var description = attribute.Description?.Trim(); return new ParameterSchema( new BindablePropertyDescriptor(property), attribute.Order, name, - attribute.IsRequired, + isRequired, description, attribute.Converter, attribute.Validators diff --git a/CliFx/Utils/Extensions/CollectionExtensions.cs b/CliFx/Utils/Extensions/CollectionExtensions.cs index 23dfcf1..d61ae2b 100644 --- a/CliFx/Utils/Extensions/CollectionExtensions.cs +++ b/CliFx/Utils/Extensions/CollectionExtensions.cs @@ -1,4 +1,5 @@ -using System.Collections; +using System; +using System.Collections; using System.Collections.Generic; using System.Linq; @@ -37,4 +38,14 @@ internal static class CollectionExtensions dictionary .Cast() .ToDictionary(entry => (TKey) entry.Key, entry => (TValue) entry.Value, comparer); + + public static Array ToNonGenericArray(this IEnumerable source, Type elementType) + { + var sourceAsCollection = source as ICollection ?? source.ToArray(); + + var array = Array.CreateInstance(elementType, sourceAsCollection.Count); + sourceAsCollection.CopyTo(array, 0); + + return array; + } } \ No newline at end of file diff --git a/CliFx/Utils/Extensions/PropertyExtensions.cs b/CliFx/Utils/Extensions/PropertyExtensions.cs new file mode 100644 index 0000000..505dcda --- /dev/null +++ b/CliFx/Utils/Extensions/PropertyExtensions.cs @@ -0,0 +1,18 @@ +using System; +using System.Linq; +using System.Reflection; + +namespace CliFx.Utils.Extensions; + +internal static class PropertyExtensions +{ + public static bool IsRequired(this PropertyInfo propertyInfo) => + // Match attribute by name to avoid depending on .NET 7.0+ and to allow polyfilling + propertyInfo.GetCustomAttributes().Any(a => + string.Equals( + a.GetType().FullName, + "System.Runtime.CompilerServices.RequiredMemberAttribute", + StringComparison.Ordinal + ) + ); +} \ No newline at end of file diff --git a/CliFx/Utils/Extensions/TypeExtensions.cs b/CliFx/Utils/Extensions/TypeExtensions.cs index 8f05cc7..ed27f9a 100644 --- a/CliFx/Utils/Extensions/TypeExtensions.cs +++ b/CliFx/Utils/Extensions/TypeExtensions.cs @@ -11,7 +11,8 @@ internal static class TypeExtensions public static bool Implements(this Type type, Type interfaceType) => type.GetInterfaces().Contains(interfaceType); - public static Type? TryGetNullableUnderlyingType(this Type type) => Nullable.GetUnderlyingType(type); + public static Type? TryGetNullableUnderlyingType(this Type type) => + Nullable.GetUnderlyingType(type); public static Type? TryGetEnumerableUnderlyingType(this Type type) { @@ -44,16 +45,6 @@ internal static class TypeExtensions ); } - public static Array ToNonGenericArray(this IEnumerable source, Type elementType) - { - var sourceAsCollection = source as ICollection ?? source.ToArray(); - - var array = Array.CreateInstance(elementType, sourceAsCollection.Count); - sourceAsCollection.CopyTo(array, 0); - - return array; - } - public static bool IsToStringOverriden(this Type type) { var toStringMethod = type.GetMethod(nameof(ToString), Type.EmptyTypes); diff --git a/Readme.md b/Readme.md index b34f1db..e3c3b20 100644 --- a/Readme.md +++ b/Readme.md @@ -144,7 +144,7 @@ public class LogCommand : ICommand { // Order: 0 [CommandParameter(0, Description = "Value whose logarithm is to be found.")] - public double Value { get; init; } + public required double Value { get; init; } // Name: --base // Short name: -b @@ -382,7 +382,7 @@ If the user does not provide value for such option through command line argument [Command] public class AuthCommand : ICommand { - [CommandOption("token", IsRequired = true, EnvironmentVariable = "AUTH_TOKEN")] + [CommandOption("token", EnvironmentVariable = "AUTH_TOKEN")] public required string AuthToken { get; init; } public ValueTask ExecuteAsync(IConsole console) @@ -499,10 +499,10 @@ This special exception can be used to print an error message to the console, ret [Command] public class DivideCommand : ICommand { - [CommandOption("dividend", IsRequired = true)] + [CommandOption("dividend")] public required double Dividend { get; init; } - [CommandOption("divisor", IsRequired = true)] + [CommandOption("divisor")] public required double Divisor { get; init; } public ValueTask ExecuteAsync(IConsole console)