Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better NullReferenceException for non-null properties #6

Closed
Kimi-Arthur opened this issue Jun 13, 2022 · 6 comments
Closed

Better NullReferenceException for non-null properties #6

Kimi-Arthur opened this issue Jun 13, 2022 · 6 comments

Comments

@Kimi-Arthur
Copy link
Owner

Potentially a solution for dotnet/runtime#3858, which may only work for non-null properties, based on source generators.

It's still quite useful, given that most of the NullReferenceExceptions come from the following sources (with the exception of class's fields)

  1. Local variable is unexpectedly null
  2. Return value of a method is unexpectedly null
  3. Property retrieved (not called) is unexpectedly null

While new unexpected nulls come from 3 if all obvious warnings are fixed (e.g. returning nullable value in a method). While you can always initialize a non-null property to a value, it is not always reasonable to do so (e.g. inializing a string property to "" while it actually has to be specified by client code).
One suggestion from Microsoft is to initialize the property to null!. However, in this case, checker and compiler won't do anything, and runtime won't do anything either, even when the null value is obtained. Instead it only produces useless NullReferenceException: Object reference not set to an instance of an object error when its members are used.

The option is to declare the property's getter as => _backField ?? throw new InvalidOperationException("Uninitialized property: " + nameof(ShippingAddress)), which is much nicer, but needs quite some boilerplate than the option above.

This issue will try to use Source Generator to provide a clean solution for any property.

For any non-null property,

public string Id { get; set; }

It will produce the property as

private string? _id;

public string Id {
  set {
    if (value == null) {
        throw new NullReferenceException("Cannot assign null value to non-null property Id of class xxx");
    }
    _id = value;
  }

  get => _id ?? throw new NullReferenceException("Property Id of class xxx is expected to be non-null, but is actually null.");
}

This error message is already much more helpful than nothing (the current state). Normally there won't two accesses of the same property of different objects in the same line. Two caveats exist though,

  1. It is not actually a NullReferenceException, but rather a precaution beforehand (maybe InvalidOperationException is more reasonable?). The code may not fail for NullReferenceException if without the added check. However, when the exception is thrown here, it's definitely a codesmell and should be fixed.
  2. It cannot pinpoint and notify which variable is null in the first place (like Java 14's solution).

So with this feature, it works something like lateinit in Kotlin or late in Dart.

Potential improvements include

  1. Make the property required when defining the class
@Kimi-Arthur
Copy link
Owner Author

Shit. If without this or this, this can't be implemented nicely with SourceGenerator.

@Kimi-Arthur
Copy link
Owner Author

So here is a slight better solution without needing the Source Generator magic:

public static class Safe {
    public static T Get<T>(T? value) {
        if (value != null) {
            return value;
        }

        var method = new StackFrame(1).GetMethod();
        if (method == null) {  // This if may be skipped IMO
            throw new NullReferenceException(
                "Unexpectedly failed to find info of getting a null value.");
        }

        throw new NullReferenceException(
            $"Property {method.Name[4..]} of class {method.DeclaringType} is expected to be non-null, but is actually null.");
    }

    public static void Set<T>(ref T field, T? value) {
        if (value == null) {
            var method = new StackFrame(1).GetMethod();
            if (method == null) {  // This if may be skipped IMO
                throw new NullReferenceException(
                    "Unexpectedly failed to find info of setting a null value.");
            }

            throw new NullReferenceException(
                $"Cannot assign null value to non-null property {method.Name[4..]} of class {method.DeclaringType}.");
        }

        field = value;
    }
}

And the usage looks like:

private string? _id;

public string Id {
    get => Safe.Get(_id);
    set => Safe.Set(ref _id, value);
}

It's also possible to implement the Set method to return instead of ref.

@Kimi-Arthur
Copy link
Owner Author

For struct types like int to work. You may need to add

    public static T Get<T>(T? value) where T : struct {
        if (value != null) {
            return value.Value;
        }

        var method = new StackFrame(1).GetMethod();
        if (method == null) {
            throw new NullReferenceException(
                "Unexpectedly failed to find info of getting a null value.");
        }

        throw new NullReferenceException(
            $"Property {method.Name[4..]} of class {method.DeclaringType} is expected to be non-null, but is actually null.");
    }

where the where T : struct is the key.

@Kimi-Arthur
Copy link
Owner Author

Performance wise, only failure handling is impacted by reflection. So it should be OK. Especially errors like this should be fixed instead of tolerated, so most likely it should only happen once in each run and should be in common in developing time.

Kimi-Arthur added a commit that referenced this issue Jun 18, 2022
@Kimi-Arthur
Copy link
Owner Author

As suggested by this answer, [CallerMemberName] will help a lot, but there is no [CallerTypeName] is not available (see here).

Since I don't feel file path is a reasonable solution, we will stick to StackFrame approach for now.

@Kimi-Arthur
Copy link
Owner Author

This change conflicts with Json.net. Fixed in 9ade493

In general, I think this is solved.

Kimi-Arthur added a commit that referenced this issue Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant