Why do we need to do this? I would have assumed (by virtue of topological sort) that all sources (ie. previous temps) would have already been assigned an identifier. Feels like we may be assigning identifiers more than once.
Consider adding assertions to prevent double set. #Closed

Have added a comment. Sometimes the evaluations seem to be the same object instance, other times not, and I'm not sure why, but I do expect these equivalent evaluations to get de-duped during lowering.

I think this needs more understanding. We should only have to process nodes forwards. I don't understand why we'd ever have to look back.
What breaks if this is removed?

Same question on case below (which labels its input on line 680).

Interesting effects from deleting the source.Id assignments here:

• If we delete only the one for BoundDagEvaluation then all the tests actually pass
• If we delete only the one for BoundDagTest then 1 of the tests fails
• If we delete both then 3 of the tests fail

It feels like sometimes the "source" evaluation objects are shared, other times not. Perhaps a matter of convenience during dag construction. I did poke around for a while in binding and lowering of the dags to get a better grasp on this but don't feel 100% on how de-duping works. De-duping seems to sometimes be dependent on the presence/content of when clauses as well.

I would also emphasize that the assumption made here is the same as the assumption made in the original dump code, i.e. it always uses the evaluation object as a dictionary key to look up the id, relying on its equality implementation in order to get the same answer for different instances with equivalent values. I feel like both of the assignments should just be left there since there probably exists a scenario where the absence of just one of them results in uninitialized ids.

One scenario which breaks when we remove these assignments: DecisionDag_Dump_SwitchStatement_01

[1]: t1 = (string)t0; [2]
 [2]: t1 == ""a"" ? [3] : [4]
 [3]: leaf `case ""a"":`
-[4]: t2 = t1.Length; [5]
+[4]: t2 = <uninitialized>.Length; [5]
 [5]: t2 == 1 ? [6] : [13]
 [6]: when <true> ? [7] : <unreachable>
 [7]: leaf `case string { Length: 1 } s:`

I'll give this a try locally.

I stepped through DecisionDag_Dump_SwitchStatement_01 and I think your change is right.

Another thing I find really useful is the tree dump of the final dag using rootDecisionDagNode.Dump() after:

I usually find myself using that instead - it's easier to reason about and it reflects the final codegen better.
However, currently it uses the generic tree dumper with a lot of noise which makes it hard to see what's going on.
Can we possibly mix this two representations and have a custom dumper impl that only prints relevant fields?

Interesting, do you think you could mock up a before/after of the dump that contains the information you want? I guess I am wondering if it seems like we should, for example, repeatedly print certain nodes in the dump when there are multiple ways to reach that node. It also feels like we would want to cut out all the "source" properties in this representation since those basically always point "backwards".

Something like:

testDecisionDagNode
├─test: <debugger display>
├─whenTrue
│ └─evaluationDecisionDagNode
│   ├─evaluation: <debugger display>
│   └─next
|     └─testDecisionDagNode
|       ├─test: <debugger display>
|       ├─whenTrue
|       |  └─evaluationDecisionDagNode
|       |    ├─evaluation: <debugger display>
|       |    └─next
|       |      └─testDecisionDagNode
|       |        ├─test: <debugger display>
|       |        ├─whenTrue: leaf
|       |        └─whenFalse
...

Instead of:

testDecisionDagNode
├─test
│ └─dagNonNullTest
│   ├─isExplicitTest: True
│   ├─input
│   │ └─dagTemp
│   │   ├─type: X
│   │   ├─source
│   │   ├─index: 0
│   │   └─hasErrors: False
│   └─hasErrors: False
├─whenTrue
│ └─evaluationDecisionDagNode
...

I think I won't have time to do this myself but would be happy to accept a change which changes the implementation of Dump() on dag nodes to work in this manner.

Minor: perhaps

var parameters = d.DeconstructMethod.Aggregate(
    "",
    (s, p) => $"{(s.Length == 0 ? "" : ", ")}Item{p.Ordinal + 1}");

var pooledBuilder = PooledStringBuilder.GetInstance();

Perhaps just create a new StringBuilder() instead, to avoid the extra complexity evaluating in the debugger.