Make PowerShell Core enumerate COM collections by daxian-dbw · Pull Request #4553 · PowerShell/PowerShell · GitHub

Fix #3775

'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.

LGTM with minor comment

Add extra line at the end.

Added a new line.

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.

Should we check comObject != null ?

Will add a null check here.

Closed.

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.

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.

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

We have new commits - please take another look.

Jason Shirk (@lzybkr) Aditya Patwardhan (@adityapatwardhan) We have new commits - please take another look.

LGTM

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?

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

Fixed.

Jason Shirk (@lzybkr) Can you approve?

Ilya (@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.

Dongbo Wang (@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.

Ilya (@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.

Dongbo Wang (@daxian-dbw) I wonder why CI AppVeyor failed?

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.

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

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

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

Dongbo Wang (@daxian-dbw) Merged.
Great work! Thanks!

Ilya (@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.

Dongbo Wang (@daxian-dbw) Thanks for sharing your experience!

Make PowerShell Core enumerate COM collections

Dongbo Wang (daxian-dbw) added 2 commits August 10, 2017 18:22

Make PowerShell Core enumerate COM collections

15d6ce2

Add tests

3ee11d8

Dongbo Wang (daxian-dbw) assigned Ilya (iSazonov) Aug 11, 2017

Dongbo Wang (daxian-dbw) requested review from Aditya Patwardhan (adityapatwardhan) and Ilya (iSazonov) August 11, 2017 17:10

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Aug 11, 2017

Skip COM tests on Unix

d49de32

Jason Shirk (lzybkr) previously approved these changes Aug 11, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) reviewed Aug 11, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) previously approved these changes Aug 11, 2017

View reviewed changes

Dongbo Wang (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

Ilya (iSazonov) reviewed Aug 12, 2017

View reviewed changes

Address 2 more comments

5490f76

Aditya Patwardhan (adityapatwardhan) approved these changes Aug 15, 2017

View reviewed changes

Ilya (iSazonov) reviewed Aug 15, 2017

View reviewed changes

Dongbo Wang (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

Dongbo Wang (daxian-dbw) requested review from Bruce Payette (BrucePay), James Truher (JamesWTruher), Travis Plunk (TravisEz13) and sergei (vors) as code owners August 15, 2017 19:25

Dongbo Wang (daxian-dbw) removed request for Bruce Payette (BrucePay) and sergei (vors) August 15, 2017 19:26

Dongbo Wang (daxian-dbw) removed request for James Truher (JamesWTruher) and Travis Plunk (TravisEz13) August 15, 2017 19:26

Ilya (iSazonov) approved these changes Aug 16, 2017

View reviewed changes

James Truher (JamesWTruher) reviewed Aug 16, 2017

View reviewed changes

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

Replace shell-windows tests with shell-folder tests

7a06a82

Minor change in test

459518d

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

Dongbo Wang (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

Dongbo Wang (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

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Make PowerShell Core enumerate COM collections (PowerShell#4553 )

cc58456

Make PowerShell Core enumerate COM collections

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

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

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

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

Make PowerShell Core enumerate COM collections by daxian-dbw · Pull Request #4553 · PowerShell/PowerShell · GitHub

Uh oh!

Make PowerShell Core enumerate COM collections - #4553

Make PowerShell Core enumerate COM collections#4553Ilya (iSazonov) merged 11 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom daxian-dbw:COMdaxian-dbw/PowerShell:COMCopy head branch name to clipboard

Conversation

Dongbo Wang (daxian-dbw) commented Aug 11, 2017

Uh oh!

Aditya Patwardhan (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!

Dongbo Wang (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!

Ilya (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!

Dongbo Wang (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!

Dongbo Wang (daxian-dbw) commented Aug 14, 2017

Uh oh!

Ilya (iSazonov) commented Aug 15, 2017

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (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!

Ilya (iSazonov) commented Aug 16, 2017

Uh oh!

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

Uh oh!

Uh oh!

Ilya (iSazonov) commented Aug 16, 2017

Uh oh!

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 16, 2017

Uh oh!

Ilya (iSazonov) commented Aug 17, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 17, 2017

Uh oh!

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

Uh oh!

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 17, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Aug 17, 2017

Uh oh!

Make PowerShell Core enumerate COM collections#4553
Ilya (iSazonov) merged 11 commits into
PowerShell:masterfrom
daxian-dbw:COM

Ilya (iSazonov) Aug 15, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) Aug 15, 2017 •
edited
Loading

Ilya (iSazonov) Aug 15, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) commented Aug 16, 2017 •
edited
Loading

Ilya (iSazonov) commented Aug 17, 2017 •
edited
Loading