Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToCsharpCode: format block inside expression in { ... } #354

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

exyi
Copy link
Contributor

@exyi exyi commented Dec 29, 2022

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 exyi force-pushed the tocsharp-blockexpression branch from 57ff039 to e1f27ce Compare December 29, 2022 20:22
@exyi
Copy link
Contributor Author

exyi commented Dec 29, 2022

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 exyi force-pushed the tocsharp-blockexpression branch from e1f27ce to 59fc220 Compare December 30, 2022 09:52
@dadhi
Copy link
Owner

dadhi commented Dec 31, 2022

Thanks for PR. I will check later.

@dadhi
Copy link
Owner

dadhi commented Jan 12, 2023

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 added a commit that referenced this pull request Sep 5, 2023
…crementAssign to nullable member now works
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.
@exyi exyi force-pushed the tocsharp-blockexpression branch from 59fc220 to 3369c2b Compare October 1, 2023 12:47
@exyi
Copy link
Contributor Author

exyi commented Oct 1, 2023

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 exyi force-pushed the tocsharp-blockexpression branch 2 times, most recently from 78193fa to 47fbdf4 Compare October 1, 2023 14:38
@exyi exyi force-pushed the tocsharp-blockexpression branch from 47fbdf4 to f0ca1d8 Compare October 1, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants