Add nullable reference types support (Sentry, Sentry.Protocol) #509
Conversation
Codecov Report
@@ Coverage Diff @@
## main #509 +/- ##
==========================================
- Coverage 86.82% 86.53% -0.29%
==========================================
Files 120 120
Lines 2998 3001 +3
Branches 685 687 +2
==========================================
- Hits 2603 2597 -6
Misses 214 214
- Partials 181 190 +9
Continue to review full report at Codecov.
|
Tyrrrz
changed the title
There was a problem hiding this comment.
I'm using the LOGAF scale here: https://blog.danlew.net/2020/04/15/the-logaf-scale/
| /// <inheritdoc /> | ||
| public SentryId CaptureEvent(SentryEvent @event, Scope scope = null) | ||
| public SentryId CaptureEvent(SentryEvent? @event, Scope? scope = null) |
There was a problem hiding this comment.
Event shouldn't really be nullable (even though the code checks for it below to avoid NPE).
I'll address it in a follow up PR
| { | ||
| if (_options.SampleRate is float sample) | ||
| if (_options.SampleRate != null) |
There was a problem hiding this comment.
Any specific reason to change this? Not sure why change from the pattern matching to null check and a call on the property again.
One of the reasons I prefer the pattern matching is that the value is available on the reference declared, as opposed to a second access to property, that could possibly have change in between the two reads if mutable.
I realize the amount of bikesheeding that C# has created by having so many ways to do the same thing
Work in progress
Some notes:
There are two sides to nullable reference types. One is the metadata we provide to users as part of the published library, which helps them correct nullability issues in their integration with Sentry, the other is for static analysis within Sentry itself. While the former can be achieved on all .NET frameworks with backporting (using NuGet package Nullable), the latter can only be done with .NET Core 3.0+ or .NET Std 2.1+. The reason for that is, because earlier versions of the framework are not annotated with NRT attributes, enabling warnings on them will result in huge amount of false positives. So as a solution, the
<Nullable>project property is set toenableonly when targetingnetstandard2.1, while on other targets it's set toannotations(which allows us to annotate, but doesn't produce warnings for our code). Sincenetstandard2.1target wasn't there, I added it. This is the framework we should be using when developing in the IDE, so that we get proper nullability warnings/insights. As long as that target is also built on CI, we will get warnings there as well. The added bonus of this approach is that warnings are not duplicated per target either (otherwise we'd have one nullability warning per framework).Note that NRTs works at compile time and not runtime and since it's not actually part of the type system but rather a mere annotation, it doesn't protect against
NullReferenceExceptions. That said, I would suggest not to check for null and rely on static analysis to tell users when they're wrong. Thanks to backporting, NRTs should be available for users targeting older versions of the framework too. Ideally, we should also either hide nullable parameters behind overloads or default them to null so it's a bit more clear.Some methods were easy enough to analyze in terms of nullability. Some were more difficult. Because there methods that rely a lot on mutability, it was hard to determine whether the value can be null or not at a specific point. In most cases I just marked such instances as nullable. It revealed many places where nulls were not handled. I added some TODOs for these.
I've noticed some types are designed as data contracts with public get/set properties. I see that some of these properties are meant to be required, but since it cannot be enforced at a type system level without a constructor (until C# 9 and records), I marked them as nullable. That's because you simply can't know whether they've been set to a valid value or not. Code that interacts with those types needs to be aware of that too.
There were some instances of defensive programming in the code base that checked method parameters for null even when they weren't really expected to be null. I marked those instances as nullable to be safe, but we should ideally avoid defensive programming and let type system and static analysis take care of such situations for us. In the future we should consider refactoring this.
I found that some method overloads can be simplified by using default parameter value of null where null was expected anyway. Unfortunately, removing overloads is a breaking change so I kept them and added a corresponding TODO to remember for the future.
As I was annotating stuff, I also cleaned up some XML docs by adding a period in the end where applicable (accessibility) and removing empty nodes (e.g.
<returns></returns>).With the advent of .NET 5 we should probably consider dropping runtime-specific target frameworks (i.e.
net461etc) and only keep .NET Standard.Closes #229
Related to #503
Related to #504