From 2377f5954777fc2044c91f9a42711bc3d3fd6395 Mon Sep 17 00:00:00 2001 From: Andrey Nasonov Date: Sat, 5 May 2018 15:32:20 +0300 Subject: [PATCH 1/5] Issue #283: report exceptions in property.SetValue as parsing errors ToDo: add tests and nameinfo --- src/CommandLine/CommandLine.csproj | 12 +++- src/CommandLine/Core/InstanceBuilder.cs | 30 +++++----- src/CommandLine/Core/ReflectionExtensions.cs | 59 +++++++------------- src/CommandLine/Error.cs | 28 +++++++++- src/CommandLine/Text/SentenceBuilder.cs | 2 + 5 files changed, 74 insertions(+), 57 deletions(-) diff --git a/src/CommandLine/CommandLine.csproj b/src/CommandLine/CommandLine.csproj index 8f829c9f..b445db7c 100644 --- a/src/CommandLine/CommandLine.csproj +++ b/src/CommandLine/CommandLine.csproj @@ -169,8 +169,16 @@ - - + + + + ..\..\packages\FSharp.Core\lib\net40\FSharp.Core.dll + True + True + + + + ..\..\packages\FSharp.Core\lib\portable-net45+netcore45\FSharp.Core.dll diff --git a/src/CommandLine/Core/InstanceBuilder.cs b/src/CommandLine/Core/InstanceBuilder.cs index 0c24fddd..fd9ac4d7 100644 --- a/src/CommandLine/Core/InstanceBuilder.cs +++ b/src/CommandLine/Core/InstanceBuilder.cs @@ -80,22 +80,23 @@ public static ParserResult Build( var specPropsWithValue = optionSpecPropsResult.SucceededWith().Concat(valueSpecPropsResult.SucceededWith()); - Func buildMutable = () => + var setPropertyErrors = new List(); + + Func buildMutable = () => { var mutable = factory.MapValueOrDefault(f => f(), Activator.CreateInstance()); - mutable = - mutable.SetProperties(specPropsWithValue, sp => sp.Value.IsJust(), sp => sp.Value.FromJustOrFail()) - .SetProperties( - specPropsWithValue, - sp => sp.Value.IsNothing() && sp.Specification.DefaultValue.IsJust(), - sp => sp.Specification.DefaultValue.FromJustOrFail()) - .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; + 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 buildImmutable = () => @@ -121,6 +122,7 @@ join sp in specPropsWithValue on prms.Name.ToLower() equals sp.Property.Name.ToL .Concat(optionSpecPropsResult.SuccessfulMessages()) .Concat(valueSpecPropsResult.SuccessfulMessages()) .Concat(validationErrors) + .Concat(setPropertyErrors) .Memorize(); var warnings = from e in allErrors where nonFatalErrors.Contains(e.Tag) select e; diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs index 068b3034..0e8811ca 100644 --- a/src/CommandLine/Core/ReflectionExtensions.cs +++ b/src/CommandLine/Core/ReflectionExtensions.cs @@ -80,51 +80,30 @@ public static TargetType ToTargetType(this Type type) : TargetType.Scalar; } - public static T SetProperties( + public static IEnumerable SetProperties( this T instance, IEnumerable specProps, Func predicate, Func selector) { - return specProps.Where(predicate).Aggregate( - instance, - (current, specProp) => - { - specProp.Property.SetValue(current, selector(specProp)); - return instance; - }); - } - - private static T SetValue(this PropertyInfo property, T instance, object value) - { - Action fail = inner => { - throw new InvalidOperationException("Cannot set value to target instance.", inner); - }; - - try - { - property.SetValue(instance, value, null); - } -#if !PLATFORM_DOTNET - catch (TargetException e) - { - fail(e); - } -#endif - catch (TargetParameterCountException e) - { - fail(e); - } - catch (MethodAccessException e) - { - fail(e); - } - catch (TargetInvocationException e) - { - fail(e); - } - - return instance; + return specProps.Where(predicate).SelectMany(specProp => specProp.Property.SetValue(instance, selector(specProp))); + } + + private static IEnumerable SetValue(this PropertyInfo property, T instance, object value) + { + try + { + property.SetValue(instance, value, null); + return Enumerable.Empty(); + } + catch (TargetInvocationException e) + { + return new[] { new SetValueExceptionError(e.InnerException) }; + } + catch (Exception e) + { + return new[] { new SetValueExceptionError(e) }; + } } public static object CreateEmptyArray(this Type type) diff --git a/src/CommandLine/Error.cs b/src/CommandLine/Error.cs index 475ac8e3..0cbf4938 100644 --- a/src/CommandLine/Error.cs +++ b/src/CommandLine/Error.cs @@ -60,7 +60,11 @@ public enum ErrorType /// /// Value of type. /// - VersionRequestedError + VersionRequestedError, + /// + /// Value of type. + /// + SetValueExceptionError } /// @@ -471,4 +475,26 @@ internal VersionRequestedError() { } } + + /// + /// Models as error generated when exception is thrown at Property.SetValue + /// + public sealed class SetValueExceptionError : Error + { + private readonly Exception exception; + + internal SetValueExceptionError(Exception exception) + : base(ErrorType.SetValueExceptionError) + { + this.exception = exception; + } + + /// + /// The expection thrown from Property.SetValue + /// + public Exception Exception + { + get { return exception; } + } + } } \ No newline at end of file diff --git a/src/CommandLine/Text/SentenceBuilder.cs b/src/CommandLine/Text/SentenceBuilder.cs index 1c28e959..44ca5b8f 100644 --- a/src/CommandLine/Text/SentenceBuilder.cs +++ b/src/CommandLine/Text/SentenceBuilder.cs @@ -137,6 +137,8 @@ public override Func FormatError case ErrorType.RepeatedOptionError: return "Option '".JoinTo(((RepeatedOptionError)error).NameInfo.NameText, "' is defined multiple times."); + case ErrorType.SetValueExceptionError: + return "Error setting option value: ".JoinTo(((SetValueExceptionError)error).Exception.Message); } throw new InvalidOperationException(); }; From b8d0352978e6969f07ed27d4baaa729e331bf97d Mon Sep 17 00:00:00 2001 From: Andrey Nasonov Date: Sat, 5 May 2018 15:43:12 +0300 Subject: [PATCH 2/5] Issue #283: Add NameInfo information to SetValueExceptionError --- src/CommandLine/Core/ReflectionExtensions.cs | 36 ++++++++++---------- src/CommandLine/Text/SentenceBuilder.cs | 3 +- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs index 0e8811ca..20ec8209 100644 --- a/src/CommandLine/Core/ReflectionExtensions.cs +++ b/src/CommandLine/Core/ReflectionExtensions.cs @@ -86,24 +86,24 @@ public static IEnumerable SetProperties( Func predicate, Func selector) { - return specProps.Where(predicate).SelectMany(specProp => specProp.Property.SetValue(instance, selector(specProp))); - } - - private static IEnumerable SetValue(this PropertyInfo property, T instance, object value) - { - try - { - property.SetValue(instance, value, null); - return Enumerable.Empty(); - } - catch (TargetInvocationException e) - { - return new[] { new SetValueExceptionError(e.InnerException) }; - } - catch (Exception e) - { - return new[] { new SetValueExceptionError(e) }; - } + return specProps.Where(predicate).SelectMany(specProp => specProp.SetValue(instance, selector(specProp))); + } + + private static IEnumerable SetValue(this SpecificationProperty specProp, T instance, object value) + { + try + { + specProp.Property.SetValue(instance, value, null); + return Enumerable.Empty(); + } + catch (TargetInvocationException e) + { + return new[] { new SetValueExceptionError(specProp.Specification.FromSpecification(), e.InnerException) }; + } + catch (Exception e) + { + return new[] { new SetValueExceptionError(specProp.Specification.FromSpecification(), e) }; + } } public static object CreateEmptyArray(this Type type) diff --git a/src/CommandLine/Text/SentenceBuilder.cs b/src/CommandLine/Text/SentenceBuilder.cs index 44ca5b8f..1c150b67 100644 --- a/src/CommandLine/Text/SentenceBuilder.cs +++ b/src/CommandLine/Text/SentenceBuilder.cs @@ -138,7 +138,8 @@ public override Func FormatError return "Option '".JoinTo(((RepeatedOptionError)error).NameInfo.NameText, "' is defined multiple times."); case ErrorType.SetValueExceptionError: - return "Error setting option value: ".JoinTo(((SetValueExceptionError)error).Exception.Message); + var setValueError = (SetValueExceptionError)error; + return "Error setting value to option '".JoinTo(setValueError.NameInfo.NameText, "': ", setValueError.Exception.Message); } throw new InvalidOperationException(); }; From 39901f734ca616f42133785f1a60e6a426cb1cae Mon Sep 17 00:00:00 2001 From: Andrey Nasonov Date: Sun, 6 May 2018 00:04:53 +0300 Subject: [PATCH 3/5] Include the value that has caused an exception to SetValueExceptionError class --- src/CommandLine/Core/ReflectionExtensions.cs | 4 ++-- src/CommandLine/Error.cs | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/CommandLine/Core/ReflectionExtensions.cs b/src/CommandLine/Core/ReflectionExtensions.cs index 20ec8209..1fd3e515 100644 --- a/src/CommandLine/Core/ReflectionExtensions.cs +++ b/src/CommandLine/Core/ReflectionExtensions.cs @@ -98,11 +98,11 @@ private static IEnumerable SetValue(this SpecificationProperty specPro } catch (TargetInvocationException e) { - return new[] { new SetValueExceptionError(specProp.Specification.FromSpecification(), e.InnerException) }; + return new[] { new SetValueExceptionError(specProp.Specification.FromSpecification(), e.InnerException, value) }; } catch (Exception e) { - return new[] { new SetValueExceptionError(specProp.Specification.FromSpecification(), e) }; + return new[] { new SetValueExceptionError(specProp.Specification.FromSpecification(), e, value) }; } } diff --git a/src/CommandLine/Error.cs b/src/CommandLine/Error.cs index 0cbf4938..2b586912 100644 --- a/src/CommandLine/Error.cs +++ b/src/CommandLine/Error.cs @@ -479,14 +479,16 @@ internal VersionRequestedError() /// /// Models as error generated when exception is thrown at Property.SetValue /// - public sealed class SetValueExceptionError : Error + public sealed class SetValueExceptionError : NamedError { private readonly Exception exception; + private readonly object value; - internal SetValueExceptionError(Exception exception) - : base(ErrorType.SetValueExceptionError) + internal SetValueExceptionError(NameInfo nameInfo, Exception exception, object value) + : base(ErrorType.SetValueExceptionError, nameInfo) { this.exception = exception; + this.value = value; } /// @@ -496,5 +498,14 @@ public Exception Exception { get { return exception; } } + + /// + /// The value that had to be set to the property + /// + public object Value + { + get { return value; } + } + } } \ No newline at end of file From fe5c2239ff3d33752e1dc23ffc1e7a8f387501c3 Mon Sep 17 00:00:00 2001 From: Andrey Nasonov Date: Sun, 6 May 2018 00:06:17 +0300 Subject: [PATCH 4/5] Add a test for property throwing an exception --- .../CommandLine.Tests.csproj | 1 + ...ptions_With_Property_Throwing_Exception.cs | 29 +++++++++++++++++++ .../Unit/Core/InstanceBuilderTests.cs | 17 +++++++++++ 3 files changed, 47 insertions(+) create mode 100644 tests/CommandLine.Tests/Fakes/Options_With_Property_Throwing_Exception.cs diff --git a/tests/CommandLine.Tests/CommandLine.Tests.csproj b/tests/CommandLine.Tests/CommandLine.Tests.csproj index 5914a3a9..90b0ccd5 100644 --- a/tests/CommandLine.Tests/CommandLine.Tests.csproj +++ b/tests/CommandLine.Tests/CommandLine.Tests.csproj @@ -61,6 +61,7 @@ + diff --git a/tests/CommandLine.Tests/Fakes/Options_With_Property_Throwing_Exception.cs b/tests/CommandLine.Tests/Fakes/Options_With_Property_Throwing_Exception.cs new file mode 100644 index 00000000..031dcb6c --- /dev/null +++ b/tests/CommandLine.Tests/Fakes/Options_With_Property_Throwing_Exception.cs @@ -0,0 +1,29 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace CommandLine.Tests.Fakes +{ + class Options_With_Property_Throwing_Exception + { + private string optValue; + + [Option('e')] + public string OptValue + { + get + { + return optValue; + } + set + { + if (value != "good") + throw new ArgumentException("Invalid value, only accept 'good' value"); + + optValue = value; + } + } + } +} diff --git a/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs b/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs index 7d7544e6..6ff592e4 100644 --- a/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs +++ b/tests/CommandLine.Tests/Unit/Core/InstanceBuilderTests.cs @@ -984,6 +984,23 @@ public void Parse_to_type_with_single_string_ctor_builds_up_correct_instance() // Teardown } + [Fact] + public void Parse_option_with_exception_thrown_from_setter_generates_SetValueExceptionError() + { + // Fixture setup + var expectedResult = new[] { new SetValueExceptionError(new NameInfo("e", ""), new ArgumentException(), "bad") }; + + // Exercize system + var result = InvokeBuild( + new[] { "-e", "bad" }); + + // Verify outcome + ((NotParsed)result).Errors.ShouldBeEquivalentTo(expectedResult); + + // Teardown + } + + [Theory] [InlineData(new[] { "--stringvalue", "x-" }, "x-")] [InlineData(new[] { "--stringvalue", "x--" }, "x--")] From 2a1e13cdddd8f6b0759d90b262e41a745896a526 Mon Sep 17 00:00:00 2001 From: Andrey Nasonov Date: Sun, 6 May 2018 00:17:40 +0300 Subject: [PATCH 5/5] Revert changes to CommandLine.csproj --- src/CommandLine/CommandLine.csproj | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/CommandLine/CommandLine.csproj b/src/CommandLine/CommandLine.csproj index b445db7c..8f829c9f 100644 --- a/src/CommandLine/CommandLine.csproj +++ b/src/CommandLine/CommandLine.csproj @@ -169,16 +169,8 @@ - - - - ..\..\packages\FSharp.Core\lib\net40\FSharp.Core.dll - True - True - - - - + + ..\..\packages\FSharp.Core\lib\portable-net45+netcore45\FSharp.Core.dll