Skip to content

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Jun 26, 2018

PR Summary

The ConvertToClassInfo method attempts to convert a PowerShell class into a PSClassInfo. When it encounters a hidden property, it fails as the PropertyType member is null. Fix is to ignore hidden members to allow it to succeed.

Looks like the bug has always been there, but exposed by #6025

Fix #7187

PR Checklist

Copy link
Collaborator

@BrucePay BrucePay left a comment

Choose a reason for hiding this comment

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

LGTM

{
PropertyMemberAst propAst = member as PropertyMemberAst;
if (propAst != null)
if (propAst != null && (propAst.PropertyAttributes & PropertyAttributes.Hidden) != PropertyAttributes.Hidden)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use 'is' : if (member is PropertyMemberAst propAst && (propAst.PropertyAttributes & PropertyAttributes.Hidden) != PropertyAttributes.Hidden)

}

It "Get-Help should not fail searching for class help" {
Get-Help -Category Class | Should -BeNullOrEmpty
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test can take a very long time. Can we use -Name parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we don't ship any script classes, this operation doesn't take very long. It's only hit when the test modules are in your PSModulePath specifically the PSLogItem class. If you run this test directly w/o using build.psm1 which adds the test modules to your path, this test will fail as it wouldn't find PSLogItem class. I think for now we can just leave it as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a class in the test to exclude problems in future?
I run on Windows PowerShell - it is very long.

Copy link
Member Author

Choose a reason for hiding this comment

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

@iSazonov that's a reasonable request. I was testing on macOS and it's pretty fast.

{
PropertyMemberAst propAst = member as PropertyMemberAst;
if (propAst != null)
if (member is PropertyMemberAst propAst && (propAst.PropertyAttributes & PropertyAttributes.Hidden) != PropertyAttributes.Hidden)
Copy link
Member

Choose a reason for hiding this comment

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

(propAst.PropertyAttributes & PropertyAttributes.Hidden) != PropertyAttributes.Hidden

This can be changed to !propAst.PropertyAttributes.HasFlag(PropertyAttributes.Hidden). The perf issue with HasFlag is fixed in 2.1

'@
$modulesFolder = Join-Path $TestDrive "Modules"
$modulePath = Join-Path $modulesFolder "TestModule"
New-Item -ItemType Directory -Path $modulePath -Force
Copy link
Member

Choose a reason for hiding this comment

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

> $null

hidden static $monthNames = @('Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun','Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec')
}
'@
$modulesFolder = Join-Path $TestDrive "Modules"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use -AdditionalChildPath instead of two Join-Path

Copy link
Member Author

Choose a reason for hiding this comment

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

$modulesFolder and $modulePath are used independently

@daxian-dbw daxian-dbw merged commit 3cc9d26 into PowerShell:master Jun 29, 2018
@SteveL-MSFT SteveL-MSFT deleted the get-help-category-class branch October 26, 2018 21:35
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.

Get-Help -Category Class results in unhandled exception

5 participants