Merge pull request #394 from commandlineparser/fix/issue-380

Enhance error reporting when Options class has Value or Option attribute on unsettable property and no constructors available
commandlineparser · Feb 1, 2019 · 9f91088 · 9f91088
2 parents 3d4fec5 + d9efdac
commit 9f91088
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 42 deletions.
diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs
@@ -82,44 +82,19 @@ static class InstanceBuilder
                 var specPropsWithValue =
                     optionSpecPropsResult.SucceededWith().Concat(valueSpecPropsResult.SucceededWith()).Memorize();
 
-                var setPropertyErrors = new List<Error>();                
+                var setPropertyErrors = new List<Error>();
 
-                Func <T> buildMutable = () =>
+                //build the instance, determining if the type is mutable or not.
+                T instance;
+                if(typeInfo.IsMutable() == true)
                 {
-                    var mutable = factory.MapValueOrDefault(f => f(), Activator.CreateInstance<T>());
-                    setPropertyErrors.AddRange(mutable.SetProperties(specPropsWithValue, sp => sp.Value.IsJust(), sp => sp.Value.FromJustOrFail()));
-                    setPropertyErrors.AddRange(mutable.SetProperties(
-                                                 specPropsWithValue,
-                                                 sp => sp.Value.IsNothing() && sp.Specification.DefaultValue.IsJust(),
-                                                 sp => sp.Specification.DefaultValue.FromJustOrFail()));
-                    setPropertyErrors.AddRange(mutable.SetProperties(
-                                                 specPropsWithValue,
-                                                 sp =>
-                                                     sp.Value.IsNothing() && sp.Specification.TargetType == TargetType.Sequence
-                                                     && sp.Specification.DefaultValue.MatchNothing(),
-                                                 sp => sp.Property.PropertyType.GetTypeInfo().GetGenericArguments().Single().CreateEmptyArray()));
-                     return mutable;
-                };
-
-                Func<T> buildImmutable = () =>
+                    instance = BuildMutable(factory, specPropsWithValue, setPropertyErrors);
+                }
+                else
                 {
-                    var ctor = typeInfo.GetTypeInfo().GetConstructor((from sp in specProps select sp.Property.PropertyType).ToArray());
-                    var values = (from prms in ctor.GetParameters()
-                        join sp in specPropsWithValue on prms.Name.ToLower() equals sp.Property.Name.ToLower() into spv
-                        from sp in spv.DefaultIfEmpty()
-                        select
-                          sp == null
-                            ? specProps.First(s => String.Equals(s.Property.Name, prms.Name, StringComparison.CurrentCultureIgnoreCase))
-                              .Property.PropertyType.GetDefaultValue()
-                            : sp.Value.GetValueOrDefault(
-                                sp.Specification.DefaultValue.GetValueOrDefault(
-                                    sp.Specification.ConversionType.CreateDefaultForImmutable()))).ToArray();
-                    var immutable = (T)ctor.Invoke(values);
-                    return immutable;
-                };
-
-                var instance = typeInfo.IsMutable() ? buildMutable() : buildImmutable();
-                
+                    instance = BuildImmutable(typeInfo, factory, specProps, specPropsWithValue, setPropertyErrors);
+                }
+
                 var validationErrors = specPropsWithValue.Validate(SpecificationPropertyRules.Lookup(tokens));
 
                 var allErrors =
@@ -150,5 +125,67 @@ static class InstanceBuilder
 
             return result;
         }
+
+        private static T BuildMutable<T>(Maybe<Func<T>> factory, IEnumerable<SpecificationProperty> specPropsWithValue, List<Error> setPropertyErrors )
+        {
+            var mutable = factory.MapValueOrDefault(f => f(), Activator.CreateInstance<T>());
+
+            setPropertyErrors.AddRange(
+                mutable.SetProperties(
+                    specPropsWithValue, 
+                    sp => sp.Value.IsJust(), 
+                    sp => sp.Value.FromJustOrFail()
+                )
+            );
+
+            setPropertyErrors.AddRange(
+                mutable.SetProperties(
+                    specPropsWithValue,
+                    sp => sp.Value.IsNothing() && sp.Specification.DefaultValue.IsJust(),
+                    sp => sp.Specification.DefaultValue.FromJustOrFail()
+                )
+            );
+
+            setPropertyErrors.AddRange(
+                mutable.SetProperties(
+                    specPropsWithValue,
+                    sp => sp.Value.IsNothing() 
+                        && sp.Specification.TargetType == TargetType.Sequence 
+                        && sp.Specification.DefaultValue.MatchNothing(),
+                    sp => sp.Property.PropertyType.GetTypeInfo().GetGenericArguments().Single().CreateEmptyArray()
+                )
+            );
+
+            return mutable;
+        }
+
+        private static T BuildImmutable<T>(Type typeInfo, Maybe<Func<T>> factory, IEnumerable<SpecificationProperty> specProps, IEnumerable<SpecificationProperty> specPropsWithValue, List<Error> setPropertyErrors)
+        {
+            var ctor = typeInfo.GetTypeInfo().GetConstructor(
+                specProps.Select(sp => sp.Property.PropertyType).ToArray()
+            );
+
+            if(ctor == null)
+            {
+                throw new InvalidOperationException($"Type appears to be immutable, but no constructor found for type {typeInfo.FullName} to accept values.");
+            }
+
+            var values =
+                    (from prms in ctor.GetParameters()
+                     join sp in specPropsWithValue on prms.Name.ToLower() equals sp.Property.Name.ToLower() into spv
+                     from sp in spv.DefaultIfEmpty()
+                     select
+                 sp == null
+                        ? specProps.First(s => String.Equals(s.Property.Name, prms.Name, StringComparison.CurrentCultureIgnoreCase))
+                        .Property.PropertyType.GetDefaultValue()
+                        : sp.Value.GetValueOrDefault(
+                            sp.Specification.DefaultValue.GetValueOrDefault(
+                                sp.Specification.ConversionType.CreateDefaultForImmutable()))).ToArray();
+
+            var immutable = (T)ctor.Invoke(values);
+
+            return immutable;
+        }
+
     }
-}
+}
diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs
@@ -122,12 +122,13 @@ public static object GetDefaultValue(this Type type)
 
         public static bool IsMutable(this Type type)
         {
-            Func<bool> isMutable = () => {
-                var props = type.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).Any(p => p.CanWrite);
-                var fields = type.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any();
-                return props || fields;
-            };
-            return type != typeof(object) ? isMutable() : true;
+            if(type == typeof(object))
+                return true;
+
+            var props = type.GetTypeInfo().GetProperties(BindingFlags.Public | BindingFlags.Instance).Any(p => p.CanWrite);
+            var fields = type.GetTypeInfo().GetFields(BindingFlags.Public | BindingFlags.Instance).Any();
+
+            return props || fields;
         }
 
         public static object CreateDefaultForImmutable(this Type type)

diff --git a/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs b/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs
@@ -1133,6 +1133,21 @@ public void Parse_TimeSpan()
             // Teardown
         }
 
+        [Fact]
+        public void OptionClass_IsImmutable_HasNoCtor()
+        {
+            Action act = () => InvokeBuild<ValueWithNoSetterOptions>(new string[] { "Test" }, false, false);
+
+            act.Should().Throw<InvalidOperationException>();
+        }
+
+        private class ValueWithNoSetterOptions
+        {
+            [Value(0, MetaName = "Test", Default = 0)]
+            public int TestValue { get; }
+        }
+
+
         public static IEnumerable<object[]> RequiredValueStringData
         {
             get