From 5572f223b87c2bf75df2bc7f1ce0bf510e90e3d9 Mon Sep 17 00:00:00 2001 From: Abdallah Darwish Date: Wed, 31 May 2023 04:46:46 +0400 Subject: [PATCH 1/5] Fix --- tests/CommandLine.Tests/Unit/Issue890Tests.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 tests/CommandLine.Tests/Unit/Issue890Tests.cs diff --git a/tests/CommandLine.Tests/Unit/Issue890Tests.cs b/tests/CommandLine.Tests/Unit/Issue890Tests.cs new file mode 100644 index 00000000..9bb97e58 --- /dev/null +++ b/tests/CommandLine.Tests/Unit/Issue890Tests.cs @@ -0,0 +1,36 @@ +using System.Linq; +using CommandLine.Tests.Fakes; +using CommandLine.Text; +using FluentAssertions; +using Xunit; +using Xunit.Abstractions; + +//Issue #890 +//When options class is mutable but doesn't have a parameterless constructor parsing fails. + +namespace CommandLine.Tests.Unit +{ + public class Issue890Tests + { + [Fact] + public void Create_mutable_instance_without_parameterless_ctor_should_not_fail() + { + var result = Parser.Default.ParseArguments(new[] { "-a" }); + + Assert.Equal(ParserResultType.Parsed, result.Tag); + Assert.NotNull(result.Value); + Assert.Equal("a", result.Value.Option); + + Assert.Empty(result.Errors); + } + private class Options + { + public Options(string option) + { + Option = option; + } + [Option("a", Required = false)] + public string Option { get; set; } + } + } +} From 4b212d001de449e2efb62728829effd429bcb9d7 Mon Sep 17 00:00:00 2001 From: Abdallah Darwish Date: Wed, 31 May 2023 04:47:11 +0400 Subject: [PATCH 2/5] fix --- src/CommandLine/Core/ReflectionExtensions.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs index 622e1e6e..3d0e59e5 100644 --- a/src/CommandLine/Core/ReflectionExtensions.cs +++ b/src/CommandLine/Core/ReflectionExtensions.cs @@ -131,18 +131,18 @@ public static bool IsMutable(this Type type) // Find all inherited defined properties and fields on the type var inheritedTypes = type.GetTypeInfo().FlattenHierarchy().Select(i => i.GetTypeInfo()); - foreach (var inheritedType in inheritedTypes) + foreach (var inheritedType in inheritedTypes) { - if ( - inheritedType.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).Any(p => p.CanWrite) || - inheritedType.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any() - ) + if (inheritedType.GetTypeInfo().GetConstructor(Type.EmptyTypes) == null || ( + inheritedType.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).All(p => !p.CanWrite) && + !inheritedType.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any() + )) { - return true; + return false; } } - return false; + return true; } public static object CreateDefaultForImmutable(this Type type) From b1aac40bac14c08ef04b65418b734127cbe8a7bc Mon Sep 17 00:00:00 2001 From: Abdallah Darwish Date: Wed, 31 May 2023 05:09:55 +0400 Subject: [PATCH 3/5] Fix test --- tests/CommandLine.Tests/Unit/Issue890Tests.cs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/CommandLine.Tests/Unit/Issue890Tests.cs b/tests/CommandLine.Tests/Unit/Issue890Tests.cs index 9bb97e58..e1eab1bc 100644 --- a/tests/CommandLine.Tests/Unit/Issue890Tests.cs +++ b/tests/CommandLine.Tests/Unit/Issue890Tests.cs @@ -12,25 +12,27 @@ namespace CommandLine.Tests.Unit { public class Issue890Tests { + const char OptionSwitch = 'o'; [Fact] public void Create_mutable_instance_without_parameterless_ctor_should_not_fail() { - var result = Parser.Default.ParseArguments(new[] { "-a" }); + const string optionValue = "val"; + + var result = Parser.Default.ParseArguments(new string[] { $"-{OptionSwitch}", optionValue }); Assert.Equal(ParserResultType.Parsed, result.Tag); Assert.NotNull(result.Value); - Assert.Equal("a", result.Value.Option); - + Assert.Equal(optionValue, result.Value.Opt); Assert.Empty(result.Errors); } private class Options { - public Options(string option) + public Options(string opt) { - Option = option; + Opt = opt; } - [Option("a", Required = false)] - public string Option { get; set; } + [Option(OptionSwitch, "opt", Required = false)] + public string Opt { get; set; } } } } From 8ffc8d609dd42d804812733938bc4f2a0747c401 Mon Sep 17 00:00:00 2001 From: Abdallah Darwish Date: Wed, 31 May 2023 05:31:30 +0400 Subject: [PATCH 4/5] New approach --- src/CommandLine/Core/InstanceBuilder.cs | 6 +++--- src/CommandLine/Core/ReflectionExtensions.cs | 19 ++++++++++++------- src/CommandLine/Parser.cs | 2 +- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs index f48127b1..2a85fc15 100644 --- a/src/CommandLine/Core/InstanceBuilder.cs +++ b/src/CommandLine/Core/InstanceBuilder.cs @@ -4,10 +4,10 @@ using System.Collections.Generic; using System.Globalization; using System.Linq; +using System.Reflection; using CommandLine.Infrastructure; using CSharpx; using RailwaySharp.ErrorHandling; -using System.Reflection; namespace CommandLine.Core { @@ -62,7 +62,7 @@ public static ParserResult Build( .Memoize(); Func makeDefault = () => - typeof(T).IsMutable() + typeof(T).IsMutable() && typeof(T).HasParameterlessConstructor() ? factory.MapValueOrDefault(f => f(), () => Activator.CreateInstance()) : ReflectionHelper.CreateDefaultImmutableInstance( (from p in specProps select p.Specification.ConversionType).ToArray()); @@ -110,7 +110,7 @@ public static ParserResult Build( //build the instance, determining if the type is mutable or not. T instance; - if(typeInfo.IsMutable() == true) + if (typeInfo.IsMutable() && typeInfo.HasParameterlessConstructor()) { instance = BuildMutable(factory, specPropsWithValue, setPropertyErrors); } diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs index 3d0e59e5..9dbb9a1d 100644 --- a/src/CommandLine/Core/ReflectionExtensions.cs +++ b/src/CommandLine/Core/ReflectionExtensions.cs @@ -133,16 +133,21 @@ public static bool IsMutable(this Type type) foreach (var inheritedType in inheritedTypes) { - if (inheritedType.GetTypeInfo().GetConstructor(Type.EmptyTypes) == null || ( - inheritedType.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).All(p => !p.CanWrite) && - !inheritedType.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any() - )) + if ( + inheritedType.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).Any(p => p.CanWrite) || + inheritedType.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any() + ) { - return false; + return true; } } - return true; + return false; + } + + public static bool HasParameterlessConstructor(this Type type) + { + return type.GetTypeInfo().GetConstructor(Type.EmptyTypes) != null; } public static object CreateDefaultForImmutable(this Type type) @@ -156,7 +161,7 @@ public static object CreateDefaultForImmutable(this Type type) public static object AutoDefault(this Type type) { - if (type.IsMutable()) + if (type.IsMutable() && type.HasParameterlessConstructor()) { return Activator.CreateInstance(type); } diff --git a/src/CommandLine/Parser.cs b/src/CommandLine/Parser.cs index 4301aa52..e7f1e939 100644 --- a/src/CommandLine/Parser.cs +++ b/src/CommandLine/Parser.cs @@ -87,7 +87,7 @@ public ParserResult ParseArguments(IEnumerable args) { if (args == null) throw new ArgumentNullException("args"); - var factory = typeof(T).IsMutable() + var factory = typeof(T).IsMutable() && typeof(T).HasParameterlessConstructor() ? Maybe.Just>(Activator.CreateInstance) : Maybe.Nothing>(); From b484d50e401b549d6a6d308a24a126b2471d2ce3 Mon Sep 17 00:00:00 2001 From: Abdallah Darwish Date: Wed, 31 May 2023 05:48:12 +0400 Subject: [PATCH 5/5] Use factory if provided --- src/CommandLine/Core/InstanceBuilder.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs index 2a85fc15..0690aaf9 100644 --- a/src/CommandLine/Core/InstanceBuilder.cs +++ b/src/CommandLine/Core/InstanceBuilder.cs @@ -5,6 +5,7 @@ using System.Globalization; using System.Linq; using System.Reflection; +using System.Threading.Tasks; using CommandLine.Infrastructure; using CSharpx; using RailwaySharp.ErrorHandling; @@ -61,11 +62,12 @@ public static ParserResult Build( .OfType() .Memoize(); - Func makeDefault = () => - typeof(T).IsMutable() && typeof(T).HasParameterlessConstructor() - ? factory.MapValueOrDefault(f => f(), () => Activator.CreateInstance()) - : ReflectionHelper.CreateDefaultImmutableInstance( - (from p in specProps select p.Specification.ConversionType).ToArray()); + Func makeDefaultImmutable = () => ReflectionHelper.CreateDefaultImmutableInstance((from p in specProps select p.Specification.ConversionType).ToArray()); + + Func makeDefault = + typeof(T).IsMutable() + ? () => factory.MapValueOrDefault(f => f(), () => typeof(T).HasParameterlessConstructor() ? Activator.CreateInstance() : makeDefaultImmutable()) + : makeDefaultImmutable; Func, ParserResult> notParsed = errs => new NotParsed(makeDefault().GetType().ToTypeInfo(), errs); @@ -110,7 +112,7 @@ public static ParserResult Build( //build the instance, determining if the type is mutable or not. T instance; - if (typeInfo.IsMutable() && typeInfo.HasParameterlessConstructor()) + if (typeInfo.IsMutable() && (!factory.IsNothing() || typeInfo.HasParameterlessConstructor())) { instance = BuildMutable(factory, specPropsWithValue, setPropertyErrors); }