Debug revamp, make all debug points explicit

[dsyme]

This contains a significant revamp of parts of our debug point implementation.

1. Previous "implicit" debug points (for things like method calls) are now kept as explicit nodes in the expression tree Expr.DebugPoint. This makes them muchmore reliable as the tree undergoes transformations, e.g. inlining of a method call. This change should have been made long ago as many of our debug problems were related to the lack of explicit representation of the information here.

2. Debug points for sequential code and targets of matches/conditionals and leaf expressions (e.g. method calls) are removed in favour of explicit debug points

3. Debugging for 'for' loops was broken in various ways, this is a systematic deep sweep over the whole matrix described below, including for computation expressions. As part of this, the debug point on the in or to keyword for a for loop is made more explicit in the code. For a a computation expression (, e.g. a task) the debug point must be propagated to inline code, and this is done through a potentially-general mechanism __debugPoint that allows inlined code to lay down "named" debug points associated with the context into which the code is inlined. However currently only __debugPoint "ForLoop.InOrToKeyword" is supported

4. Debug points for the "with" and "finally" parts of try/with and try/finally in computation expressions were always missing. This adds them.

5. We add DebuggerNonUserCodeAttribute and GeneratedCodeAttribute attributes to any lambda generated method where the code has no debug points, rather than adding a false debug point. This covers a range of "micro-lambdas" generated for computation expressions.

6. Two optimizations were not preservig debug point sequencing. For example, for

    let x = (let y = ey in e2) in ... ---> let y = ey in let x = e2 in ... 

   Here the debug point for binding "x" and "y" were being switched around. There was also this case:

   let x = e1 in e2 --> e1; e2  // if x is unused and e1 has an effect
   let x = e1 in e2 --> e2   // if x is unused and e1 has no effect

   Here the debug point associated with the binding for x was being lost. It's now preserved - note this results in additional debug points and nop in release code, but that is what we expect and is not a performance problem since the JIT ignores the nop and debug points unless actually debugging.

7. We add debug points either side of e1 && e2 and e1 || e2 - that is, these constructs are now considered part of control-flow.

8. FCS API is adjusted to reveal various debug points

Cleanup

The changes stem mainly from the above, with a little cleanup:

• primMkCond, mkCond, AddResultTarget, mkSequential, mkThenDoSequential, mkSequentials and friends no longer needs to take an explicit debug point, because the debug points are explicit in the branches
• TOp.For renamed to TOp.IntegerForLoop
• Expr.Sequential no longer takes a debug point, since they are explicit on each of the leaf parts etc. of the sequential
• expr.Range moved to TypedTree.fs
• implementationExpressionWithSignature renamed to implExprWithSig
• In AbstractIL the overall DebugPoint associated with a method is renamed to DebugRange since that's what it is . However this data is ignored in Portable PDB in any case
• Make some simplifications in the parser to make the calls to mkSynBinding more explicit
• SPAlways and SPSuppress and a whole host of difficult logic for debug point placement is removed from IlxGen.fs

Test baselines

As usual these changes are overly invasive but I've been going through and checking them clearly.

• Some nop are added

Boolean logic

See example below

a.-.Microsoft.Visual.Studio.Administrator.2022-02-04.13-12-49.mp4

for loops

I looked into #12410 and take a deeper look at how we do debugging for for .. in ... do for-each loops, including all the variations we emit

• for-each on GetEnumerator pattern (e.g. a method, not IEnumerable)
• for-each on IEnumerable<_>
• for-each on F# list
• for-each on array
• for-each on span
• for-each on string
• for-each on "1 .. 3" integer ranges
• for-each on "3 .. -1 .. 1" integer ranges
• for-integer "for i = 1 to 3 do"
• for-integer "for i = 3 downto 1 do"

All of which can occur in any of these:

• regular imperative code
• for in async { ... }
• for in task { .. }
• for in seq { ... }
• for in list [ ... ]/array `[| ... |]
• for in other user-defined computation expressions, e.g. cancellable
• for in other user-defined computation expressions using resumable code

I also looked carefully where C# puts debug points. This is a set of fixes related to that.

Before - one debug point across the "for ... do". This is mistaken, as it is triggered multiple times on first loop entry (e.g. when the enumeration is computed) and fails to get the debug points right when the loop body is a single statement. If you look carefully at the video of repeated F10 stepping this shows several bugs (most of them "double hit" on entry)

thebigfileofdebugstepping.-.Microsoft.Visual.Studio.Current.Administrator.2021-11-19.23-51-49.mp4

After: like C#, we now have one debug point for for on loop entry, and one on in for each iteration as elements are drawn from the enumeration. Similarly for for x = 1 to 5 do ... where the debug points are on for and to.

thebigfileofdebugstepping.-.Microsoft.Visual.Studio.Current.Administrator.2021-11-19.23-49-45.mp4

This also

• Cleans up some naming
• Makes debug points available to clients of FSharpExpr

[Happypig375]

for and for! in computation expressions should be checked too.

[dsyme]

for in computation expressions should be checked too.

Yes indeed. There are_ three different cases

• for in async async { ... }
• for in tasks task { .. }
• for in seq seq { ... }
• for in list [ ... ]/array `[| ... |]

I've added all these to the manual test matrix in TheBigFileOfDebugStepping.fsx and went through things carefully again. There are numerous problems revealed by this

1. The stepping in all imperative loops works OK except this one still has a problem:

    let testSimpleForEachListLoopWithOneStatement (inp:int list) =
        for x in inp do
            printfn $"hello, x = {x}"

2. The task, seq, list, async and array all have multiple hits on for and problems with other stepping.

I've marked this WIP until we get those sorted out

[dsyme]

It looks like we also optimize for over string so I will add that to the test matrix

[dsyme]

I have started working on the "async" and other computation expression debugging problems for for loops.

This has been a lot more work than I expected and eventually I decided to work from the ground up, looking at our debugging for very simple computation expressions and indeed for any inlined code, not just for for loops but more generally too.

Anyway, this is still work in progress but I believe I'm making progress slowly.

[dsyme]

Some failing tests:

[dsyme]

I've been going through the codegen changes file by file, marking them for whether the changes are fully expected or not

[dsyme]

I think this should go green now - struggled with one failing test but should be good

[dsyme]

OK, all green now and ready for review

[dsyme]

@KevinRansom @vzarytovskii @brettfo @TIHan @auduchinok Can we have reviews on this? Thanks!

[TIHan]

Ok, so we are not serializing Expr.DebugPoint. Does this mean that inline functions won't have this information when they are used?

[dsyme]

Ok, so we are not serializing Expr.DebugPoint. Does this mean that inline functions won't have this information when they are used?

That's correct, inline data carries no source information at present, and hasn't done so previously.

I did experiments in allowing inline data to carry source, but the call stacks get badly messed up, because the calling stack frame is present but the location is the inline code, and it's really hard to work out what's going on.

[TIHan]

I think there is a missing case here for DebugPointAtLeafExpr.No - which I assume we should just print the inner expression at that point:

| DebugPoint (DebugPointAtLeafExpr.No, e) -> e.ToDebugString(depth + 1)

[dsyme]

There's actually no No case for DebugPointAtLeafExpr

[TIHan]

Changes look good! Just a few comments.

[vzarytovskii]

@dsyme
Question: we skip generating sequence points on the auto-generated methods (for example, for records) now, right?
If yes, shall this be in documentation?

[auduchinok]

@dsyme Please consider a situation where a new FCS is used with an older compiler/SDK or a new compiler is used together with an older FCS. Will ValidateBreakpointLocationImpl still provide good enough breakpoint ranges that would correspond to the actual sequence point in a compiled assembly? Could some of the changes be guarded with a language version check if there're cases where breakpoint ranges and sequence points may mismatch?

[dsyme]

@dsyme Please consider a situation where a new FCS is used with an older compiler/SDK or a new compiler is used together with an older FCS. Will ValidateBreakpointLocationImpl still provide good enough breakpoint ranges that would correspond to the actual sequence point in a compiled assembly? Could some of the changes be guarded with a language version check if there're cases where breakpoint ranges and sequence points may mismatch?

It is hard to tell what we should do here, and in this PR I think we should just focus on getting the experience fully fixed, rather than backwards compat for previous (buggy) versions. I'm confident the ValidateBreakpointLocationImpl will be good enough - what you'll get is debug locations being "extended" when debugging actually attaches from say the first part of a pipeline to the full pipeline, or similarly for boolean logic.

If we were to try to do compat going forward, language version isn't the right thing to switch on - we should switch on the compiler version used to generate the target assembly being debugged. But of course different assemblies in an application could have been made with different compiler versions. C# emits the full DLL verisons and command line compilation switches into the PDB for each assembly, and perhaps some compiler version information as well, I'm not sure. so ValidateBreakpointLocationImpl would need to be passed in this kind of information. But prior to debugging you don't actually have this anyway, so the editor has to "guess" from project settings (which may or may not match the actual target binary, and may not even be available for out-of-project source).

[dsyme]

Question: we skip generating sequence points on the auto-generated methods (for example, for records) now, right? If yes, shall this be in documentation?

Yes, and Yes - I'll add it now

[dsyme]

@vzarytovskii I've updated the docs in this PR, thanks

[auduchinok]

@dsyme Since these expressions are only created during type check stage, could it be better to produce typed tree nodes instead?

[dsyme]

They are produced by desugaring computation expressions, which is done as SyntaxTree -> SyntaxTree. It's not easy to internalise the individual nodes produced

[mlaily]

Hello!

Does this PR fix breakpoints failing to bind in [ for x in xs -> ... ] expressions?

If yes, is there a way to tell in which version (of VS? F#?) this will be released?

[dsyme]

Does this PR fix breakpoints failing to bind in [ for x in xs -> ... ] expressions?

It's a good question - do you have a more specific repro of this problem please?

[mlaily]

It's a good question - do you have a more specific repro of this problem please?

Just tested with the following demo:

[<EntryPoint>]
let main args =

    let test1 () =
        [ 3; 2; 1 ]
        |> List.map (fun x -> x + 10)
        |> List.sort

    let test2 = [
        for x in test1() do
            let xx = x * x
            printf "test"
            xx
        ]

    let test3 = [
        for x in test1() ->
            let xx = x * x
            printf "test"
            xx
        ]

    printf "The end"

Notice that:

• I can add breakpoints in both test2 and test3 before starting the debugger (F5)
• When debugging, only the breakpoints in test2 stop the debugger
• If I remove/re-add a breakpoint, it works in test2, it fails with the message "The breakpoint failed to bind." in test3

I'm using Visual Studio Community 2022 (64-bit) - Version 17.1.0, and it's a .NET 6.0 project.

[dsyme]

@mlaily Thanks for the repro, I'll open a separate issue on this