-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix class searcher to ignore hidden properties #7188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix class searcher to ignore hidden properties #7188
Conversation
BrucePay
left a comment
There was a problem hiding this 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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
PR Summary
The ConvertToClassInfo method attempts to convert a PowerShell class into a PSClassInfo. When it encounters a hidden property, it fails as the
PropertyTypemember 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
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests