Make PowerShell Core enumerate COM collections #4553

daxian-dbw · 2017-08-11T17:10:22Z

'GetEnumerator()' is not supported on COM collections in .NET Core (see https://github.com/dotnet/corefx/issues/19731), so we need to have our own implementation to enumerate COM collections.

adityapatwardhan

LGTM with minor comment

adityapatwardhan · 2017-08-11T18:18:35Z

src/System.Management.Automation/engine/COM/ComUtil.cs

+
+            return enumVariant != null ? new ComEnumerator(enumVariant) : null;
+        }
+    }


Add extra line at the end.

Added a new line.

daxian-dbw · 2017-08-11T23:02:00Z

I should use Measure-Object in the test but instead used Measure-Command, which pops for mandatory parameter input and thus caused the build to hang. It's fixed now.

iSazonov · 2017-08-12T19:52:24Z

src/System.Management.Automation/engine/COM/ComUtil.cs

+
+        internal static ComEnumerator Create(object comObject)
+        {
+            if (!comObject.GetType().IsCOMObject) { return null; }


Should we check comObject != null ?

Will add a null check here.

iSazonov · 2017-08-12T19:55:15Z

src/System.Management.Automation/engine/COM/ComUtil.cs

+                }
+            }
+
+            return enumVariant != null ? new ComEnumerator(enumVariant) : null;


Maybe use else to exclude duplicate check with line 399?

It's not a duplicate check, line 416 could make enumVariant not null or continue to be null.

I meant that here slightly hidden logic. Maybe:

// if IEnumVARIANT interface var enumVariant = comObject as COM.IEnumVARIANT; if (enumVariant != null) { return new ComEnumerator(enumVariant); } // if a collection var enumerable = comObject as IEnumerable; ... enumVariant = result as COM.IEnumVARIANT; if (enumVariant != null) { return new ComEnumerator(enumVariant); } ... return null; // any warnings for users?

This is more of a coding style rather than an issue that needs to be corrected.

Wait, I see your point now -- there will be one more null check if the COM object is already an enum variant. Will update the code.

Code updated.

Thanks for understanding and sorry that I was unclear.

iSazonov · 2017-08-12T19:58:23Z

test/powershell/engine/COM/COM.Basic.Tests.ps1

+    Describe 'Basic COM Tests' -Tags "CI" {
+        It "Should enumerate ShellWindows" {
+            $shell = New-Object -ComObject "Shell.Application"
+            $windows = $shell.Windows()


Is it work on Core and IoT?

Good catch. NanoServer and IoT don't have the shell, so these tests don't apply. Will update.

daxian-dbw · 2017-08-14T17:48:58Z

@iSazonov I have addressed your comments. Please take another look. Thanks!

We have new commits - please take another look.

iSazonov · 2017-08-15T05:34:32Z

@lzybkr @adityapatwardhan We have new commits - please take another look.

adityapatwardhan

LGTM

iSazonov · 2017-08-15T06:18:02Z

src/System.Management.Automation/engine/COM/ComUtil.cs

+                }
+            }
+
+            return enumVariant != null ? new ComEnumerator(enumVariant) : null;


I meant that here slightly hidden logic. Maybe:

// if IEnumVARIANT interface var enumVariant = comObject as COM.IEnumVARIANT; if (enumVariant != null) { return new ComEnumerator(enumVariant); } // if a collection var enumerable = comObject as IEnumerable; ... enumVariant = result as COM.IEnumVARIANT; if (enumVariant != null) { return new ComEnumerator(enumVariant); } ... return null; // any warnings for users?

iSazonov · 2017-08-15T06:19:03Z

src/System.Management.Automation/engine/COM/ComUtil.cs

+                    }
+                    catch (Exception)
+                    {
+                        /* Catch all exception. */


Obvious comment and typo (exception -> exceptions). Catch all exceptions - why? Please enhance the comment or remove.

iSazonov · 2017-08-16T12:53:38Z

@lzybkr Can you approve?

daxian-dbw · 2017-08-16T16:43:27Z

@iSazonov Jason probably doesn't have time to come back and approve it again, so it's up to the maintainer's judgment to see if it's necessary for reviewers who had approved to approve again. There are some guidelines you can use, for example:

are the changes after the previous approval fundamental or just touch-ups
are the changes to address comments left by the reviewer who previously approved

Certainly, you will get your own guidelines as you become more experienced in this role.

iSazonov · 2017-08-16T16:56:25Z

@daxian-dbw Thank you for the helpful comment!

In this case, it's more of a courtesy - I have to show respect and ask him after I formally dismissed his approval.

test/powershell/engine/COM/COM.Basic.Tests.ps1

daxian-dbw · 2017-08-16T17:46:41Z

@iSazonov Another related comment. I usually don't dismiss a reviewer's approval unless the PR needs some fundamental changes (e.g. design flaw, logic changes and etc.). If the following-up changes are touch-ups or minor fixes (like typo, a null condition check, test fix-up), I think it's fine to keep the previous approval, and once the minor issues are addressed, you can approve the PR and merge it.

iSazonov · 2017-08-17T03:56:05Z

@daxian-dbw I wonder why CI AppVeyor failed?

daxian-dbw · 2017-08-17T05:41:38Z

I have investigated the failure, it actually a bug in Binder, more specifically at here, the restriction generated in the call to this.DeferForPSObject doesn't respect the fact that the baseobject needs to be a COM object.

A repro would be:

$shell = New-Object -ComObject "Shell.Application"
$folder = $shell.Namespace("F:\tmp")
#$item = $folder.Items().Item(0)
$item = $folder.Items() | select -Last 1
$item.Name


$members = new-object System.Collections.ObjectModel.Collection[System.Management.Automation.PSMemberInfo]
$n=new-object Management.Automation.PSNoteProperty a,1
$members.Add($n)
$r=add-member -InputObject a -MemberType MemberSet -Name Name -Value $members -passthru
$r.Name.a

The $r.Name.a is supposed to return 1. If you run the lower part first then upper part, then all work fine.
This happens when a COM object is wrapped into a PSObject, which is the case when the COM object is returned from a pipeline, like the above repro code.

I cannot open an issue until this PR is merged because the repro requires enumerating a COM collection.
I have updated the test to not access Name member, so it would be good now. When I resolve the bug, I will update the COM basic test to keep accessing the Name member.

iSazonov · 2017-08-17T07:24:09Z

@daxian-dbw Maybe made two commits for code and tests before rebase and merge?

daxian-dbw · 2017-08-17T16:07:40Z

@iSazonov Please just choose 'Squash and merge'. It's a simple fix and there is no need to keep history commits.

daxian-dbw · 2017-08-17T16:17:17Z

A follow-up of the binder bug:
The issue repros without COM enumeration. #4607 opened to track it.

iSazonov · 2017-08-17T16:24:33Z

@daxian-dbw Merged.
Great work! Thanks!

daxian-dbw · 2017-08-17T16:44:04Z

@iSazonov Thank you for the review and good work as a maintainer 😄

Here are some of my experiences to choose among Squash and merge, Rebase and merge and Create a merge commit:

number of meaningful commits (commits that are part of the PR work, or commits to address major comments that require non-trivial design/logic changes) is one thing to consider
the complexity of the PR work is also something to consider.

Some PR may involve many commits, but the fix or feature work is not complex, in those cases, just Squash and merge.
Some PR may have clean history commits, but again, the fix/work is simple, in those cases, Squash and merge
A PR may be complex work, and the history is not clean -- complex work usually means a lot major changes during the review process -- in such case, choose create a merge commit to preserve the history even though it's not clean. Example: Native pipe #2450
A PR is non-trivial and the history is clean, then it's worthy to keep the history. In this case, consider using rebase and merge to keep the main branch history clean if the number of commits is not too many (~5/6). Otherwise, use create a merge commit.
A PR may contain more than 1 fix, in this case we usually should ask the author to break it into two PRs.

As you can see from the main branch history, most of time we use (and prefer to) squash and merge.

As for what is a simple/complex/non-trivial fix/work, it's hard to describe but you will have your judgement call as you gain more expeirences.

iSazonov · 2017-08-17T18:04:27Z

@daxian-dbw Thanks for sharing your experience!

daxian-dbw added 2 commits August 10, 2017 18:22

Make PowerShell Core enumerate COM collections

15d6ce2

Add tests

3ee11d8

daxian-dbw assigned iSazonov Aug 11, 2017

daxian-dbw requested review from adityapatwardhan and iSazonov August 11, 2017 17:10

msftclas added the cla-not-required label Aug 11, 2017

Skip COM tests on Unix

d49de32

lzybkr previously approved these changes Aug 11, 2017

View reviewed changes

adityapatwardhan reviewed Aug 11, 2017

View reviewed changes

adityapatwardhan previously approved these changes Aug 11, 2017

View reviewed changes

daxian-dbw and others added 2 commits August 11, 2017 15:28

Add a new line to the end of ComUtil.cs

4d6e365

Fix failing test

3b53c0f

iSazonov reviewed Aug 12, 2017

View reviewed changes

Address 2 more comments

5490f76

adityapatwardhan approved these changes Aug 15, 2017

View reviewed changes

iSazonov reviewed Aug 15, 2017

View reviewed changes

daxian-dbw added 3 commits August 15, 2017 09:19

Add more comment for the catch-all-exception block

1d0ce46

Add comment block for the method

50ae76c

Address another comment and add one more test

864a58d

daxian-dbw requested review from BrucePay, JamesWTruher, TravisEz13 and vors as code owners August 15, 2017 19:25

daxian-dbw removed request for BrucePay and vors August 15, 2017 19:26

daxian-dbw removed request for JamesWTruher and TravisEz13 August 15, 2017 19:26

iSazonov approved these changes Aug 16, 2017

View reviewed changes

JamesWTruher reviewed Aug 16, 2017

View reviewed changes

test/powershell/engine/COM/COM.Basic.Tests.ps1 Show resolved Hide resolved

Replace shell-windows tests with shell-folder tests

7a06a82

Minor change in test

459518d

iSazonov merged commit befc5f8 into PowerShell:master Aug 17, 2017

daxian-dbw deleted the COM branch August 17, 2017 16:24

This was referenced Feb 7, 2020

Fix how COM objects are enumerated #11795

Merged

Code OK on V5.1 does not run on V7.0.0RC2 #11782

Closed

daxian-dbw mentioned this pull request Jul 23, 2020

Remove ComEnumerator as COM objects using IEnumerator is now supported in .NET 5 #13259

Merged

14 tasks

iRon7 mentioned this pull request Jun 24, 2023

ComObject Member-access enumeration #19854

Open

Make PowerShell Core enumerate COM collections #4553

Make PowerShell Core enumerate COM collections #4553

Uh oh!

Conversation

daxian-dbw commented Aug 11, 2017

Uh oh!

adityapatwardhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Aug 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Aug 15, 2017 • 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 Aug 15, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Aug 14, 2017

Uh oh!

iSazonov commented Aug 15, 2017

Uh oh!

adityapatwardhan left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov Aug 15, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 16, 2017

Uh oh!

daxian-dbw commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Aug 16, 2017

Uh oh!

Uh oh!

daxian-dbw commented Aug 16, 2017

Uh oh!

iSazonov commented Aug 17, 2017

Uh oh!

daxian-dbw commented Aug 17, 2017

Uh oh!

iSazonov commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Aug 17, 2017

Uh oh!

daxian-dbw commented Aug 17, 2017

Uh oh!

iSazonov commented Aug 17, 2017

iSazonov Aug 15, 2017 •

edited

Loading

daxian-dbw Aug 15, 2017 •

edited

Loading

iSazonov Aug 15, 2017 •

edited

Loading

daxian-dbw commented Aug 16, 2017 •

edited

Loading

iSazonov commented Aug 17, 2017 •

edited

Loading