Improve debugging of retail and inline code by not erasing locals and debug points intra-assembly

[dsyme]

I'm thinking of making a change to debugging, locals and inlining

Currently when we inline anything we "erase" locals and debug points completely - that is, we mark locals "compiler generated" (hence they don't show in a deubbger locals window) and remove debug points from the entire code being inlined. This means

1. Release code is often sparse of information, with many breakpoints failing to bind if the code has been inlined.
2. In Debug code, if the user uses ​let inline,​ then they won't hit any breakpoints in that code.

This change experiments with two things

1. Stop removing debug points for intra-assembly inlining. This means some debug points in let inline code may bind (unless they are at code points entirely optimized away)
2. Stop marking inlined locals as "compgen" for intra-assembly inlining. This means extra locals will show in the debug display, particularly at debug points activated by (1) - because that is where those locals will be in scope, if they have survived further optimization.

There is also a pair of bug fixes to InnerLambdasToTopLevelFuncs.fs and DetupleArgs.fs to stop corrupting ranges of applications when optimizing code (correcting very noticeable defects in release mode debugging)

The reason to restrict to intra-assembly is that otherwise the user sees local names and gets request for code from random other libraries which have provided inlined code, even when using F10 stepping. That seems a fundamentally wrong thing to do.

I've been experimenting on this code sample inspired by another bug:

let inline bodyRunner z body =
    let x = 1 + z
    printfn "running"
    body x

let bodyWrapper =
    let x = 1 + System.Random().Next()
    bodyRunner 3 (fun n ->
        printfn "line1, x = %d" x
        printfn "line2, n = %d" n)

let bodyWrapper2 =
    let x = 1 + System.Random().Next()
    bodyRunner 3 <| (fun n ->
        printfn "line1, x = %d" x
        printfn "line2, n = %d" n)

Now lets splatter breakpoints everywhere before launching to make breakpoint requests for each of these locations:

Now let's see what happens when we launch debug and release code with existing and updated (this PR) compilers

Existing compiler, debug

All the breakpoints bind, but those in inline code are not hit

Existing compiler, release

A couple of breakpoints are missing due to inlining. Those in inline code are not hit. This is misleading to the user

Updated compiler, debug

All the breakpoints bind. Those in inline code are hit correctly.

There are still negatives

• The inlined code doesn't show a stack frame and step-out (shift-F11) can be particularly misleading. However that is in the nature of inline code, and .NET debugging doesn't really give us facilities to avoid that AFAIK, I think it is acceptable.

• When breaking in the inlined code, all the locals show for the place where this code is inlined to (which may be very far from the inlined code), see this:

Here two x are shown, one from the function where the code is being inlined into, and one for the inlined code. While it would be ideal to hide those outer-frame identifiers, it's not at all easy to arrange that. Overall I think it's still an improvement though this is a potential source of confusion to the user using inline. However the fact we restrict to intra-assembly means the user is already opt-ing in to inline already. Importantly these locals don't tend to show outside the inline code (because they are scoped correctly to just the inline code).

Updated compiler, release

A couple of breakpoints are missing due to inlining. But the breakpoints in the inline code get hit (except one which has been optimized away). Importantly they get hit consistently even though the code has been inlined twice

[dsyme]

I checked, this is an improvement, it's a breakpoint here:

let productFirstChars = 
    query {
        for p in products do
        select p.ProductName.[0]   <--  breakpoint now covers `p.ProductName.[0]` not just a single `.` (no idea why it was so bad before)
    }

[dsyme]

This change in error location is acceptable. This is a late last-minute check from IlxGen based on inline code which now reports a different range. The error should really be reported during type checking, or the code generation succeed.

[dsyme]

These tests were a bit messed up, just removing this desktop stuff as part of this PR

[dsyme]

These changes are from the cases we expect, here C.F is being inlined and we now have breakpoints for that:

// A code+optimization pattern, see https://github.com/Microsoft/visualfsharp/issues/6532
type C() = 
    static member inline F (?x1: A, ?x2: A) = 
        let count = 0
        let count = match x1 with None -> count | Some _ -> count + 1
        let count = match x2 with None -> count | Some _ -> count + 1
        let attribs = ResizeArray<_>(count)
        match x1 with None -> () | Some v1 -> attribs.Add(v1)
        match x2 with None -> () | Some v2 -> attribs.Add(v2)
        attribs

//Expect rough equivalent of:
//        let d = ResizeArray<_>(0)
//        d
let test() = 
    C.F ()

[dsyme]

This is an improved range, where the breakpoing on p b now covers all of p b not just p

    let (b : bool, l : int64) as t = System.Int64.TryParse "123"
    p b
    p l

[dsyme]

This is now ready, I've added notes in the tests about the various changes to debug points and locals

[dsyme]

@TIHan @vzarytovskii @KevinRansom @jonsequitur THis is ready

[TIHan]

What was the remarkExpr doing before?

[dsyme]

"mark" is an old name for "range" so "remarkExpr" means "re-range expression" meaning replacing the ranges with one specific range. We use the same routine to make some other changes too (locals to compiler generated), and it is always run for inline code.

In the above line, we were needlessly and incorrectly calling "remarkExpr" even when doing simple reduction on applying generic functions (e.g. when optimizing let id x = x in id 3 - here id is generic and the optimizer decides to inline it). We shouldn't be remarking the body of id here. For example consider

let f () = 
    let id x =
        printfn "hello"
        printfn "hello"
        x
    id 3 + id 7

The debug points for the printfn shouldn't disappear in Release code just because id gets inlined (that's assuming it does actually get inlined)

[TIHan]

This looks good, only a few questions.

[dsyme]

This change didn't improve debug stacks the way I hoped in Release and debug inline code in the compiler, and in some ways regressed.

In particular, when the implementation of the Cancellable computation expression in the compiler is inlined, all the stack frames shown now relate to the inline control code of the CE, not the more important places where the code is inlined into.

For example, at the breakpoint below the method name in the stack frame says we're in a closure associated with a routine "ParseAndCheckInputs", but the actual code location is from the inlined Cancellable code. Previously the re-marking of inline code meant that the stack frames shows the causal stack trace. However now the inlining preserves the lines from illib.fs "Cancellable" implementation. So the stack traces is actually less useful than it was before, though in some strict logical sense more accurate (showing the underlying Cancellable implementation details, but not what causes them)

As a result I will either revert this change and consider a new PR. For now we should return to re-marking inline code by default.