Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Events without the boilerplate #15282

Closed
iam3yal opened this issue Nov 16, 2016 · 19 comments
Closed

[Proposal] Events without the boilerplate #15282

iam3yal opened this issue Nov 16, 2016 · 19 comments
Labels
Area-Language Design Feature - Replace/Original Replace/Original Feature Request Language-C# Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.

Comments

@iam3yal
Copy link

iam3yal commented Nov 16, 2016

The Problem:

Today, we need to repeat ourselves quite a bit when we want to define an event, we need to have an event declaration and we need to create an event invoker.

The reasons we need to create an event invoker are as follow:

  1. Ensure that the delegate instance we are calling to isn't null.

  2. Events can't be invoked directly in derived classes so this help us achieve exactly that.

  3. Finally, in case it's virtual it allows us to override it in derived classes even though events can actually be marked virtual.

However, the event invoker itself, in many cases is fairly straightforward and the exact same pattern is repeated all over the place.

Here is a typical example:

class KeyboardKeyPressEventArgs
{
	public KeyboardKeyPressEventArgs(char keyChar)
	{
	}
}

class Keyboard
{
	public event EventHandler<KeyboardKeyPressEventArgs> KeyPress;

	protected void OnKeyPress(char keyChar) => KeyPress?.Invoke(this, new KeyboardKeyPressEventArgs(keyChar));

	public void PressKey()
	{
		OnKeyPress('X');
	}
}

class GamingKeyboard : Keyboard
{
	public void MacroKeyPress()
	{
		OnKeyPress('M');
	}
}

The Solution:

So the solution is the ability to specify everything as part of the event declaration.

I'm thinking something like this: public event <modifers> EventHandler<KeyboardKeyPressEventArgs> KeyPress;

Here are some rules that I think would make sense:

  1. The access modifier cannot be public or internal.

  2. Other modifiers such as virtual, static should be allowed.

So for the following statement:

public event protected EventHandler<KeyboardKeyPressEventArgs> KeyPress;

The compiler will emit the foillowing code:

protected void OnKeyPress(char keyChar) => KeyPress?.Invoke(this, new KeyboardKeyPressEventArgs(keyChar));

Event Invocation:

It's allowed to invoke delegates that are associated with events directly but only in classes that they were declared and as noted above it's not safe, also, attempting to do it from derived classes results an error.

However, with my proposal I'm hoping that the following would be possible KeyPress('K').

In this case under the hood the compiler will make a call to the generated event invoker as opposed to invoking the event itself.

Maybe it's going to be a bit weird that there's different rules based on the declaration of the event so maybe we can use some keyword here such as raise so it would look like this raise KeyPress('K'), not sure whether it make sense but maybe, I really hope that it wouldn't be necessary.

Method Overriding:

I think that it wouldn't be possible to override the event invoker without some special syntax because the compiler is the one that generates it so I'm proposing the following syntax for it:

protected override event KeyPress(char keyChar) {
}

So after all these changes the above implementation would look like this:

class Keyboard
{
	public event protected EventHandler<KeyboardKeyPressEventArgs> KeyPress;

	public void PressKey()
	{
		KeyPress('X');
	}
}

class GamingKeyboard : Keyboard
{
	public void MacroKeyPress()
	{
		KeyPress('M');
	}
}

Some notes:

  1. In my experience, in most cases the sender is always the instance of the class so in this case I think that the compiler should always emit this.

  2. The signature of the generated event invoker should always match the signature of the class constructor that is passed to EventHandler or it should be parameterless in the case of EventHandler.

  3. We generally use the following delegates EventHandler or EventHandler for events but it can actually be any delegate, does it make sense to restrict it to these two types? personally I think it does because after all this suppose to be a syntactic sugar, people that want more freedom/power can use the traditional way.

@jnm2
Copy link
Contributor

jnm2 commented Nov 16, 2016

Does the compiler generate an OnKeyPress method for every KeyboardKeyPressEventArgs constructor, or how did it know which one you wanted?

public event protected <- I'm having a hard time liking this. As is, the proposal seems to me like it would lower the clarity of the code.

@iam3yal
Copy link
Author

iam3yal commented Nov 16, 2016

@jnm2

Does the compiler generate an OnKeyPress method for every KeyboardKeyPressEventArgs constructor, or how did it know which one you wanted?

What exactly do you mean by "how did it know which one you wanted?" why would it generate multiple event handlers? generally you have a single event handler per event not multiples.

public event protected <- I'm having a hard time liking this. As is, the proposal seems to me like it would lower the clarity of the code.

Well, I dislike the boilerplate even more, can you suggest an alternative?

I'd argue that creating an event handler many times, one per event lowers the clarity of the code multiple times more than getting used to to a new, more succinct syntax but if you have a better idea I'm all for it. 😄

protected void OnKeyPress(...) => KeyPress?.Invoke(this, ...);

protected void OnKeyUp(...) => KeyUp?.Invoke(this, ...);

protected void OnKeyDown(...) => KeyDown?.Invoke(this, ...);

// Many more events here...

@jnm2
Copy link
Contributor

jnm2 commented Nov 16, 2016

First of all, OnKeyPress which raises KeyPress is not called an event handler. The event handler is the thing you attach via KeyPress +=. I guess you could call OnKeyPress a raiser or invoker.

Second, what OnListChanged signature would you generate for ListChangedEventArgs?

As for boilerplate, I only add On* methods if they need to be protected for an implementing class. And in that case I want to cultivate them explicitly, including adding XML documentation since they are publicly visible.

@HaloFour
Copy link

How common is it to actually advertise an OnEvent methods? Even in the case of a derived class I can't imagine it's that common. If I'm writing such a method it's generally private and only a helper to manage the event for the current class if there is custom logic/transformation involved, none of which could be captured by a generated method.

For people who do want such generated methods I'd argue that this could be handled through a source generator rather than requiring changes to the language.

@iam3yal
Copy link
Author

iam3yal commented Nov 16, 2016

@jnm2

First of all, OnKeyPress which raises KeyPress is not called an event handler.

Yeah, you right, didn't give that much thought.

The event handler is the thing you attach via KeyPress +=. I guess you could call OnKeyPress a raiser or invoker.

Yeah, I know, I didn't know why I called it an event handler, my bad, will update.

Second, what OnListChanged signature would you generate for ListChangedEventArgs?

Yeah got you, well the compiler will generate all of signatures or only the ones you call.

As for boilerplate, I only add On* methods if they need to be protected for an implementing class. And in that case I want to cultivate them explicitly, including adding XML documentation since they are publicly visible.

Well, I don't document these methods but sure, you got a point.

@iam3yal
Copy link
Author

iam3yal commented Nov 16, 2016

@HaloFour

How common is it to actually advertise an OnEvent methods? Even in the case of a derived class I can't imagine it's that common. If I'm writing such a method it's generally private and only a helper to manage the event for the current class if there is custom logic/transformation involved, none of which could be captured by a generated method.

In UI frameworks/library it's fairly common, it all depends on what you're doing.

For people who do want such generated methods I'd argue that this could be handled through a source generator rather than requiring changes to the language.

Yeah, you probably right.

@HaloFour
Copy link

HaloFour commented Nov 16, 2016

I will note that the CLR provides a fire method slot for events as it does for add and remove method slots. C# never provides a method for this slot.

@iam3yal
Copy link
Author

iam3yal commented Nov 16, 2016

@HaloFour Yeah, I rarely write any VB.NET code, well didn't touch it for very long time but I think it's used there iirc, not sure.

@jnm2
Copy link
Contributor

jnm2 commented Nov 16, 2016

@HaloFour which really messed with my head when I switched to C#. Seems an eternity ago now... Also, I haven't missed indexed properties like I thought I would. :D

@HaloFour
Copy link

@eyalsk @jnm2

And while VB.NET does fill that slot if you specify a custom event it does so with a private method so it doesn't help if you're planning on allowing derived classes to raise that event. You'd still need to define a separate OnEvent method for that.

@iam3yal
Copy link
Author

iam3yal commented Nov 16, 2016

@HaloFour I see, well, after you brought up the source generator feature I really feel like this proposal is not so attractive but the question is when we gonna see it. 😄

@HaloFour
Copy link

@eyalsk

Indeed. Hopefully source generators are coming sooner rather than later as I think that they'll scratch a lot of itches.

As for this proposal specifically, I think I'd prefer something a little more like auto-properties.

public class Foo {
    public event EventHandler MyEvent {
        protected fire;
    }
}

public class Bar : Foo {
    public void DoSomething() {
        base.MyEvent(this, default(EventArgs)); // invokes protected fire method
    }
}

I'm not bothering to get into the specific syntax details, e.g. whether or not add or remove would be required or if they could be automatically implemented (in the proper thread-safe manner) or if any/all of the specific accessor methods can be given a body. I just wanted to illustrate the idea.

@iam3yal
Copy link
Author

iam3yal commented Nov 16, 2016

@HaloFour I agree, I think this makes much more sense than putting the access modifier as part of the declaration, I like it, I'll update it after the Connect(); event haha..

@jnm2
Copy link
Contributor

jnm2 commented Nov 16, 2016

@HaloFour Why specify the name fire if it doesn't show up anywhere else in your code?
Edit: think I misunderstood. If it's a keyword I'd prefer raise.

@HaloFour
Copy link

@jnm2

The name isn't that important. I only reused the name of the method slot in the CLR metadata to illustrate the idea.

@vbcodec
Copy link

vbcodec commented Nov 16, 2016

@eyalsk

Instead defining OnKeyPress, PressKey and MacroKeyPress, write universal function that use reflection to call events on any object.

@jnm2
Copy link
Contributor

jnm2 commented Nov 16, 2016

@vbcodec what could possibly go wrong? 😆

@gafter gafter added Area-Language Design Feature Request Language-C# Feature - Replace/Original Replace/Original Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. labels Nov 17, 2016
@gafter
Copy link
Member

gafter commented Nov 17, 2016

This is a great use case for generators. I don't think we would address this problem in isolation.

@gafter gafter closed this as completed Nov 17, 2016
@iam3yal
Copy link
Author

iam3yal commented Nov 17, 2016

@gafter Thanks, I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Language Design Feature - Replace/Original Replace/Original Feature Request Language-C# Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.
Projects
None yet
Development

No branches or pull requests

5 participants