Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial switch to breakpoint APIs #1119

Merged

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Dec 2, 2019

This moves to the new breakpoint APIs in PS 7 which enables the ability to debug actual untitled files!

This will also allow things like Azure Functions to not need a Wait-Debugger in their script.

I also accidentally supported Log Points!

To do this, I created a new service BreakpointService that helps manage this.

I took a bunch of the reflection code from @rjmholt's branch of a similar feature.

NOTE: We should probably discuss the Function breakpoints feature... Today, it works in all versions of PowerShell by creating a temp file on the fly and using that for debugging. That doesn't seem like the best approach... because temp files could go south. Ideally we should use same strategy that I'm doing here for function breakpoints as well... but I feel like that's out of scope for this PR.

Testing checklist

lauch and attaches

  • Launch current file
  • Launch current file temp console
  • attach to process 6.2 to 6.2 (to test legacy behavior)
  • attach to process 7.0 to 7.0 (to test new behavior)
  • attach to process 7.0 to 6.2 (to test legacy behavior)
  • attach to process 6.2 to 7.0 (to test legacy behavior)
  • all breakpoint types

Untitled files

  • Launch current file
  • Launch current file temp console can't be supported without a language server in the temp session... so fail nicely
  • all breakpoint types

Breakpoint types

  • Regular
  • Hit count
  • expression
  • Log points (new!)

Other

  • Watch expresssions

@TylerLeonhardt
Copy link
Member Author

Ah looks like I never wired up Actions at all :) still need to do that

@TylerLeonhardt
Copy link
Member Author

TylerLeonhardt commented Dec 4, 2019

Honestly... After doing this, I kinda wanna rip out the BreakpointDetails types. That will simplify the code I think.

As it is, Action breakpoints dont work at all.

@rkeithhill
Copy link
Collaborator

rkeithhill commented Dec 4, 2019

I might be remembering this wrong but in the old architecture, there was a bit of a wall between the protocol handlers specific to VSCode/LSP/DAP and the backend PSES "services" which could be used elsewhere apart from the LSP/DAP protocol. But since then the VSCode LSP is now standard (maybe DAP will be eventually) perhaps that separation isn't as important if you expect everyone to use PSES via LSP/DAP.

@rjmholt
Copy link
Collaborator

rjmholt commented Dec 4, 2019

Honestly... After doing this, I kinda wanna rip out the BreakpointDetails types. That will simplify the code I think.

I had the same thought -- think it's the right way to go

@TylerLeonhardt TylerLeonhardt changed the title Initial switch to breakpoint APIs WIP: Initial switch to breakpoint APIs Dec 11, 2019
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had two pending review comments apparently, also here's what's up with the hang on older versions.

string globalHitCountVarName =
$"$global:{s_psesGlobalVariableNamePrefix}BreakHitCounter_{breakpointHitCounter}";

builder.Insert(0, $"if (++{globalHitCountVarName} -eq {parsedHitCount}) {{ ")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is out of scope of this PR, but man I'd love to see these global's removed. We should have a "visible but internal" static API that manages stuff like this I think. Something like:

namespace Whatever.EditorServices.Internal
{
    [EditorBrowsable(EditorBrowsableState.Never)]
    public static class BreakpointStateManager
    {
        [Hidden, EditorBrowsable(EditorBrowsableState.Never)]
        public static bool TestHitCount(string hitCountKeyName, int parsedHitCount);
    }
}

@TylerLeonhardt TylerLeonhardt changed the title WIP: Initial switch to breakpoint APIs Initial switch to breakpoint APIs Jan 31, 2020
@TylerLeonhardt
Copy link
Member Author

I'm removing the WIP. Things seem to be working.

@@ -18,6 +18,7 @@
using Microsoft.PowerShell.EditorServices.Services.TextDocument;
using Microsoft.PowerShell.EditorServices.Services.PowerShellContext;
using Microsoft.PowerShell.EditorServices.Services.DebugAdapter;
using System.Collections.Concurrent;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this being used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

@PowerShell PowerShell deleted a comment from rjmholt Feb 4, 2020
@PowerShell PowerShell deleted a comment from SeeminglyScience Feb 4, 2020
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple requests.

Also I didn't see it in the list, but make sure watch expressions are tested in 7.

@@ -333,6 +341,12 @@ public async Task<Unit> Handle(PsesAttachRequestArguments request, CancellationT
string debugRunspaceCmd;
if (request.RunspaceName != null)
{
var ids = await _powerShellContextService.ExecuteScriptStringAsync($"Get-Runspace -Name {request.RunspaceName} | % Id");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be switched to PSCommand?

new PSCommand()
    .AddCommand("Microsoft.PowerShell.Utility\\Get-Runspace")
    .AddParameter("Name", request.RunspaceName)
    .AddCommand("Microsoft.PowerShell.Utility\\Select-Object")
    .AddParameter("ExpandProperty", "Id")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@TylerLeonhardt
Copy link
Member Author

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 4
           

Complexity increasing per file
==============================
- src/PowerShellEditorServices/Services/DebugAdapter/Debugging/BreakpointApiUtils.cs  10
- src/PowerShellEditorServices/Services/DebugAdapter/Handlers/LaunchAndAttachHandler.cs  2
- src/PowerShellEditorServices/Services/DebugAdapter/Handlers/BreakpointHandlers.cs  1
- src/PowerShellEditorServices/Services/DebugAdapter/BreakpointService.cs  13
         

Clones removed
==============
+ src/PowerShellEditorServices/Services/DebugAdapter/DebugService.cs  -2
         

See the complete overview on Codacy

@TylerLeonhardt TylerLeonhardt merged commit aa00910 into PowerShell:master Feb 10, 2020
@TylerLeonhardt TylerLeonhardt deleted the switch-to-breakpoint-apis branch February 10, 2020 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants