Issue 1671: Add MemberNotNullWhen attribute for Content property in IApiResponse<T>

[sguryev]

Potential fix for #1671

An alternative way:
new bool IsSuccessStatusCode { get; } property in the IApiResponse<T> interface with all 3 attributes above

[HavenDV]

This is similar to Breaking Change because IApiResponse<out T> is no longer IApiResponse and this may affect those using such a cast.

I suggest doing this via virtual, like here - https://stackoverflow.com/questions/72232151/membernotnullwhen-or-other-code-analysis-attributes-for-inherited-members

For some reason I was sure that it was possible to use virtual. Ok, then we should consider using new, but this can also cause some problems

[sguryev]

@ChrisPulman oopsie, I'm sorry. I have forgotten to add IApiResponse interface to the ApiResponse<T> class.

Added here: https://github.com/reactiveui/refit/pull/1672/files#diff-092434a9c47d4491a15166652006ca2e0263b01b63c2029aa8653e165bfa543bR32

All tests are green now locally

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 87.68%. Comparing base (6ebeda5) to head (e27b0a6).
Report is 23 commits behind head on main.

Files	Patch %	Lines
Refit/ApiResponse.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1672      +/-   ##
==========================================
- Coverage   87.73%   87.68%   -0.05%     
==========================================
  Files          33       33              
  Lines        2348     2323      -25     
  Branches      294      289       -5     
==========================================
- Hits         2060     2037      -23     
  Misses        208      208              
+ Partials       80       78       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sguryev]

@ChrisPulman did you have time to check how analyzer react on new T? Content property?
I have tried to set target net6.0,net7.0 for Refit itself in order to check how does it work for IApiResponse<T>
It works for IApiResponse but it doesn't work for IApiResponse<T>
My original idea was to modify the Refit the way I don't need to suppress analyzer for instances I have checked IsSuccessStatusCode. Now I have to check the flag and content together if (!saleSessionResponse.IsSuccessStatusCode || saleSessionResponse.Content == null) (and keep in mind that Content can't be null for true)

[ChrisPulman]

I have a PR that needs to go in to revert the targets back to Net6.0, Net8.0, the current code still has the NET6.0_Or_Greater flags

[ChrisPulman]

I have now merged my PR, I will add the tests you have shared pictorially, and we can go from there.

Note for next time, adding tests to show how your change will work helps us to understand your use case, it also means that once we accept that we can ensure nothing else breaks this functionality without us knowing.

[ChrisPulman]

I ran the tests and the attributes don't seem to do anything, something is wrong here.

I understand the Attributes are supposed to return a non-null value when the condition is met.
Therefore, the test below should have been success, however it fails.

class MemberNotNullWhenTest(bool isSuccessStatusCode)
{
    public object? Content { get; set; }
    public object? ContentHeaders { get; set; }
    public object? Error { get; set; }


    [MemberNotNullWhen(true, nameof(Content))]
    [MemberNotNullWhen(true, nameof(ContentHeaders))]
    [MemberNotNullWhen(false, nameof(Error))]
    public bool IsSuccessStatusCode { get; } = isSuccessStatusCode;
}

[Fact]
public void MemberNotNullWhenTest_IsSuccessStatusCodeReturnsNonNullValues()
{
var memberNotNullWhenTest = new MemberNotNullWhenTest(true);
    if (memberNotNullWhenTest.IsSuccessStatusCode)
    {
        Assert.NotNull(memberNotNullWhenTest.Content);
        Assert.NotNull(memberNotNullWhenTest.ContentHeaders);
        Assert.Null(memberNotNullWhenTest.Error);
    }
    else
    {
        Assert.False(true);
    }
}

What are your thoughts on this?
I should have added the tests before merging this, but it made sense at the time.
In my opinion, the Attributes are failing to do their job.

[sguryev]

@ChrisPulman let me look closer later today

[ChrisPulman]

@sguryev thank you, the attributes really seem to be doing nothing, I have tested them in a plain library ensured that it was Nullable but nothing happens. In theory they are great but only if they do the task required

[sguryev]

@ChrisPulman Unfortunately it's not something you can easily test with unit testing. It's about null-state static analysis.

You can find explanation here: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis#helper-methods-membernotnull-and-membernotnullwhen

I have also prepared the sample you can try. Please notice the WarningsAsErrors attribute in the csproj file
csproj:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>

        <WarningsAsErrors>CS8600</WarningsAsErrors>
        <!--<WarningsAsErrors>CS8600;CS8601;CS8602;CS8603;CS8604;CS8613;CS8614;CS8619;CS8620;CS8622;CS8625;CS8629;CS8633,CS8767,CS8524,CS8509</WarningsAsErrors>-->
    </PropertyGroup>

</Project>

Program:

// See https://aka.ms/new-console-template for more information

using System.Diagnostics.CodeAnalysis;

string str = "sample";

var nonAttrModel = new NonAttrModel();
if (nonAttrModel.HasContent)
    str = nonAttrModel.Content;

var attrModel = new AttrModel();
if (attrModel.HasContent)
    str = attrModel.Content;

Console.WriteLine($"Hello, World! {str}");

public class NonAttrModel
{
    public bool HasContent { get; set; }
    public string? Content { get; set; }
}

public class AttrModel
{
    [MemberNotNullWhen(true, nameof(Content))]
    public bool HasContent { get; set; }
    public string? Content { get; set; }
}

You should see something like this:

There is a bunch of warnings which can be threated as errors in order to prevent assign nullable ref types to non-null ref types to avoid null-ref exceptions in runtime.

According to the current code in Refit I think than new modifier breaks the analysis. So attribute works for the IApiResponse.Content and not for IApiResponse<T>.Content because of the new modified.

Please let me know if you have any questions

Attributes interpreted by the compiler: Nullable static analysis - C# reference

Learn about attributes that are interpreted by the compiler to provide better static analysis for nullable and non-nullable reference types.

[ChrisPulman]

I also saw the difference with the new

In addition to this I have the following interfaces

The IApiResponse interface
```
///
public interface IApiResponse : IApiResponseBase
{
///

/// Deserialized request content as .
///

T? Content { get; }

    /// <summary>
    /// Indicates whether the request was successful.
    /// </summary>

#if NET6_0_OR_GREATER
[MemberNotNullWhen(true, nameof(Content), nameof(ContentHeaders))]
[MemberNotNullWhen(false, nameof(Error))]
#endif
bool IsSuccessStatusCode { get; }

    /// <summary>
    /// HTTP response content headers as defined in RFC 2616.
    /// </summary>
    HttpContentHeaders? ContentHeaders { get; }

    /// <summary>
    /// The <see cref="ApiException"/> object in case of unsuccessful response.
    /// </summary>
    ApiException? Error { get; }
}

The IApiResponse interface

/// <summary>
/// IApiResponse.
/// </summary>
/// <seealso cref="Refit.IApiResponseBase" />
public interface IApiResponse : IApiResponseBase
{
    /// <summary>
    /// Indicates whether the request was successful.
    /// </summary>

#if NET6_0_OR_GREATER
[MemberNotNullWhen(true, nameof(ContentHeaders))]
[MemberNotNullWhen(false, nameof(Error))]
#endif
bool IsSuccessStatusCode { get; }

    /// <summary>
    /// HTTP response content headers as defined in RFC 2616.
    /// </summary>
    HttpContentHeaders? ContentHeaders { get; }

    /// <summary>
    /// The <see cref="ApiException"/> object in case of unsuccessful response.
    /// </summary>
    ApiException? Error { get; }
}

The Base Interface

///

/// Base interface used to represent an API response.
///

public interface IApiResponseBase : IDisposable
{
///

/// HTTP response headers.
///

HttpResponseHeaders Headers { get; }

/// <summary>
/// HTTP response status code.
/// </summary>
HttpStatusCode StatusCode { get; }

/// <summary>
/// The reason phrase which typically is sent by the server together with the status code.
/// </summary>
string? ReasonPhrase { get; }

/// <summary>
/// The HTTP Request message which led to this response.
/// </summary>
HttpRequestMessage? RequestMessage { get; }

/// <summary>
/// HTTP Message version.
/// </summary>
Version Version { get; }

}

I will be travelling for the majority of Tuesday and Wednesday but will pick this up again when I can.

[sguryev]

My idea was to add IApiResponseBase and extract IsSuccessStatusCode property from the base interface in order to be able to add attributes over IsSuccessStatusCode in each implementation.

AFAIU these attributes don't like base or hidden properties inside.
We don't need interface-specific IsSuccessStatusCode implementation since it has identical signature.

Also we don't need to modify the class ApiReponse<T> itself because attributes there are consistent.

[ChrisPulman]

Really sorry, would it be possible for you to create a new PR with the updated version. I will try to come up with a test that we can prove this works and ensure that future changes maintain the functionality

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.