If V4 goes for nullable properties - mark them with ? nullable ref type syntax to clearly indicate this

[alex-jitbit]

Describe the bug

For example ListObjectsResponse.S3Objects can be null if no objects are found.

Should be marked with ? - indicating nullable reference type.

Regression Issue

• Select this option if this issue appears to be a regression.

Expected Behavior

ListObjectsResponse.S3Objects should be declared like this

public List<S3Object>? S3Objects //like this

Current Behavior

Currently it's not using nullable reference type. So no warnings from the compiler

Reproduction Steps

n/a

Possible Solution

No response

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

AWSSDK.S3 4.0.1

Targeted .NET Platform

.NET 8

Operating System and version

all

[normj]

We had lots of debates on whether to use .NET nullable reference types. The major problems we had with using it was we still need to support .NET Framework which has questionable support for using nullable reference types. The other challenge is with the design of the SDK and every POCO having get and set methods for the properties we would essentially have to mark everything as nullable which would create a swarm of compiler warnings/errors for everybody.

We did add some Roslyn analyzers in V4 to help detect some of the null collection use cases. I think we need to invest more effort into our Roslyn analyzers to help catch potential null exceptions at compile time.

[alex-jitbit]

@normj It's OK to not want to that , but you EITHER have to indicate nullable intention at the code level OR mention this in the docs & xml-comments for public members.

For example. Look at the ListObjectResponse docs https://docs.aws.amazon.com/sdkfornet/v4/apidocs/items/S3/TListObjectsResponse.html There is no indication that S3Objects property can be null. None. Nada. F*cking zero. I had to debug my code that started thrown NREs after migrating to v4. (Even the example code on that page has no null checks)

If the property was nullable (or had proper XML docs) any developer that hovers this property with a mouse - would know what's going on.

[normj]

@alex-jitbit You are right we should make this change more obvious in the API docs. I put out this PR that updates our generator to inject the comment into the ndoc which will flow out to the API reference. #3874

[alex-jitbit]

@normj thanks!! appreciate

[normj]

@alex-jitbit PR is out to also update the sample that was in the docs to have the null check: #3878

[Dreamescaper]

The major problems we had with using it was we still need to support .NET Framework which has questionable support for using nullable reference types.

You only need to define a couple of internal attributes to be able to annotate nullability for NET Framework (like it's already done for AOT attributes).

Still, I'm wondering.
Would it make sense to change Lists in responses DTOs to IReadOnlyLists ?
In that case empty arrays could be used instead of nulls, and empty arrays don't allocate.

(I realize that it woundn't happen for V4. Still, wondering whether there are any reasons it shouldn't be done)

[normj]

@Dreamescaper I have wondered as well if we should have used read only lists. It would have caused more code breakage as we have seen a lot of code out there where users add items to collection directly from the request object. It would have been a compile time issue though. With V4 GA now we don't have a way of changing that now and need to keep looking for ways to warn users about the potential.

I'm going to close the issue now that we have updated the docs to warn users and update the examples in the docs to have the null checks.