From 9e80f968ae51ac917bbefe2216942eecd374c8f1 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 3 Dec 2019 12:41:23 -0800 Subject: [PATCH 1/5] Ignore hidden members in CacheTable.FirstOrDefault --- .../engine/CoreAdapter.cs | 127 +++++++++++++----- .../engine/ManagementObjectAdapter.cs | 2 +- 2 files changed, 94 insertions(+), 35 deletions(-) diff --git a/src/System.Management.Automation/engine/CoreAdapter.cs b/src/System.Management.Automation/engine/CoreAdapter.cs index f78777c840b..5ae374d3733 100644 --- a/src/System.Management.Automation/engine/CoreAdapter.cs +++ b/src/System.Management.Automation/engine/CoreAdapter.cs @@ -1983,13 +1983,39 @@ internal static void DoBoxingIfNecessary(ILGenerator generator, Type type) #endregion base } + + /// + /// The abstract cache entry type. + /// All specific cache entry types should derive from it. + /// + internal abstract class CacheEntry + { + /// + /// Gets the boolean value to indicate if the member is hidden. + /// + /// + /// Currently, we only check the 'HiddenAttribute' declared for properties and methods, + /// because it can be done for them through the 'hidden' keyword in PowerShell Class. + /// + /// We can't currently write a parameterized property in a PowerShell class so it's not too important + /// to check for the 'HiddenAttribute' for parameterized properties. But if someone added the attribute + /// to their C#, it'd be good to set this property correctly. + /// + internal virtual bool IsHidden => false; + } + /// /// Ordered and case insensitive hashtable. /// internal class CacheTable { + /// + /// An object collection is used to help make populating method cache table more efficient + /// . + /// internal Collection memberCollection; private Dictionary _indexes; + internal CacheTable() { memberCollection = new Collection(); @@ -2012,17 +2038,30 @@ internal object this[string name] return null; } - return this.memberCollection[indexObj]; + return memberCollection[indexObj]; } } + /// + /// Get the first no-hidden member that satisfies the predicate. + /// + /// + /// Hidden members are not returned for any fuzzy searches (searching by 'match' or enumerating a collection). + /// A hidden member is returned only if the member name is explicitly looked for. + /// internal object GetFirstOrDefault(MemberNamePredicate predicate) { foreach (var entry in _indexes) { if (predicate(entry.Key)) { - return this.memberCollection[entry.Value]; + object member = memberCollection[entry.Value]; + if (member is CacheEntry cacheEntry && cacheEntry.IsHidden) + { + continue; + } + + return member; } } @@ -2565,9 +2604,9 @@ private static readonly Dictionary> s_ private static readonly Dictionary> s_staticEventCacheTable = new Dictionary>(); - internal class MethodCacheEntry + internal class MethodCacheEntry : CacheEntry { - internal MethodInformation[] methodInformationStructures; + internal readonly MethodInformation[] methodInformationStructures; /// /// Cache delegate to the ctor of PSMethod<> with a template parameter derived from the methodInformationStructures. /// @@ -2585,9 +2624,33 @@ internal MethodInformation this[int i] return methodInformationStructures[i]; } } + + private bool? _isHidden; + internal override bool IsHidden + { + get + { + if (_isHidden == null) + { + bool hasHiddenAttribute = false; + foreach (var method in methodInformationStructures) + { + if (method.method.GetCustomAttributes(typeof(HiddenAttribute), inherit: false).Length != 0) + { + hasHiddenAttribute = true; + break; + } + } + + _isHidden = hasHiddenAttribute; + } + + return _isHidden.Value; + } + } } - internal class EventCacheEntry + internal class EventCacheEntry : CacheEntry { internal EventInfo[] events; @@ -2597,7 +2660,7 @@ internal EventCacheEntry(EventInfo[] events) } } - internal class ParameterizedPropertyCacheEntry + internal class ParameterizedPropertyCacheEntry : CacheEntry { internal MethodInformation[] getterInformation; internal MethodInformation[] setterInformation; @@ -2676,7 +2739,7 @@ internal ParameterizedPropertyCacheEntry(List properties) } } - internal class PropertyCacheEntry + internal class PropertyCacheEntry : CacheEntry { internal delegate object GetterDelegate(object instance); internal delegate void SetterDelegate(object instance, object setValue); @@ -2916,6 +2979,20 @@ internal SetterDelegate setterDelegate internal bool isStatic; internal Type propertyType; + private bool? _isHidden; + internal override bool IsHidden + { + get + { + if (_isHidden == null) + { + _isHidden = member.GetCustomAttributes(typeof(HiddenAttribute), inherit: false).Length != 0; + } + + return _isHidden.Value; + } + } + private AttributeCollection _attributes; internal AttributeCollection Attributes { @@ -3578,8 +3655,7 @@ private T GetDotNetPropertyImpl(object obj, string propertyName, MemberNamePr case null: return null; case PropertyCacheEntry cacheEntry when lookingForProperties: - var isHidden = cacheEntry.member.GetCustomAttributes(typeof(HiddenAttribute), false).Any(); - return new PSProperty(cacheEntry.member.Name, this, obj, cacheEntry) { IsHidden = isHidden } as T; + return new PSProperty(cacheEntry.member.Name, this, obj, cacheEntry) { IsHidden = cacheEntry.IsHidden } as T; case ParameterizedPropertyCacheEntry paramCacheEntry when lookingForParameterizedProperties: // TODO: check for HiddenAttribute @@ -3612,17 +3688,8 @@ private T GetDotNetMethodImpl(object obj, string methodName, MemberNamePredic var isCtor = methods[0].method is ConstructorInfo; bool isSpecial = !isCtor && methods[0].method.IsSpecialName; - bool isHidden = false; - foreach (var method in methods.methodInformationStructures) - { - if (method.method.GetCustomAttributes(typeof(HiddenAttribute), false).Any()) - { - isHidden = true; - break; - } - } - return PSMethod.Create(methods[0].method.Name, this, obj, methods, isSpecial, isHidden) as T; + return PSMethod.Create(methods[0].method.Name, this, obj, methods, isSpecial, methods.IsHidden) as T; } internal T GetDotNetProperty(object obj, string propertyName) where T : PSMemberInfo @@ -3712,10 +3779,12 @@ internal void AddAllProperties(object obj, PSMemberInfoInternalCollection { if (!ignoreDuplicates || (members[propertyEntry.member.Name] == null)) { - var isHidden = propertyEntry.member.GetCustomAttributes(typeof(HiddenAttribute), false).Any(); - members.Add(new PSProperty(propertyEntry.member.Name, this, - obj, propertyEntry) - { IsHidden = isHidden } as T); + members.Add( + new PSProperty( + name: propertyEntry.member.Name, + adapter: this, + baseObject: obj, + adapterData: propertyEntry) { IsHidden = propertyEntry.IsHidden } as T); } } } @@ -3754,17 +3823,7 @@ internal void AddAllMethods(object obj, PSMemberInfoInternalCollection mem if (!ignoreDuplicates || (members[name] == null)) { bool isSpecial = !isCtor && method[0].method.IsSpecialName; - bool isHidden = false; - foreach (var m in method.methodInformationStructures) - { - if (m.method.GetCustomAttributes(typeof(HiddenAttribute), false).Any()) - { - isHidden = true; - break; - } - } - - members.Add(PSMethod.Create(name, this, obj, method, isSpecial, isHidden) as T); + members.Add(PSMethod.Create(name, this, obj, method, isSpecial, method.IsHidden) as T); } } } diff --git a/src/System.Management.Automation/engine/ManagementObjectAdapter.cs b/src/System.Management.Automation/engine/ManagementObjectAdapter.cs index 1f8c2b8be98..44788eb1d4c 100644 --- a/src/System.Management.Automation/engine/ManagementObjectAdapter.cs +++ b/src/System.Management.Automation/engine/ManagementObjectAdapter.cs @@ -32,7 +32,7 @@ internal abstract class BaseWMIAdapter : Adapter /// by Get-Member cmdlet, original MethodData and computed method information such /// as whether a method is static etc. /// - internal class WMIMethodCacheEntry + internal class WMIMethodCacheEntry : CacheEntry { public string Name { get; } From 6a8166db15a3d324c10c2c2eb352d325997613a7 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Tue, 3 Dec 2019 13:01:55 -0800 Subject: [PATCH 2/5] Add test --- .../engine/Formatting/BugFix.Tests.ps1 | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 test/powershell/engine/Formatting/BugFix.Tests.ps1 diff --git a/test/powershell/engine/Formatting/BugFix.Tests.ps1 b/test/powershell/engine/Formatting/BugFix.Tests.ps1 new file mode 100644 index 00000000000..2aa2cce44d9 --- /dev/null +++ b/test/powershell/engine/Formatting/BugFix.Tests.ps1 @@ -0,0 +1,34 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +Describe 'Bug fixes related to formatting' -Tag CI { + + It "Formatting for an object with no property/field should use 'ToString'" { + class Empty { + [String]ToString() { return 'MyString' } + } + + $outstring = [Empty]::new() | Out-String + $outstring.Trim() | Should -BeExactly "MyString" + } + + It "Formatting for an object with only hidden property should use 'ToString'" { + class Hidden { + hidden $Param = 'Foo' + [String]ToString() { return 'MyString' } + } + + $outstring = [Hidden]::new() | Out-String + $outstring.Trim() | Should -BeExactly "MyString" + } + + It 'Formatting for an object with no-hidden property should use the default view' { + class Params { + $Param = 'Foo' + [String]ToString() { return 'MyString' } + } + + $outstring = [Params]::new() | Out-String + $outstring.Trim() | Should -BeExactly "Param$([System.Environment]::NewLine)-----$([System.Environment]::NewLine)Foo" + } +} From ba4349f9d9c78e4ca1f0340315f9df025464ba03 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Thu, 5 Dec 2019 13:35:03 -0800 Subject: [PATCH 3/5] Address comments --- src/System.Management.Automation/engine/CoreAdapter.cs | 2 +- test/powershell/engine/Formatting/BugFix.Tests.ps1 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/CoreAdapter.cs b/src/System.Management.Automation/engine/CoreAdapter.cs index 5ae374d3733..bd5c6a53503 100644 --- a/src/System.Management.Automation/engine/CoreAdapter.cs +++ b/src/System.Management.Automation/engine/CoreAdapter.cs @@ -2043,7 +2043,7 @@ internal object this[string name] } /// - /// Get the first no-hidden member that satisfies the predicate. + /// Get the first non-hidden member that satisfies the predicate. /// /// /// Hidden members are not returned for any fuzzy searches (searching by 'match' or enumerating a collection). diff --git a/test/powershell/engine/Formatting/BugFix.Tests.ps1 b/test/powershell/engine/Formatting/BugFix.Tests.ps1 index 2aa2cce44d9..0b6ef15a6a7 100644 --- a/test/powershell/engine/Formatting/BugFix.Tests.ps1 +++ b/test/powershell/engine/Formatting/BugFix.Tests.ps1 @@ -1,7 +1,7 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. -Describe 'Bug fixes related to formatting' -Tag CI { +Describe "Hidden properties should not be returned by the 'FirstOrDefault' primitive" -Tag CI { It "Formatting for an object with no property/field should use 'ToString'" { class Empty { From 82c1538a66a65ba34b4cd48df59b05043889d488 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Thu, 5 Dec 2019 16:31:22 -0800 Subject: [PATCH 4/5] Add more tests as suggested --- .../engine/Formatting/BugFix.Tests.ps1 | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/powershell/engine/Formatting/BugFix.Tests.ps1 b/test/powershell/engine/Formatting/BugFix.Tests.ps1 index 0b6ef15a6a7..52bb878bde9 100644 --- a/test/powershell/engine/Formatting/BugFix.Tests.ps1 +++ b/test/powershell/engine/Formatting/BugFix.Tests.ps1 @@ -10,6 +10,11 @@ Describe "Hidden properties should not be returned by the 'FirstOrDefault' primi $outstring = [Empty]::new() | Out-String $outstring.Trim() | Should -BeExactly "MyString" + + class Empty2 { } + + $outstring = [Empty2]::new() | Out-String + $outstring.Trim() | Should -BeLike "*.Empty2" } It "Formatting for an object with only hidden property should use 'ToString'" { @@ -20,6 +25,13 @@ Describe "Hidden properties should not be returned by the 'FirstOrDefault' primi $outstring = [Hidden]::new() | Out-String $outstring.Trim() | Should -BeExactly "MyString" + + class Hidden2 { + hidden $Param = 'Foo' + } + + $outstring = [Hidden2]::new() | Out-String + $outstring.Trim() | Should -BeLike "*.Hidden2" } It 'Formatting for an object with no-hidden property should use the default view' { @@ -30,5 +42,13 @@ Describe "Hidden properties should not be returned by the 'FirstOrDefault' primi $outstring = [Params]::new() | Out-String $outstring.Trim() | Should -BeExactly "Param$([System.Environment]::NewLine)-----$([System.Environment]::NewLine)Foo" + + class Params2 { + $Param = 'Foo' + [String]ToString() { return 'MyString' } + } + + $outstring = [Params2]::new() | Out-String + $outstring.Trim() | Should -BeExactly "Param$([System.Environment]::NewLine)-----$([System.Environment]::NewLine)Foo" } } From 266b5a104cf18037cb5c24fdc19220f8c241dbeb Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Thu, 5 Dec 2019 16:33:38 -0800 Subject: [PATCH 5/5] Minor fix to the test --- test/powershell/engine/Formatting/BugFix.Tests.ps1 | 1 - 1 file changed, 1 deletion(-) diff --git a/test/powershell/engine/Formatting/BugFix.Tests.ps1 b/test/powershell/engine/Formatting/BugFix.Tests.ps1 index 52bb878bde9..65b71113389 100644 --- a/test/powershell/engine/Formatting/BugFix.Tests.ps1 +++ b/test/powershell/engine/Formatting/BugFix.Tests.ps1 @@ -45,7 +45,6 @@ Describe "Hidden properties should not be returned by the 'FirstOrDefault' primi class Params2 { $Param = 'Foo' - [String]ToString() { return 'MyString' } } $outstring = [Params2]::new() | Out-String