Skip to content

Fix uncaught exception when SafeToString returns null #1140

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

Merged
merged 1 commit into from
Jan 3, 2020

Conversation

jborean93
Copy link
Contributor

@jborean93 jborean93 commented Dec 23, 2019

Fixes PowerShell/vscode-powershell#2386

Some values return null from .ToString(). This PR makes sure we check for null and return the type information if it is null.

A simple way to test this out is to store a var like $var = [NullString]::Value and debug the script with breakpoints. The powershell extension will error out.

@jborean93 jborean93 requested a review from rjmholt as a code owner December 23, 2019 23:33
@TylerLeonhardt
Copy link
Member

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

Complexity increasing per file
==============================
- src/PowerShellEditorServices/Services/DebugAdapter/Debugging/VariableDetails.cs  1
         

Clones added
============
- test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs  4
         

See the complete overview on Codacy

@@ -815,6 +815,34 @@ public async Task DebuggerVariableHashtableDisplaysCorrectly()
this.powerShellContext.AbortExecution();
}

[Fact]
public async Task DebufferVariableNullStringDisplaysCorrectly()
Copy link
Member

Choose a reason for hiding this comment

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

These tests don't actually get run in CI as of right now because of the large migration but you don't need to worry about that. I'm going to enable them again soon.

Thanks for adding this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to run it locally but honestly wasn't sure if it actually ran, had no failures (that were related to this though).

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@jborean93
Copy link
Contributor Author

Just ran the following locally on powershell-preview in vscode

$automationNull = [System.Management.Automation.Internal.AutomationNull]::Value
$dbNull = [DBNull]::Value
$nullString = [NullString]::Value
$nullValue = $null

# Breakpoint here
$a = ''

The debugger values are

image

I'm not aware of any other null like values, happy to test any other if they exist. In any case $automationNull and $dbNull are an empty string because that is what .ToString() returns. $nullString is [NullString] based on the changes here because .ToString() returns null. And finally $nullValue already returns $null due to pre-existing code checks.

Thanks @TylerLeonhardt for guiding me through this process.

@TylerLeonhardt TylerLeonhardt merged commit d46589e into PowerShell:master Jan 3, 2020
@jborean93 jborean93 deleted the nullvalue-string branch January 3, 2020 06:40
@rjmholt
Copy link
Contributor

rjmholt commented Jan 3, 2020

I'm not aware of any other null like values

For reference: https://github.com/PowerShell/PowerShell/blob/08d6be90397e382c808dbc347b8e4ae459239f53/src/System.Management.Automation/engine/LanguagePrimitives.cs#L1068-L1074

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.

Preview extension 2019.12.0 crashes when debugging some scripts
3 participants