Enable C# 8 Nullable Reference Types by RehanSaeed · Pull Request #259 · RehanSaeed/Schema.NET

RehanSaeed · 2021-03-15T11:03:43Z

Fixes #127

RehanSaeed · 2021-03-15T11:06:22Z

I spent some time starting this. I have fixed most issues (errors look worse than they are because the types can't be generated until the build is fixed) but there is some internal stuff that I'm unsure of. It seems that there are a lot of places where we are not handling potential nulls correctly.

@Turnerj Would you like to cast your eye over the last 10% of errors? Then I'll see if I can fix anything that's left.

Benchmarks/Schema.NET.Benchmarks/Core/BookBenchmark.cs

Turnerj · 2021-03-15T11:07:38Z

Yep, no problem!

RehanSaeed · 2021-03-15T11:18:56Z

Source/Schema.NET/ValuesJsonConverter.cs

                    }

-                    try
+                    if (Enum.TryParse(targetType, enumString, out result))


Enum.TryParse instead of try/catch. This could also be in another PR.

Don't know why we didn't do this sooner!

Exactly! Performance win too!

I think I found out why - that method doesn't exist in .NET Standard 2.0 😣

Perhaps we can write our own TryParse that supports old platforms? That would move the ugly #if out of this code and turn this into a one liner.

Done! Created an EnumHelper static class with a TryParse method that copies the signature for Enum.TryParse.

Source/Schema.NET/OneOrMany{T}.cs

Turnerj · 2021-03-16T08:39:54Z

Nullable references support for the tool is done. Will look into the other bits left for the various Values<> types and then a few more in the ValuesJsonConverter later tonight.

Tools/Schema.NET.Tool/GeneratorModels/GeneratorSchemaEnumeration.cs

Tools/Schema.NET.Tool/GeneratorModels/GeneratorSchemaObject.cs

Tools/Schema.NET.Tool/GeneratorModels/GeneratorSchemaProperty.cs

Tools/Schema.NET.Tool/GeneratorModels/GeneratorSchemaObject.cs

Source/Schema.NET/DateTimeHelper.cs

Source/Schema.NET/ValuesJsonConverter.cs

Tools/Schema.NET.Tool/GeneratorModels/ICloneable{T}.cs

Turnerj · 2021-03-16T13:42:26Z

Looks like all the nullability changes should be in now though am getting an error on .NET Standard 2.0 at the moment:

I think it is just Visual Studio being weird at the moment.

Turnerj · 2021-03-16T13:44:15Z

Source/Schema.NET/ValuesJsonConverter.cs

+#if NETSTANDARD2_0 || NET472 || NET461
+                    try
+                    {
+                        result = Enum.Parse(targetType, enumString);
+                        success = true;
+                    }
+                    catch (Exception)
+#else
                    if (Enum.TryParse(targetType, enumString, out result))


This seemed like an OK compromise. Not a huge fan of using 3 targets like I am here though.

It did make me wonder, why do we target .NET 4.7.2 and .NET 4.6.1 directly when we also target .NET Standard 2.0?

I repeat this type of check here as well with my switched up way to generate a ToString for PropertyValueSpecification: 85dde5a#diff-4170f976c7ab6112252917fe8fa2c6a2ce7bed515547e0e42e11ea4d67402f9dR47-R51

Unfortunately there are severe bugs in .NET 4.7.2 and .NET 4.6.1 full framework when targetting netstandard2.0. I haven't used full framework for years but have had issues raised to add full framework back in (it's surprising how many people still use it).

Turnerj · 2021-03-17T03:15:06Z

What are your thoughts about disabling nullable reference types for our tests? Our tests are designed to check very specific conditions anyway and will fail on any exception. We might be just adding all the additional markup for it just for it to be mostly irrelevant.

RehanSaeed

The tests I've enabled nullable for have mostly involved liberal use of ! and ? with the occasional switch to immutable types. So nothing too onerous. We don't have many changes to tests currently but not sure if that'll change once everything is converted. Agree that it did feel mostly irrelevant, although I did manage to make the odd improvement.

Overall, I've been surprised at how little code has had to change to my other repos (this one seems the most complex) and nullable seems to have only made a marginal improvement.

Source/Schema.NET/PropertyValueSpecification.Partial.cs

RehanSaeed · 2021-03-17T16:49:37Z

Source/Schema.NET/ValuesJsonConverter.cs

                    }

-                    try
+                    if (Enum.TryParse(targetType, enumString, out result))


Perhaps we can write our own TryParse that supports old platforms? That would move the ugly #if out of this code and turn this into a one liner.

Tools/Schema.NET.Tool/GeneratorModels/ICloneable{T}.cs

RehanSaeed · 2021-03-18T09:41:02Z

Tests/Schema.NET.Test/MixedTypesTest.cs

-            var book = JsonConvert.DeserializeObject<Book>(this.json, TestDefaults.DefaultJsonSerializerSettings);
+            var book = JsonConvert.DeserializeObject<Book>(this.json, TestDefaults.DefaultJsonSerializerSettings)!;

            Assert.True(book.Author.HasValue);


Apparently Assert.NotNull supports nullable reference types, so if you assert that and use it's return value, you can avoid the exclamation here.

Had a look but it doesn't seem to be the case - Assert.NotNull returns voids.

RehanSaeed · 2021-03-18T09:41:26Z

Tests/Schema.NET.Test/ThingTest.cs

        {
-            Assert.Equal(a.GetHashCode(), b?.GetHashCode());
+            Assert.NotNull(a);
+            Assert.Equal(a!.GetHashCode(), b?.GetHashCode());


Same here about Assert.NotNull

RehanSaeed · 2021-03-18T09:43:02Z

Build is green!

RehanSaeed · 2021-03-19T11:46:15Z

LGTM, are we ready to merge?

Turnerj · 2021-03-19T11:53:52Z

Yeah, I'm happy with it!

Enable C# 8 nullable reference types

77eeb00

RehanSaeed added enhancement Issues describing an enhancement or pull requests adding an enhancement. minor Pull requests requiring a minor version update according to semantic versioning. labels Mar 15, 2021

RehanSaeed requested a review from Turnerj March 15, 2021 11:03

RehanSaeed changed the title ~~Enable C# 8 nullable reference types~~ Enable C# 8 Nullable Reference Types Mar 15, 2021

RehanSaeed commented Mar 15, 2021

View reviewed changes

Benchmarks/Schema.NET.Benchmarks/Core/BookBenchmark.cs Show resolved Hide resolved

RehanSaeed commented Mar 15, 2021

View reviewed changes

Handle nullable references for Schema.NET.Tool

0d633da

Turnerj reviewed Mar 16, 2021

View reviewed changes

Source/Schema.NET/OneOrMany{T}.cs Show resolved Hide resolved

RehanSaeed commented Mar 16, 2021

View reviewed changes

Tools/Schema.NET.Tool/GeneratorModels/GeneratorSchemaEnumeration.cs Outdated Show resolved Hide resolved

RehanSaeed commented Mar 16, 2021

View reviewed changes

Source/Schema.NET/DateTimeHelper.cs Show resolved Hide resolved

Fix HashCode nullable parameters

03dff8f

RehanSaeed commented Mar 16, 2021

View reviewed changes

Source/Schema.NET/ValuesJsonConverter.cs Show resolved Hide resolved

RehanSaeed commented Mar 16, 2021

View reviewed changes

Tools/Schema.NET.Tool/GeneratorModels/ICloneable{T}.cs Outdated Show resolved Hide resolved

Turnerj added 2 commits March 16, 2021 23:36

Tweaking generator class behaviour per discussion

da7e097

Various nullability fixes

5d328c1

Turnerj reviewed Mar 16, 2021

View reviewed changes

Turnerj added 2 commits March 17, 2021 13:18

Nullable reference type fixes for date and time converters

f14bf04

Nullable reference type fixes for PropertyValueSpecification

85dde5a

RehanSaeed commented Mar 17, 2021

View reviewed changes

Turnerj added 4 commits March 18, 2021 10:37

Removed cloneable interface

74365f4

Additional fixes for nullable reference type support in tests

8634eab

Added missing brace

7a9b6aa

Efficient string building for PropertyValueSpecification

7be5bfb

RehanSaeed commented Mar 18, 2021

View reviewed changes

Added EnumHelper to offload cross-target workarounds

677945b

Turnerj merged commit 5c222d6 into main Mar 20, 2021

Turnerj deleted the enable-nullable-reference-types branch March 20, 2021 05:52

Uh oh!

Conversation

RehanSaeed commented Mar 15, 2021

Uh oh!

RehanSaeed commented Mar 15, 2021

Uh oh!

Uh oh!

Turnerj commented Mar 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Turnerj commented Mar 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Turnerj commented Mar 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Turnerj commented Mar 17, 2021

Uh oh!

RehanSaeed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RehanSaeed commented Mar 18, 2021

Uh oh!

RehanSaeed commented Mar 19, 2021

Uh oh!

Turnerj commented Mar 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants