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

Add artificial stack frame to represent contexts without one #1898

Merged

Conversation

SeeminglyScience
Copy link
Collaborator

Fixes #1897
Fixes PowerShell/vscode-powershell#4103

PR Summary

When stepping into certain contexts like a param block's default value expression, the engine does not provide a call stack frame to represent it. In this scenario we want to create an artificial call stack frame to represent the context we've stepped into.

PR Context

@SeeminglyScience SeeminglyScience requested a review from a team August 16, 2022 20:48
@SeeminglyScience SeeminglyScience changed the title Add artificial stack frame to represent context Add artificial stack frame to represent contexts where the engine does not create one Aug 16, 2022
@SeeminglyScience SeeminglyScience changed the title Add artificial stack frame to represent contexts where the engine does not create one Add artificial stack frame to represent contexts without one Aug 16, 2022
if (!string.IsNullOrEmpty(scriptExtent.File)
&& !PathUtils.IsPathEqual(scriptExtent.File, targetFrame.ScriptPath))
{
await debugInfoHandle.WaitAsync().ConfigureAwait(false);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Normally I'd prefer to push this into the code for FetchStackFramesAndVariablesAsync but that would require giving it more context than it typically needs. Since this should be an edge case rather than the norm, I lean towards this being preferable.

That said I'm open to switching it up and just passing more context to fetch.

/// <see cref="StringComparison.Ordinal" /> for case sensitive file systems and <see cref="StringComparison.OrdinalIgnoreCase" />
/// in case insensitive file systems.
/// </summary>
internal static readonly StringComparison PathComparison = RuntimeInformation.IsOSPlatform(OSPlatform.Linux)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Platform checks are sort of littered throughout the project. It'd be nice to go through and replace these with centralized logic. (It's optimized by the JIT anyway iirc so it's not a perf boost at least outside of Windows PowerShell, but nice for maintenance)

Copy link
Member

Choose a reason for hiding this comment

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

💯 agree!

Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

LGTM!

@andyleejordan andyleejordan enabled auto-merge (squash) August 17, 2022 23:18
When stepping into certain contexts like a param block's default value
expression, the engine does not provide a call stack frame to represent
it. In this scenario we want to create an artifical call stack frame to
represent the context we've stepped into.
@andyleejordan
Copy link
Member

Ah ha! I figured out how to rebase it myself.

@andyleejordan andyleejordan merged commit 27e2d2f into PowerShell:main Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
2 participants