.NET: Add nullable annotations to Protobuf library #9801

erikmav · 2022-04-18T17:30:43Z

Note this is for the Protobuf library itself, not related to generated code. Large but shallow changes:

Added Directory.Build.props with C# 10 targeted and nullable enabled across all projects.
Updated C# README to note already-existing dependency on VS2022 and .NET 6.
Added nullable annotations across all C# code, drawing down to zero warnings/issues.
Made use of a few newer C# features like pattern matching while fixing nullable issues. Wider use of other C# features in a few cases but mostly deferred to later PR(s).

Validations:

Unit tests passing including null param exceptions.
Manually verified existing calls to null param check utility code (ProtoPreconditions.CheckNotNull*) line up with non-null annotations.
Dropped main assembly into a private codebase with several gRPC contracts, generated protobuf code, and nullable issues set to error. Verified nullable issues match expected results, i.e. cause no unexpected errors.

jskeet · 2022-04-19T13:33:44Z

Thanks for the PR. I'll try to review it this week, or at least make a start. We'll need to figure out what we do about the compatibility tests, which are failing on Kokoro. (I need to remind myself exactly what they're doing, for a start...)

erikmav · 2022-04-19T18:02:03Z

Thanks @jskeet . My take on the failures is that the nullable changes would require updating the compatibility csproj to use C# 8.0 at minimum. I hadn't realized nullable was not backward compatible to older C# versions. If true, the implication of my change is that it requires users to update their language settings as well, which at minimum would imply additional release note info, and likely only makes sense to do at a minor version number update.

jskeet · 2022-04-19T18:04:37Z

I hadn't realized nullable was not backward compatible to older C# versions.

It's not compatible in terms of source code, but it is binary compatible. We won't actually be requiring users to change their language settings (and if we did I'd suggest we shouldn't do this at all)... but we may need to change the settings for the compatibility project file. Basically I don't expect any other users to be compiling Google.Protobuf themselves, so it's fine to need a change for those language settings.

jskeet

First chunk of review - I've reviewed as far as the end of the tests. (Somewhat skimming, admittedly - for the production code I'll be looking more closely.)

csharp/src/Google.Protobuf.Benchmarks/ByteStringBenchmark.cs

csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs

csharp/src/Google.Protobuf.Test/Collections/MapFieldTest.cs

csharp/src/Google.Protobuf.Test/Collections/ProtobufEqualityComparersTest.cs

csharp/src/Google.Protobuf.Test/Collections/RepeatedFieldTest.cs

csharp/src/Google.Protobuf.Test/ExtensionSetTest.cs

csharp/src/Google.Protobuf.Test/LegacyGeneratedCodeTest.cs

csharp/src/Google.Protobuf.Test/Reflection/DescriptorDeclarationTest.cs

erikmav · 2022-05-07T08:10:48Z

@jskeet if you like I can break this PR up into a series of smaller ones with less cognitive overload. Probably starting with updating to C#10 and some new feature use, then follow with turning on nullable a few files at a time.

jskeet · 2022-05-07T08:38:04Z

@Erikma: Let me see if I can get to it next week. (It won't be on Monday as I'm at a conference.) It's on my list, just not high up on my list of priorities - that's what's taking time rather than the size of the PR.

jskeet

Next set of comments. Apologies that this is taking a long time. I've now viewed everything under Collections and Compatibility.

csharp/src/Google.Protobuf/ByteString.cs

csharp/src/Google.Protobuf/CodedOutputStream.cs

csharp/src/Google.Protobuf/Collections/Lists.cs

csharp/src/Google.Protobuf/Collections/MapField.cs

csharp/src/Google.Protobuf/Collections/ReadOnlyDictionary.cs

csharp/src/Google.Protobuf/Collections/RepeatedField.cs

Note this is for the Protobuf library itself, not related to generated code. Large but shallow changes: - Added Directory.Build.props with C# 10 targeted and nullable enabled across all projects. - Updated C# README to note already-existing dependency on VS2022 and .NET 6. - Added nullable annotations across all C# code, drawing down to zero warnings/issues. - Made use of a few newer C# features like pattern matching while fixing nullable issues. Wider use of other C# features in a few cases but mostly deferred to later PR(s). Validations: - Unit tests passing including null param exceptions. - Manually verified existing calls to null param check utility code (ProtoPreconditions.CheckNotNull*) line up with non-null annotations. - Dropped main assembly into a private codebase with several gRPC contracts, generated protobuf code, and nullable issues set to error. Verified nullable issues match expected results, i.e. cause no unexpected errors.

jskeet

Next set of comments, and some thoughts about how we deal with this.

I want to reiterate that I'm immensely grateful to you for this effort, and that I do want to get it done. But I think that doing it correctly is going to require splitting it up into multiple PRs simply because my eyes are glazing over too often.

Do you think it's feasible to do this over the course of multiple PRs, even if there's a release during that time? I would hope that if we do this carefully, that won't actually cause a problem other than customers potentially seeing multiple "phases" of warnings. I can look into whether we can effectively make this all internal (i.e. not "advertise" the NRT annotations until we're ready) if that would be useful.

(I'd definitely suggest keeping this branch around as a sort of "reference point" so we can see what's left at any point.)

Do you have a sense of an ordering which would make sense to do this in, e.g. some "core" parts that everything else refers to, then "outer" parts?

Finally, I think there will be some places where we want to make a judgement call about whether introducing a null check would make sense. For example, currently we allow an Extension<TTarget, TValue> to be created with a null codec, but that will always throw an exception if you then call CreateValue on it. I think it's reasonable to consider that a bug, and tighten it up to validate the codec on construction. That may make several aspects easier.

jskeet · 2022-06-01T07:39:20Z

csharp/src/Google.Protobuf/Extension.cs

-    public sealed class RepeatedExtension<TTarget, TValue> : Extension where TTarget : IExtendableMessage<TTarget>
+    public sealed class RepeatedExtension<TTarget, TValue> : Extension
+        where TTarget : IExtendableMessage<TTarget>
+        where TValue : notnull


Same concerns as earlier about whether adding a new generic constraint could be a breaking change. I need to revisit that side of things in general.

jskeet · 2022-06-01T07:44:49Z

csharp/src/Google.Protobuf/ExtensionSet.cs

@@ -78,7 +77,7 @@ public static class ExtensionSet
                {
                    return extensionValue.GetValue();
                }
-                else if (value.GetValue() is TValue underlyingValue)
+                else if (value!.GetValue() is TValue underlyingValue)


If we know this is non-null due to TryGetValue returning true, we should add an appropriate attribute to that method (NotNullWhen I suspect) so that we don't require consumers (this and others) to use the ! operator or check.

If it's actually because this is a special case, that's a different matter.

Looking at the code, I think it would be appropriate to add the NotNullWhen annotation. What do you think?

jskeet · 2022-06-01T07:45:03Z

csharp/src/Google.Protobuf/ExtensionSet.cs

            {
                if (value is RepeatedExtensionValue<TValue> extensionValue)
                {
                    return extensionValue.GetValue();
                }
                else
                {
-                    var valueType = value.GetType().GetTypeInfo();
+                    var valueType = value!.GetType().GetTypeInfo();


(Again, this wouldn't be necessary with an annotation.)

jskeet · 2022-06-01T07:46:45Z

csharp/src/Google.Protobuf/ExtensionSet.cs

        {
-            IExtensionValue value;
+            IExtensionValue? value;


Do we actually need this to be nullable? We're assuming that it's non-null later on, so it would be good to lean on the compiler here.

jskeet · 2022-06-01T07:49:21Z

csharp/src/Google.Protobuf/Extension.cs

@@ -77,7 +77,7 @@ public Extension(int fieldNumber, FieldCodec<TValue> codec) : base(fieldNumber)
            this.codec = codec;
        }

-        internal TValue DefaultValue => codec != null ? codec.DefaultValue : default(TValue);
+        internal TValue? DefaultValue => codec != null ? codec.DefaultValue : default;


It's not clear to me why the codec could be null, to be honest. We do use that a couple of times in tests, but I'm not sure whether it's actually valid. If we could tighten that up, we could potentially make this non-nullable, which would be good. (I haven't looked at FieldCodec yet.)

jskeet · 2022-06-01T07:53:44Z

csharp/src/Google.Protobuf/ExtensionSet.cs

            {
-                IExtensionValue value = extension.CreateValue();
+                IExtensionValue value = extension!.CreateValue();


I think we should be able to get rid of this one too.

jskeet · 2022-06-01T07:55:33Z

csharp/src/Google.Protobuf/ExtensionValue.cs

    }

    internal sealed class ExtensionValue<T> : IExtensionValue
    {
-        private T field;
-        private FieldCodec<T> codec;
+        private T? field;


I think we should be able to make this non-nullable, if we can make codec.DefaultValue non-nullable. It looks like we're relying on it not being null anyway (e.g. in GetHashCode).

(That then changes a bunch of things below as well.)

jskeet · 2022-06-01T07:57:06Z

csharp/src/Google.Protobuf/FieldCodec.cs

@@ -206,7 +205,7 @@ public static FieldCodec<double> ForDouble(uint tag)
        /// <param name="toInt32">A conversion function from <see cref="Int32"/> to the enum type.</param>
        /// <param name="fromInt32">A conversion function from the enum type to <see cref="Int32"/>.</param>
        /// <returns>A codec for the given tag.</returns>
-        public static FieldCodec<T> ForEnum<T>(uint tag, Func<T, int> toInt32, Func<int, T> fromInt32)
+        public static FieldCodec<T?> ForEnum<T>(uint tag, Func<T?, int> toInt32, Func<int, T> fromInt32)


Does this need to be nullable? It's only used for enums, isn't it? (It's unfortunate that we can't add constraints for that now in terms of compatibility.)

erikmav · 2022-06-02T06:03:19Z

@jskeet no worries on redoing incrementally. Give me a couple of days to restart, I'll link each sub-PR into this one for tracking. Initial guess at ordering:

Enable C# 10 in repo and do some of the easy semantic sugar conversions across the codebase to make later PRs smaller.
A round of prep work from your comments - change null semantics prior to applying annotations, remove unused class, and so on.
Start with inner classes and their downstream updates in the public classes. Not very many of those, Maybe all at once or maybe just <= handful at a time.
1-2 public classes at a time after that to make reviews digestible.

jskeet · 2022-06-02T06:19:36Z

Thanks @Erikma - and I can faithfully promise that with smaller changes, I'll get round to them quite quickly. I'm terrible when it comes to putting off large reviews, I'm afraid - sorry you've caught that!

erikmav · 2022-08-19T19:49:51Z

Closing this PR in favor of completed, open, and future PRs as slices of this one.

perezd added the c# label Apr 18, 2022

perezd requested review from jtattermusch and jskeet April 18, 2022 22:57

perezd added release notes: yes labels Apr 18, 2022

protobuf-kokoro removed the kokoro:run label Apr 18, 2022

erikmav force-pushed the dev/erikma/nullable1 branch from 411b074 to 557d86a Compare April 19, 2022 18:34

elharo added kokoro:run labels Apr 20, 2022

protobuf-kokoro removed kokoro:run labels Apr 20, 2022

erikmav force-pushed the dev/erikma/nullable1 branch from 9ecf0fd to 8a75f14 Compare April 23, 2022 04:51

jskeet reviewed Apr 27, 2022

View reviewed changes

jskeet reviewed May 16, 2022

View reviewed changes

erikmav force-pushed the dev/erikma/nullable1 branch 2 times, most recently from 6a2847e to 10be4a4 Compare May 20, 2022 23:38

erikmav added 9 commits May 31, 2022 20:36

Fix proj

e097065

Small updates

3384d62

More PR updates

a535706

PR comments

fb671b2

PR comments

ebae6c1

Remote nullable constraint

99842b7

PR comments

f85865c

Fix up after removal of notnulls

276b26e

erikmav force-pushed the dev/erikma/nullable1 branch from 10be4a4 to 276b26e Compare June 1, 2022 03:38

jskeet self-assigned this Jun 1, 2022

jskeet reviewed Jun 1, 2022

View reviewed changes

erikmav mentioned this pull request Jun 3, 2022

Update to C# 10 and upgrade code style #10105

Merged

erikmav mentioned this pull request Jun 24, 2022

Set all but main project to nullable #10170

Closed

erikmav closed this Aug 19, 2022

JamesNK mentioned this pull request Jul 31, 2023

Add option to enable csharp nullable reference types #13218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.NET: Add nullable annotations to Protobuf library #9801

.NET: Add nullable annotations to Protobuf library #9801

erikmav commented Apr 18, 2022

jskeet commented Apr 19, 2022

erikmav commented Apr 19, 2022

jskeet commented Apr 19, 2022

jskeet left a comment

erikmav commented May 7, 2022

jskeet commented May 7, 2022

jskeet left a comment

jskeet left a comment

jskeet Jun 1, 2022

jskeet Jun 1, 2022

jskeet Jun 1, 2022

jskeet Jun 1, 2022

jskeet Jun 1, 2022

jskeet Jun 1, 2022

jskeet Jun 1, 2022

jskeet Jun 1, 2022

jskeet Jun 1, 2022

erikmav commented Jun 2, 2022

jskeet commented Jun 2, 2022

erikmav commented Aug 19, 2022

.NET: Add nullable annotations to Protobuf library #9801

.NET: Add nullable annotations to Protobuf library #9801

Conversation

erikmav commented Apr 18, 2022

jskeet commented Apr 19, 2022

erikmav commented Apr 19, 2022

jskeet commented Apr 19, 2022

jskeet left a comment

Choose a reason for hiding this comment

erikmav commented May 7, 2022

jskeet commented May 7, 2022

jskeet left a comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikmav commented Jun 2, 2022

jskeet commented Jun 2, 2022

erikmav commented Aug 19, 2022