FastExpressionCompiler icon indicating copy to clipboard operation
FastExpressionCompiler copied to clipboard

ToCsharpCode: format block inside expression in `{ ... }`

Open exyi opened this issue 3 years ago • 4 comments

When a block is inside another expression (binary, unary, assignment) it is now formatted into braces. It's not valid C# code, but neither was it valid before this change. This makes the code eaiser to read (IMHO), since the block scope is clearly visible.

My example expression, before the change:

{
    ....
    return ((object)(Command)(() => //$
{
    vm_this.Title = ((string)((string)
        HierarchyControl tmp0;                       // <- variable randomly declared after the cast
        tmp0 = ((HierarchyControl)control1 != null ?
            control1.GetClosestControlBindingTarget() :
            default(DotvvmBindableObject));
        tmp0 != null ?
            tmp0.GetValue(
                ...,
                true) :
            default(object)));
    return 
        Task.CompletedTask;
}));
    object__5216800:;
});

After the change:

(BindingDelegate)((DotvvmBindableObject currentControl) =>
{
    ...
    return ((object)(Command)(() =>
    {
        vm_this.Title = ((string){
                HierarchyControl tmp0;
                tmp0 = ((HierarchyControl)(control1 != null) ?
                    control1.GetClosestControlBindingTarget() :
                    null);
                (tmp0 != null) ?
                    tmp0.GetValue(
                        ...,
                        true) :
                    null;
            });
        return Task.CompletedTask;
    }));
    object__48756912:;
});

If you have any other ideas how to format such code, I'd be happy to incomporate them. I was considering these alternatives:

  • Instead of { ... }, rewrite it as (Func<ResultType>() => { })(), since that would compile.
    • However, it would break return/break/goto semantics, so it's "correct" solution either.
    • It's more noisy and might be confusing - it creates a lambda function, while there was nothing like in the expression.
  • Rewrite unary(block(a, b, c, d)) -> block(a, b, c, unary(d)).
    • That would be nicer, but would only work for this specific case.
    • Adding it to all cases would be a lot of changes.
    • And, this approach can't handle more complex cases like binary(Method1(), Block(Method2())), I'd either have to reorder the method calls (incorrect), or add temporary variables (complex and noisy)

exyi avatar Dec 29 '22 20:12 exyi

Sorry, I forgot that I also changed handling of default(T) for reference types. If you don't like this, I'll revert it or put it into separate PR. The reason to do it is that default(TheType) gets annoyingly long to read when the type name are more complex and I think it's rarely unclear of which type is the null. I understand that type clarity might be of higher priority to you, so I'm not gonna argue about it if you don't agree :)

exyi avatar Dec 29 '22 20:12 exyi

Thanks for PR. I will check later.

dadhi avatar Dec 31 '22 09:12 dadhi

There are too many uncertainties with those changes.

Regarding null instead of default(Foo.Bar), maybe there should be enum configuration for such kind of thing. Because the preference may depend on context.

Regarding the proper expression block formatting. I want either a compile-able code or something (e.g. comments) that explains how to convert the code into the compile-able one.

Let's keep this PR open and see what we can improve.

dadhi avatar Jan 12 '23 16:01 dadhi

Sorry, I forgot about this for some time... I have removed the unrelated change, will make a separate PR for it.

It will now print comments about the invalid syntax. In general, I could not come up with a way to output valid C#, but we can add some heuristics for the special cases when it's possible.

  • If there is no control flow across the boundary, we can emit Javascript-like IIFE (() => { ...block; return result })()
  • Sometimes the parent expressions could get reordered into to return value of the block.
    • For example, reorder x = { ...block; result } into { ...block; x = result }
    • We must be sure that the left hand expression has no side effects (cannot throw, x.y = ... could not be reordered)
  • Break down expressions using temporary variables
    • For example, x.y.Method(1 + 1, 2 + { ... block; result }) -> t1 = ref x.y; t2 = 1 + 1; t3 = 2; ...block; t4 = blockresult; t1.Method(t2, t3 + t4);
    • However, I find this unfeasible to implement correctly since we'd have to support this breakdown for every single expression
    • I expect most people use ToCSharpCode method to display the Linq.Expressions, and this will lead to quite unreadable code.
  • Detect patterns C# can represent as expressions
    • constructor call with initializer (new(1, 2) { X = 1 })
    • invocation with nullcheck (x?.Method(x) or x?.Y)
    • pattern matching with variables (a is X x && x.Something == 12)

exyi avatar Oct 01 '23 14:10 exyi