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

Are nullable annotations on events meaningful/allowed? #29839

Open
AlekseyTs opened this issue Sep 13, 2018 · 11 comments
Open

Are nullable annotations on events meaningful/allowed? #29839

AlekseyTs opened this issue Sep 13, 2018 · 11 comments

Comments

@AlekseyTs
Copy link
Contributor

This issues is created for a

            // PROTOTYPE(NullableReferenceTypes): are annotations on events meaningful/allowed?

that was in AnnotationWithoutNonNullTypes unit-test.

@Joe4evr
Copy link

Joe4evr commented Sep 14, 2018

What if for an automatic/field-like event, the backing field is implicitly considered nullable? Essentially:

public event EventHandler OnFoo; //Compiler generates: private EventHandler? m_OnFoo;

private void Foo()
{
    //....
    OnFoo(this, EventArgs.Empty); //warning: event could NRE if there are no subscribers
}

A special fix would be suggested to use null-safe invocation: OnFoo?.Invoke(this, EventArgs.Empty).

@jcouv
Copy link
Member

jcouv commented Mar 7, 2019

Closing. Events can be set to null, so annotating them is meaningful:

using System;
public class C {
    event Action E;
    public void M() 
    {
        E = null;
        E();
    }
}

@jcouv jcouv closed this as completed Mar 7, 2019
@AlekseyTs
Copy link
Contributor Author

If we allow annotations for events:

  • Do we require non-nullable field-like events to be set to non-null value in a constructor?
  • Do we warn on event1 += null?

It feels like it is very common for an event to not have any subscribers. It would also be good to get clarity on what do we consider a safety issue around an event? An invocation of a field-like event that might have no subscribers? Something else.

@AlekseyTs AlekseyTs reopened this Mar 11, 2019
@jcouv jcouv modified the milestones: 16.1, 16.2 Apr 23, 2019
@jcouv
Copy link
Member

jcouv commented Jun 18, 2019

I added some references to this issue in the code. In particular, if nullability is meaningful for events, we'll also want to honor annotations ([Disallow/Allow/Maybe/NotNull]).

@jasonmalinowski
Copy link
Member

@jcouv I see this is marked as "archived" in the project board -- it is still being tracked?

@jcouv
Copy link
Member

jcouv commented Jun 26, 2019

@jasonmalinowski I don't see this issue marked as "archived":
image

@jasonmalinowski
Copy link
Member

@jcouv That's the little icon before the word "event". I had no idea that was a feature until seeing it just now.

@jcouv
Copy link
Member

jcouv commented Jun 26, 2019

@jasonmalinowski Indeed. Not sure what happened. Restored it now. Thanks!

@jcouv
Copy link
Member

jcouv commented Jul 8, 2019

Tagging @agocke I'm pushing this whole bucket (nullability analysis of events) post C# 8 RTM

@jcouv jcouv modified the milestones: 16.3, Compiler.Next Jul 8, 2019
@jcouv
Copy link
Member

jcouv commented Jul 17, 2019

This was discussed in LDM today (7/17/2019) with the conclusion that ? annotations on events are meaningful and affect the adder, setter and backing field (therefore the .Invoke() as well).

@RikkiGibson
Copy link
Contributor

@AlekseyTs is this issue still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

7 participants