[Projection tooling] Modular string-based C# and Swift code emitters

[kotlarmilos]

Description

The current implementation of string-based code emitters is becoming more complex due to nested conditional statements. As new Swift language features are introduced, the complexity of these emitters is expected to grow. For example:

private void EmitReturnMethod(CSharpWriter csWriter, SwiftWriter swiftWriter)
{
    if (_requiresIndirectResult)
    {
        csWriter.WriteLine($"return SwiftMarshal.MarshalFromSwift<{_wrapperSignature.ReturnType}>((SwiftHandle)swiftIndirectResult.Value);");
    }
    else
    {
        if (_requiresSwiftAsync)
        {
            csWriter.WriteLine("return task.Task;");
        }
        else
        {
            if (_env.MethodDecl.CSSignature.First().SwiftTypeSpec.IsEmptyTuple)
            {
                csWriter.WriteLine("return;");
            }
            else
            {
                csWriter.WriteLine("return result;");
            }
        }
    }
}

The proposal is to refactor the code emitters by implementing the builder/strateg/visitor design patterns. Some of these patterns are already in place, they are not being fully utilized. The idea is to delegate handling to sub-handlers and iterate over these handlers in each part of the emission process, rather than executing them explicitly in specific order.

_handlers = new Dictionary<>
{
    new IndirectResultHandler()
    new AsyncReturnHandler()
    new VoidReturnHandler()
    new ResultReturnHandler()
    ...
};

References

Here are the existing string-based emitters with smaller scope compared to the Swift bindings:

• https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs
• https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
• https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs
• https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Options/gen/Emitter.cs
• https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs

[kotlarmilos]

@jkurdek @matouskozak @mcumming Please summarize your ideas/ problems here. The goal is to use this issue as an input for design at some point.

[jkurdek]

The current implementation of string-based code emitters is becoming more complex due to nested conditional statements.

I agree that if we are not careful we are going to end up with convoluted logic on the emitter. And this is not an easy task because we are solving two problems at once, we are building a tree and we are serializing it.

The proposal is to refactor the code emitters by implementing the builder/strateg/visitor design patterns. Some of these patterns are already in place, they are not being fully utilized. The idea is to delegate handling to sub-handlers and iterate over these handlers in each part of the emission process, rather than executing them explicitly in specific order.

I dont think that structuring is the problem. The problem are the rules. The above code can be rewritten as such:

private void EmitReturnMethod(CSharpWriter csWriter, SwiftWriter swiftWriter)
{
    if (_requiresIndirectResult)
    {
        csWriter.WriteLine($"return SwiftMarshal.MarshalFromSwift<{_wrapperSignature.ReturnType}>((SwiftHandle)swiftIndirectResult.Value);");
        return;
    }
    if (_requiresSwiftAsync) // && !_requiresIndirectResult
    {
        csWriter.WriteLine("return task.Task;");
        return;
    }
    if (!_isVoid) // && !_requiresIndirectResult && !_requiresSwiftAsync
    {
        csWriter.WriteLine($"return {resultName}");
        return;
    }

    csWriter.WriteLine("return;"); // ! (_requiresIndirectResult || _requiresSwiftAsync || _hasNamedReturn)
}

Order encodes additional information. We could get rid of the order and create separate handlers but then when we add a new rule we would have to go and update every handler.

[kotlarmilos]

I think this is a good approach. I suggest going one step further and group them into handlers, where public bool Handles evaluates the rule while Construct builds an expression. Similar to:

runtimelab/src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

Lines 9 to 31 in 95e8fe9

	/// <summary>
	/// Represents a method handler factory.
	/// </summary>
	public class MethodHandlerFactory : IFactory<BaseDecl, IMethodHandler>
	{
	/// <summary>
	/// Checks if the factory can handle the declaration.
	/// </summary>
	/// <param name="decl">The base declaration.</param>
	/// <returns></returns>
	public bool Handles(BaseDecl decl)
	{
	return decl is MethodDecl;
	}
	/// <summary>
	/// Constructs a handler.
	/// </summary>
	public IMethodHandler Construct()
	{
	return new MethodHandler();
	}
	}

This should reduce redundancy and allow for extensibility.

[jkurdek]

I think that grouping them into handlers doesn't add any value but just hides complexity behind OO layer and introduces a lot of boiler plate code.

Lets consider the Handles method:

// ResultReturnHandler
public bool Handles(MethodDecl methodDecl)
{
    return methodDecl.HasIndirectResult();
}

// AsyncResultHandler
public bool Handles(MethodDecl methodDecl)
{
    return !methodDecl.HasIndirectResult() && methodDecl.IsAsync();
}

// ResultReturnHandler
public bool Handles(MethodDecl methodDecl)
{
    return !methodDecl.HasIndirectResult() && !methodDecl.IsAsync() && !methodDecl.IsVoid();
}

// VoidReturnHandler
public bool Handles(MethodDecl methodDecl)
{
    return !methodDecl.HasIndirectResult() && !methodDecl.IsAsync() && methodDecl.IsVoid();
}

Then when we add a new rule we have go an update n handlers in multiple locations. I think if statement is simpler.

[kotlarmilos]

This is true when negation is used; however, in the rules, we usually use a positive condition.

[jkurdek]

I'm not sure I follow. Could you share some example?

[kotlarmilos]

The problem with the rules is independent of how we group them. By using positive expressions, we minimize the need to update them when adding a new rule. Currently, our handlers operate like that:

if (_requiresIndirectResult) // instead of !...
  // expression 1
if (_requiresSwiftAsync) // instead of !...
  // expression 2
if (_isVoid) // instead of !...
  // expression 3
default
  // expression 4

[kotlarmilos]

We still have a relatively manageable number of supported scenarios. For example, consider having method handlers for async and non-async methods. The signature and body differ significantly, and by making changes in one of the handlers, you can ensure that the impact is localized to a particular use case. This means that they could still use the same building blocks, but they are constructed differently.

[jkurdek]

I agree with you that it is beneficial to have specialized handlers when the rules are independent. This pattern is already used in the code (constructors and non-constructors have some different handlers). They still need to be applied in correct order though.

And to be specific to the example you shared, we can extract those checks into separate entities but if you want to use positive expressions for the rules they will still have to be applied in correct order (as they are not independent).

I disagree however with the need to start wrapping them in separate types as I fail to notice what improvements would it bring.

[kotlarmilos]

I agree with you that it is beneficial to have specialized handlers when the rules are independent. This pattern is already used in the code (constructors and non-constructors have some different handlers). They still need to be applied in correct order though.

I think this is enough for the existing use-cases.

And to be specific to the example you shared, we can extract those checks into separate entities but if you want to use positive expressions for the rules they will still have to be applied in correct order (as they are not independent).

I disagree however with the need to start wrapping them in separate types as I fail to notice what improvements would it bring.

Let me try to reflect changes in one of the PRs to demonstrate the level of granularity.