Use nullable reference types throughout CSLA codebase

Is your feature request related to a problem? Please describe. C#8 adds nullable reference types. Not only will CSLA need to support this, but it should also annotate its members with not just ?, but with the new nullable attributes.

Describe the solution you'd like NRT support throughout CSLA.

Describe alternatives you've considered Nothing :)

Additional context Use this reference for details/information.

@rockfordlhotka I'd love to tackle this issue. I don't like null :-) Btw the nullable annotation context is not a breaking change. The annotation is only a compiler feature during development and not something the runtime checks.

I'd love to have you do this!

It'll be CSLA 9 thing, as I'm sure there'll be breaking changes.

@rockfordlhotka I'll link you here from now on for questions regarding the nullable context :-) First question: https://github.com/MarimerLLC/csla/blob/579c84dd14398549c1b48be3892d543a1a956260/Source/Csla/Core/FieldManager/FieldData.cs#L28-L42

Is the empty ctor only there for the MobileFormatter? It's at least not used within CSLA itself. I'd love to mark it Obsolete(true) because it would remove a lot of "maybe nullable" things in the class itself. For example the name can than be guaranteed.

MobileFormatter does require an empty default ctor. Whether that is explicit or implicit, it is required for all types that implement IMobileObject.

In that case I'll mark it as Obsolete. MobileFormatter can still use it but it's not usable any more at compile time :-)

And should I really add now everywhere a null argument check where it's currently just assumed to be not null? 😅🤔

That seems wise doesn't it?

I concur with @rockfordlhotka. If a parameter is a NRT, someone could still pass in null, so an ArgumentNullException.ThrowIfNull() call is warranted.

Hey - this is now part of the permanent record!! @JasonBock agrees with me!! 😁

@rockfordlhotka don't make me do it....

@rockfordlhotka How should we handle null within the framework to e.g. reset/unset the ClientContext? How do we handle things like this: https://github.com/MarimerLLC/csla/blob/0f469350a0d72bf35d35deebb6660c6231c50b5d/Source/Csla/ApplicationContext.cs#L143-L147 I personally don't like to set something null when I just can assign a new instance to avoid that. At the cost of memory consumption obviously. But - and I think that's the biggest downside on allwoing null in such cases - we have to mark ClientContext as nullable which means every code says "Hey! This could be null here. Please check that" eventhough we know that won't ever be the case. But that's not expressable by the current annotations as far as I am aware.

If a property can be null, it needs to be annotated as such. And yes, that means that every usage of that property needs to do a null check. If the context should never be null, it should not be annotated with ?, but, arguably, when you have a mutable property, you will need to do this regardless. The annotation is essentially a compilation helper, in that it can provide warnings to users to not do bad things with members that don't expect values to be null. But it doesn't stop it. You still have to check for null. And when the property is mutable, it means you always have to check it.

Consider this:

public sealed class Data
{
  public Data(string value)
  {
    ArgumentNullException.ThrowIfNull(value);
    this.Value = value;
  }

  public string Value { get; }
}

Since the property isn't settable after construction, there's no way it could be null. If it was, you would've received an exception during the creation of a Data instance. (Yes, you could do Reflection shenanigans to change that property value via the backing field, but I personally wouldn't be concerned with people digging themselves into holes this route). Therefore, I don't have to worry about checking Value all the time for a null value.

But if I do this:

public sealed class Data
{
  public Data(string value)
  {
    ArgumentNullException.ThrowIfNull(value);
    this.Value = value;
  }

  public string Value { get; set; }
}

Now I have to check for null every time I access Value. To keep the semantics of "Value will never be null", I would need to change the setter to do a null check, which would then ensure that Value wouldn't be null.

Relating this back to ClientContext, either you annotate it with ? because it's possible it could be null, or you don't, and then ensure that ClientContext can never be null. As you mentioned, this may mean you to provide an "empty" or "default" ClientContext instance. I don't know how that would work in CSLA. Or, since the current behavior is that it could be null, then put ? on it, and make it explicit for callers.

Of course if a 3rd party can directly modifiy a property we have to make that nullable or change it to a backing field with a guard clause. In the case of ClientContext we have full control over it due to get/set methods where we can deny null as an acceptable value. The question is should we do that or not? I personally would say: Yes, we should. It would not only make our code more robust and clean but our users one, too.

The current behavior states that null is a valid value, so it should be annotated with ?. However, I'm guessing this will be part of a breaking change release, so maybe the right thing to do is to not annotate it, and force that either a non-null reference is provided, or an exception will occur. We know what will happen if we leave it nullable, because that's the current state of CSLA. Making it non-nullable may be the "right" change. I'm guessing @rockfordlhotka should weigh in on this.

@JasonBock Yes. I'm asking because we are working on a breaking change version. Otherwise you are perfectly right. I would have annotatd it and keep the null behavior. Since I'm adding it and we are in a breaking change version I'd like to make it a bit more nice imho :-)

Thinking about it. The nullable feature would mean you can't use any auto properties because they don't adhere to the annotations. There are cases even in the asp.net core framework like HttpContext.Items which are not nullable but the framework does not prevent you from setting it null. So my stand will probably be: We use the feature so respect the feature. Otherwise garbage in -> garbage out :D

The data portal has optimizations to minimize data on the wire through the use of null.

So if someone never uses a collection like ClientContext, then only a null token is serialized instead of the empty collection, which is much larger because it includes the type info.

As a result, anyone using a remote data portal is helped anytime a complex type has a null value instead of an instance of the type.

When it comes to ClientContext (and LocalContext) the current implementation is to ensure that no one ever gets a null value, but behind the scenes the value might be null so we get the serialization optimization.

This means that the getter is what ensures that the value is never null. If the value is null, the setter creates it and returns the value.

That way, if someone doesn't use those context values (and this is a lot of users), then they pay no price for the feature existing in CSLA. But if someone does use the context dictionaries, then they are created on demand.

As a result, in my view, the public API shouldn't change - people shouldn't worry that the hidden/private value behind the scenes could be null. We can change the private implementation if desired - annotating the field with ?, and the getter keeps doing what it does today.

I don't see any good reason to break everyone's code with a public API change. Either they use the context dictionaries and the dictionaries get lazy created, or they don't use them and the private field behind the scenes remains null.

The data portal has optimizations to minimize data on the wire through the use of null.

So if someone never uses a collection like ClientContext, then only a null token is serialized instead of the empty collection, which is much larger because it includes the type info.

As a result, anyone using a remote data portal is helped anytime a complex type has a null value instead of an instance of the type.

That's resonable. For me the correct place for such optimization would be the serializer and not the class to be serialized. But that's out of scope and maybe we'll tackle it at a later date.

@rockfordlhotka Could you please take a look at Csla.DataPortalClient.DataPortalProxyDescriptor. This class is no where referenced. Neither by type or as a magic string. Otherwise we can remove it. And Csla.DataPortalClient.IApplicationContextProvider looks dead, too.

Please also take a look at Csla.DataPortalClient.LocalProxyExtensions.UseLocalProxy(config, options). That implementation looks broken to me 🤔

Please look at Csla.Reflect.DynamicMethodHandle ctor. The parameter parameters isn't used. Either that's a bug or it can be removed. For the latter I'll remove it :)

DynamicMethodHandle

It appears in use to me?

        if (parameters == null)
        {
          inParams = [null];

        }
        else
        {
          inParams = parameters;
        }

DynamicMethodHandle

It appears in use to me?

        if (parameters == null)
        {
          inParams = [null];

        }
        else
        {
          inParams = parameters;
        }

Yes. But inParams is not used. So when I remove inParams parameters is now unused, too.

I think we need to create an issue to actually use the parameters. They are passed in when the ctor is called, and presumably are supposed to be used to ensure we use the right overload of the method.

I'm now working on Csla.Rules.

@rockfordlhotka I need some input please :) We have these two rules: MinValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L371-L377 and MaxValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L459-L465

What would you expect to happen, when value == null in the line of comparison? That's a possibility there. We can decide it counts as not violating the rule or we make the rule restriction tighter. For example T must be a struct.

Another question: Any good reason why this lists can't just be initialized and hence empty if not neede and/or used? Makes the nullable stuff a lot easier and even more intuitive to use. Currently accessing DirtyProperties in a rule would return null instead of an empty list... https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L84-L98 https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L66-L82

@rockfordlhotka I need some input please :) We have these two rules: MinValue<T> where T: IComparable

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L371-L377

and MaxValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L459-L465

What would you expect to happen, when value == null in the line of comparison? That's a possibility there. We can decide it counts as not violating the rule or we make the rule restriction tighter. For example T must be a struct.

I think in this context we should consider null to be default(T) for the purpose of comparison.

Another question: Any good reason why this lists can't just be initialized and hence empty if not neede and/or used? Makes the nullable stuff a lot easier and even more intuitive to use. Currently accessing DirtyProperties in a rule would return null instead of an empty list...

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L84-L98

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L66-L82

CSLA often doesn't initialize lists of types that might be serialized, because it is much smaller to serialize null than it is to serialize an empty list. This is because the serializer doesn't need to flow the list's type information for null, but it does need to flow the type information for an empty list so the empty list can be deserialized on the other end of the process.