Discussion: OPC-UA structures are not optional

[quinmars]

Recently, I'm playing around with C#'s nullable references. And it is a little bit annoying because there are many references that the compiler identifies as possible nullable, but in reallity are never null. I first thought it is some kind of weakness of the C# type system, but if you look at a different angle on it, the nature of OPC UA structure could help us. OPC UA structures are not optional, i.e., they cannot be null, similar to C# structures. We cannot use C# structures because of the lack of inheritance support. But we can make the instances non-optional. Let's take an look on an example:

 public class HistoryReadRequest : IServiceRequest
 {
        public RequestHeader? RequestHeader { get; set; }
        public HistoryReadDetails? HistoryReadDetails { get; set; }
        // ... 
        public virtual void Decode(IDecoder decoder)
        {
            decoder.PushNamespace("http://opcfoundation.org/UA/2008/02/Types.xsd");
            RequestHeader = decoder.ReadEncodable<RequestHeader>("RequestHeader");
            HistoryReadDetails = decoder.ReadExtensionObject<HistoryReadDetails>("HistoryReadDetails");
            // ...
            decoder.PopNamespace();
        }

While we always know it is non-null when read from stream (I know a request is not read from stream...); after a casual creation with new it is null.

We could change that! So that all structures are preinitialized before (note: I'm not talking about ExtensionObject). Like:

 public class HistoryReadRequest : IServiceRequest
 {
        public RequestHeader RequestHeader { get;  } = new RequestHeader();
        public HistoryReadDetails? HistoryReadDetails { get; set; }
        // ... 
        public virtual void Decode(IDecoder decoder)
        {
            decoder.PushNamespace("http://opcfoundation.org/UA/2008/02/Types.xsd");
            RequestHeader.Decode(decoder);
            HistoryReadDetails = decoder.ReadExtensionObject<HistoryReadDetails>("HistoryReadDetails");
            // ...
            decoder.PopNamespace();
        }

ExtensionObjects and arrays, which both could be null, would stay as they are. But ordenary structures would become non-nullable! The above examples probably does not work for JSON or XML encoding. Maybe we'd need to add a second method for encoding like Decode(IDecoder decoder, string fieldname).

Anyway, this is just to start a discussion, because a change would cause a code break for nearly everyone, we should only do this after a careful consideration.

[awcullen]

What do you find most annoying about the nullable properties?

When sending an IServiceRequest, the library looks for null RequestHeader and fills in a default one. Are there other fields that could be auto-filled?

see https://github.com/convertersystems/opc-ua-client/blob/d737a76eb119cea536405d11b72c1952eb142967/UaClient/ServiceModel/Ua/Channels/UaTcpSecureChannel.cs#L1671

[quinmars]

Most annoying is that the type system does not reflect some guaranties we have with OPC UA. I know, giving a "request" as an example, was not optimal. I just looked for an example where a Structure and an ExtensionObject is used at the same time. A more compelling example would be a "response". For example here:

https://github.com/convertersystems/opc-ua-client/blob/d737a76eb119cea536405d11b72c1952eb142967/UaClient/ServiceModel/Ua/Channels/UaTcpSecureChannel.cs#L1328

we have to suppress an nullable warning. We know that a after recieving a structure, the structure cannot be null. Nonetheless we have to perform a check or suppress a compiler warning. The real problem is that:

var response = new HistoryReadResponse();
var header = response.ResponseHeader.Timestamp; // ouch, ResponseHeader is null

So if we want to reduce the warnings on accessing structures, we have to make the C# types to be non-null after construction. One way would be my example above. We could make my proposable more backward compatible by still allowing setting the property, i.e.,

 public class HistoryReadRequest : IServiceRequest
 {
        public RequestHeader RequestHeader { get;  set; } = new RequestHeader();
        public HistoryReadDetails? HistoryReadDetails { get; set; }
        // ... 

Then we do not have to mark it by a question mark and use it as a non-nullable.

[awcullen]

Some users may set the TimeoutHint in the SecureChannel. These settings are used when the RequestHeader is null.

        /// <summary>
        /// Gets the default number of milliseconds that may elapse before an operation is cancelled by the service.
        /// </summary>
        public uint TimeoutHint { get; }

        /// <summary>
        /// Gets the default diagnostics flags to be requested by the service.
        /// </summary>
        public uint DiagnosticsHint { get; }

Should we get rid of these settings above, and use constants in the RequestHeader constructor?

        public RequestHeader()
        {
            TimeoutHint = 15000;
            ReturnDiagnostics = 0;
        }

[quinmars]

I'm not sure if we are talking about the same things. What I'm talking about are the nullability warning that you are getting with latest version of C#/dotnet. It don't think that UaClient library is doing something wrong. In fact it's doing right, but the compile cannot and does not recognize this. Let me give you the following example:

sharplab

Both examples RequestA and RequestB do work, but RequestA generates a compile time warning, because it would be technically possible to return a value, that contains a null reference.