2
\$\begingroup\$

Original post here:

MIST - auto-implemented, attribute-driven .NET notification mechanism

I've developed (and use) an IL "weaving" solution for auto-implementing property change notification in .NET applications, but I'm considering using a different tactic for making the IL modifications.

I can replace the InsertNotificationsIntoProperty method with something like the following:

protected static void InsertNotificationsIntoProperty(PropertyDefinition propDef, MethodReference notifyTarget, IEnumerable<string> notifyPropertyNames) {
    if (propDef.SetMethod == null)
        //This is a read-only property, there's nothing to do.
        return;
    else if (propDef.SetMethod.Body == null) {
        //This is an abstract property, we don't do these either.
        throw new InvalidNotifierException();
    }

    var setMeth = propDef.SetMethod;
    var newMeth = new MethodDefinition(setMeth.Name, setMeth.Attributes, setMeth.ReturnType);
    var msil = newMeth.Body.GetILProcessor();
    var instructions = new List<Instruction>();

    newMeth.Name = setMeth.Name;
    newMeth.DeclaringType = setMeth.DeclaringType;
    newMeth.Parameters.Add(new ParameterDefinition(setMeth.Parameters[0].ParameterType));

    instructions.AddRange(new[] {
        msil.Create(OpCodes.Ldarg_0),
        msil.Create(OpCodes.Ldarg_1),
        msil.Create(OpCodes.Call, setMeth)
    });

    foreach (var notifyPropertyName in notifyPropertyNames) {
        instructions.AddRange(new[] {
            msil.Create(OpCodes.Ldarg_0),
            msil.Create(OpCodes.Ldstr, notifyPropertyName),
            msil.Create(OpCodes.Call, notifyTarget),
            msil.Create(OpCodes.Nop)
        });
    }

    foreach (var i in instructions) {
        newMeth.Body.Instructions.Add(i);
    }

    newMeth.Body.Instructions.Add(msil.Create(OpCodes.Ret));
    setMeth.Name = setMeth.Name + "`Impl";
    propDef.SetMethod = newMeth;
    newMeth.DeclaringType.Methods.Add(newMeth);
}

Vs. the original implementation:

/// <summary>
    /// Weaves notifiers into the property.  This is where the magic happens.
    /// </summary>
    /// <param name="propDef">The property definition.</param>
    /// <param name="notifyTarget">The notify target.</param>
    /// <param name="notifyPropertyNames">The notify property names.</param>
    protected static void InsertNotificationsIntoProperty2(PropertyDefinition propDef, MethodReference notifyTarget, IEnumerable<string> notifyPropertyNames) {

        if (propDef.SetMethod == null)
            //This is a read-only property
            return;
        else if (propDef.SetMethod.Body == null) {
            //This is an abstract property, we don't do these either.
            throw new InvalidNotifierException();
        }

        var methodBody = propDef.SetMethod.Body;

        //Retrieve an IL writer
        var msil = methodBody.GetILProcessor();

        //Insert a Nop before the first instruction (like... at the beginning).
        var begin = msil.Create(OpCodes.Nop);
        msil.InsertBefore(methodBody.Instructions[0], begin);

        //Call the notification target method for 
        foreach (var notifyPropertyName in notifyPropertyNames) {

            var beginInstructions = new Instruction[0];
            var endInstructions = new Instruction[0];

            //Load the value of the property name to be passed to the notify target onto the stack.
            var propertyName = notifyPropertyName == null ?
                msil.Create(OpCodes.Ldnull) :
                msil.Create(OpCodes.Ldstr, notifyPropertyName);


            //Emit a call to the notify target
            var callNotifyTarget = msil.Create(OpCodes.Call, notifyTarget);
            endInstructions = new[] {
                        msil.Create(OpCodes.Ldarg_0),
                        propertyName,
                        msil.Create(OpCodes.Call, notifyTarget),
                        msil.Create(OpCodes.Nop)
                    };
            switch (notifyTarget.Parameters.Count) {
                case 0:
                    endInstructions = new[] {
                        msil.Create(OpCodes.Ldarg_0),
                        msil.Create(OpCodes.Call, notifyTarget),
                        msil.Create(OpCodes.Nop)
                    };
                    break;
                case 1:
                    endInstructions = new[] {
                        msil.Create(OpCodes.Ldarg_0),
                        propertyName,
                        msil.Create(OpCodes.Call, notifyTarget),
                        msil.Create(OpCodes.Nop)
                    };
                    break;
                default:
                    throw new InvalidNotifyTargetException(notifyTarget.FullName);
            }

            //Insert IL instructions before end of method body
            //Find all return statements in the method and raise notification there, this is a little more complicated.
            //...Also any statements that branch to them and make correction.
            var returnPoints = methodBody.Instructions.Where(a => a.OpCode == OpCodes.Ret).ToArray();
            foreach (var instruction in returnPoints) {

                InsertBefore(msil, endInstructions, instruction);
                var branches = methodBody.Instructions.Where(a => a.OpCode == OpCodes.Br_S && a.Operand == instruction).ToArray();
                var branchTarget = endInstructions[0];

                foreach (var b in branches) {
                    b.Operand = branchTarget;
                }
            }

        }
    }

This works as it passes all the unit tests and solves the problem I was having in cases where the provides a property implementation. What happens here is that instead of altering the IL of the set method, it wraps that as method of its own which also makes the calls to the notification target.

I kind of like this approach but I have a few reservations about blowing up the msil with all these extra method signatures (but does it really matter?). I feel like it's less invasive, because with this approach I leave the user's code untouched. Instead of barging in and rearranging things I gently relocate the method and its family of instructions to a nice, safe little box where it can live in peace, completely surrounded and protected under the benevolent eye of my framework (which now, maybe sounds a little worse).

I also feel like this approach is more resistant to differences in the IL that might be produced from one .NET framework version to another.

Thoughts/opinions? Barring any unpleasant surprises I think this is the approach I will go with.

\$\endgroup\$
1
  • \$\begingroup\$ I should be able to determine whether an implementation has been provided and only swap-in wrapper methods in those instances, this is a slightly forkier and more complex solution but I think in the end it takes the best from both approaches. \$\endgroup\$
    – Mik3c
    Jun 3, 2016 at 1:37

1 Answer 1

2
\$\begingroup\$

This

if (propDef.SetMethod == null)
    //This is a read-only property, there's nothing to do.
    return;
else if (propDef.SetMethod.Body == null) {
    //This is an abstract property, we don't do these either.
    throw new InvalidNotifierException();
}  

would be more clear to the reader if you would use two if instead of if..else if like so

if (propDef.SetMethod == null)
{
    //This is a read-only property, there's nothing to do.
    return;
}

if (propDef.SetMethod.Body == null) {
    //This is an abstract property, we don't do these either.
    throw new InvalidNotifierException();
}  

Your style about braces {} isn't consistent. For the if you had omitted them but for the else if you have added them. Which style you use is up to you and your responsibility but you should only use one style.

Generally speaking a C# developer would expect the opening brace { to be on a new line.

Nevertheless I would like to encourage you to always use braces to make your code less error prone and better structured.


This

newMeth.Name = setMeth.Name;  

is superflous because the Name property is set through the constructor call

var newMeth = new MethodDefinition(setMeth.Name, setMeth.Attributes, setMeth.ReturnType);  

This

foreach (var i in instructions) {
    newMeth.Body.Instructions.Add(i);
}

newMeth.Body.Instructions.Add(msil.Create(OpCodes.Ret));
setMeth.Name = setMeth.Name + "`Impl";
propDef.SetMethod = newMeth;
newMeth.DeclaringType.Methods.Add(newMeth);  

would be better readable structured like this

foreach (var i in instructions) {
    newMeth.Body.Instructions.Add(i);
}
newMeth.Body.Instructions.Add(msil.Create(OpCodes.Ret));

setMeth.Name = setMeth.Name + "`Impl";
propDef.SetMethod = newMeth;
newMeth.DeclaringType.Methods.Add(newMeth);  

while we are here, the name of looping variable i is not well choosen. i is acceptable for an indexing variable in a for loop but for an foreach loop you shouldn't use single letter variables. Why don't you rename it to instruction ?


Generally speaking, you shouldn't use abbreviations for naming things. E.g newMeth would be better named newMethodDefinition etc,


Otherwise your code looks good. You are only using comments where you should. Thats much better than in the former code.

\$\endgroup\$
1
  • \$\begingroup\$ All good points. I'm doing a bit of refactoring now and will take these suggestions into account. \$\endgroup\$
    – Mik3c
    Jun 3, 2016 at 2:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.