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

Non-nullable reference types support #7988

Closed
fubar-coder opened this issue May 2, 2017 · 19 comments
Closed

Non-nullable reference types support #7988

fubar-coder opened this issue May 2, 2017 · 19 comments
Labels
area-VM-coreclr backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity
Milestone

Comments

@fubar-coder
Copy link
Contributor

Maybe it's the ideal time to add non-nullable reference types when the CLR already needs to be changed for #7734?

This would allow dotnet/csharplang#36 to avoid the creation of if (ReferenceEquals(x, null)) throw ... code which would penalize the usage of non-nullable references.

@shmuelie
Copy link
Contributor

shmuelie commented May 2, 2017

While this would be great from a forward compat perspective dotnet/csharplang#36 would still need to work downlevel so it wouldn't remove the need for that.

@fubar-coder
Copy link
Contributor Author

fubar-coder commented May 2, 2017

When implemented/supported in/by the CLR:

  • Pro: The CLR could make the null check. This has the advantage that it will only be done when really needed (e.g. when passing a "may-be-null" to a "not-null" reference variable).
  • Pro: This additional check code goes away over time, because more and more assemblies will be using non-nullable reference types
  • Cons: Work must be done in the CLR (but the CLR already gets changed for Default Interface Method support  #7734)

When implemented by the compiler:

  • Cons: Must check the reference variable parameters (permanent slow-down)
  • Cons: Check remains there even though the consumer of your assembly also uses non-nullable reference types (remaining slow-downs, more slow-downs due to checking the null-ness of return values)
  • Pro: ... a warning when you probably did something horribly wrong? that's it.

@mikedn
Copy link
Contributor

mikedn commented May 2, 2017

The CLR could make the null check. This has the advantage that it will only be done when really needed (e.g. when passing a "may-be-null" to a "not-null" reference variable

And how does CLR know it's really needed or not?

@shmuelie
Copy link
Contributor

shmuelie commented May 2, 2017

@fubar-coder So now I can only use this feature if I target CLR vNext? I'd rather have the slow down (which honestly on a modern CPU is nothing) than need a newer CLR on a machine.

@fubar-coder
Copy link
Contributor Author

@SamuelEnglard We already need a new CLR for #7734. So why not doing non-nullable reference types the right way?

@fubar-coder
Copy link
Contributor Author

fubar-coder commented May 2, 2017

@mikedn It should be specified in the assembly meta data if the type is a non-nullable reference type. When the value it has is already of a non-nullable ref type, then it doesn't need to produce code to check if it's null.

@shmuelie
Copy link
Contributor

shmuelie commented May 2, 2017

@fubar-coder because we have no idea when #7734 will land. CLR changes are extremely slow moving and hard to get through. While I'm not against the idea of the CLR having the ability to be aware of non-nullable ref types C#'s feature should not be tied to it. That feature is already going to take long enough as it is.

@fubar-coder
Copy link
Contributor Author

fubar-coder commented May 2, 2017

@SamuelEnglard The proposed feature is already there when you use ReSharper. IOW: You don't have an advantage when you already use R#. And when we use the "fake" non-nullable-ref types, then we're in a situation similar to Java with its generics and its type erasure (which was AFAIK considered as a possible implementation), but the F# guys pushed for the "real thing".

EDIT: IMHO it's better to already have the feature baked-in and let the programming language(s) catch up. Later, the CLR guys won't have much interest in adding this feature.

EDIT 2: According to dotnet/csharplang#52 it's targeted for C# 8.

@mikedn
Copy link
Contributor

mikedn commented May 2, 2017

It should be specified in the assembly meta data if the type is a non-nullable reference type. When the value it has is already of a non-nullable ref type, then it doesn't need to produce code to check if it's null.

If the CLR can avoid the null check in the given circumstances what stops the C# compiler from doing the same thing?

Non-nullability is largely a language thing because it's the language that's supposed to stop you to pass a null where you should not. The runtime cannot do that, except by throwing exceptions and that doesn't solve anything.

@shmuelie
Copy link
Contributor

shmuelie commented May 2, 2017

@fubar-coder I'm going to "step back" here since I fully admit my issues here are less how would this work/is this a good idea for CLR and more SHOULD the CLR do this. That's a debate that can best happen after I think.

@fubar-coder
Copy link
Contributor Author

fubar-coder commented May 2, 2017

@mikedn It's about the guarantees given by the CLR. When your function wants a non-nullable ref type, then you shouldn't be able to get null. Especially in combination with code from older compilers only the CLR is able to provide this guarantees, not the compiler.

Edit: non-nullability from a language is convention. From the CLR, it's a guarantee. The CLR already throws exceptions, so this is nothing unheard of.

@mikedn
Copy link
Contributor

mikedn commented May 5, 2017

It's still not very clear why the VM has to do this rather than the C# compiler. There may be a good reason for this if you're willing to end up with a NullReferenceException rather than some custom exception (e.g. ArgumentNullException).

The thing is that the runtime (well, the JIT actually) can produce null checks that are slightly cheaper than if (x == null) throw....

Considering a method like void foo(string! s) (or whatever the syntax for non-nullable reference type will be) the JIT could generate code like:

mov ecx, ...   ; get this from somewhere
mov edx, ...   ; get the s argument from somewhere
cmp [edx], edx ; null check s
call foo       ; call foo

If s happens to be null then a NullReferenceException will be generated at the callsite, similar to the NullReferenceException you get when calling on a null this.

And since the JIT is in charge of generating all native code, no matter what managed compiler produced the IL then you'd be guaranteed that s is never null inside foo, even if called from a managed language that doesn't understand non-nullable references and doesn't generate null checks by itself.

@fubar-coder
Copy link
Contributor Author

fubar-coder commented May 5, 2017

That's exactly my point. I want cheap null checks (or none at all). When the CLR knows - in your example - that edx can never be null, because the callers variable is a non-nullable reference type, then the check can be removed too.

If the C# compiler must generate those, then those will become expensive, because they must be done inside foo like this:

public void foo(string s) {
  if (s == null) throw ArgumentNullException(nameof(s));
  // do something with s
}

Regarding the exception thrown when trying to pass a null to a non-nullable reference type, a NullReferenceException is perfectly fine. A simple ArgumentNullException (with default message) and the argument in question might be OK even though it might be slower, because you'll lose this disadvantage as soon as the caller uses non-nullable reference types too.

BTW: This sounds like a misunderstanding. The C# compiler for me is Roslyn which only produces IL. The machine code is only produced by the CLR using its JIT compiler (if available) for the given platform.

@mikedn
Copy link
Contributor

mikedn commented May 5, 2017

When the CLR knows - in your example - that edx can never be null, because the callers variable is a non-nullable reference type, then the check can be removed too.

Yes, the JIT is already capable of generating and eliminating null checks in some cases so it seems that this could build upon existing infrastructure.

If the C# compiler must generate those, then those will become expensive, because they must be done inside foo like this:

Yes, and the JIT has no way to eliminate those because it doesn't know what code will call foo since it doesn't do inter-procedural analysis.

Sounds like a good plan and perhaps can be extended to other cases beyond method calls. For example, assigning to a non-nullable field could also generate a null check.

@jkotas Opinions?

@jkotas
Copy link
Member

jkotas commented May 5, 2017

  • Cheap null checks can be achieved using ldvirtftn (on .NET Framework / .NET Core at least, not sure about Mono). This pattern has been used in some boutique language compiler. I do not remember the language name, but I do remember we got a bug when we have regressed it by accident.
ldvirtftn Object.ToString
pop
  • The JIT can in theory take advantage of the non-null variable or argument annotations to eliminate some extra null checks. However, I am not sure whether it would be a win overall because of dealing with such annotations is not exactly free. A better solution may be to get the C# compiler to emit no.nullcheck prefix that is described in ECMA-335 spec. Unfortunately, this prefix is not recognized by all runtimes today and so it would have to be under RuntimeFeatures check.

@AaronRobinsonMSFT
Copy link
Member

@jkotas Is there something we can link this to for 3.0? I think this support is actually in unless I am missing something.

@jkotas
Copy link
Member

jkotas commented Apr 15, 2019

The C# 8 nullable reference types annotations have been aimed at diagnostics. They are not used for any optimizations today.

This issue is about using these annotations for optimizations. I think that the most promising path for that is https://github.com/dotnet/coreclr/issues/17469#issuecomment-481481078 .

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@ghost
Copy link

ghost commented Oct 15, 2023

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

@ghost ghost added backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity labels Oct 15, 2023
@ghost
Copy link

ghost commented Oct 29, 2023

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@ghost ghost closed this as completed Oct 29, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 28, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity
Projects
None yet
Development

No branches or pull requests

6 participants