New Feature "Field Initialization" in 2.1.1 Results in System.InvalidProgramException

[BrunoJuchli]

2.1.1 introduced a new feature "Field Initialization", see PR 133.

Unfortunately sometimes the generated IL is incorrect, which results in a System.InvalidProgramException.

Minimal Repro

The easy way: see PR #144

The issue can be reproduced using the following code:

class Program
{
    static void Main(string[] args)
    {
        // this will throw a
        // System.InvalidProgramException : Common Language Runtime detected an invalid program.
        Activator.CreateInstance<Child>();

        Console.WriteLine("program ended");
    }
}

public class Child : Base<int>
{
    private bool value;

    private Child(int enumValue)
        : base(enumValue)
    {
        this.value = true;
    }
}

public abstract class Base<TValue>
{
    protected Base(TValue value)
    {
    }
}

Note, that this is a minimal repro:

• removing the <TValue> generic parmeter from Base will make the problem go away
• removing the TValue value argument from Base ctor will make the problem go away
• removing the int enumValue argument from Child ctor will make the problem go away
• removing the this.value = true from the Child ctor will make the problem go away
  -- note, though, that the assignment can be replaced with a call to a local method like this.Foo()

Workaround

This can be worked around by opting out of field initialization via FodyWeavers.xml:

<EmptyConstructor DoNotPreserveInitializers="true" />

Discussion of Issue Resolution

May I ask @NorekZ what your use case for Field Initialization / EmptyConstructor.Fody is?

Our use case is:
We use EmptyConstructor.Fody and Virtuosity.Fody in order for our types to become mockable (i.E. inherited from and methods overridable) without us needing to create an interface for every type.
We're only interested in the type, not the behavior, thus EmptyConstructor.Fody adding any logic to the constructor is just superfluous. And in some cases, as seen above, actually produces invalid IL.

I'm asking you on how you're using it in order to better understand the user base of EmptyConstructor.Fody, this again in order to provide information for a decision:

Should this feature be changed to opt-in instead of opt-out?

If most users will need the behavior, it obviously makes sense to be opt out.
If most users don't need it, but it doesn't interfere with their usage, it may be opt out.
If most users don't need it and lots of them need to turn it off, then it should be opt out.

As I understand, this is a a new feature, so it's easy to conclude that users of EmptyConstructor.Fody 2.1.0 and before didn't need the behavior. So the question is: how will the user base grow with this feature?

Here's another take:

• the new feature (currently) breaks some of the old usages (as experienced by us)
• in some cases, the new feature produces invalid IL. Should it be opt-in until it's stable enough to not produce invalid IL? Or do we live with it (edge case?)? Or do we try to fix it ASAP?

[NorekZ]

Hello @BrunoJuchli ,

its quite suprising for me that the change can lead to invalid IL because I followed description of how compiler handles property & field intializers.

Thank you for providing minimal repro. Currently I have no time for looking on it more deeply, but I will try in next weeks.

The reason I decided for making the feature as opt-out is that I assumed the former behavior was considered buggy/not intended in issue #3 .

Our use case is:
We want POCO classes to both

1. be deserializable (by Newtonsoft.Json/System.Text.Json libraries), so all properties should have public getter and setter
2. enforce some rules when instances are being created (e.g. Customer cannot exist without Name, so we add constructor taking name)

It works quite well except some edge cases, e.g. code using empty constructor can be compiled while IDE underscores the line like an error.

[BrunoJuchli]

@NorekZ
Thanks for getting back to me.
I don't exactly understand why you need the feature yet. It's only relevant when deserializing, and the deserializer is creating instances, right? The deserializer will set all properties. So initializing them via the ctor is unnecessary, right?
That leaves fields which are not exposed via properties (as you said all properties have setters for deserialization). What are they for? Internal state tracking?

---

I've seen Issue #3 as well. It seems like at one point it was a feature. However, as you had to completely introduce the feature (and not just fix a broken implementation) it actually seems that that feature was once removed (maybe v1 or v2). So for v2 this is a new feature, not just a bug fix.

This reassures my belief it should best be made an opt-in feature.