Consider enable nullable annotation

[Tunduk]

Since .NET 6 nullable reference types are enabled by default. It would be good to enable it in Flurl too.
There are a lot of comments like "otherwise null".
Of course, I am ready to help with this issue.

[tmenier]

Can you help me understand if using Flurl in a project where nullable reference types are enabled may cause actual problems? Is it just that nullable types have a certain "self-documenting" nature to them? I'll admit I'm not fully up to speed on them, but as far as I know, properly introducing them in big(ish), established codebase may require lots of changes all over the place, so I want to understand if the benefits are worth the effort.

[rcdailey]

I'd like to jump in here and share my thoughts. One case I ran up on is this:

case FlurlHttpException e2:
    _log.Error("HTTP error: {Message}", e2.SanitizedExceptionMessage());
    var errors = await e2.GetResponseJsonAsync<IReadOnlyCollection<ValidationErrorHttpResponse>>();
    foreach (var error in errors)
    {
        _log.Error("Reason: {Error}", error.ErrorMessage);
    }
    break;

The documentation wasn't very helpful, but it seems like GetResponseJsonAsync can return null:

		public async Task<T> GetJsonAsync<T>() {
			if (_streamRead) {
				if (_capturedBody == null) return default;
				if (_capturedBody is T body) return body;
			}

			var call = ResponseMessage.RequestMessage.GetFlurlCall();
			_serializer = _serializer ?? call.Request.Settings.JsonSerializer;

			try {
				if (_streamRead) {
					// Stream was read but captured as a different type than T. If it was captured as a string,
					// we should be in good shape. If it was deserialized to a different type, the best we can
					// do is serialize it and then deserialize to T, and we could lose data. But that's a very
					// uncommon scenario, hopefully. https://github.com/tmenier/Flurl/issues/571#issuecomment-881712479
					var s = _capturedBody as string ?? _serializer.Serialize(_capturedBody);
					_capturedBody = _serializer.Deserialize<T>(s);
				}
				else {
					using (var stream = await ResponseMessage.Content.ReadAsStreamAsync().ConfigureAwait(false))
						_capturedBody = _serializer.Deserialize<T>(stream);
				}
				return (T)_capturedBody;
			}
			catch (Exception ex) {
				_serializer = null;
				_capturedBody = await ResponseMessage.Content.ReadAsStringAsync().ConfigureAwait(false);
				_streamRead = true;
				call.Exception = new FlurlParsingException(call, "JSON", ex);
				await FlurlRequest.HandleExceptionAsync(call, call.Exception, CancellationToken.None).ConfigureAwait(false);
				return default;
			}
			finally {
				_streamRead = true;
			}
		}

The above code showed me that if Deserialize() threw an exception, the catch you have is instructed to return default which will be null in my case since T is a record type.

That means the code from my first snippet is being misled. The Roslyn analyzers assume that the return value may not be null because the return type is Task<T> instead of Task<T?>. That means my foreach loop will result in a NRE if the response schema doesn't match the DTO.

[EricStG]

To add to this, we ran into a NRE in the AfterCall callback on a timeout, because IFlurlResponse.Response was null, and we weren't expecting it.

[Tunduk]

I should pay more attention to my mailbox =)
Thank you @rcdailey and @EricStG for your feedback!
These are great examples!

@tmenier
You can see that nullable reference types isn't just self-documenting feature.
I agree that it could be hard to introduce them to the existing project but not impossible.

What do you think? We could do it partially by using nullable pragma

[tmenier]

I'll add this to the backlog.

You can see that nullable reference types isn't just self-documenting feature.

I'm not sure I agree. Both examples above demonstrate the same basic problem: getting a null result was surprising. That said, they're good examples of why this would be a good enhancement.

[Tunduk]

That's great!
Thanks!

[tmenier]

@Tunduk Also I appreciate your offer to help! That would be great, but I would say don't start quite yet. I'm catching up on a few things and will be making some major commits for 4.0 soon. I think it would be smart to come through with your enhancements after I'm mostly done ripping things up. :)

One other thing to consider: Flurl has broad cross-platform support, per best practices, including platforms that do not support nullable reference types. This looks like a useful guide:

https://www.meziantou.net/how-to-use-nullable-reference-types-in-dotnet-standard-2-0-and-dotnet-.htm

But I'll warn you in advance: I don't know that I want the code to be littered with tons of #if directives everywhere just to support this. If it starts looking that way let's discuss - I might want to hold off until such time that it's recommended to finally leave netstandard2 behind.

[rcdailey]

I don't have much experience with them myself, but in the link you shared, it does talk about nullability attributes. I didn't read the full page, but that seems to correlate with other pages like this one. Maybe the attributes can be used without sprinkling #if logic everywhere.

[cremor]

I also think that you wouldn't need #if in your normal code. You'd only need it for the attributes definitions that are missing in older target frameworks.
You could also use https://github.com/Sergio0694/PolySharp to automatically generate the missing attributes (and much more if you like).