ArxOne / MrAdvice

.NET aspect weaver (build task under NuGet package)
MIT License
311 stars 45 forks source link

struct with properties or methods skips logic #142

Closed FrankLakmann closed 5 years ago

FrankLakmann commented 5 years ago

Hi,

after dorian reported one issue with structs (https://github.com/ArxOne/MrAdvice/issues/108) we found another one: Structs with auto properties or methods seem to just skip the logic when a MethodAdvice aspect is applied.

    public struct TestStruct
    {
        public int N { get; set; }
    }
    public class Program
    {
        static void Main(string[] args)
        {
            var t = new TestStruct();
            t.N = 12;
            var x = t.N;
            Console.WriteLine("Is the value actually readable from the autoprop of the struct?: " + x);
        }
    }

results in x=0.

This is the code after weaving according to ILSpy:

// TestStruct
using ArxOne.MrAdvice;
using ArxOne.MrAdvice.Annotation;
using StructIssueDemo;
using System;
using System.Runtime.CompilerServices;

public struct TestStruct
{
    public int N
    {
        [CompilerGenerated]
        get
        {
            return (int)⚡Invocation.ProceedAspect(this, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/);
        }
        [CompilerGenerated]
        set
        {
            ⚡Invocation.ProceedAspect(this, new object[1]
            {
                value
            }, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/, (RuntimeMethodHandle)/*OpCode not supported: LdMemberToken*/);
        }
    }

    [ExecutionPoint]
    private int N.get′()
    {
        return N;
    }

    private static object get_N″(object P_0, object[] P_1)
    {
        return ((TestStruct)P_0).N.get′();
    }

    [ExecutionPoint]
    private void N.set′(int value)
    {
        N = value;
    }

    private static object set_N″(object P_0, object[] P_1)
    {
        ((TestStruct)P_0).N.set′((int)P_1[0]);
        return null;
    }
}

Here is a demo example: https://github.com/FrankLakmann/MrAdviceTest Sorry I wasn't able to put it in one cs file because of StackOverflowExceptions. Thank you

picrap commented 5 years ago

Thanks. I need to take a look but I suspect that the struct itself is duplicated instead of being boxed.

FrankLakmann commented 5 years ago

Thank you. In the meantime I tried to make it ignore structs by spreading ExcludeAdvices in our code. Unfortunately structs can't get these: Attribute 'ArxOne.MrAdvice.Annotation.ExcludeAdvices' is not valid on this declaration type. It is only valid on 'class, method' declarations.

Most are just autoprops that don't make much sense and can be changed to public fields.

picrap commented 5 years ago

I'll try to provide at least a fix for the ExcludeAdvices today.

FrankLakmann commented 5 years ago

Thank you. I was able to change autoprops to fields and the remaining structs to classes. So it's not urgent for us but it would be good to have the exclude option.

picrap commented 5 years ago

Version 2.8.4 (just released) excludes structs from weaving... Until 2.8.5 handles them correctly 😉

FrankLakmann commented 5 years ago

Thanks alot

picrap commented 5 years ago

This seems to be fixed (I wrote 2 tests, which is humongous). Sometimes I surprise myself by being so fast 😄

picrap commented 5 years ago

Also good luck to access struct data during advice: if you cast the Target to your struct in pointcut, then you will change your local copy. The only ways I found are using reflection or a dynamic.