Add BoundNode.Source() method. Improve AssertEx.SetEqual() method by jcouv · Pull Request #31526 · dotnet/roslyn

jcouv · 2018-12-04T21:43:44Z

Source() is similar to Dump(), but it produces a C#-like format. It's not meant to be complete or valid code, but I found it extremely useful for working on lowering async-iterator methods. Below is an example.
SetEqual() produces an error message which respects the original ordering of the expectations (more usable diff).

Example of output of Source() on the following BoundTryStatement after async rewriting:

        try
        {
            yield return 1;
            Write(""Try "");
            yield return 2;
        }
        finally
        {
            Write(""Finally "");
            await System.Threading.Tasks.Task.CompletedTask;
        }

{
    int temp1;
    temp1 = this.<>1__state;
    try
    {
        switch (temp1)
        {
            case 0:
            case 1:
                goto <tryDispatch-47>;
                break;
            case 2:
                goto <stateMachine-51>;
                break;
        }
        {
            {
                {
                    this.<>s__1 = default;
                    this.<>s__2 = default;
                    {
                        <tryDispatch-47>: ;
                        try
                        {
                            switch (temp1)
                            {
                                case 0:
                                    goto <stateMachine-43>;
                                    break;
                                case 1:
                                    goto <stateMachine-45>;
                                    break;
                            }
                            {
                                {
                                    {
                                        this.<>2__current = 1;
                                        this.<>1__state = temp1 = 0;
                                        goto <yieldReturn-42>;
                                        <stateMachine-43>: ;
                                        this.<>1__state = temp1 = -1;
                                        {
                                            if (!this.<>w__disposeMode) goto <afterif-44>;
                                            goto <finallyLabel-37>;
                                            <afterif-44>: ;
                                        }
                                    }
                                    Write("Try ");
                                    {
                                        this.<>2__current = 2;
                                        this.<>1__state = temp1 = 1;
                                        goto <yieldReturn-42>;
                                        <stateMachine-45>: ;
                                        this.<>1__state = temp1 = -1;
                                        {
                                            if (!this.<>w__disposeMode) goto <afterif-46>;
                                            goto <finallyLabel-37>;
                                            <afterif-46>: ;
                                        }
                                    }
                                }
                                goto <finallyLabel-37>;
                                {
                                }
                            }
                        }
                        catch (Object) 
                        {
                        }
                    }
                    {
                        {
                            <finallyLabel-37>: ;
                            {
                                Write("Finally ");
                                {
                                    System.Runtime.CompilerServices.TaskAwaiter temp2;
                                    {
                                        temp2 = Task.get_CompletedTask().GetAwaiter();
                                        {
                                            if (! BoolLogicalNegation temp2.get_IsCompleted()) goto <afterif-52>;
                                            {
                                                this.<>1__state = temp1 = 2;
                                                this.<>u__1 = temp2;
                                                { temp3 = this; this.<>t__builder.AwaitUnsafeOnCompleted(temp2, temp3) };
                                                goto <exitLabel-41>;
                                                <stateMachine-51>: ;
                                                temp2 = this.<>u__1;
                                                this.<>u__1 = default;
                                                this.<>1__state = temp1 = -1;
                                            }
                                            <afterif-52>: ;
                                        }
                                    }
                                    temp2.GetResult();
                                }
                            }
                            {
                                object temp4;
                                temp4 = this.<>s__1;
                                {
                                    if (!temp4 ObjectNotEqual null) goto <afterif-39>;
                                    {
                                        System.Exception temp5;
                                        temp5 = AsOperator
                                        ;
                                        {
                                            if (!temp5 ObjectEqual null) goto <afterif-38>;
                                            throw temp4;
                                            <afterif-38>: ;
                                        }
                                        Capture(temp5).Throw();
                                    }
                                    <afterif-39>: ;
                                }
                            }
                            this.<>s__2;
                        }
                        {
                            if (!this.<>w__disposeMode) goto <afterif-53>;
                            goto <exprReturn-40>;
                            <afterif-53>: ;
                        }
                    }
                }
                this.<>s__1 = null;
            }
            goto <exprReturn-40>;
        }
    }
    catch (Exception) 
    {
        this.<>1__state = -2;
        this.<>v__promiseOfValueOrEnd.SetException(temp6);
        goto <exitLabel-41>;
    }
    <exprReturn-40>: ;
    this.<>1__state = -2;
    {
        this.<>v__promiseOfValueOrEnd.SetResult(False);
        return;
        <yieldReturn-42>: ;
        this.<>v__promiseOfValueOrEnd.SetResult(True);
    }
    <exitLabel-41>: ;
    return;
}

src/Test/Utilities/Portable/Assert/AssertEx.cs

jcouv · 2018-12-07T17:19:58Z

test roslyn-CI please

jcouv · 2018-12-13T18:48:25Z

Hiding this PR for now. Our review pipeline is clogged as it is, with higher priority changes.

jaredpar · 2018-12-13T20:07:24Z

src/Compilers/CSharp/Portable/BoundTree/BoundNode_Source.cs

+        /// <summary>
+        /// Gives an approximate printout of a bound node as C# code.
+        /// </summary>
+        internal string Source()


Feel like this should be GetSource or CalculateSource. Given it's not a property expecting some verb.

This is meant to be similar to Dump(). I'll change to DumpSource().

jaredpar · 2018-12-13T20:09:18Z

src/Compilers/CSharp/Portable/BoundTree/BoundNode_Source.cs

+
+                void incrementIndent()
+                {
+                    indent += 4;


Instead of hard coding 4 everywhere consider making indent size a constant for the outer function.

jaredpar · 2018-12-13T20:10:15Z

src/Test/Utilities/Portable/Assert/AssertEx.cs


+        public static void SetEqual(IEnumerable<string> expected, IEnumerable<string> actual, IEqualityComparer<string> comparer = null, string message = null, string itemSeparator = "\r\n", Func<string, string> itemInspector = null)
+        {
+            var indexes = new Dictionary<string, int>();


Shouldn't this take comparer as a parameter (after calculating a default value when null).

jaredpar · 2018-12-13T20:10:49Z

src/Test/Utilities/Portable/Assert/AssertEx.cs

+
        public static void SetEqual<T>(IEnumerable<T> expected, IEnumerable<T> actual, IEqualityComparer<T> comparer = null, string message = null, string itemSeparator = "\r\n", Func<T, string> itemInspector = null)
        {
            var expectedSet = new HashSet<T>(expected, comparer);


Shouldn't we be calculating a default here ...

That's not necessary. Passing null is ok.
That's existing logic and HashSet and Dictionary can handle null comparers.

jaredpar · 2018-12-13T20:11:34Z

Done with review pass (iteration 1)

jcouv · 2018-12-18T20:49:48Z

@jaredpar Please take another look when you get the chance. Addressed your feedback. Thanks

jcouv added Area-Compilers Test Test failures in roslyn-CI labels Dec 4, 2018

jcouv added this to the 16.0.P2 milestone Dec 4, 2018

jcouv self-assigned this Dec 4, 2018

jcouv requested a review from a team as a code owner December 4, 2018 21:43

jhinder reviewed Dec 4, 2018

View reviewed changes

src/Test/Utilities/Portable/Assert/AssertEx.cs Outdated Show resolved Hide resolved

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 10, 2018

Add BoundNode.Source() method. Improve AssertEx.SetEqual() method

c7a746b

jcouv force-pushed the test-infra branch from a7313ef to c7a746b Compare December 10, 2018 18:11

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 10, 2018

jcouv mentioned this pull request Dec 12, 2018

Async-streams: Disposal in async-iterator methods #31527

Merged

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 13, 2018

jaredpar reviewed Dec 13, 2018

View reviewed changes

Address feedback from Jared

010dcce

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 18, 2018

jaredpar approved these changes Dec 19, 2018

View reviewed changes

jcouv merged commit b001793 into dotnet:master Dec 19, 2018

jcouv deleted the test-infra branch December 19, 2018 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BoundNode.Source() method. Improve AssertEx.SetEqual() method#31526

Add BoundNode.Source() method. Improve AssertEx.SetEqual() method#31526
jcouv merged 2 commits intodotnet:masterfrom
jcouv:test-infra

jcouv commented Dec 4, 2018

Uh oh!

Uh oh!

jcouv commented Dec 7, 2018

Uh oh!

jcouv commented Dec 13, 2018

Uh oh!

jaredpar Dec 13, 2018

Uh oh!

jcouv Dec 18, 2018

Uh oh!

jaredpar Dec 13, 2018

Uh oh!

jaredpar Dec 13, 2018

Uh oh!

jaredpar Dec 13, 2018

Uh oh!

jcouv Dec 18, 2018

Uh oh!

jaredpar commented Dec 13, 2018

Uh oh!

jcouv commented Dec 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jcouv commented Dec 4, 2018

Uh oh!

Uh oh!

jcouv commented Dec 7, 2018

Uh oh!

jcouv commented Dec 13, 2018

Uh oh!

jaredpar Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

jaredpar Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

jaredpar Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

jaredpar Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Dec 13, 2018

Uh oh!

jcouv commented Dec 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants