Immutable constructor argument order issues result in NullReferenceException

[ohadschn]

For example:

    class FooOptions
    {
        public FooOptions(string option, Uri foo)
        {
            Foo = foo;
            Option = option;
        }

        [Option("foo")]
        public Uri Foo { get; }    

        [Option("option")]
        public string Option { get;  }
    }

Will result in:

nhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
   at CommandLine.Infrastructure.ReflectionHelper.CreateDefaultImmutableInstance(Type type, Type[] constructorTypes)
   at CommandLine.Core.ReflectionExtensions.AutoDefault(Type type)
   at CommandLine.Core.InstanceChooser.<>c__DisplayClass1_0.<MatchVerb>b__1()
   at CommandLine.Core.InstanceBuilder.<>c__0`1.<Build>b__0_0(Func`1 f)
   at CSharpx.MaybeExtensions.MapValueOrDefault[T1,T2](Maybe`1 maybe, Func`2 func, T2 noneValue)
   at CommandLine.Core.InstanceBuilder.Build[T](Maybe`1 factory, Func`3 tokenizer, IEnumerable`1 arguments, StringComparer nameComparer, Boolean ignoreValueCase, CultureInfo parsingCulture, IEnumerable`1 nonFatalErrors)
   at CommandLine.Core.InstanceChooser.MatchVerb(Func`3 tokenizer, IEnumerable`1 verbs, IEnumerable`1 arguments, StringComparer nameComparer, CultureInfo parsingCulture, IEnumerable`1 nonFatalErrors)
   at CommandLine.Core.InstanceChooser.<>c__DisplayClass0_0.<Choose>b__0()
   at CommandLine.Core.InstanceChooser.Choose(Func`3 tokenizer, IEnumerable`1 types, IEnumerable`1 arguments, StringComparer nameComparer, CultureInfo parsingCulture, IEnumerable`1 nonFatalErrors)
   at CommandLine.Parser.ParseArguments(IEnumerable`1 args, Type[] types)
   at CommandLine.ParserExtensions.ParseArguments[T1,T2](Parser parser, IEnumerable`1 args)
   at CommandLineParserTest.Program.Main()

The ideal solution IMO would be to find the right ctor by param names (rather than order). But even if that's not implemented, a better and more meaningful exception would be very helpful in debugging such issues.

[nemec]

You're right, in theory the order of properties/fields in a class is undefined (although in practice, it's processed line-by-line). We should not rely on the order as it's defined in the class.

[ohadschn]

Agreed. BTW, this can get even more confusing when option class inheritance is in play. Spoiler: the inherited options (from the base options class) are expected at the end of the constructor parameter list.

[johndkane]

I'm opening up conversation about this issue because it interests me to find a solution.

Throwing out a few ideas for consideration and discussion:

• To not introduce breaking changes the default/classic behaviour should remain intact which is: the immutable logic assumes the constructor parameters are in the same order as the properties are found on the class, top to bottom;
• any new auto-mapping mechanism (by parameter name) should be explicitly enabled by the developer...
• Introduce a new ParserSettings setting to cause mapping of incoming arguments to ctor by param names;
• introduce the parser setting AutoMap=false(default)/true, or allow it to accept an enum value to additionally support case matching like enum {Off, Exact, CaseInsensitive}.
• Alternatively, because .NET Attributes are already in use in the project, a new [AutoMap] attribute can be introduced (allowed on class or constructor) to invoke the auto-mapping.
• The benefit of putting an [AutoMap] attribute on the constructor instead of class is to select a specific constructor in an overloaded scenario.

Is there any interest in supporting direct property setters to bypass the constructor? This might help reduce complication of option class inheritance.

[ohadschn]

Is there any interest in supporting direct property setters to bypass the constructor?

This defeats the point of immutable types (I suppose one could make the setters private but it would still not be truly immutable).

[moh-hassan]

The Exception, as described in #499, have been changed to:

Type appears to be immutable, but no constructor found for type <typeInfo.FullName> to accept values.

[ohadschn]

@moh-hassan I would add some text like:

Note that constructor argument order must match property definition order in the options class (inherited options are expected at the end of the constructor parameter list).

Otherwise a client may not understand why no matching constructor was found...

[brian-reichle]

You're right, in theory the order of properties/fields in a class is undefined (although in practice, it's processed line-by-line). We should not rely on the order as it's defined in the class.

I have had a report of at least one case where the properties returned by Type.GetProperties() did not match document order and as a result, it threw an exception. I do not yet know what caused it to return them in a different order.

sealed class CommandLineArgs
{
    public CommandLineArgs(string inputFilename, string? outputFilename)
    {
        InputFilename = inputFilename;
        OutputFilename = outputFilename;
    }

    [Value(0, Required = true, Hidden = true)]
    public string InputFilename { get; }

    [Option('o', "out")]
    public string? OutputFilename { get; }
}

  Unhandled Exception: System.InvalidOperationException: Type WTG.Glow.ConfigurationModel.Data.Generator.CommandLineArgs appears to be Immutable with invalid constructor. Check that constructor arguments have the same name and order of their underlying Type.  Constructor Parameters can be ordered as: '(outputfilename, inputfilename)'
     at CommandLine.Core.InstanceBuilder.BuildImmutable[T](Type typeInfo, Maybe`1 factory, IEnumerable`1 specProps, IEnumerable`1 specPropsWithValue, List`1 setPropertyErrors)
     at CommandLine.Core.InstanceBuilder.<>c__DisplayClass0_0`1.<Build>b__5()
     at CommandLine.Parser.ParseArguments[T](IEnumerable`1 args)
     at WTG.Glow.ConfigurationModel.Data.Generator.Program.Main(String[] args)

[brian-reichle]

• To not introduce breaking changes the default/classic behaviour should remain intact which is: the immutable logic assumes the constructor parameters are in the same order as the properties are found on the class, top to bottom;

As it currently stands, both the order and the names are required to match. If we simply remove the requirement for the order to match, then the bulk of the change in behaviour will be with code that is already broken. The Only case I can think of where code transitions from "working" to "broken" is if people define multiple parameters/properties/fields where the name only differs by case. Such code would seem problematic to me even without this change.

If we are worried about this change, then perhaps add another property to ValueAttribute and OptionAttribute which allows the developer to explicitly specify the constructor argument name (using a case sensitive match) then fall back to the existing behaviour for "whatever is left over".

[brian-reichle]

This appears to be culture related. In particular, if I switch to tr-TR then the expected argument order changes causing the exception. If I set the culture to en-US then it works fine.

I would consider this to be a fairly significant bug as it means that immutable argument objects can't be used in any application that might reasonably be used in a region like Turkey.