Add support for rendering `ValueWithName` expressions in the decompiler

[eiriktsarpalis]

This PR adds support for rendering ValueWithName expressions in the decompiler. This also fixes an annoying issue we have been experiencing when using Unquote's assertions, namely the following snippet:

open Swensen.Unquote.Assertions

type Foo = { Foo : string ; Bar : int }

let someTest () = 
    let x = { Foo = "foo" ; Bar = 41 }
    test <@ x.Bar = 42 @>

Currently results in the following output:

Test failed:

{Foo = "foo";
 Bar = 41;}.Bar = 42
41 = 42
false

whereas the change improves the formatting into the following:

Test failed:

x.Bar = 42
41 = 42
false

The point of this change is that in real tests, the object graph referenced by x is typically a large object so having failing assertions typically causes the assertion checker to flood output with irrelevant data that happen to be captured by the quotation. I believe that this change slightly improves the situation, allowing us to focus on the actual assertion.

[vasily-kirichenko]

Great.

[stephen-swensen]

@eiriktsarpalis thanks for the PR!

This looks compelling. I have the same annoyance with record type noise in assertions. One thing I am wondering though is about other cases where we might prefer to see the value. e.g.

let x = 41
test <@ x = 42 @>

I'm not sure if seeing x = 42 or 41 = 42 is better in the failure message...

And also there are cases where we may be comparing two records directly and actually want to see all the fields to spot where structural equality fails (which can also be noisy but something I sometimes do).

One idea is to only use ValueWithName pattern in the context of PropertyGet (not sure if that is exactly right). e.g. decompile <@ x.Bar @> -> x.Bar but decompile <@ x @> -> {Foo = "foo"; Bar = 41;}

Anyhow, just thinking out loud with all of the above. I'll dig in deeper later.

(p.s. thanks for the backwards compat thoughtfulness!)

[ploeh]

Apologies that I've been too lazy to read through the actual code, so here are instead some observations and questions.

Overall, I think this could be beneficial. I agree that for large records, it can sometimes be hard to sift through all the data in order to pinpoint the element that differs.

I could imagine, however, that there are scenarios where it could be helpful with the full context. Is there a way to get the old/current behaviour?

Also, is this change only for records, or does it also impact (C#) objects?

As an example, I often use Unquote for integration tests against REST APIs, and I use the HttpClient API for that. Often, I simply write an assertion like this:

test <@ response.IsSuccessStatusCode @>

where response is an HttpResponseMessage. If that assertion fails, it produces output like this:

StatusCode: 404, ReasonPhrase: 'Not Found', Version: 1.1, Content: System.Net.Http.StreamContent, Headers:
{
}.IsSuccessStatusCode
false

Although I don't care about the exact status code or reason phrase as long as the test passes, I do care about it when it fails. This is particularly useful when doing outside-in TDD, because using that workflow, you start with a failing integration test, and you can essentially let the errors guide you on what to do next. I'd rather like to keep that behaviour.

[eiriktsarpalis]

@ploeh I used to think that outputting the entire object graph in the output of a failing assertion would be beneficial, however in practice I have found this to be far from the truth, for the following reasons:

1. What is being output is not a full dump of the test's state, it's just whatever objects happen to be captured in the quotation tree: 95% of cases this capture is incidental artifacts rather than relevant to what the test is checking. In the end, there is no good substitute for attaching your test to the debugger.
2. You could somehow ameliorate this effect by applying the transformation let field = bigGraph.Foo.Bar.X in test <@ field = 42 @>. However this a) makes an ugly assertion and b) is difficult to explain or enforce as a practice in the context of a team.

For me, the big value of test comes from its ability to recover the actual assertion code and render it appropriately in case of a failing test. The fact that a failing assertion renders in stdout as

response.IsSuccessStatusCode
false

gives instant visual feedback as to what exactly went wrong. This is absolutely critical when going through thousands of lines of build outputs where multiple failing tests may pop up. The fact that often the build output doubles because pretty-printed session objects is currently an obstacle to our ability to identify what went wrong.

[ploeh]

I'm sorry if I wasn't clear. My opinion is that often, the entire record/object is just noise, but sometimes, it's not.

I tried to give an example of a scenario where I find the current behaviour useful. It's OK if you don't agree, but since I interpreted your tweet as a request for feedback, that's my feedback.

I'd very much like to be able to have the old behaviour. Not always, but sometimes. It's okay if it becomes an opt-in feature, because I agree that most of the time, I don't need all the noise.

Hence my question: is there a way to retain the old behaviour?

[eiriktsarpalis]

I'm pretty sure your point came through perfectly clear, and I appreciate your feedback. I was just providing counterpoints to it. Namely questioning whether it is a useful thing to have at all. (Not convinced 100% either, just want to prompt the discussion).

Hence my question: is there a way to retain the old behaviour?

If you look at the diff, the change is actually only touching the implementation of decompile, adding proper support for ValueWithNode expressions. Rendering is changed in test as a side-effect of that change. So to answer your question, no, unless the evaluator in test is somehow augmented so that ValueWithName nodes are stripped.

[ploeh]

Thank you. Then my feedback is that this change would break my workflow in certain cases.

Couldn't it instead be a different method? I've never liked the name test anyway... I think it should be assert or require or something like that...

[eiriktsarpalis]

I think everybody would prefer assert, but it's a keyword in F#

[ploeh]

Right, I forgot that 😳

[stephen-swensen]

What if we had it both ways, so a failing test like

let someTest () = 
    let x = { Foo = "foo" ; Bar = 41 }
    test <@ x.Bar = 42 @>

would have output like

Test failed:

x.Bar = 42
{Foo = "foo";
 Bar = 41;}.Bar = 42
41 = 42
false

i.e. we introduce a ValueWithName -> Value reduction step in https://github.com/SwensenSoftware/unquote/blob/master/Unquote/Reduction.fs (not entirely sure if possible here but I think so)? Yes there is still noise in cases we don't want it, but we do get the actual assertion code added in without breaking existing workflows that rely on full value printing.

[isaacabraham]

My feelings are generally the same as most other people here: I think 95% of the time I don't need the full object (so much so that I've started moving from quotations to just the =! operator), especially as the test explorer in VS doesn't make it easy to work with large error messages.

Perhaps there could an alternative to the test function e.g. testFull which emits the full graph, and test would start to emit the shortened version?

[ploeh]

the test explorer in VS doesn't make it easy to work with large error messages

Use TestDriven.Net 😜

Perhaps there could an alternative to the test function e.g. testFull which emits the full graph, and test would start to emit the shortened version?

Yes, that's what I so inadequately and incoherently tried to say. Thank you for making it comprehensible 👍 That would address my concern.

[stephen-swensen]

Can we clarify that it is mainly record types that are the source of noise annoyance? Note that the Value pattern printing is super-valuable in the general case (in my estimation). e.g. as-is, this PR would change output of

let x = (1,2)
let y = (2,3)
test <@ x = y @>

from

(1,2) = (2,3)
false

to

x = y
false

which I don't see as an improvement! Though

x = y
(1,2) = (2,3)
false

would be nice (done via reduction step).

Note that under-the-hood, the Value pattern typically uses sprintf "%A" for printing values: https://github.com/SwensenSoftware/unquote/blob/master/Unquote/Decompilation.fs#L196 ... that leads me to think of things like

1. you can use the record type StructuredFormatDisplay attribute to provide less noisy output (not great solution because burdensome): https://msdn.microsoft.com/visualfsharpdocs/conceptual/core.structuredformatdisplayattribute-class-%5bfsharp%5d
2. perhaps custom value printer could be registered or injected

I'm a bit wary of expanding the test API surface area or complexity, would rather see if we can sufficiently solve the problems of intent and context if possible.

[eiriktsarpalis]

@stephen-swensen What if we added we added ValueWithName --> Value as an intermediate evaluation step? Then exposing a variant of test that disables printing of evaluation steps apart from the first step (i.e. just the code of the assertion). We are then able to both add support for ValueWithName in the decompiler without removing the possibility for a verbose dump of contents.

[isaacabraham]

@eiriktsarpalis can you explain the difference between using =! and the proposed changes with a couple of examples? Ta.

[stephen-swensen]

@eiriktsarpalis "...expose a variant of test that disables printing of evaluation steps apart from the first step" that's an interesting proposal that may be less invasive. Implementation-wise, I think it would be the same as the "What if we had it both ways" I put out (i.e. we always decompile ValueWithName over Value, but have a ValueWithName -> Value reduction step) but with a new assertion function that only outputs the first step:

let inline testSimple (expr:Expr<bool>) =
    let u = unquote expr
    match u.FinalReduction with
    | DerivedPatterns.Bool(true) -> ()
    | _ ->  
        try
            testFailed [u.Reductions.Head] "" //print just the head here
        with 
        | e -> raise e //we catch and raise e here to hide stack traces for clean test framework output

then, testSimple <@ x.Bar = 42 @> would output

Test failed:

x.Bar = 42

while good 'ol test <@ x.Bar = 42 @> would output

Test failed:

x.Bar = 42
{Foo = "foo";
 Bar = 41;}.Bar = 42
41 = 42
false

To @isaacabraham 's question, its similar in spirit to what the =! operators provide, but x.Bar =! 42 outputs the value of left and right hand expressions like

Test failed:

41 = 42
false

[eiriktsarpalis]

@isaacabraham the =! family of operators cannot extract useful metadata like variable names, just the two values. So doing

let someTest () = 
    let y = { Foo = "foo" ; Bar = 41 }
    test <@ y.Bar = 42 @>

emits

Test failed:

y.Bar = 42
41 = 42
false

while

let someTest () = 
    let y = { Foo = "foo" ; Bar = 41 }
    y.Bar =! 42

emits

Test failed:

41 = 42
false

[stephen-swensen]

@eiriktsarpalis maybe even better, we print out the first and last (significant) steps:

let inline testSimple (expr:Expr<bool>) =
    let u = unquote expr
    match u.FinalReduction with
    | DerivedPatterns.Bool(true) -> ()
    | _ ->  
        try
            let simpleSteps =
                if u.Reductions.Length >= 3 then
                    [u.Reductions.Head; u.Reductions.[u.Reductions.Length-2]]
                else 
                    [u.Reductions.Head]
            testFailed simpleSteps ""
        with 
        | e -> raise e //we catch and raise e here to hide stack traces for clean test framework output

so testSimple <@ x.Bar = 42 @> would output

Test failed:

x.Bar = 42
41 = 42

[bartelink]

Perhaps a reduced output version could be called tst (or ast (or ass ?!)) to reflect the reduced output ?

I think the large root object tree (while common in certain kinds of testing) issue is not as general a concern as painted here; As illustrated by the let ... in ... example, it's always possible to work around (and breaking out a let result = ... at the end of the Arrange phase can make the overall test easier to scan). (One can also break the quotation into two lines, often leading to a more legible form.)

Thus I'd be in favour of retaining the existing behavior as it's a good general default.

[stephen-swensen]

All,

It's been a while, but thank you for discussion on this PR! I've been working on a biggish maintenance release of unquote in https://github.com/SwensenSoftware/unquote/pull/146 and I believe I can address everything discussed in a happy, non-breaking way:

• We will implement ValueWithName decompilation and reduction to Value in a way that has no impact on whether or not large object graphs are produced from test assertions: https://github.com/SwensenSoftware/unquote/issues/133
• we will implement a new testSimple assertion operator which will provide a less noisy alternative to test (test will not change): https://github.com/SwensenSoftware/unquote/issues/148 - in this ticket I provide two options, please head on over there to discuss.