Fix some "nullable reference type" warnings#26
Open
rmunn wants to merge 6 commits into
Open
Conversation
This only works in .NET 5.0 or later, though, so for net461 we'll need a different approach.
This reverts commit 758aaaf.
The pattern where the constructor calls an Initialize() method is one that the C# compiler can't analyze to detect that nullable reference types are actually assigned a real value. So we move most of the code into the constructor (and the static fields are initializes in a static constructor) so that the compiler will stop warning us about them. This still leaves a few compiler warnings that are real, such as the one about _realDictionaryPath, which could indeed be accessed before it is initialized. That one, and a few others, should truly be typed nullable.
Not all nullable warnings will be able to be removed as long as this code still targets net461; for example, string.IsNullOrEmpty guarantees that the string isn't null, but the annotations required for the compiler to know that weren't added until the .NET Core era. So until the projects targets a modern version of .NET, the only way to get rid of nullable references like these is to use the ! operator every time, which will quickly get tedious.
Contributor
|
After some discussion in the office, we will hold off on merging this until we can understand more about the effort it will take to upgrade to .Net 8 windows forms. Upgrading will eliminate some compiler warnings and reveal the ones that are actually a problem. |
Contributor
|
@rmunn would you say that this work shouldn't be merged, and we should abandon this PR in favor up a full upgrade to .Net 8? If so, I'm happy to close the PR without merging. Thanks for your investigation of this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes some of the compiler warnings about nullable reference types that you get when you build this with a modern C# compiler. It doesn't fix nearly all of them, because I realized that a lot of these can't be easily fixed as long as the project is targeting
net461. For example, this section of code produces a compiler warning on line 276 thatswitchToCitationFormmight be null:solid/src/SolidGui/QuickFixer.cs
Lines 269 to 276 in 96dc332
It's immediately obvious that
switchToCitationFormcannot be null on line 276, but the compiler doesn't know that as long as the target framework isnet461. That's because the annotations that tell the compiler "If this function returns true/false, then its parameter can/cannot be null" were added to library code sometime during the .NET Core / .NET 5 era, and the net461 framework libraries don't have those annotations. So the compiler can't infer anything about the nullness of the parameter to string.IsNullOrEmpty, and therefore it gives a warning. You could suppress it by using the!operator, as I did in this particular case, but doing that everywhere is tedious. And ultimately pointless, when switching to a more modern .NET target will end up fixing the issue anyway.Therefore, I suggest postponing the rest of this work until after the code is switched to target a more modern version of .NET, say 6 or 8. At that point many of the compiler warnings will go away, and the ones that are left will be ones actually worth fixing.