Start a new topic
Planned

Enforce Non-nullability on rules and bag methods returning reference types

In the migFx Studio, for a manual method, the nullability of the return value is explicitly specified in the Null checkbox. In the below screenshot from the Workshop target Map of our training exercises it is explicitly stated, that this method cannot return null.



As a side note, nullability is also relevant for Lookup methods. Only here the nullability is automatically determined by migFx from a number of factors. 



c# rationale

To clearly explain the core of this proposal it is necessary to dive a bit into c# - and Value types versus Reference types


When the method return type is translated by the migFx Engine generator into c#, most migFx data types map to c# Value types - but 2 migFx data types map to c# Reference types:


migFx
c#
c# (nullable)
Value type
Reference type
Boolean
bool
bool?


Char
string
string


Date
DateTime
DateTime?


Float
double
double?


Num
decimal
decimal?


Time
TimeSpan
TimeSpan?


Timestamp
DateTime
DateTime?


Binary
byte[]
byte[]



While there are several differences in c# between Value and Reference types, the one difference important in this context is the fact that 


  • Reference types can contain the value null
  • Value types cannot contain the value null


To enable nullability on Value types, the migFx Engine Generator translates a nullable migFx data type into the corresponding nullable type in c# by adding the nullable annotation ? after the c# type. 


But as the Char and Binary migFx data types both translates into c# Reference types (string and byte[] respectively) this nullable annotation is not available for these 2 data types.


Returning Value types

The end result is that the intent expressed in Studio in regards to nullability is clearly expressed in the method signature generated for methods returning c# Value types, here illustrated by a method returning the migFx Num data type


// Returning nullable migFx Num
public override decimal? SomeMethod(FlagHandler flag)
{
    if (some condition)
    {
        // This is of course fine
        return 0; 
    }
    else
    {
        // This is fine too
        return null; 
    }
}

// Returning non-nullable migFx Num
public override decimal SomeMethod(FlagHandler flag)
{
    if (some condition)
    {
        // This is still fine
        return 0; 
    }
    else
    {
        // Attempting to return null will not even compile
        return null; 
    }
}


So, in this case, the intent expressed in the migFx Studio when defining the manual method is clearly visible and enforced by the c# compiler when the method is implemented in c#.


Returning Reference types

But when the method returns a migFx data type that translates into a reference type, the method signature is identical whether nullability was specified in Studio or not, here illustrated with a method returning the migFx Char data type:


// Returning nullable and non-nullable migFx Char 
// results in identical signature
public override string SomeMethod(FlagHandler flag)
{
    if (some condition)
    {
        return "some string value"; // This is fine
    }
    else
    {
        return null; // This is fine too
    }
}


In this case, when implementing the method in c#, there is no indication whether null is an acceptable return value or not. As a result, in practice it is unavoidable that some manual method implementations will return null even if this runs against the intent expressed in the migFx Studio.


The rest of this proposal will only deal with methods returning non-nullable Reference types (migFx Char or Binary, c# string and byte[] respectively). 



Present situation

At present, the migFx Engine Generator ensures that any field (Target Interface Field in the Source Engine or Target Field in the Target Engine) will never receive a null value for such a method by including a coalesce operator (??) in the code assigning the return value of a rule/bag method to such a field. Below illustrated by 2 code snippets from the generated Workshop Target Engine:


// Coalescing the return value of a method returning 
// non-nullable Char to string.Empty
SomeMethod() ?? string.Empty /* Coalesce because method does not allow return of null */

// Coalescing the return value of a method returning 
// non-nullable Binary to an empty byte array
SomeMethod() ?? new byte[0] /* Coalesce because method does not allow return of null */


Code snippets like these can be located in a generated Target Engine ItemImporter or Source Engine ItemExporter by searching for the coalesce operator ??.


Limitations

  • As stated above, when implementing the method in c#, the developer is left in the dark whether the method is allowed to return null or not. It is not realistic to expect the developer in every instance to remember to go back to the Studio to verify the intent in regards to nullability of the return value of the method.

  • The fact that the return value of the method is coalesced in the generated code after the method call can lead to confusion and wasted effort when tracing issues in the migration.

    The obvious trap is looking at the method implementation and clearly verifying the method returns null a given situation while for instance an empty string has been assigned to the field in the xml data flowing through the migration.  



Proposal

While it is not feasible to duplicate the behavior of methods returning non-nullable Value types where any mistake in the method implementation is caught early and safely by the c# compiler, we can get significantly closer to the mark:

  • Ensure clear difference between methods returning Reference types, reflecting whether the method is allowed to return null or not. This will assist the developer and avoid inconsistency between the method implementation and the intent expressed in Studio

  • Instead of just coalescing the return value to string.Empty/byte[0] 'behind the back' of Studio intent and method implementation, we can at runtime validate that the method in fact does return a non-null value and generate a System Event if this is not the case    


Simply exchanging the coalesce in the generated code to throw an exception if the method returns null will of course address the second point above - but it would still leave the developer in the dark when implementing the method. 


In addition, the stack trace of this kind of exception would merely show the point in the generated code where the method is called. It would be far more helpful to show the point in the method implementation where return of a a null value was attempted.


A new type to return

A better solution - and indeed the solution proposed - is to alter the generated signature of the of the method, so it no longer returns string (or byte[]) but rather a new custom type, that helps guard against a null return value.


The new method signature would look like this (illustrated by a method that returns a non-nullable migFx Char):


// Returning a non-nullable migFx Char
public override NonNullableString SomeMethod(FlagHandler flag)
{
    return new NonNullableString("The return value");
}


One advantage already apparent is that this new method signature by - specifying the return type to be NonNullableString - clearly tells the implementing developer that this method is not supposed to return null.


NonNullableString and NonNullableBinary

This proposal will introduce 2 new custom types (c# classes) to be returned from methods returning non-nullable Reference types.

  • NonNullableString for methods returning an non-nullable migFx Char
  • NonNullableBinary for methods returning a non-nullable migFx Binary


The two classes will be identical in behavior. For the ease of explanation, the following will use the NonNullableString class to explain the concepts.


The class will hold the inferred string value in a property, and the method implementation can get or set this Value property freely. The constructor of the class takes the inferred string as an arguments and internally assigns it to the Value property. And - most importantly - the setter for the Value property creates an error, if the method tries to assign null:


public class NonNullableString
{
    #region All fields

    private string _value;

    #endregion
    #region All constructors

    public class NonNullableString(string value)
    {
        Value = value;
    }

    #endregion
    #region Public and protected properties

    public string Value
    {
        get { return _value; }
        set 
        {
            if (null == value) 
            {
                throw new ArgumentNullException("Assigning null to the Value property");
            }

            _value = value;
        }
    }

    #endregion
}


In this way, an attempt to assign a null value to the Value property of the NonNullableString class will create an error, regardless whether this is done by setting the Value property to null directly or by calling the constructor with a null string.


Similarly to other errors (exceptions) created when calling a rule/bag method, this error will be caught by generated engine code and converted into a System Event. 


The implementation of NonNullableBinary would look similar (and of course, at the end of the day, the internal implementation in migFx will probably lift common functionality to a generic base class NonNullable<T>).


Continuing on the method sample above, method implementations would look like this:


// Returning a non-nullable migFx Char
public override NonNullableString SomeMethod(FlagHandler flag)
{
    // The NonNullableString instance can be stored locally
    var returnValue = new NonNullableString("The return value");

    // The Value property can be accessed
    if (string.Equals("something", returnValue.Value)
    {
        // Assigning null to the Value property will throw an exception
        returnValue.Value = null; 
    }

    // Likewise, calling the constructor with a null value will cause an error
    returnValue = new NonNullableString(null); 

    return returnValue;
}


Cast operators

By adding an implicit cast operator to the definition of the NonNullableString class, c# can be instructed to automatically convert (cast) a string value to a NonNullableString instance. 


With the implicit cast in place, the sample method can be implemented by simply returning a string value - it is no longer necessary explicitly to return a NonNullableString - the implicit cast takes care of this. And when casting to the NonNullableString, the check for null will still be in place and effective.


With the implicit cast in place, this would be valid code in the sample method:


// Returning a non-nullable migFx Char by invoking the implicit cast
public override NonNullableString SomeMethod(FlagHandler flag)
{
    // If no row is found or the StringColumn on the row contains null 
    // the variable stringColumn may (or may not) contain null
    var stringColumn = Valueset.SomeValueset
        .Where(r => r.Column1 == "test")
        .Select(r => new { r.StringColumn })
        .FirstOrDefault();

    // The implicit cast implemented in NonNullableString will 
    // enforce the null check for you
    return stringColumn;
}


The one situation that cannot be fully handled

Since the NonNullableString is itself a Reference type, it is still possible to simply return a raw null value from the method:


// Avoiding the null check by returning null
public override NonNullableString SomeMethod(FlagHandler flag)
{
    // Situation 1: Will not be caught by the implicit cast
    return null; 

    // Situation 2: Will be caught by the implicit cast
    return null as string; 
}


Returning a raw null value as in the situation 1 in the sample above is not caught by the implicit cast, since the cast operator does not recognize the raw null value as a string type. In comparison, in situation 2 above the null value is cast to a string type by the as operator before return. This situation will be caught.


Unfortunately there is no suitable mechanism in c# to catch and handle such a return of the raw null value from a method. As part of this proposal, the Engine Generator will be modified, so the generated code will check for a raw null return value from a non-nullable method and create an error. 


However, it is not quite as good: The stack trace of this error will point to the generated code where the method is called, not to the point in the method itself, where a raw null value is returned.    


Implicit cast implentation

For completeness and interest, below the implementation of the implicit cast operator in the NonNullableString class. The cast implementation instructs c# what to do in order to cast (convert) a string value to a NonNullableString instance.


#region Implicit casts

public static implicit operator NonNullableString(string value)
{
    return new NonNullableString(value);
}

#endregion



Impact and refactor

No refactor of Studio mapping is required.


In Visual Studio, manual refactor will be required for manual rules/bag methods that in Studio have been specified to return non-nullable Char or Binary. The return type of the generated virtual method signature for these will change to the new NonNullable types. In the corresponding rule/bag implementation, the overriding method signature must be changed to return the correct NonNullable types. After generation, the c# compiler will flag all these.

  

It is not necessary to refactor the method body of the manual implementations. The implicit cast operators described above will ensure that the existing body still compiles and that the return of null string or null byte[] is caught.


It will be possible by asking Visual Studio to find all references to for instance NonNullableString, to go through these manual implementations and look for returns of the raw null value (return null;). While this is not required, getting rid of these will result in a better stack trace on System Errors generated due to return of raw null values from NonNullable rules/bag methods.



1 person likes this idea
Login or Signup to post a comment