Nullable reference types #581

jnm2 · 2019-10-14T01:19:06Z

Closes #579.

I tried to keep the big initial annotation commit as straightforward as possible, saving more complex fixes for later commits. For files where the changes aren't absolutely obvious, please consider reviewing single commits at a time.

➡ Please comment if you'd like any of the types in these APIs to be annotated differently. I simply annotated them as nullable if null was tolerated in previous releases and left them non-nullable if null would have caused an exception. All nullable annotations (? and attributes) are a description of my understanding of the past rather than a statement from me about the future.

src/Shouldly.Tests/ConventionTests/ApprovePublicApi.ShouldlyApi.approved.cs

jnm2 · 2019-10-15T22:07:33Z

src/Shouldly.Tests/Shouldly.Tests.csproj

-    <PackageReference Include="PublicApiGenerator" Version="8.0.1" />
+    <PackageReference Include="PublicApiGenerator" Version="10.0.0-beta1" />
    <PackageReference Include="TestStack.ConventionTests" Version="3.0.1" />
-    <DotNetCliToolReference Include="dotnet-xunit" Version="2.3.1" />


dotnet-xunit was discontinued: https://github.com/xunit/xunit/issues/1835#issuecomment-431199286

Version 2.3.1 has no rollforward policy and can only run on .NET Core 2.0 which is EOL.

jnm2 · 2019-10-16T22:30:41Z

src/Shouldly.Tests/Shouldly.Tests.csproj

  <PropertyGroup>
    <TargetFrameworks>netcoreapp2.1</TargetFrameworks>
    <TargetFrameworks Condition=" '$(OS)' == 'Windows_NT' ">$(TargetFrameworks);net472;</TargetFrameworks>
-    <LangVersion>latest</LangVersion>


Moved to Directory.Build.props so that it could be next to <Nullable>enable</Nullable>

jnm2 · 2019-10-16T22:50:41Z

src/Shouldly/Internals/EqualityComparer.cs

-                if (object.Equals(x, default(T)))
-                    return object.Equals(y, default(T));
+                if (object.Equals(x, null))
+                    return object.Equals(y, null);

-                if (object.Equals(y, default(T)))
+                if (object.Equals(y, null))


default(T) is always null given the if condition. This is nicer than doing default(T)!.

jnm2 · 2019-10-16T22:51:04Z

src/Shouldly/Internals/EqualityComparer.cs

                return true;

            // Null?
            if (!type.IsValueType() || (type.IsGenericType() && type.GetGenericTypeDefinition().IsAssignableFrom(NullableType)))


This check would be simpler as if (default(T)! is null). I could find all references of IsValueType and NullableType in a separate PR, but I'd like this PR to be as noise-free as possible.

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEUCuA7AHwAEAmARgFgAoUgBmtIGZqBvagAk/dJI67apch7ACowAzhgA8ASzwYAfAAoARHIwqAlAG4+w0ROnqA/MrXzjW3YP0HJUomVpnHtK3vYBfDx4AOUGQA3AEMMGG4yJG4UO2kRZVd2MHEAC2CoXwBlAE95YIRNDwFbCIBOJQASFQATGAAzYJwAGwwlFmS0jJy8hE9NdhlxdjxmppB2FlqG5taRfsHh0c93G05vKk8gA=

jnm2 · 2019-10-16T23:04:43Z

src/Shouldly/Internals/EqualityComparerAdapter.cs

+        public new bool Equals(object? x, object? y)
        {
-            return _innerComparer.Equals((T) x, (T) y);
+            return _innerComparer.Equals((T)x!, (T)y!);


The cast will always succeed when T is a nullable type at runtime (either a reference type or Nullable<>). The ! operator suppresses the warning that an exception will be thrown if x or y is null and T is a non-nullable value type. Shouldly expects this, just as an exception will be thrown if an object of the wrong type is passed.

jnm2 · 2019-10-16T23:27:19Z

src/Shouldly/Internals/Is.cs

    internal static class Is
    {
-        public static bool InRange<T>(T comparable, T @from, T to) where T : IComparable<T>
+        public static bool InRange<T>([DisallowNull] T comparable, [AllowNull] T @from, [AllowNull] T to) where T : IComparable<T>


[AllowNull] and [DisallowNull] are the only way to annotate these parameters since C# still does not allow T? here. (Allowing T? without a class or struct constraint would be a CLR change.)

jnm2 · 2019-10-16T23:33:52Z

src/Shouldly/ShouldlyExtensionMethods/ShouldBeEnumerableTestExtensions.cs

        }

-        public static void ShouldBeEmpty<T>(this IEnumerable<T> actual)
+        public static void ShouldBeEmpty<T>([NotNull] this IEnumerable<T>? actual)


[NotNull] initially seems to conflict with the ?, but there is no conflict. [NotNull] is indicating that if this call returns, the expression that had been passed to actual cannot have been null. At the same time, the method accepts null inputs without a compile-time warning.

You could also choose to go with ShouldBeEmpty<T>(this IEnumerable<T> actual) in which case the compiler would warn if the user does foo.ShouldBeEmpty(); without first proving to the compiler that foo can't be null (or using !).

jnm2 · 2019-10-16T23:37:03Z

src/Shouldly/Internals/Numerics.cs

        /// <param name="obj">The object to check</param>
        /// <returns>true if the object is a numeric type</returns>
-        public static bool IsNumericType(Object obj)
+        public static bool IsNumericType([NotNullWhen(true)] object? obj)


NotNullWhen is used to tell the compiler's flow state analysis what happens after a call to this method:

nullableThing = ... if (IsNumericType(nullableThing)) { nullableThing.GetHashCode(); // No warning because IsNumericType returned true } nullableThing.GetHashCode(); // Compiler warning because `nullableThing` could be null at this point

jnm2 · 2019-10-16T23:42:55Z

src/Shouldly/Internals/ShouldThrowAssertionContext.cs

-            System.Diagnostics.StackTrace stackTrace = null,
-            [CallerMemberName] string shouldlyMethod = null) : base(shouldlyMethod, expected, actual, stackTrace)
+            System.Diagnostics.StackTrace? stackTrace = null,
+            [CallerMemberName] string shouldlyMethod = null!) : base(shouldlyMethod, expected, actual, stackTrace)


I don't think there's a situation where the default value null could be used.
https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.callermembernameattribute#remarks

jnm2 · 2019-10-16T23:45:51Z

src/Shouldly/Internals/SourceCodeTextGetter.cs

                var codeLines = string.Join("\n", File.ReadAllLines(FileName).Skip(LineNumber).ToArray());

-                var indexOf = codeLines.IndexOf(_shouldMethod);
+                var indexOf = codeLines.IndexOf(_shouldMethod!);


The way to avoid all these dammit operators (!) would be to pass the value between methods rather than making it a class field. The state of a mutable class field is also harder to reason about between methods for people as well as compilers, so I would recommend this.

jnm2 · 2019-10-16T23:49:37Z

src/Shouldly/Internals/StringHelpers.cs

+        static string DelimitWith<T>(this IEnumerable<T> enumerable, string? separator) where T : class?
        {
-            return string.Join(separator, enumerable.Select(i => Equals(i, default(T)) ? null : i.ToString()).ToArray());
+            return string.Join(separator, enumerable.Select(i => Equals(i, null) ? null : i.ToString()).ToArray());


default(T) was always null already because of the class constraint. Using null is even nicer now that the other option is default(T)!.

jnm2 · 2019-10-16T23:52:30Z

src/Shouldly/MessageGenerators/DictionaryShouldContainKeyAndValueMessageGenerator.cs

+            Debug.Assert(context.Actual is IDictionary);
+            Debug.Assert(context.Key is object);


⚠ Not sure how you'd like to handle classes like these. This method is relying on the fragile assumption that remote code will have set these fields, and these asserts silence the warnings this causes now that NRTs are enabled. The way I would handle it would be to require these values to be passed to the DictionaryShouldContainKeyAndValueMessageGenerator constructor.

jnm2 · 2019-10-16T23:59:21Z

src/Shouldly/ShouldlyExtensionMethods/ShouldBe/GenericShouldBeTestExtensions.cs


        [ContractAnnotation("actual:null,expected:null => halt")]
-        public static void ShouldNotBe<T>(this T actual, T expected)
+        public static void ShouldNotBe<T>([AllowNull] this T actual, [AllowNull] T expected)


[AllowNull] is needed on each so that generic inference doesn't get upset while deciding whether T is nullable or not when one expression is nullable and the other isn't.

jnm2 · 2019-10-17T00:06:07Z

src/Shouldly/ShouldlyExtensionMethods/ShouldBe/StringNullOrEmptyTestExtensions.cs

 {
    public static partial class ShouldBeStringTestExtensions
    {
        [ContractAnnotation("actual:notnull => halt")]


Side note: I think these JetBrains annotations might actually be incorrect. If actual is "", a non-null value, the method does not halt.

jnm2 · 2019-10-17T00:09:11Z

src/Shouldly/ShouldlyExtensionMethods/ShouldBeBooleanExtensions.cs

    public static class ShouldBeBooleanExtensions
    {
-        public static void ShouldBeTrue(this bool actual)
+        public static void ShouldBeTrue([DoesNotReturnIf(false)] this bool actual)


DoesNotReturnIf is the annotation that powers scenarios like this:

nullableThing.GetHashCode(); // Compiler warning, possible NRE (nullableThing != null).ShouldBeTrue(); nullableThing.GetHashCode(); // No warning because ShouldBeTrue did not throw an exception

jnm2 · 2019-10-17T00:10:38Z

src/Shouldly/ShouldlyExtensionMethods/ShouldBeDictionaryTestExtensions.cs

+    public static class ShouldBeDictionaryTestExtensions
    {
        public static void ShouldContainKey<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)
+            where TKey : notnull


This is necessary in order to use TKey for IDictionary<TKey, TValue> due to the generic constraint added to IDictionary<,> in the BCL.

jnm2 · 2019-10-17T00:19:11Z

src/Shouldly/ShouldlyExtensionMethods/ShouldBeNullExtensions.cs

    {
        [ContractAnnotation("actual:notnull => halt")]
-        public static void ShouldBeNull<T>(this T actual)
+        public static void ShouldBeNull<T>([MaybeNull] this T actual)


If this method returns, the actual expression is definitely null. [MaybeNull] is the strongest the .NET annotations get. Threw it in for completeness.

var nonnullableThing = new object(); nonnullableThing.GetHashCode(); // No warning nonnullableThing.ShouldBeNull(); nonnullableThing.GetHashCode(); // Warning that nonnullableThing may be null

jnm2 · 2019-10-17T00:20:46Z

src/Shouldly/ShouldlyMessage.cs

        }
    }

    internal class ExpectedEquvalenceShouldlyMessage : ShouldlyMessage


This is a typo.

…ception has no inner exceptions

…exceptions

…the structure of the search

…ing GetMethod unnecessarily

… sorting methods by parameter count)

jnm2 · 2019-12-13T14:38:22Z

@josephwoodward New force push. Two of the commits have already been merged in #596 and I fixed merge conflicts caused by #595.

josephwoodward · 2019-12-13T17:09:25Z

@jnm2 Nice! So is this good to go? (pending review of course)

jnm2 · 2019-12-13T17:53:13Z

@josephwoodward Yes!

… is invalid" on Mono

jnm2 · 2020-03-02T19:54:41Z

@josephwoodward I merged with master to fix merge commits, then I recognized the build failure as being related to certain versions of Mono and Cake pre-0.37 and fixed that, then I fixed some new warnings that will crop up in VS 16.5.

I'm not sure reviewing and merging will get more appealing as time goes on, and I don't know what to do to make that easier.

jnm2 · 2020-07-05T02:12:11Z

@SimonCropp I was going to push a merge commit to resolve the conflicts. Would it make more sense to wait till the dust settles from the PRs you're merging, or would there be another approach altogether that would work better for you?

SimonCropp · 2020-07-05T03:27:32Z

i can hold off for a few hours. or i can handle the conflicts and merge for u

jnm2 · 2020-07-05T17:48:02Z

@SimonCropp You might have better context than me for fixing the conflicts since you are familiar with what's happened on master in the meantime. That sounds best, but I'm also happy to do it. Once this PR merges, there is a good chance that most outstanding PRs will have merge conflicts too. The tradeoff is fixing things individually versus adding the equivalent work in bulk to this PR. Is one better than the other?

SimonCropp · 2020-07-06T07:18:03Z

@jnm2 i manually merged here #634. there still are a few nullability warnings. i was wondering if you could have a look at the warnings on master, given you clearly know more about nullability than me

jnm2 · 2020-07-06T12:25:44Z

@SimonCropp For sure!

jnm2 force-pushed the nullable_reference_types branch 4 times, most recently from facfc3e to 787f73c Compare October 14, 2019 02:16

jnm2 commented Oct 14, 2019

View reviewed changes

src/Shouldly.Tests/ConventionTests/ApprovePublicApi.ShouldlyApi.approved.cs Outdated Show resolved Hide resolved

jnm2 commented Oct 15, 2019

View reviewed changes

jnm2 commented Oct 16, 2019

View reviewed changes

jnm2 force-pushed the nullable_reference_types branch from 8009c7a to 1774735 Compare October 16, 2019 23:26

jnm2 commented Oct 16, 2019

View reviewed changes

jnm2 commented Oct 17, 2019

View reviewed changes

src/Shouldly/ShouldlyMessage.cs

}

}

internal class ExpectedEquvalenceShouldlyMessage : ShouldlyMessage

Copy link

Collaborator Author

jnm2 Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a typo.

jnm2 force-pushed the nullable_reference_types branch 5 times, most recently from e0cd649 to bfff1ba Compare October 20, 2019 20:53

jnm2 mentioned this pull request Oct 31, 2019

Fix broken tests from master #589

Merged

jnm2 added 13 commits December 13, 2019 09:33

Failing test for NRE and incorrect exception message when AggregateEx…

fb58999

…ception has no inner exceptions

Fix NRE when AggregateException has no inner exceptions

823ca18

Fix incorrect exception message when AggregateException has no inner …

63b4647

…exceptions

Clarify the circumstances under which InnerException will not be null

4471efb

Document Task.Exception.InnerException nullability

f84b0cf

Failing test for NRE when expected instance returns null from ToString

dfe799b

Fix NRE when expected instance returns null from ToString

d60440e

Move defaults to default ctor so that properties can be non-nullable

d15a645

Let GetFrame decide whether i is out of bounds since it returns null

b19ee1d

Rewrite ParseStackTrace to check for nulls appropriately and clarify …

1850bb4

…the structure of the search

Rewrite TestMethodInfo to check for nulls appropriately and stop call…

291053d

…ing GetMethod unnecessarily

Rewrite test method finders to check for nulls appropriately

1d0c0c5

Move to ApiApprover v10.0.0 and stop using obsoleted API (beta6 began…

e74d529

… sorting methods by parameter count)

jnm2 force-pushed the nullable_reference_types branch from 4573ed5 to e74d529 Compare December 13, 2019 14:36

jnm2 added 3 commits March 2, 2020 14:17

Fix merge conflicts

e75715f

Cake 0.37 fixes build script failing to start with "The assembly name…

2eb4f65

… is invalid" on Mono

Fix new warnings coming in VS 16.5

1c81432

jnm2 mentioned this pull request Mar 10, 2020

Fix CI so that pull requests can be merged #605

Merged

SimonCropp mentioned this pull request Jul 6, 2020

Jnm2 nullable reference types #634

Merged

SimonCropp closed this Jul 6, 2020

jnm2 mentioned this pull request Jul 7, 2020

Redo nullable reference types fixes from original branch #641

Merged

		Debug.Assert(context.Actual is IDictionary);
		Debug.Assert(context.Key is object);

Uh oh!

Nullable reference types #581

Nullable reference types #581

Uh oh!

Conversation

jnm2 commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jnm2 Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jnm2 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jnm2 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jnm2 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jnm2 commented Dec 13, 2019

Uh oh!

josephwoodward commented Dec 13, 2019

Uh oh!

jnm2 commented Dec 13, 2019

Uh oh!

jnm2 commented Mar 2, 2020

Uh oh!

jnm2 commented Jul 5, 2020

Uh oh!

SimonCropp commented Jul 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Jul 5, 2020

Uh oh!

SimonCropp commented Jul 6, 2020

Uh oh!

jnm2 commented Jul 6, 2020

Uh oh!

jnm2 commented Oct 14, 2019 •

edited

Loading

jnm2 Oct 15, 2019 •

edited

Loading

jnm2 Oct 16, 2019 •

edited

Loading

jnm2 Oct 16, 2019 •

edited

Loading

jnm2 Oct 16, 2019 •

edited

Loading

jnm2 Oct 16, 2019 •

edited

Loading

jnm2 Oct 16, 2019 •

edited

Loading

jnm2 Oct 16, 2019 •

edited

Loading

jnm2 Oct 17, 2019 •

edited

Loading

SimonCropp commented Jul 5, 2020 •

edited

Loading