Skip to content

Engine: Fix several code cleanup issues #6552

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.

Sign up for GitHub

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

Merged
daxian-dbw merged 2 commits into from
Apr 4, 2018
Merged

Engine: Fix several code cleanup issues #6552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c50fbf
[Feature] Some random fixes when reading code
daxian-dbw Mar 30, 2018
aeef7a9
[Feature] Change 'PSObject.dotNetInstanceAdapter' to 'base' in Proper…
daxian-dbw Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/System.Management.Automation/engine/CoreAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4252,7 +4252,7 @@ protected override T GetMember<T>(object obj, string memberName)
// are ignored.
if (typeof(T) == typeof(PSMemberInfo))
{
T returnValue = PSObject.dotNetInstanceAdapter.GetDotNetMethod<T>(obj, memberName);
T returnValue = base.GetDotNetMethod<T>(obj, memberName);
Copy link
Contributor

@lzybkr lzybkr Apr 3, 2018

Choose a reason for hiding this comment

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

Do you have an example that demonstrates this is a bug?

It might be a change in behavior, e.g. if the instance was a "property only" adapter, GetMember might no longer return any methods whereas it would have before.

Copy link
Member Author

@daxian-dbw daxian-dbw Apr 3, 2018

Choose a reason for hiding this comment

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

if the instance was a "property only" adapter

This type is BaseDotNetAdapterForAdaptedObjects and it derives from DotNetAdapter directly. An instance of it won't be a "property only" adapter. It serves as the fall-back DotNet adapter for custom adapters.
I may miss an aspect of your comment, could you please explain your concern a bit more?

Copy link
Member Author

@daxian-dbw daxian-dbw Apr 3, 2018

Choose a reason for hiding this comment

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

And performance shouldn't be a problem because the cache tables are static to the process, so every instance of DotNetAdapter can benefit from them.

Copy link
Contributor

@lzybkr lzybkr Apr 3, 2018

Choose a reason for hiding this comment

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

I see, I thought this might be used by ThirdPartyAdapter or other PropertyOnlyAdapter implementations, but it's the fallback adapter.

After reviewing BaseDotNetAdapterForAdaptedObjects and PropertyOnlyAdapter and related methods, I don't understand why it exists at all.

Copy link
Member Author

@daxian-dbw daxian-dbw Apr 3, 2018

Choose a reason for hiding this comment

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

I have made the similar change to PropertyOnlyAdapter.

Copy link
Contributor

@PaulHigin PaulHigin Apr 3, 2018

Choose a reason for hiding this comment

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

Shouldn't this also be changed at line 4607? It seems like GetDotNetMethod at least should be a static method, and a static instance is not needed.

Copy link
Member Author

@daxian-dbw daxian-dbw Apr 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I think the use of PSObject.dotNetInstanceAdapter in PropertyOnlyAdapter can be changed to base too. PropertyOnlyAdapter derives from DotNetAdapter.

Even with PropertyOnlyAdapter, I don't see a problem calling the base methods. @lzybkr could you please elaborate your concerns?

Copy link
Member Author

@daxian-dbw daxian-dbw Apr 3, 2018

Choose a reason for hiding this comment

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

I have made the similar change to PropertyOnlyAdapter.

// We only return a method if there is no property by the same name
// to match the behavior we have in GetMembers
if (returnValue != null && property == null)
Expand All @@ -4263,7 +4263,7 @@ protected override T GetMember<T>(object obj, string memberName)

if (IsTypeParameterizedProperty(typeof(T)))
{
PSParameterizedProperty parameterizedProperty = PSObject.dotNetInstanceAdapter.GetDotNetProperty<PSParameterizedProperty>(obj, memberName);
PSParameterizedProperty parameterizedProperty = base.GetDotNetProperty<PSParameterizedProperty>(obj, memberName);
// We only return a parameterized property if there is no property by the same name
// to match the behavior we have in GetMembers
if (parameterizedProperty != null && property == null)
Expand Down Expand Up @@ -4604,7 +4604,7 @@ protected override T GetMember<T>(object obj, string memberName)

if (typeof(T).IsAssignableFrom(typeof(PSMethod)))
{
T returnValue = PSObject.dotNetInstanceAdapter.GetDotNetMethod<T>(obj, memberName);
T returnValue = base.GetDotNetMethod<T>(obj, memberName);
// We only return a method if there is no property by the same name
// to match the behavior we have in GetMembers
if (returnValue != null && property == null)
Expand All @@ -4614,7 +4614,7 @@ protected override T GetMember<T>(object obj, string memberName)
}
if (IsTypeParameterizedProperty(typeof(T)))
{
PSParameterizedProperty parameterizedProperty = PSObject.dotNetInstanceAdapter.GetDotNetProperty<PSParameterizedProperty>(obj, memberName);
PSParameterizedProperty parameterizedProperty = base.GetDotNetProperty<PSParameterizedProperty>(obj, memberName);
// We only return a parameterized property if there is no property by the same name
// to match the behavior we have in GetMembers
if (parameterizedProperty != null && property == null)
Expand Down Expand Up @@ -4644,11 +4644,11 @@ protected override PSMemberInfoInternalCollection<T> GetMembers<T>(object obj)
{
DoAddAllProperties<T>(obj, returnValue);
}
PSObject.dotNetInstanceAdapter.AddAllMethods(obj, returnValue, true);
base.AddAllMethods(obj, returnValue, true);
if (IsTypeParameterizedProperty(typeof(T)))
{
var parameterizedProperties = new PSMemberInfoInternalCollection<PSParameterizedProperty>();
PSObject.dotNetInstanceAdapter.AddAllProperties(obj, parameterizedProperties, true);
base.AddAllProperties(obj, parameterizedProperties, true);
foreach (PSParameterizedProperty parameterizedProperty in parameterizedProperties)
{
try
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/EventManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2029,7 +2029,7 @@ internal PSEventSubscriber(ExecutionContext context, int id, Object source,

internal void RegisterJob()
{
// And this event subscriber to the job repository if it's not a support event.
// Add this event subscriber to the job repository if it's not a support event.
if (!SupportEvent)
{
if (this.Action != null)
Expand Down
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/InitialSessionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5219,7 +5219,7 @@ internal static void AnalyzePSSnapInAssembly(Assembly assembly, string name, PSS
{
clone[entry.Key] = (SessionStateProviderEntry)entry.Value.Clone();
}
s_providerCache.Value[assembly] = providers;
s_providerCache.Value[assembly] = clone;
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/System.Management.Automation/engine/MshObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ private static PSMemberInfoInternalCollection<T> AdapterGetMembersDelegate<T>(PS
private static PSMemberInfoInternalCollection<T> DotNetGetMembersDelegate<T>(PSObject msjObj) where T : PSMemberInfo
{
// Don't lookup dotnet members if the object doesn't insist.
if (null != msjObj.InternalAdapterSet.DotNetAdapter)
if (null != msjObj.InternalBaseDotNetAdapter)
{
PSMemberInfoInternalCollection<T> retValue = msjObj.InternalAdapterSet.DotNetAdapter.BaseGetMembers<T>(msjObj._immediateBaseObject);
PSMemberInfoInternalCollection<T> retValue = msjObj.InternalBaseDotNetAdapter.BaseGetMembers<T>(msjObj._immediateBaseObject);
PSObject.memberResolution.WriteLine("DotNet members: {0}.", retValue.VisibleCount);
return retValue;
}
Expand All @@ -184,9 +184,9 @@ private static PSMemberInfoInternalCollection<T> DotNetGetMembersDelegate<T>(PSO
private static T DotNetGetMemberDelegate<T>(PSObject msjObj, string name) where T : PSMemberInfo
{
// Don't lookup dotnet member if the object doesn't insist.
if (null != msjObj.InternalAdapterSet.DotNetAdapter)
if (null != msjObj.InternalBaseDotNetAdapter)
{
T retValue = msjObj.InternalAdapterSet.DotNetAdapter.BaseGetMember<T>(msjObj._immediateBaseObject, name);
T retValue = msjObj.InternalBaseDotNetAdapter.BaseGetMember<T>(msjObj._immediateBaseObject, name);
PSObject.memberResolution.WriteLine("DotNet member: {0}.", retValue == null ? "not found" : retValue.Name);
return retValue;
}
Expand Down
11 changes: 5 additions & 6 deletions src/System.Management.Automation/engine/SessionStateProviderAPIs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,11 @@ internal Dictionary<ProviderInfo, PSDriveInfo> ProvidersCurrentWorkingDrive
/// <param name="providerEntry"></param>
internal void AddSessionStateEntry(SessionStateProviderEntry providerEntry)
{
ProviderInfo provider = AddProvider(providerEntry.ImplementingType,
providerEntry.Name,
providerEntry.HelpFileName,
providerEntry.PSSnapIn,
providerEntry.Module
);
AddProvider(providerEntry.ImplementingType,
providerEntry.Name,
providerEntry.HelpFileName,
providerEntry.PSSnapIn,
providerEntry.Module);
}

private ProviderInfo AddProvider(Type implementingType, string name, string helpFileName, PSSnapInInfo psSnapIn, PSModuleInfo module)
Expand Down