[C#] Add nullable type attributes to Grpc.Core.Api #27887

JamesNK · 2021-10-31T21:50:11Z

https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references

captainsafia · 2021-11-02T03:03:19Z

src/csharp/Grpc.Core.Api/AsyncCallState.cs

@@ -64,21 +64,21 @@ internal struct AsyncCallState
        internal Task<Metadata> ResponseHeadersAsync()
        {
            var withState = responseHeadersAsync as Func<object, Task<Metadata>>;
-            return withState != null ? withState(callbackState)
+            return withState != null ? withState(callbackState!)


Suggested change

return withState != null ? withState(callbackState!)

return withState != null && callbackState != null ? withState(callbackState)

If withState is not null then callbackState has to also be not null. I chose not to use a Debug.Assert because this library targets netstandard2.0 and Debug.Assert isn't annotated there to correctly suppress the nullable warning.

Makes sense. It would be neat if we could use MemberNotNullWhen but I'm not sure if it can be used in private members.

captainsafia · 2021-11-02T03:10:01Z

src/csharp/Grpc.Core.Api/Interceptors/InterceptingCallInvoker.cs

@@ -43,7 +43,7 @@ public InterceptingCallInvoker(CallInvoker invoker, Interceptor interceptor)
        /// <summary>
        /// Intercepts a simple blocking call with the registered interceptor.
        /// </summary>
-        public override TResponse BlockingUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request)
+        public override TResponse BlockingUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string? host, CallOptions options, TRequest request)


Why are we allowing a null host in all these APIs?

Generated code passes in null -

grpc/src/csharp/Grpc.Examples/MathGrpc.cs

Line 224 in 6d28740

return CallInvoker.BlockingUnaryCall(__Method_Div, null, options, request);

There is also some Grpc.Core.Api code that passes down this code path, but I don't recall exactly where it is to link to it.

host == null means "use the default host / authority". You can set "host" explicitly if you want to use multiple virtual hosts on a single host.
See "Authority" in https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md

So setting host=null is actually the most common use case.

jtattermusch · 2021-11-09T12:37:20Z

I will look at this in detail once I successfully upgrade the dotnet SDK in the grpc/grpc repo: #27966 (it's proving difficult).

jtattermusch · 2021-11-17T09:43:23Z

SDK has been updated in #27966, you can rebase now.

jtattermusch · 2021-11-29T12:30:29Z

It seems like #27886 and this PR are fully independent, so I don't see a reason for this PR to be based on top of #27886.

src/csharp/Grpc.Core.Api/Grpc.Core.Api.csproj

jtattermusch

LGTM once the changes from #27886 are separated from this PR.

Just to double check please confirm that this feature is 100% backward compatible with older runtimes / codebases. Specifically:

the public API will stay binary compatible with older releases (IMHO that's because the internally, the ? and ! operators only add attributes to the the existing types (which stay unchanged)
unless the nullable context is explicitly set by the user in their project, the behavior of all their existing code will stay the same. (as by default the nullable checks are disabled).

jtattermusch · 2021-11-29T12:39:31Z

src/csharp/Grpc.Core.Api/Interceptors/InterceptingCallInvoker.cs

@@ -43,7 +43,7 @@ public InterceptingCallInvoker(CallInvoker invoker, Interceptor interceptor)
        /// <summary>
        /// Intercepts a simple blocking call with the registered interceptor.
        /// </summary>
-        public override TResponse BlockingUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request)
+        public override TResponse BlockingUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string? host, CallOptions options, TRequest request)


host == null means "use the default host / authority". You can set "host" explicitly if you want to use multiple virtual hosts on a single host.
See "Authority" in https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md

So setting host=null is actually the most common use case.

JamesNK · 2021-11-29T19:43:14Z

LGTM once the changes from #27886 are separated from this PR.

Done

the public API will stay binary compatible with older releases (IMHO that's because the internally, the ? and ! operators only add attributes to the the existing types (which stay unchanged)

Yes

unless the nullable context is explicitly set by the user in their project, the behavior of all their existing code will stay the same. (as by default the nullable checks are disabled).

Yes, it is off by default. FYI there is a chance that someone who has already enabled nullable warnings in an app could get new warnings after this assembly is annotated. That's pretty normal as more libraries are annotated, and it is only compile time warnings. There is no change at runtime.

jtattermusch

LGTM, thanks!

@apolcyn can you please add another LGTM.

JamesNK requested review from apolcyn, jtattermusch and nicolasnoble as code owners October 31, 2021 21:50

JamesNK changed the title ~~[C#] Add cancellation token overloads to streaming interfaces~~ [C#] Add nullable type attributes to Grpc.Core.Api Oct 31, 2021

captainsafia reviewed Nov 2, 2021

View reviewed changes

ctiller added the kokoro:force-run label Nov 4, 2021

grpc-kokoro removed the kokoro:force-run label Nov 4, 2021

grpc-checks bot added the bloat/none label Nov 4, 2021

jtattermusch self-assigned this Nov 5, 2021

JamesNK force-pushed the jamesnk/nullable branch from ae5aa65 to 464c4e6 Compare November 17, 2021 17:15

jtattermusch added release notes: yes Indicates if PR needs to be in release notes lang/C# labels Nov 29, 2021

jtattermusch reviewed Nov 29, 2021

View reviewed changes

src/csharp/Grpc.Core.Api/Grpc.Core.Api.csproj Outdated Show resolved Hide resolved

jtattermusch approved these changes Nov 29, 2021

View reviewed changes

Add nullable type attributes to Grpc.Core.Api

04979b9

JamesNK force-pushed the jamesnk/nullable branch from 464c4e6 to 04979b9 Compare November 29, 2021 19:34

jtattermusch approved these changes Nov 30, 2021

View reviewed changes

apolcyn approved these changes Nov 30, 2021

View reviewed changes

jtattermusch merged commit a0f30a4 into grpc:master Nov 30, 2021

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C#] Add nullable type attributes to Grpc.Core.Api #27887

[C#] Add nullable type attributes to Grpc.Core.Api #27887

JamesNK commented Oct 31, 2021 •

edited by jtattermusch

captainsafia Nov 2, 2021

JamesNK Nov 2, 2021

captainsafia Nov 3, 2021

captainsafia Nov 2, 2021

JamesNK Nov 2, 2021

jtattermusch Nov 29, 2021

jtattermusch commented Nov 9, 2021

jtattermusch commented Nov 17, 2021

jtattermusch commented Nov 29, 2021

jtattermusch left a comment

jtattermusch Nov 29, 2021

JamesNK commented Nov 29, 2021

jtattermusch left a comment

	return withState != null ? withState(callbackState!)
	return withState != null && callbackState != null ? withState(callbackState)

[C#] Add nullable type attributes to Grpc.Core.Api #27887

[C#] Add nullable type attributes to Grpc.Core.Api #27887

Conversation

JamesNK commented Oct 31, 2021 • edited by jtattermusch

captainsafia Nov 2, 2021

Choose a reason for hiding this comment

JamesNK Nov 2, 2021

Choose a reason for hiding this comment

captainsafia Nov 3, 2021

Choose a reason for hiding this comment

captainsafia Nov 2, 2021

Choose a reason for hiding this comment

JamesNK Nov 2, 2021

Choose a reason for hiding this comment

jtattermusch Nov 29, 2021

Choose a reason for hiding this comment

jtattermusch commented Nov 9, 2021

jtattermusch commented Nov 17, 2021

jtattermusch commented Nov 29, 2021

jtattermusch left a comment

Choose a reason for hiding this comment

jtattermusch Nov 29, 2021

Choose a reason for hiding this comment

JamesNK commented Nov 29, 2021

jtattermusch left a comment

Choose a reason for hiding this comment

JamesNK commented Oct 31, 2021 •

edited by jtattermusch