`CreateCSharpType` is returning nullable result

[ArcturusZhang]

method CreateCSharpType is returning nullable result, but does it make sense? Searching all the usage of this method within our repo, we are actually ignoring the nullability of this result. One InputType must be converted to a valid CSharpType And actually, in our current implementation of this method (here), there is no terminal branch that returns a null value.

[ArcturusZhang]

So I saw the CreateModel and CreateEnum is returning nullable results. is the purpose of returning nullable because we want to enable our plugin writer to "remove" a model/enum in this way by returning a null? This might be problematic because we will have to handle null values properly in a lot of places - and in fact, we are just ignoring them. I think for models and enums, if one wants to return null here, it should mean they do not want to generate this type. But returning null is not solving this - you always need a type for this model in other places, for example what if this model is used as a property type in another model? When the generator wants to generate this property, it has to have a type for it.

So I think, maybe this should be: We return non-nullable values for CreateModel and CreateEnum, if they want to return something else, they could either (choose one):

1. override the CreateCSharpTypeCore and escape this case there, so that when it gets inside CreateModelCore, it always returning some ModelProvider
2. override the CreateModelCore, and creates their own ModelProvider which returns typeof(object) or something they want in the Type property (because we always need a type) None of these solutions require these return types to be nullable.

[JoshLove-msft]

I think for models and enums, if one wants to return null here, it should mean they do not want to generate this type. But returning null is not solving this - you always need a type for this model in other places, for example what if this model is used as a property type in another model? When the generator wants to generate this property, it has to have a type for it.

The purpose of it being nullable is to allow plugin authors to delete a type. This means they would need to handle deleting/updating any references to that type such as properties or method signatures that use it.

[ArcturusZhang]

I think for models and enums, if one wants to return null here, it should mean they do not want to generate this type. But returning null is not solving this - you always need a type for this model in other places, for example what if this model is used as a property type in another model? When the generator wants to generate this property, it has to have a type for it.

The purpose of it being nullable is to allow plugin authors to delete a type. This means they would need to handle deleting/updating any references to that type such as properties or method signatures that use it.

OK - does this make us to have to do a lot of extra null checks? We are using this method in quite a few places - and in most of the cases I have found, we are just doing this: TypeFactory.CreateCSharpType(input)! So now it looks like, if someone have a plugin, and override this method to return null, they would get infinite issues.

My opinion here is that, they do not want to write out a model, then they will have to replace this type of model with something else (for example object), but this null return value only solves the first problem, how about the second?

I believe this could also solve their issue:

public class ObjectTypeProvider : ModelProvider
{
    protected override CSharpType BuildType => typeof(object);
}

public class MyTypeFactory : TypeFactory
{
    protected override ModelProvider CreateModelCore(InputModelType model)
    {
        return new ObjectTypeProvider();
    }
}

public class MyOutputLibrary: OutputLibrary
{
    // override the method to get the list of types to write, filter out all the `ObjectTypeProvider` instances.
}

[JoshLove-msft]

OK - does this make us to have to do a lot of extra null checks? We are using this method in quite a few places - and in most of the cases I have found, we are just doing this: TypeFactory.CreateCSharpType(input)!

Yes, we should be doing extra null checks. I see there are a couple places where we are not doing this in ModelProvider so we should fix those instances.

[JoshLove-msft]

I think if a model is used somewhere by a client, and the customer tries to remove this model, we need to think about what they would do with the operation. If they want to remove the entire operation, they could add a MethodProvider visitor and remove the method. The default pruning should then handle deleting the model. If they wanted to keep the method but just remove an optional parameter, they could also do this with a MethodProvider visitor. I think in either case adding visitors to remove references should result in the model being unreferenced and therefore pruned, so perhaps this is a viable approach to deleting types as opposed to the current solution.

[ArcturusZhang]

I think if a model is used somewhere by a client, and the customer tries to remove this model, we need to think about what they would do with the operation. If they want to remove the entire operation, they could add a MethodProvider visitor and remove the method. The default pruning should then handle deleting the model. If they wanted to keep the method but just remove an optional parameter, they could also do this with a MethodProvider visitor. I think in either case adding visitors to remove references should result in the model being unreferenced and therefore pruned, so perhaps this is a viable approach to deleting types as opposed to the current solution.

in the case you described, what if: they only write visitors to remove the usage of those parameters/methods using the model that they do not want to see, and our post processor should automatically prune all those unused models. This also does not need the nullable return type on CreateCSharpType.