Skip to content

Fix PowerShell class to support deriving from an abstract class with abstract properties #21331

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
Mar 13, 2024
Merged

Fix PowerShell class to support deriving from an abstract class with abstract properties #21331

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
ebb9cfc
Fix PowerShell class to support deriving from an abstract class with …
daxian-dbw Mar 12, 2024
d7e4fc1
Add a test for the error case
daxian-dbw Mar 13, 2024
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
21 changes: 16 additions & 5 deletions src/System.Management.Automation/engine/parser/PSType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ private sealed class DefineTypeHelper
internal readonly TypeBuilder _staticHelpersTypeBuilder;
private readonly Dictionary<string, PropertyMemberAst> _definedProperties;
private readonly Dictionary<string, List<Tuple<FunctionMemberAst, Type[]>>> _definedMethods;
private HashSet<Tuple<string, Type>> _interfaceProperties;
private HashSet<Tuple<string, Type>> _abstractProperties;
internal readonly List<(string fieldName, IParameterMetadataProvider bodyAst, bool isStatic)> _fieldsToInitForMemberFunctions;
private bool _baseClassHasDefaultCtor;

Expand Down Expand Up @@ -446,9 +446,9 @@ private Type GetBaseTypes(Parser parser, TypeDefinitionAst typeDefinitionAst, ou

private bool ShouldImplementProperty(string name, Type type)
{
if (_interfaceProperties == null)
if (_abstractProperties == null)
{
_interfaceProperties = new HashSet<Tuple<string, Type>>();
_abstractProperties = new HashSet<Tuple<string, Type>>();
var allInterfaces = new HashSet<Type>();

// TypeBuilder.GetInterfaces() returns only the interfaces that was explicitly passed to its constructor.
Expand All @@ -467,12 +467,23 @@ private bool ShouldImplementProperty(string name, Type type)
{
foreach (var property in interfaceType.GetProperties())
{
_interfaceProperties.Add(Tuple.Create(property.Name, property.PropertyType));
_abstractProperties.Add(Tuple.Create(property.Name, property.PropertyType));
}
}

if (_typeBuilder.BaseType.IsAbstract)
{
foreach (var property in _typeBuilder.BaseType.GetProperties())
Copy link
Collaborator

@SeeminglyScience SeeminglyScience Mar 12, 2024

Choose a reason for hiding this comment

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

I believe this will need to include BindingFlags.NonPublic as protected members should still be found right? Likewise GetAccessors below likely needs nonPublic: true.

Then a check like

accessor.IsFamilyOrAssembly || accessor.IsFamily || assessor.IsPublic

Interfaces may also actually need that check too since C# 8 added the ability to mark members as protected. Though that wouldn't be fixing a regression so likely fine to leave out of this PR.

Copy link
Member Author

@daxian-dbw daxian-dbw Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But we don't create properties or methods with the "protected" (family) modifier, and we don't have a keyword in PowerShell language to declare a property as protected, we only create public (by default or with the hidden keyword) or static (static keyword) members. So, even if we check for protected abstract properties here, the public property we are going to create later won't be considered as an implementation of the protected property.

When a base type (MyBase) has a protected abstract property Name, the class definition class MySub : MyBase { [string]$Name } will fail on 7.2 and 7.3 as well today (see the screenshots below). So, I think we just don't support deriving from a class that has protected abstract properties/methods.

7.2.18
image

7.3.11
image

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Mar 13, 2024

Choose a reason for hiding this comment

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

Ah alright if it failed before then good enough for me!

{
if (property.GetAccessors().Any(m => m.IsAbstract))
{
_abstractProperties.Add(Tuple.Create(property.Name, property.PropertyType));
}
}
}
}

return _interfaceProperties.Contains(Tuple.Create(name, type));
return _abstractProperties.Contains(Tuple.Create(name, type));
}

public void DefineMembers()
Expand Down
44 changes: 44 additions & 0 deletions test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,3 +628,47 @@ class Derived : Base
$sb.Invoke() | Should -Be 200
}
}

Describe 'Base type has abstract properties' -Tags "CI" {
Copy link
Collaborator

@JamesWTruher JamesWTruher Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

do we have validation which ensures that an error is produced if the abstract property is not defined? is it needed?

Copy link
Member Author

@daxian-dbw daxian-dbw Mar 12, 2024

Choose a reason for hiding this comment

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

Good point. I will add one.

Copy link
Member Author

@daxian-dbw daxian-dbw Mar 13, 2024

Choose a reason for hiding this comment

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

A test for the error case was added.

It 'can derive from `FileSystemInfo`' {
## FileSystemInfo has 3 abstract members that a derived type needs to implement
## - public abstract bool Exists { get; }
## - public abstract string Name { get; }
## - public abstract void Delete ();

class myFileSystemInfo : System.IO.FileSystemInfo
{
[string] $Name
[bool] $Exists

myFileSystemInfo([string]$path)
{
# ctor
$this.Name = $path
$this.Exists = $true
}

[void] Delete()
{
}
}

$myFile = [myFileSystemInfo]::new('Hello')
$myFile.Name | Should -Be 'Hello'
$myFile.Exists | Should -BeTrue
}

It 'deriving from `FileSystemInfo` will fail when the abstract property `Exists` is not implemented' {
$script = [scriptblock]::Create('class WillFail : System.IO.FileSystemInfo { [string] $Name }')
$failure = $null
try {
& $script
} catch {
$failure = $_
}

$failure | Should -Not -BeNullOrEmpty
$failure.FullyQualifiedErrorId | Should -BeExactly "TypeCreationError"
$failure.Exception.Message | Should -BeLike "*'get_Exists'*"
}
}