Implement PSObject's IConvertible #19170

iSazonov · 2023-02-17T13:10:50Z

PR Summary

Fix #17627
Replace #19071
Pester tests come from #19071

Implement IConvertible interface for PSObject.

Since PSObject can wrap an object of any type the implementation is dynamic too: it calls appropriate methods from PSObject's base class.

PR Context

PR Checklist

iSazonov · 2023-02-17T17:01:17Z

@daxian-dbw Could you please help? I cannot find where fix PSInvokeMemberBinder.
Follow test fails:

$semver = [System.Management.Automation.SemanticVersion]::new(1, 0, 0)
$ser = [System.Management.Automation.PSSerializer]::Serialize($semver)
$des = [System.Management.Automation.PSSerializer]::Deserialize($ser)

$des.<Ctrl-Space> # Shows all IConvertible methods including ToString() but next fails

$des.ToString()

InvalidOperation: Method invocation failed because [Deserialized.System.Management.Automation.SemanticVersion] does not contain a method named 'ToString'.

Get-Error

Exception             :
    Type        : System.Management.Automation.RuntimeException
    ErrorRecord :
        Exception             :
            Type    : System.Management.Automation.ParentContainsErrorRecordException
            Message : Method invocation failed because [Deserialized.System.Management.Automation.SemanticVersion] does not contain a method named 'ToString'.
            HResult : -2146233087
        TargetObject          : ToString
        CategoryInfo          : InvalidOperation: (ToString:String) [], ParentContainsErrorRecordException
        FullyQualifiedErrorId : MethodNotFound
        InvocationInfo        :
            ScriptLineNumber : 1
            OffsetInLine     : 1
            HistoryId        : 4
            Line             : $des.ToString()
            PositionMessage  : At line:1 char:1
                               + $des.ToString()
                               + ~~~~~~~~~~~~~~~
            CommandOrigin    : Internal
        ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1
    TargetSite  :
        Name          : InvokeAdaptedMember
        DeclaringType : System.Management.Automation.Language.PSInvokeMemberBinder, System.Management.Automation, Version=7.3.0.0, Culture=neutral, PublicKeyToken=31bf
3856ad364e35
        MemberType    : Method
        Module        : System.Management.Automation.dll
    Message     : Method invocation failed because [Deserialized.System.Management.Automation.SemanticVersion] does not contain a method named 'ToString'.
    Data        : System.Collections.ListDictionaryInternal
    Source      : System.Management.Automation
    HResult     : -2146233087
    StackTrace  :
   at System.Management.Automation.Language.PSInvokeMemberBinder.InvokeAdaptedMember(Object obj, String methodName, Object[] args) in C:\Users\1\Documents\GitHub\iSazo
nov\PowerShell\src\System.Management.Automation\engine\runtime\Binding\Binders.cs:line 7483
   at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0)
   at System.Management.Automation.Interpreter.DynamicInstruction`2.Run(InterpretedFrame frame) in C:\Users\1\Documents\GitHub\iSazonov\PowerShell\src\System.Managemen
t.Automation\engine\interpreter\DynamicInstructions.Generated.cs:line 141
   at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame) in C:\Users\1\Documents\GitHub\iSazonov\PowerShell\src\Syste
m.Management.Automation\engine\interpreter\ControlFlowInstructions.cs:line 355
TargetObject          : ToString
CategoryInfo          : InvalidOperation: (ToString:String) [], RuntimeException
FullyQualifiedErrorId : MethodNotFound
InvocationInfo        :
    ScriptLineNumber : 1
    OffsetInLine     : 1
    HistoryId        : 4
    Line             : $des.ToString()
    PositionMessage  : At line:1 char:1
                       + $des.ToString()
                       + ~~~~~~~~~~~~~~~
    CommandOrigin    : Internal
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1

ghost · 2023-02-25T02:00:52Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov · 2023-02-28T18:47:13Z

More simple repro:

$a = [psobject]::new()
$a.<Ctrl-Tab>
$a.psobject.<Ctrl-Tab>

SteveL-MSFT · 2023-03-06T17:49:58Z

src/System.Management.Automation/engine/MshObjectIConvertible.cs

+            object obj = PSObject.Base(pso);
+            if (obj is PSObject || obj is not IConvertible value)
+            {
+                ThrowInvalidCastException();


Since this method is only used once here, would it be better to simply have this block throw the exception and not have the workaround?

throw blocks inlining. .Net suggests long term patterns like ArgumentNullException.ThrowIf(). We can benefit from this too.

daxian-dbw

I did some debugging but couldn't find the root cause. It cannot resolve the ToString member from the deserialized PSObject instance. The InstanceMethods member of the PSObject instance also contains different entries comparing with when running in a build without this change.

Specifically, when running $des.ToString, the _hasInstanceMember is false with the changes in this PR while it's true when running without.

PowerShell/src/System.Management.Automation/engine/runtime/Binding/Binders.cs

Lines 5209 to 5212 in 6dfba03

    
           Expression expr = null; 
        
           if (_hasInstanceMember && TryGetInstanceMember(target.Value, Name, out memberInfo)) 
        
           { 
        
               // If there is an instance member, we generate (roughly) the following:

So, something very confusing may happen in the deserialization/serialization code.

Also, I found you can tab complete the IConvertible interface methods on $des, which is a Deserialized.xxx PSObject, but not on a newly created PSObject [psobject]::new(). But $des.ToBoolean() for example will fail too.

daxian-dbw · 2023-03-06T23:14:06Z

src/System.Management.Automation/engine/MshObjectIConvertible.cs

+        public TypeCode GetTypeCode()
+        {
+            object obj = PSObject.Base(this);
+            return obj is PSObject ? TypeCode.Object : Convert.GetTypeCode(obj);
+        }
+
+        /// <inheritdoc/>
+        public bool ToBoolean(IFormatProvider? provider) => GetIConvertibleOrThrow(this).ToBoolean(provider);


All members should be explicit definitions, like bool IConvertible.ToBoolean(IFormatProvider? provider) => ...

However, changing to explicit definitions does NOT address the exception we saw. You get the same error 😦

iSazonov · 2023-03-07T04:28:51Z

I did some debugging but couldn't find the root cause.

Eh, Debugger initializes PowerShell caches and hides real steps. As result we don't see details. :-(

I added Console.WriteLine in the new code and I did not see they are called.

Then I searched for IConvertible and it only exists in PSObject.ToString(). This is not what we need.

I searched for IConvertible in .Net and there is nothing in the C# code either.

So I assumed that the reason is in the .Net reflection code, somewhere in C++.
In other words I assume that PowerShell calls type.GetMembers() somewhere to get the list of members of the type and the result depends on IConvertible. But I don't know where that point is in PowerShell. I hope you know. Perhaps the easiest way to find it is to tab-completion code.

daxian-dbw · 2023-03-07T19:54:22Z

PowerShell calls type.GetMembers() somewhere

You can find that code in the adaptor code, DotnetAdaptor in particular for reflection operations.

daxian-dbw · 2023-03-07T19:57:58Z

The fact that PSObject representing a deserialized .NET object behaves differently from a regular PSObject is very bizarre.

…into iconvertible

iSazonov · 2023-03-08T17:30:47Z

Serialization fix
Serializer uses

PowerShell/src/System.Management.Automation/engine/serialization.cs

Lines 5517 to 5526 in 66a8982

    
           internal static string GetToString(object source) 
        
           { 
        
               Dbg.Assert(source != null, "caller should have validated the information"); 
        
               // fall back value 
        
               string result = null; 
        
               try 
        
               { 
        
                   result = Convert.ToString(source, CultureInfo.InvariantCulture);

to get ToString method representation. It uses Convert.ToString() which uses IConvertible.ToString() on first place
https://github.com/dotnet/runtime/blob/967250cc86b4c613015066b47b882e7424fdb590/src/libraries/System.Private.CoreLib/src/System/Convert.cs#L1922-L1929
This is inside the .Net. This explains why we didn't see this problem in the debugger.
So I had to use PSObject.ToString() method as IConvertible.ToString() implementation.
I think it is more universal and more save than place the fix in GetToString().

Type conversion fix
The problem was in PSObject -> type cast. PowerShell uses compex type converters and one branch involves IConvertible

PowerShell/src/System.Management.Automation/engine/LanguagePrimitives.cs

Line 5674 in 66a8982

if (typeof(IConvertible).IsAssignableFrom(fromType))

Fix is to block the branch if fromType is PSObject so that return previous behavior.

Alternatively, we could change the IConvertible.ToType() implementation, but through the .Net code this would again take us back to the complex PowerShell type conversion code. This seems unpredictable, dangerous (recursion), and most importantly, unreasonable.

Thoughts about other IConvertible methods

The current implementation calls a method of base type (e.g., ToInt32()) for which the TypeCode is known beforehand. We could call the universal LanguagePrimitives.ConvertTo() method, but we cannot implement a dynamic value for TypeCode because of a contradiction - TypeCode must be known before the conversion, but we can only compute it after the conversion. This also contradicts the documentation requirement to return TypeCode.Object for all other types.

GitHub
runtime/Convert.cs at 967250cc86b4c613015066b47b882e7424fdb590 · dotnet/runtime
.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps. - runtime/Convert.cs at 967250cc86b4c613015066b47b882e7424fdb590 · dotnet/runtime

pull-request-quantifier-deprecated · 2023-03-08T18:55:31Z

This PR has 206 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +204 -2
Percentile : 60.6%

Total files changed: 5

Change summary by file extension:
.cs : +182 -2
.ps1 : +22 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

daxian-dbw · 2023-03-13T18:53:59Z

Thinking about this again, I'm not sure if making PSObject implement IConvertible is the right solution. There are many popularly used interfaces out there, so if we implement IConvertible to facilitate the cases where PSObject is wrapping an object that implements IConvertible, are we going to implement other interfaces to facilitate other cases?

I think the answer should be a hard "No" ...

iSazonov · 2023-03-14T06:13:49Z

We started with a specific user case, namely System.Data makes extensive use of IConvertible. This interface is an old legacy, it will never evolve. .Net team says to use more modern APIs. I also found that ASP.NET also uses IConvertible extensively.
So the big question is whether we want to support System.Data and ASP.NET natively (I don't know anything else). If yes, then it's PR worthwhile. If not, it should be closed.

daxian-dbw · 2023-03-14T17:04:54Z

Marked the PR "Engine - WG". Will let the engine WG review.

iSazonov · 2023-03-14T17:15:46Z

For additional information about IConvertible see dotnet/runtime#73074 (comment) and below comments.

ghost · 2023-03-28T02:00:55Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

SeeminglyScience · 2023-03-28T16:46:13Z

The Engine WG discussed this and decided that we should not implement interfaces that may or may not be implemented on the base object.

A primary concern is that a is IConvertible check will return true but a cast will throw. There are also a lot of other interfaces that would need to be implemented if we wanted to go down that path.

Instead we recommend that the original issue is fixed with either:

An implementation of IDynamicInterfaceCastable (which may or may not actually be feasible)
An explicit unwrapping of PSObject in the binder. Possibly as a special case for DataTable, but ideally in a way that matches our current handling when a PSObject wrapped object is an argument in a method.

Implement PSObject's IConvertible

d6a0af6

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 17, 2023

iSazonov requested review from SteveL-MSFT and daxian-dbw February 17, 2023 13:10

iSazonov assigned daxian-dbw Feb 17, 2023

pull-request-quantifier-deprecated bot added the Large label Feb 17, 2023

Fix style issues

3a57fe7

iSazonov requested a review from SeeminglyScience February 17, 2023 16:55

ghost added the Review - Needed The PR is being reviewed label Feb 25, 2023

ghost removed the Review - Needed The PR is being reviewed label Feb 28, 2023

SteveL-MSFT reviewed Mar 6, 2023

View reviewed changes

daxian-dbw reviewed Mar 6, 2023

View reviewed changes

iSazonov added 3 commits March 8, 2023 11:43

Implement PSObject's IConvertible

3299e55

Fix style issues

37bf86a

Merge branch 'iconvertible' of https://github.com/iSazonov/PowerShell …

39d8b7a

…into iconvertible

iSazonov requested review from PaulHigin, TravisEz13, adityapatwardhan and anmenaga as code owners March 8, 2023 06:46

pull-request-quantifier-deprecated bot added Extra Large and removed Large labels Mar 8, 2023

iSazonov added 2 commits March 8, 2023 14:46

Fix ToString()

a2800d7

Fix test

8225d1b

Fix type converter

2ca8ee3

iSazonov force-pushed the iconvertible branch from 2e22636 to 2ca8ee3 Compare March 8, 2023 12:43

pull-request-quantifier-deprecated bot added Large and removed Extra Large labels Mar 8, 2023

Improve GetTypeCode()

8c1df18

Improve GetTypeCode() 2

179c0ef

daxian-dbw added the WG-Engine core PowerShell engine, interpreter, and runtime label Mar 14, 2023

vexx32 assigned SeeminglyScience Mar 20, 2023

ghost added the Review - Needed The PR is being reviewed label Mar 28, 2023

SeeminglyScience closed this Mar 28, 2023

daxian-dbw mentioned this pull request Mar 28, 2023

Fix to unwrap PSObject when setting data to DataRow and DataRowView #19071

Open

13 tasks

iSazonov deleted the iconvertible branch March 28, 2023 17:03

	Expression expr = null;
	if (_hasInstanceMember && TryGetInstanceMember(target.Value, Name, out memberInfo))
	{
	// If there is an instance member, we generate (roughly) the following:

Implement PSObject's IConvertible #19170

Implement PSObject's IConvertible #19170

Uh oh!

Conversation

iSazonov commented Feb 17, 2023

PR Summary

PR Context

PR Checklist

Uh oh!

iSazonov commented Feb 17, 2023

Uh oh!

ghost commented Feb 25, 2023

Uh oh!

iSazonov commented Feb 28, 2023

Uh oh!

SteveL-MSFT Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Mar 7, 2023

Uh oh!

daxian-dbw commented Mar 7, 2023

Uh oh!

daxian-dbw commented Mar 7, 2023

Uh oh!

iSazonov commented Mar 8, 2023 • edited by unfurl-links bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pull-request-quantifier-deprecated bot commented Mar 8, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

daxian-dbw commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 14, 2023

Uh oh!

daxian-dbw commented Mar 14, 2023

Uh oh!

iSazonov commented Mar 14, 2023

Uh oh!

ghost commented Mar 28, 2023

Uh oh!

SeeminglyScience commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daxian-dbw left a comment •

edited

Loading

daxian-dbw Mar 6, 2023 •

edited

Loading

iSazonov commented Mar 8, 2023 •

edited by unfurl-links bot

Loading

daxian-dbw commented Mar 13, 2023 •

edited

Loading