Repository navigation
Enable Nullable Reference Types #2231
Description
Activity
In my eyes NullableContextOptions is not needed anymore. Have a look at dotnet/sdk#3256
Rather set
<WarningsAsErrors>Nullable</WarningsAsErrors> <Nullable>enable</Nullable>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
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.
- Enable nullability checks globally (single PR)
updatecsprojs 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. - 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. - 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.
Reacted by James Jackson-South- Enable nullability checks globally (single PR)
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?
Will have a look asap.
One area I particularly want to have a look at is the
Image.LoadAPIs 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
IImageFormatwe would end up with a very simple and intuitive API.Reacted by Stefan NikoleiSo 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?
Yes. That looks right. I think I best have a look at this though. I want to ensure we handle default encoding behavior.
Reacted by Stefan NikoleiHow far do you wanna push this for the v3 milestone? I can probably invest some more time into it.
Thanks! Public APIs definitely. I'll need to have a look once #2320 is merged.
Most of 'nullable disable' is left in the encoders and decoders.
There I see two options.
- We stop creating a variable for the stream and start passing it as a parameter to all methods
- 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?
@stefannikolei You're not gonna like this but option 1 is how I would tackle it. too many
!make the code less readable.Reacted by Stefan NikoleiReacted by Stefan Nikolei- added a commit that references this issue
on Feb 24, 2023 Closing this as @stefannikolei has worked wonders and there's very few issues left.
Reacted by Stefan Nikolei
Prerequisites
DEBUGandRELEASEmodeImageSharp 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.
#nullable disableEnabling analysis requires adding the following to individual
.csprojfilesWe should consider doing this for V3.
Steps to Reproduce
NA
Images
No response