getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
566 stars 203 forks source link

`scope.User = null` shows warning of CS8625: Cannot convert null literal to non-nullable reference type. #3331

Closed Fwang36 closed 1 week ago

Fwang36 commented 2 weeks ago

Package

Sentry.AspNetCore

.NET Flavor

.NET

.NET Version

7.0.1

OS

macOS

SDK Version

4.4.0

Self-Hosted Sentry Version

No response

Steps to Reproduce

  1. Follow docs on how to clear the user here: https://docs.sentry.io/platforms/dotnet/guides/aspnetcore/enriching-events/identify-user/
  2. See warning on start up.

The user does seem to clear normally though, but the build will fail if you have nullable checks and enforce warnings as errors

Expected Result

No warning

Actual Result

Screenshot 2024-04-25 at 12 48 13 PM Screenshot 2024-04-25 at 12 48 49 PM
oliver-ec commented 2 weeks ago

Hi, this is something of interest to me.

We enforce nullables, and we treat those warnings as errors so it's detected at compile time.

 <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
 <Nullable>enable</Nullable>

The problem is, when you have this set, this code won't compile

SentrySdk.ConfigureScope(scope =>
{
    scope.User = null;
})

Because the decompiled code suggests the property 'User' is not nullable. The intent of being nullable is there since the private field is nullable, just not the property.

 private SentryUser? _user;

public SentryUser User
{
    get
    {
        SentryUser sentryUser = _user;
        if (sentryUser == null)
        {
            SentryUser obj = new SentryUser
            {
                PropertyChanged = UserChanged
            };
            SentryUser sentryUser2 = obj;
            _user = obj;
            sentryUser = sentryUser2;
        }

        return sentryUser;
    }
    set
    {
        if (_user != value)
        {
            _user = value;
            if (_user != null)
            {
                _user.PropertyChanged = UserChanged;
            }

            UserChanged(_user);
        }
    }
}

It would make my life happier if the property 'User' was nullable. I.e.

 private SentryUser? _user;

public SentryUser? User { get.. set... }

But I believe my suggestion is more syntactically correct since that property is intended to be nullable according to the documentation when trying to clear the user from scope.

https://docs.sentry.io/platforms/dotnet/enriching-events/identify-user/

bitsandfoxes commented 2 weeks ago

@bruno-garcia, do you have any context knowledge as to why that is? My understanding is that we (Sentry) always need a user in any case to be able to to show how many individual users (based on ID) are affected from any given error.

In any case, instead of setting it to null you can always overwrite it with a new SentryUser().

bruno-garcia commented 2 weeks ago

we just don't allow user to be null but you can set all its fields null. That is, if u did set some values and now want to undo that, just set those values to null.

So it's safe to do scope.User.Email = null and the same to any other field.

My understanding is that we (Sentry) always need a user

We don't require a user, but the API was done to avoid pitfalls like scope.User.Email = "my@email.com" then getting a NullReferenceException (remember this was done before nullability was a thing). So we lazily set an instance of SentryUser on each property accessor in case it's accessed.

As @bitsandfoxes said you can also do new SentryUser if you prefer, instead of setting props to null:

https://github.com/getsentry/sentry-dotnet/blob/8c78887bc9291b88938ac56d3dd59124668e5c18/src/Sentry/Scope.cs#L122

Fwang36 commented 2 weeks ago

Can we update the code snippet in clear user section below to a new example that would not show the warning?

Screenshot 2024-04-29 at 11 56 30 AM
jamescrosswell commented 2 weeks ago

Can we update the code snippet in clear user section below to a new example that would not show the warning?

@bitsandfoxes I added a PR to the docs repository:

bruno-garcia commented 1 week ago

Updated docs are live! thanks for reporting