Enable Nullable Reference Types

[JimBobSquarePants]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have verified that I am running the latest version of ImageSharp
• [X] I have verified if the problem exist in both DEBUG and RELEASE mode
• [X] I have searched open and closed issues to ensure it has not already been reported

ImageSharp version

v3 alpha +

Other ImageSharp packages and versions

NA

Environment (Operating system, version and so on)

NA

.NET Framework version

NA

Description

Following some initial investigation, it appears adding Nullable Reference Types to the solution is not as much work as I initially feared.

• Only 824 errors were reported
• 119 can be ignored in AotCompilerTools by explicitly disabling the analysis there via #nullable disable
• Most of the errors can be fixed by updating equality overrides which are missing attributes.

Enabling analysis requires adding the following to individual .csproj files

<NullableContextOptions>enable</NullableContextOptions>
<Nullable>enable</Nullable>

We should consider doing this for V3.

Steps to Reproduce

NA

Images

No response

[stefannikolei]

In my eyes NullableContextOptions is not needed anymore. Have a look at https://github.com/dotnet/sdk/issues/3256

Rather set <WarningsAsErrors>Nullable</WarningsAsErrors> <Nullable>enable</Nullable>

[JimBobSquarePants]

Thanks for the update. I thought they were two separate properties.

We should be able to simply use the latter declaration because we already use warnings as errors in release mode

[tocsoft]

I'll drop the comment in here to, for high level general approach on how to tackle the task.

This issue will need to be tackled in a few phases.

1. Enable nullability checks globally (single PR) update csprojs but inline disable the check for any/all files that are reporting warning because of it. This enables us to get new code checked correctly and/or existing code that is already valid staying valid but doesn't require validating all the change in one big bang.
2. Public API surface (one or more PRs) for each part of the public api surface ensure the nullablity checks are enabled and issues fixed (limiting each PR to a single logic area where possible), for example each processor in turn and associated extensions.
3. Internal APIs (one or more PRs) general clean up, again in logic areas (JPEG Decoder, PNG Decoder etc) After this stage no code should have nullable reference type checking disabled.

I believe this is the only logic approach to tacking this with the resource we have on the team for code reviews alone let alone apply all the fixes.

This approach also makes it much more reasonable to split the work, once the first PR is in different people can tackle different areas of the code base where they might be more familiar with the semantics and patterns already in place.

[stefannikolei]

So step one is done.

Will someone make a plan for the next steps? I would help with the next steps.

Perhaps a list of the affected public api?

[JimBobSquarePants]

Will have a look asap.

[JimBobSquarePants]

One area I particularly want to have a look at is the Image.Load APIs and their async variants.

Currently we throw an exception when we cannot load an image, which on the surface is well structured and should be useful. However, my observation is that issues reported by individuals that contain this message never seem to have an indication of any increased understanding as a result of it.

I would propose that we convert the methods to use the Try...Out... pattern.

For example:

public static bool TryLoad<TPixel>(Stream stream, [NotNullWhen(true)] out Image<TPixel> image)
public static async Task<(bool Success, Image<TPixel>? Image)> TryLoadAsync(DecoderOptions options, Stream stream, CancellationToken cancellationToken = default)

If we combine the approach with #2090 as suggested by @tocsoft and remove all the overloads using IImageFormat we would end up with a very simple and intuitive API.

[stefannikolei]

So this means you want to strip the api down to these Load methods (Image.FromStream):

public static Image Load(Stream stream);
public static Image Load(DecoderOptions options, Stream stream);
public static Task<Image> LoadAsync(Stream stream, CancellationToken cancellationToken = default);
public static async Task<Image> LoadAsync(DecoderOptions options, Stream stream, CancellationToken cancellationToken = default);
public static Image<TPixel> Load<TPixel>(Stream stream);
public static Image<TPixel> Load<TPixel>(DecoderOptions options, Stream stream);
public static Task<Image<TPixel>> LoadAsync<TPixel>(Stream stream, CancellationToken cancellationToken = default);
public static async Task<Image<TPixel>> LoadAsync<TPixel>(DecoderOptions options, Stream stream, CancellationToken cancellationToken = default);

This would be the structure of Image.FromStream

and then convert them to the Try pattern. Or do you want to strip them further? Is this the way to go?

[JimBobSquarePants]

Yes. That looks right. I think I best have a look at this though. I want to ensure we handle default encoding behavior.

[stefannikolei]

How far do you wanna push this for the v3 milestone? I can probably invest some more time into it.

[JimBobSquarePants]

Thanks! Public APIs definitely. I'll need to have a look once #2320 is merged.

[stefannikolei]

Most of 'nullable disable' is left in the encoders and decoders.

There I see two options.

1. We stop creating a variable for the stream and start passing it as a parameter to all methods
2. We mark all usages of stream with a '!'

The first option needs obviously more effort

@JimBobSquarePants How would you tackle it, are you ok with many '!' in the codebase?

[JimBobSquarePants]

@stefannikolei You're not gonna like this but option 1 is how I would tackle it. too many ! make the code less readable.

[JimBobSquarePants]

Closing this as @stefannikolei has worked wonders and there's very few issues left.