EventSetter. Check of HandlerType

[lobster2012-user]

Hi! (master) Problem description:

//EventSetter.cs
if (_handler.GetType() != _event.HandlerType)
{
                throw new ArgumentException(SR.Get(SRID.HandlerTypeIllegal));
}

Why is there a strict check? What if I want to set an action handler type EventArgs Action<object, RoutedEventArgs>? (updated text) This possibility is, if you do not use styles.

<control OnEvent="MySimpleHandler">

  this.AddHandler(MouseMoveEvent,  

    new RoutedEventHandler((object sender, RoutedEventArgs args) =>  ...

old question

[weltkante]

Delegates don't have any inheritance hierarchy, the only sensible check you can do is a strict check. The Invoke method is generated by the compiler on the individual subclass, so while the Invoke method on Action<object, EventArgs> may have the same arguments as the Invoke method on EventHandler they are in no way related as far as both language and runtime are concerned.

Your example goes even further because you want to mix Action<object, EventArgs> with RoutedEventHandler, which Invoke method doesn't even have the same parameters.

While in theory these things make sense they'd require new features on every layer (the runtime, compiler and WPF framework level).

[lobster2012-user]

@weltkante Did not quite understand your answer.

https://github.com/lobster2012-user/WpfEventSetterSampleApp

<Button x:Name="ButtonClick" Click="ButtonClick_Click">Click</Button>

private void ButtonClick_Click(object sender, EventArgs e)

Everything is already working, because it should work. The only problem is strict verification (for EventSetter), which is not necessary.

[weltkante]

Your example works because it has nothing to do with strict checking quoted above.

In MainWindow.g.cs (in the obj folder of the project, generated by the xaml compiler) you have the line

this.ButtonClick.Click += new System.Windows.RoutedEventHandler(this.ButtonClick_Click);

And the delegate type correctly matches the delegate type required by the event, causing the strict check to succeed. The compiler bridges the type mismatch of the arguments because it sees they are compatible (RoutedEventArgs is a subclass of EventArgs so its safe to pass in). This happens in the C# compiler so its unrelated to the check you mentioned in the issue (nobody is using Action<object, EventArgs> in the first place).

My point in the original answer was that once you use Action<object, EventArgs> literally its "too late" for anyone (compiler, WPF, runtime) to make things compatible again, since you have an instance of a delegate in memory which is a different type than the one required, and the types are not related in any way.

The only problem is strict verification (for EventSetter), which is not necessary.

If you don't have the check then it'll just fail later when it tries to cast or invoke to the delegate. Like I said above an instance of Action<object, EventArgs> and RoutedEventHandler in memory has nothing to do with each other and is not compatible. Doing it entirely dynamically via reflection is very bad for perfomance and also you usually want to detect errors as soon as possible, not wait until an event is raised to know if it was subscribed correctly or not.

Now you could theoretically build an entire framework to work around all those problems at event subscription time, implement a dynamic code compiler to convert delegates to the correct type if the caller didn't do it (or throw if incompatible), but that is certainly a lot more work than just removing a static type check.

[lobster2012-user]

I found my old project in which I ran into this problem. I created the eventsetter with my hands and passed the delegate there not strictly type. I updated the repo. Two examples

1. Add handler via AddHandler
2. Create a manual eventsetter.

[weltkante]

Yes this is now an example which triggers the strict type check and fails. You are creating a RoutedEventHandler but the MouseDownEvent event needs a MouseButtonEventHandler.

However I already covered all relevant points in my previous response, you might want to reread and try to understand the points I'm bringing up:

• Delegates of different types are incomptible for the .NET runtime and WPF framework, only the C# language compiler can provide a little bit of assistance auto-converting delegates, but this doesn't happen in your case because you don't use the strongly typed event subscription method. As such the event handlers remain incompatible.
• Removing the typecheck just fails later when the event is raised with a typecast exception, unless you hit the very slow reflection based code path. Which code path you hit depends on the specific event are looking at, some subclasses of RoutedEventArgs (for example this one) will override the InvokeEventHandler call with a typecast to be more efficient than slow reflection based invocation.
• Removing the typecheck is a bad idea because it will make finding bugs really hard if you actually get the delegate wrong in an incompatible way

Considering that its a very simple fix in the caller to provide the correct event handler delegate, the disadvantages of not doing the check outweight the minor convenience you would get by removing the check.

[lobster2012-user]

I do not propose to remove checks, checks should be.

Inside routedEvent there is less strict check

internal bool IsLegalHandler (Delegate handler)
        {
            Type handlerType = handler.GetType ();

            return ((handlerType == HandlerType) ||
                     (handlerType == typeof (RoutedEventHandler)));
}

My question is why a condition was added to eventSetter that is stricter than that.

Delegates of different types are incomptible for the .NET runtime and WPF framework

Here I do not agree (repo updated)

 public class MyArgs : EventArgs
    {

    }
    public class Tests
    {
        [SetUp]
        public void Setup()
        {
        }
        void OnEvent(object sender,EventArgs e)
        {
            Console.WriteLine(e.GetType().ToString());
        }
        [Test]
        public void Passed()
        {
            Delegate @delegate = new EventHandler(OnEvent);
            @delegate.DynamicInvoke(this, new MyArgs());
        }
        [Test]
        public void Failed()
        {
            Delegate @delegate = new EventHandler(OnEvent);
            Assert.Throws<ArgumentException>(() => @delegate.DynamicInvoke(this, 1));
        }
    }

[weltkante]

As far as I know DynamicInvoke is using reflection to look up the Invoke method on the delegate class on every single call, which is pretty slow compared to a normal method call. This is the "slow code path" I mentioned above. You probably don't want this for high frequency events (like mouse events, scrolling, drag'n'drop, etc.) which can be raised many times per second.

I do not propose to remove checks, checks should be. Inside routedEvent there is less strict check

This check works because the InvokeHandler call of RoutedEventArgs has a special case for RoutedEventHandler

It may be safe to replace the strict check in EventSetter by a call to IsLegalHandler - I'll leave it to someone from the WPF team to double check the technical details.

In any case it just adds RoutedEventHandler as additional option and won't help you calling something via Action<object, EventArgs> as you requested in your initial issue description.

[lobster2012-user]

In any case it just adds RoutedEventHandler as additional option and won't help you calling something via Action<object, EventArgs> as you requested in your initial issue description.

I replaced EventArgs with RoutedEventArgs in the first message to leave compatibility.

It may be safe to replace the strict check in EventSetter by a call to IsLegalHandler - I'll leave it to someone from the WPF team to double check the technical details.

YES.

About reflection and speed. Here I agree. With a great desire this is all solved by runtime code generation. But that's another story. In that project, I had to do it.