Powershell: Unexpected behavior on Import-CliXML for Enum values

[PrzemyslawKlys]

Steps to reproduce

$Groups = Get-ADGroup -filter *
$PrettierGroups = foreach ($Group in $Groups) {
    [PsCustomObject] @{
        'Group Name'     = $Group.Name
        'Group Category' = $Group.GroupCategory
        'Group Scope'    = $Group.GroupScope
        'Group SID'      = $Group.SID.Value
    }
}
$PrettierGroups[0].'Group Category'
$PrettierGroups | Export-Clixml -LiteralPath $PSScriptRoot\Test.xml

$PrettierGroups = Import-Clixml -LiteralPath $PSScriptRoot\Test.xml
$PrettierGroups[0].'Group Category'
if ($PrettierGroups[0].'Group Category'.Value) {
    $PrettierGroups[0].'Group Category'.Value
} else {
    $PrettierGroups[0].'Group Category'
}

MemberType      : NoteProperty
IsSettable      : True
IsGettable      : True
Value           : Security
TypeNameOfValue : Deserialized.Microsoft.ActiveDirectory.Management.ADGroupCategory
Name            : Group Category
IsInstance      : True

While I know above won't work in PS Core yet, it can be reproduced with any enum

$Test = [PSCustomObject]@{ 'DayOfWeek' = [DayOfWeek]::Monday }
$Test.DayOfWeek

$Test | Export-Clixml $PSScriptRoot\test1.xml
$Test = Import-Clixml -LiteralPath $PSScriptRoot\test1.xml
$Test.DayOfWeek

Expected behavior

Monday

Actual behavior

Value 
----- 
Monday

The XML in question has:

<Obj N="DayOfWeek" RefId="1">
  <TN RefId="1">
    <T>System.DayOfWeek</T>
    <T>System.Enum</T>
    <T>System.ValueType</T>
    <T>System.Object</T>
  </TN>
  <ToString>Monday</ToString>
  <I32>1</I32>
</Obj>

So all the data is available for Import-CliXML to rebuild Enum value. Instead of picking up I32 it should pick ToString().

Environment data

Name                           Value
----                           -----
PSVersion                      6.2.1
PSEdition                      Core
GitCommitId                    6.2.1
OS                             Microsoft Windows 10.0.18362 
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}        
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

I did write a blog post about it: https://evotec.xyz/export-clixml-and-import-clixml-serialization-woes/ with some workarounds as I need this working on 5.1 as well. Hoping in PowerShell 7 it could be addressed. I have no clue how hard it would be or if it's even possible. Maybe it's supposed to be that way but it did waste me long hours trying to find what I'm doing wrong.

[janegilring]

I also ran into the same issue today, the last command should return all stopped services:

Works as expected

Get-Service | where status -eq Stopped

Returns all objects

Get-Service | Export-Clixml C:\temp\services.xml
Import-Clixml C:\temp\services.xml

Returns nothing

Import-Clixml C:\temp\services.xml | where status -eq Stopped`

Tested on both 5.1, 6.0 and 7.0-preview3.

[SeeminglyScience]

I looked into this a bit.

The deserialized enum value is a PSObject whose BaseObject is the underlying value of the enum. For instance [DayOfWeek]::Friday would be a PSObject wrapped around the int "5". The PSObject wrapper has a field called toStringFromDeserialization, that value in this case holds the enum name "Friday".

Since the deserialized object's ToString() returns 'Friday', I would expect that this would work. However in the PSToStringBinder, if the target is a PSObject that is wrapping a real value (like 5), then the PSObject is unwrapped and the binder is reran with the unwrapped value.

This is a problem not just with this specific case but any time an ETS member could affect the outcome of a binder.

Ideally, ETS members would be considered in all binders. However, one possible solution to fix this specific case would be to check for toStringFromDeserialization prior to DeferToPSObject is called.

Here's the current code in PSToStringBinder:

https://github.com/PowerShell/PowerShell/blob/096a78fbe3fa9997b1a4ab4e8f0014317ac1e3fe/src/System.Management.Automation/engine/runtime/Binding/Binders.cs#L1073-L1076

That could be changed to something like this: (untested)

if (target.Value is PSObject pso && (PSObject.Base(target.Value) != target.Value))
{
    if (pso.ToStringFromDeserialization != null)
    {
        // Should be in CachedReflectionInfo instead
        var toStringFromDeserialization = typeof(PSObject).GetProperty(
            nameof(PSObject.ToStringFromDeserialization),
            BindingFlags.Instance | BindingFlags.NonPublic);

        var toStringValue = Expression.Property(target.Expression, toStringFromDeserialization);

        var bindingRules = BindingRestrictions.GetExpressionRestriction(
            Expression.NotEqual(
                toStringValue,
                Expression.Constant(null, typeof(string))));

        return new DynamicMetaObject(toStringValue, bindingRules).WriteToDebugLog(this);
    }

    return this.DeferForPSObject(target, args[0]).WriteToDebugLog(this);
}

Ideally this would be fixed for all binders and for all ETS members, but this would be the least disruptive way to fix deserialized enums

[vexx32]

@daxian-dbw can we get your thoughts on this?

More than happy to borrow that from @SeeminglyScience and submit a PR if he doesn't want to handle that at the moment, just want to get your thoughts on it first and confirm this is the way we want to go here. 🙂

[daxian-dbw]

This is a problem not just with this specific case but any time an ETS member could affect the outcome of a binder.
Ideally, ETS members would be considered in all binders.

Agreed. I admit there are problems on how to-string conversion is handled in PowerShell -- it lacks consistency. Not just the PSObject generated by deserialization, but also the ToString method that overrides the default ToString on the base object.
Example:

$a = 1
$a | Add-Member -MemberType ScriptMethod -Name ToString -Value { "Can you see me?" } -Force
$a
> Can you see me?
'Can you see me?' -eq $a
> False
"{0}" -f $a
> 1
$a, $a -join ";"
> Can you see me?;Can you see me?
$a -match 'can'
> False

This looks to me a bigger problem cross the board ...
As for the fix proposed by @SeeminglyScience, it's not a general fix even thought it would address some of the issues related to the PSObject created by deserialization.

Besides, by looking at the code, I think the proposed fix won't address what is specifically reported in this issue, namely where status -eq 'Stop' or where { $_.Status -eq 'Stop' }.

[SeeminglyScience]

This looks to me a bigger problem cross the board ...

Yeah I didn't get into trying to explain how to fix that because I think it's even more broad than just ToString. For instance, some operators don't seem to account for ETS registered conversions at all:

Add-Type -TypeDefinition '
    using System;
    using System.Management.Automation;

    namespace CustomThing
    {
        public class CustomTypeConverter : PSTypeConverter
        {
            public override bool CanConvertFrom(object sourceValue, Type destinationType)
            {
                return true;
            }

            public override object ConvertFrom(object sourceValue, Type destinationType, IFormatProvider formatProvider, bool ignoreCase)
            {
                return 10;
            }

            public override bool CanConvertTo(object sourceValue, Type destinationType)
            {
                return true;
            }

            public override object ConvertTo(object sourceValue, Type destinationType, IFormatProvider formatProvider, bool ignoreCase)
            {
                return 10;
            }
        }
    }'

Update-TypeData -TypeName System.Text.StringBuilder -TypeConverter CustomThing.CustomTypeConverter -Force

$sb = [System.Text.StringBuilder]::new()

# This actually works and returns true which I didn't expect.
10 -eq $sb

# These don't though.
10 + $sb
10 - $sb
10 / $sb
10 * $sb

Besides, by looking at the code, I think the proposed fix won't address what is specifically reported in this issue, namely where status -eq 'Stop' or where { $_.Status -eq 'Stop' }.

Aww shoot you're right 🙁. I had the RHS and LHS switched in my head. That makes this a bit more complicated...

[daxian-dbw]

# These don't though.
10 + $sb
10 - $sb
10 / $sb
10 * $sb

I'm not convinced if this even should work.
For 10 -eq $sb, the RHS is converted to the type of the LHS in order to do the comparison. This is by design. But it's not necessary that we shall do so for all binary operations (for string type RHS, it makes sense to try converting, but I'm not sure we should do it for other types).

[daxian-dbw]

That makes this a bit more complicated...

What makes it even more complicated is the fact that when PSObject is the target for a binding operation, it will be unwrapped before sending to the real binder. So when you reach in a FallbackBindXXXX method, the target is not a PSObject anymore. This may make things tricky for some of the binding operation.

[SeeminglyScience]

# These don't though.
10 + $sb
10 - $sb
10 / $sb
10 * $sb

I'm not convinced if this even should work.
For 10 -eq $sb, the RHS is converted to the type of the LHS in order to do the comparison. This is by design. But it's not necessary that we shall do so for all binary operations (for string type RHS, it makes sense to try converting, but I'm not sure we should do it for other types).

I figured it would work since 10 + '10' returns 20. Maybe that's an explicit exception but it feels like it should work to me 🤷‍♂

That makes this a bit more complicated...

What makes it even more complicated is the fact that when PSObject is the target for a binding operation, it will be unwrapped before sending to the real binder. So when you reach in a FallbackBindXXXX method, the target is not a PSObject anymore. This may make things tricky for some of the binding operation.

Well... that's a... that's an oof.

The prospect of changing that method is... not very appealing. Especially since this is the first I've seen this actually brought up. Though I will admit, I've generally steered clear of ETS because it never felt consistent. I'm starting to think a lot of that feeling of inconsistency is related, and a fix might go a long way to making it feel more solid.

I'm still confident that it's worth fixing, sure does make me nervous though.

[SeeminglyScience]

@daxian-dbw PSGetEnumerableBinder is another one. Not a huge deal obviously, but it would be neat if we could customize/define enumeration via ETS.