Enable breakpoints in 'with' augmentations for class types

[dedale]

Close #551

I could successfully validate my fix with @dsyme minimal repro code once my F# SDK has been clobbered ✌️

It seems the doc is wrong:

• version is not 5099 but 9055
• I'm not sure about it, but I think that most msbuild commands to test in VS in release are already included in appveyor script. So I guess the procedure might be simplified.

[msftclas]

Hi @dedale, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

[dedale]

[dedale]

Mmm, it seems it works only once. If I disable the breakpoint, I cannot set it back again 😟

[latkin]

Thanks @dedale ! Have you debugged into it to see why subsequent breakpoint sets aren't working?

[dedale]

No, I only tried in release. To debug, I will need two VS instances I guess?
Load fsharp.sln in first instance and attach to second instance?

[latkin]

You should do a debug build of the repo, then launch an instance of VS which loads those bits. The EnableOpenSource project should allow you to do that.

Attach your original instance to the debug instance, then run through the scenario and see if/how setting the second BP fails

[dedale]

OK, I'll try this.

[latkin]

Hi @dedale - I looked into this a bit today. Your change is valid, and indeed fixes a breakpoint-setting bug, but AFAICT it doesn't fix #551 at all 😄

The additional field you are capturing and walking corresponds to the copy expression in a record "copy and update" expression. So whereas one could not previously set a breakpoint here:

type Rec = { X : int }

let copyRec (r1 : Rec) (r2 : Rec) n =
    { 
        (if n % 2 = 0 then
            r1 // <-- try to set BP here
         else
            r2)
        with X = n
    }

With your change this now works.

Before:

After:

Nice job! My only suggestion would be to give this a better name: instead of os, something like copyExprOpt.

[dedale]

Thanks @latkin for your help. I'll amend my commit tonight to update PR.
For #551 I tried to debug but I could not understand more about the bug.

On Thu, Sep 3, 2015, 04:30 Lincoln Atkinson notifications@github.com
wrote:

Hi @dedale https://github.com/dedale - I looked into this a bit today.
Your change is valid, and indeed fixes a breakpoint-setting bug, but AFAICT
it doesn't fix #551 #551
at all [image: 😄]

The additional field you are capturing and walking corresponds to the copy
expression in a record "copy and update" expression. So whereas one could
not previously set a breakpoint here:

type Rec = { X : int }
let copyRec (r1 : Rec) (r2 : Rec) n =
{
(if n % 2 = 0 then
r1 // <-- try to set BP here
else
r2)
with X = n
}

With your change this now works.

Before:
[image: image]
https://cloud.githubusercontent.com/assets/5943573/9649330/544f4d4c-51a8-11e5-99ad-3fd868611e6c.png

After:
[image: image]
https://cloud.githubusercontent.com/assets/5943573/9649332/5f5e816c-51a8-11e5-997e-df1609b206a3.png

Nice job! My only suggestion would be to give this a better name: instead
of os, something like copyExprOpt.

—
Reply to this email directly or view it on GitHub
#608 (comment)
.

[enricosada]

@dedale just change the commit message, the commit is good! 👍 great fix

[latkin]

I went ahead and made the requested changes before merging, thanks!

[dedale]

Ok thanks
Sorry I could not merge earlier.

On Thu, Sep 3, 2015, 18:57 Lincoln Atkinson notifications@github.com
wrote:

Closed #608 #608 via
02ff9c3
02ff9c3
.

—
Reply to this email directly or view it on GitHub
#608 (comment).