Yes, our current setup is a bit skewed towards preferring upfront setup cost (like what you would want in a server or a client). With this new solution you would have to know the types in advance - is that something you can guarantee in your case?

Thanks for this!
We will make this possible in the SDK.
Does ReadOnlySpan<Type> make a difference here? We don't use that anywhere else in the SDK, but if this makes a difference we can use it here.

We should also document how to use this.

I don't think ReadOnlySpan<> really helps since you do this only once, only a minor reduction in allocation. Maybe just sticking with IEnumerable<T> should be enough.

To guarantee all the types are present, I think using source generation would be the best approach. I tried my hand creating a source generator for this and it seems to work well.

Also in the future, we can create a source generator that creates the ClassMapping and EnumMapping removing the need for reflection at runtime.

I've improved my source generator to generate the ClassMapping and EnumMapping and I get the results below:

The SourceGenMappings gives a very hefty performance improvement (171us vs 4208.6us)

Some issues I found in the current API is that ClassMapping and EnumMapping have private constructors and private property setters that make it difficult for any source generator to work with.

Please try out my branch to see if this is something the team is interested in integrating

@mmsmits

I've refined the new API that would be helpful for source generating the ModelInspector below:

Proposed new API

ModelInspector

public ModelInspector(string fhirVersion, IEnumerable<ClassMapping> classMappings, IEnumerable<EnumMapping> enumMappings)
{
    _classMappings = new ClassMappingCollection(classMappings);
    _enumMappings = new EnumMappingCollection(enumMappings);
    FhirVersion = fhirVersion;
    FhirRelease = FhirReleaseParser.Parse(fhirVersion);
}

public static ModelInspector ForTypes(string version, ReadOnlySpan<Type> types)
{
    var fhirRelease = FhirReleaseParser.Parse(version);
    var classMappings = new List<ClassMapping>(types.Length);
    var enumMappings = new List<EnumMapping>(types.Length);
    foreach (var type in types)
    {
        if (!type.IsEnum && ClassMapping.TryCreate(type, out var classMapping, fhirRelease))
        {
            classMappings.Add(classMapping);
        }
        else if (type.IsEnum && EnumMapping.TryCreate(type, out var enumMapping, fhirRelease))
        {
            enumMappings.Add(enumMapping);
        }
    }

    return new ModelInspector(version, classMappings, enumMappings);
}

ClassMapping

public static ClassMapping Build(
    string name,
    Type nativeType,
    FhirRelease release,
    bool isResource = false,
    bool isCodeOfT = false,
    bool isFhirPrimitive = false,
    bool isPrimitive = false,
    bool isBackboneType = false,
    string? definitionPath = null,
    bool isBindable = false,
    string? canonical = null,
    ValidationAttribute[]? validationAttributes = null,
    Func<ClassMapping, PropertyMapping[]>? propertyMapFactory = null)
{
    Func<ClassMapping, PropertyMappingCollection>? propMappingFactory = null;
    if (propertyMapFactory != null)
    {
        propMappingFactory = cm => new PropertyMappingCollection(propertyMapFactory(cm));
    }

    var mapping = new ClassMapping(name, nativeType, release, propMappingFactory)
    {
        IsResource = isResource,
        IsCodeOfT = isCodeOfT,
        IsFhirPrimitive = isFhirPrimitive,
        IsPrimitive = isPrimitive,
        IsBackboneType = isBackboneType,
        DefinitionPath = definitionPath,
        IsBindable = isBindable,
        Canonical = canonical,
        ValidationAttributes = validationAttributes ?? [],
    };

    _mappedClasses.GetOrAdd((nativeType, release), mapping);

    return mapping;
}

private ClassMapping(string name, Type nativeType, FhirRelease release, Func<ClassMapping, PropertyMappingCollection>? propertyMappingFactory = null)
{
    Name = name;
    NativeType = nativeType;
    Release = release;
    _propertyMappingFactory = propertyMappingFactory ?? inspectProperties;
}

EnumMapping

public static EnumMapping Build(string name, string? canonical, Type nativeType, FhirRelease release, string? defaultCodeSystem = null)
    => new EnumMapping(name, canonical, nativeType, release, defaultCodeSystem);

I created a source generator that generates the ClassMappings and EnumMappings and was able to achieve the numbers below:

Generated ModelInspector
https://gist.github.com/almostchristian/81a18cfa62816213e9d3133ee2b0cf7e

https://github.com/almostchristian/firely-net-sdk/tree/feature/model-inspector-sourcegen

Wow, thanks for this!
We will definitely take a good look at this!

Ok, I think we should do this. Our goal for a new PR would be to change the ClassMapping/PropertyMapping/ModelInspector in a way that we:

• Don't make breaking changes
• Make them more useable for a code generator.

Allow me to suggest some modifications:

ModelInspector API changes

• I don't see you need a new ForTypes() overload, at least, it's not used in the generated code (https://gist.github.com/almostchristian/81a18cfa62816213e9d3133ee2b0cf7e) - do we still need it?
• Instead of adding a new constructor to ModelInspector, I think we should do it as a static factory method - I'd like to keep the constructors for simple, common constructions. This is quite a "specialty" constructor, so an explicit factory method can described the intent better. E.g. what about public ForPredefinedMappings(string version, List<ClassMapping> classMappings, List<EnumMapping> enumMappings)?

ClassMapping

• I don't think we need a new Build() method. This code was from before we had the required keyword, so I needed a constructor with the "required" properties + the rest was initializable. As far as I am concerned, we can now have a parameterless constructor + make the properties in the original constructor required.
• I like the property factory parameter. but since I don't think it should be a property, can we pass this as the only parameter to the constructor?

PropertyMapping

• Same story - just a new no-param constructor with required properties.

Agree? Suggestions?

The ForTypes method is no longer necessary since using it will inevitably require reflection. I agree with using the ForPredefinedMappings factory method.

For ClassMapping and PropertyMapping, yes I agree to make the private set properties be required init so we can remove the need for the Build factory methods.

However PropertyMapping has some tricky issues:

• Release is a public readonly field, and changing it to an required init property which may break compatibility.
• The NativeProperty, which is also a public readonly field, is not required to be set and is only required to generate the getter and setter implementation. Since we can generate the getter and setter via source gen, we can remove the need for this field and in my test source generator, this is always null. We can add a function to retrieve the PropertyInfo for backward compatibility but then we'd have to save the actual property name so it can be retrieved later.

public class PropertyMapping
{
   private PropertyMapping(PropertyInfo nativeProperty)
   {
      _nativeProperty = nativeProperty;
   }

   public PropertyMapping(
      Func<object, object?> getter,
      Action<object, object?> setter)
   {
      _getter = getter;
      _setter = setter;
   }

   public PropertyInfo NativeProperty => _nativeProperty ?? LazyInitializer.EnsureInitialized(
      ref _nativeProperty,
      () => ImplementingType.GetProperties(BindingFlags.Public | BindingFlags.Instance).First(x => x.GetCustomAttribute<FhirElementAttribute>()?.Name == Name)!)!;

   private Func<object, object?> EnsureGetter()
      => _getter ?? LazyInitializer.EnsureInitialized(ref _getter, NativeProperty.GetValueGetter)!;

   private Action<object, object?> EnsureSetter()
      => _setter ?? LazyInitializer.EnsureInitialized(ref _setter, () => NativeProperty.GetValueSetter())!;
}

Also, EnumMapping and EnumMemberMapping also needs to be updated to be similar to ClassMapping in that the properties should be updated to be required init properties instead of get only properties. Also, EnumMapping will have a constructor that accepts a memberMapping factory.

public EnumMapping(string? defaultCodeSystem)
{
    _mappings = new(() => mappingInitializer(defaultCodeSystem));
}

public EnumMapping(Func<IReadOnlyDictionary<string, EnumMemberMapping>> memberMappingFactory)
{
    _mappings = new(valueFactory: memberMappingFactory);
}

I have further (breaking) improvements to make, so I am now planning to integrate this PR in the SDK 6.0