From 56a88443d5de213b6c57686c932310e19ac4078d Mon Sep 17 00:00:00 2001 From: "Mathias R. Jessen" Date: Sun, 31 May 2020 23:44:46 +0200 Subject: [PATCH 1/3] Bind self-referential generic type parameters late By supplying the interface last up front when defining a new class, we rob ourselves the opportunity to resolve the class in question as a generic type parameter. This makes the following implementation - a completely valid type definition - impossible in PowerShell: class SortableThing : IComparable[SortableThing] { [int]Value [int] CompareTo([SortableThing]$obj) { return $this.Value.CompareTo($obj.Value) } } This commit introduces an InterfaceExpression helper to bind the current typebuilder to any matching type parameter in it's own interface list _after_ initial definition. --- .../engine/parser/PSType.cs | 139 ++++++++++++------ .../scripting.Classes.inheritance.tests.ps1 | 5 + 2 files changed, 99 insertions(+), 45 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/PSType.cs b/src/System.Management.Automation/engine/parser/PSType.cs index 7ffaab3ff0a..00ee6d5066e 100644 --- a/src/System.Management.Automation/engine/parser/PSType.cs +++ b/src/System.Management.Automation/engine/parser/PSType.cs @@ -265,6 +265,39 @@ internal static void DefineCustomAttributes(EnumBuilder member, ReadOnlyCollecti } } + private class InterfaceExpression + { + private TypeConstraintAst ast; + + internal bool IsGeneric => ast?.TypeName.IsGeneric ?? false; + + internal InterfaceExpression(TypeConstraintAst ast) + { + this.ast = ast; + } + + internal Type ResolveConcreteInterfaceType(TypeBuilder parameter) + => IsGeneric ? ResolveConcreteInterfaceTypeArguments(ast.TypeName, parameter) : ast.TypeName.GetReflectionType(); + + private Type ResolveConcreteInterfaceTypeArguments(ITypeName typeName, TypeBuilder parameter) + { + var typeArgs = new List(); + if(typeName.IsGeneric && typeName is GenericTypeName genericName) + { + foreach(var typeArg in genericName.GenericArguments) + { + typeArgs.Add(ResolveConcreteInterfaceTypeArguments(typeArg, parameter)); + } + return genericName.TypeName.GetReflectionType().MakeGenericType(typeArgs.ToArray()); + } + if(parameter.FullName == typeName.FullName) + { + return parameter; + } + return typeName.GetReflectionType(); + } + } + private class DefineTypeHelper { private readonly Parser _parser; @@ -292,10 +325,14 @@ public DefineTypeHelper(Parser parser, ModuleBuilder module, TypeDefinitionAst t _parser = parser; _typeDefinitionAst = typeDefinitionAst; - List interfaces; + List interfaces; var baseClass = this.GetBaseTypes(parser, typeDefinitionAst, out interfaces); - _typeBuilder = module.DefineType(typeName, Reflection.TypeAttributes.Class | Reflection.TypeAttributes.Public, baseClass, interfaces.ToArray()); + _typeBuilder = module.DefineType(typeName, Reflection.TypeAttributes.Class | Reflection.TypeAttributes.Public, baseClass, null); + foreach(var interfaceExpression in interfaces) + { + _typeBuilder.AddInterfaceImplementation(interfaceExpression.ResolveConcreteInterfaceType(_typeBuilder)); + } _staticHelpersTypeBuilder = module.DefineType(string.Format(CultureInfo.InvariantCulture, "{0}_", typeName), Reflection.TypeAttributes.Class); DefineCustomAttributes(_typeBuilder, typeDefinitionAst.Attributes, _parser, AttributeTargets.Class); _typeDefinitionAst.Type = _typeBuilder; @@ -315,11 +352,30 @@ public DefineTypeHelper(Parser parser, ModuleBuilder module, TypeDefinitionAst t /// /// Return declared interfaces. /// - private Type GetBaseTypes(Parser parser, TypeDefinitionAst typeDefinitionAst, out List interfaces) + private Type GetBaseTypes(Parser parser, TypeDefinitionAst typeDefinitionAst, out List interfaces) { // Define base types and report errors. Type baseClass = null; - interfaces = new List(); + interfaces = new List(); + + bool TryGetInterface(TypeConstraintAst ast, out InterfaceExpression interfaceExpression) + { + interfaceExpression = new InterfaceExpression(ast); + if(ast.TypeName.IsGeneric && ast.TypeName is GenericTypeName genericTypeName) + { + if(genericTypeName.TypeName.GetReflectionType().IsInterface) + { + return true; + } + } + else if(ast.TypeName.GetReflectionType()?.IsInterface ?? false) + { + return true; + } + + interfaceExpression = null; + return false; + } // Default base class is System.Object and it has a default ctor. _baseClassHasDefaultCtor = true; @@ -339,40 +395,43 @@ private Type GetBaseTypes(Parser parser, TypeDefinitionAst typeDefinitionAst, ou } else { - baseClass = firstBaseTypeAst.TypeName.GetReflectionType(); - if (baseClass == null) + if(TryGetInterface(firstBaseTypeAst, out InterfaceExpression interfaceExpression)) { - parser.ReportError(firstBaseTypeAst.Extent, - nameof(ParserStrings.TypeNotFound), - ParserStrings.TypeNotFound, - firstBaseTypeAst.TypeName.FullName); - // fall to the default base type + // First Ast can represent interface as well as BaseClass. + interfaces.Add(interfaceExpression); + baseClass = null; } else { - if (baseClass.IsSealed) + baseClass = firstBaseTypeAst.TypeName.GetReflectionType(); + if (baseClass == null) { parser.ReportError(firstBaseTypeAst.Extent, - nameof(ParserStrings.SealedBaseClass), - ParserStrings.SealedBaseClass, - baseClass.Name); - // ignore base type if it's sealed. - baseClass = null; - } - else if (baseClass.IsGenericType && !baseClass.IsConstructedGenericType) - { - parser.ReportError(firstBaseTypeAst.Extent, - nameof(ParserStrings.SubtypeUnclosedGeneric), - ParserStrings.SubtypeUnclosedGeneric, - baseClass.Name); - // ignore base type, we cannot inherit from unclosed generic. - baseClass = null; + nameof(ParserStrings.TypeNotFound), + ParserStrings.TypeNotFound, + firstBaseTypeAst.TypeName.FullName); + // fall to the default base type } - else if (baseClass.IsInterface) + else { - // First Ast can represent interface as well as BaseClass. - interfaces.Add(baseClass); - baseClass = null; + if (baseClass.IsSealed) + { + parser.ReportError(firstBaseTypeAst.Extent, + nameof(ParserStrings.SealedBaseClass), + ParserStrings.SealedBaseClass, + baseClass.Name); + // ignore base type if it's sealed. + baseClass = null; + } + else if (baseClass.IsGenericType && !baseClass.IsConstructedGenericType) + { + parser.ReportError(firstBaseTypeAst.Extent, + nameof(ParserStrings.SubtypeUnclosedGeneric), + ParserStrings.SubtypeUnclosedGeneric, + baseClass.Name); + // ignore base type, we cannot inherit from unclosed generic. + baseClass = null; + } } } } @@ -416,26 +475,16 @@ private Type GetBaseTypes(Parser parser, TypeDefinitionAst typeDefinitionAst, ou else { Type interfaceType = baseTypeAsts[i].TypeName.GetReflectionType(); - if (interfaceType == null) + if (!TryGetInterface(baseTypeAsts[i], out InterfaceExpression interfaceExpression)) { parser.ReportError(baseTypeAsts[i].Extent, - nameof(ParserStrings.TypeNotFound), - ParserStrings.TypeNotFound, - baseTypeAsts[i].TypeName.FullName); + nameof(ParserStrings.InterfaceNameExpected), + ParserStrings.InterfaceNameExpected, + interfaceType.Name); } else { - if (interfaceType.IsInterface) - { - interfaces.Add(interfaceType); - } - else - { - parser.ReportError(baseTypeAsts[i].Extent, - nameof(ParserStrings.InterfaceNameExpected), - ParserStrings.InterfaceNameExpected, - interfaceType.Name); - } + interfaces.Add(interfaceExpression); } } } diff --git a/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 b/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 index cf304d5918e..d86a0da56a9 100644 --- a/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 +++ b/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1 @@ -87,6 +87,11 @@ Describe 'Classes inheritance syntax' -Tags "CI" { { [A]::b = "bla" } | Should -Throw -ErrorId 'ExceptionWhenSetting' } + It 'can implement generic interfaces referencing itself as a type parameter' { + $C1 = Invoke-Expression 'class ComparableClass : IComparable[ComparableClass] { [int]$Value; [int]CompareTo([ComparableClass]$obj){ return $this.Value.CompareTo($obj.Value) } } [ComparableClass]' + $C1.ImplementedInterfaces[0].TypeParameter | Should -Be $C1 + } + Context "Inheritance from abstract .NET classes" { BeforeAll { class TestHost : System.Management.Automation.Host.PSHost From 6f4fc21015b825a5985012efc9fb6c1bf19e075b Mon Sep 17 00:00:00 2001 From: "Mathias R. Jessen" Date: Mon, 1 Jun 2020 14:33:48 +0200 Subject: [PATCH 2/3] Sidestep concrete-type property resolution for unclosed generic types Changes introduced in 45727cb26 means that we might pass a partially unclosed type to ShouldImplementProperty() - this is more of a best-case workaround than a solution, ultimately we should refactor resolution of generic type parameter expressions completely, --- .../engine/parser/PSType.cs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/PSType.cs b/src/System.Management.Automation/engine/parser/PSType.cs index 00ee6d5066e..51e5a9daf52 100644 --- a/src/System.Management.Automation/engine/parser/PSType.cs +++ b/src/System.Management.Automation/engine/parser/PSType.cs @@ -309,7 +309,7 @@ private class DefineTypeHelper internal readonly TypeBuilder _staticHelpersTypeBuilder; private readonly Dictionary _definedProperties; private readonly Dictionary>> _definedMethods; - private HashSet> _interfaceProperties; + private Dictionary _interfaceProperties; internal readonly List<(string fieldName, IParameterMetadataProvider bodyAst, bool isStatic)> _fieldsToInitForMemberFunctions; private bool _baseClassHasDefaultCtor; @@ -497,31 +497,35 @@ private bool ShouldImplementProperty(string name, Type type) { if (_interfaceProperties == null) { - _interfaceProperties = new HashSet>(); + _interfaceProperties = new Dictionary(); var allInterfaces = new HashSet(); // TypeBuilder.GetInterfaces() returns only the interfaces that was explicitly passed to its constructor. // During compilation the interface hierarchy is flattened, so we only need to resolve one level of ancestral interfaces. foreach (var interfaceType in _typeBuilder.GetInterfaces()) { - foreach (var parentInterface in interfaceType.GetInterfaces()) + var typeDefinition = interfaceType; + if(interfaceType.IsGenericType && interfaceType.GenericTypeArguments.Contains(_typeBuilder)) + typeDefinition = interfaceType.GetGenericTypeDefinition(); + + foreach (var parentInterface in typeDefinition.GetInterfaces()) { allInterfaces.Add(parentInterface); } - allInterfaces.Add(interfaceType); + allInterfaces.Add(typeDefinition); } foreach (var interfaceType in allInterfaces) { foreach (var property in interfaceType.GetProperties()) { - _interfaceProperties.Add(Tuple.Create(property.Name, property.PropertyType)); + _interfaceProperties.Add(property.Name, property.PropertyType); } } } - return _interfaceProperties.Contains(Tuple.Create(name, type)); + return _interfaceProperties.TryGetValue(name, out Type returnType) && (returnType == type || (returnType.IsGenericParameter)); } public void DefineMembers() From 7ce774bed1d0a2db0a32f04d214902322d4b8dc1 Mon Sep 17 00:00:00 2001 From: "Mathias R. Jessen" Date: Fri, 12 Jun 2020 00:10:55 +0200 Subject: [PATCH 3/3] Fix CodeFactor issues --- .../engine/parser/PSType.cs | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/System.Management.Automation/engine/parser/PSType.cs b/src/System.Management.Automation/engine/parser/PSType.cs index 51e5a9daf52..3ad7ced4fdc 100644 --- a/src/System.Management.Automation/engine/parser/PSType.cs +++ b/src/System.Management.Automation/engine/parser/PSType.cs @@ -282,18 +282,21 @@ internal Type ResolveConcreteInterfaceType(TypeBuilder parameter) private Type ResolveConcreteInterfaceTypeArguments(ITypeName typeName, TypeBuilder parameter) { var typeArgs = new List(); - if(typeName.IsGeneric && typeName is GenericTypeName genericName) + if (typeName.IsGeneric && typeName is GenericTypeName genericName) { - foreach(var typeArg in genericName.GenericArguments) + foreach (var typeArg in genericName.GenericArguments) { typeArgs.Add(ResolveConcreteInterfaceTypeArguments(typeArg, parameter)); } + return genericName.TypeName.GetReflectionType().MakeGenericType(typeArgs.ToArray()); } - if(parameter.FullName == typeName.FullName) + + if (parameter.FullName == typeName.FullName) { return parameter; } + return typeName.GetReflectionType(); } } @@ -329,10 +332,11 @@ public DefineTypeHelper(Parser parser, ModuleBuilder module, TypeDefinitionAst t var baseClass = this.GetBaseTypes(parser, typeDefinitionAst, out interfaces); _typeBuilder = module.DefineType(typeName, Reflection.TypeAttributes.Class | Reflection.TypeAttributes.Public, baseClass, null); - foreach(var interfaceExpression in interfaces) + foreach (var interfaceExpression in interfaces) { _typeBuilder.AddInterfaceImplementation(interfaceExpression.ResolveConcreteInterfaceType(_typeBuilder)); } + _staticHelpersTypeBuilder = module.DefineType(string.Format(CultureInfo.InvariantCulture, "{0}_", typeName), Reflection.TypeAttributes.Class); DefineCustomAttributes(_typeBuilder, typeDefinitionAst.Attributes, _parser, AttributeTargets.Class); _typeDefinitionAst.Type = _typeBuilder; @@ -351,7 +355,7 @@ public DefineTypeHelper(Parser parser, ModuleBuilder module, TypeDefinitionAst t /// /// /// Return declared interfaces. - /// + /// The base type private Type GetBaseTypes(Parser parser, TypeDefinitionAst typeDefinitionAst, out List interfaces) { // Define base types and report errors. @@ -361,14 +365,14 @@ private Type GetBaseTypes(Parser parser, TypeDefinitionAst typeDefinitionAst, ou bool TryGetInterface(TypeConstraintAst ast, out InterfaceExpression interfaceExpression) { interfaceExpression = new InterfaceExpression(ast); - if(ast.TypeName.IsGeneric && ast.TypeName is GenericTypeName genericTypeName) + if (ast.TypeName.IsGeneric && ast.TypeName is GenericTypeName genericTypeName) { - if(genericTypeName.TypeName.GetReflectionType().IsInterface) + if (genericTypeName.TypeName.GetReflectionType().IsInterface) { return true; } } - else if(ast.TypeName.GetReflectionType()?.IsInterface ?? false) + else if (ast.TypeName.GetReflectionType()?.IsInterface ?? false) { return true; } @@ -395,7 +399,7 @@ bool TryGetInterface(TypeConstraintAst ast, out InterfaceExpression interfaceExp } else { - if(TryGetInterface(firstBaseTypeAst, out InterfaceExpression interfaceExpression)) + if (TryGetInterface(firstBaseTypeAst, out InterfaceExpression interfaceExpression)) { // First Ast can represent interface as well as BaseClass. interfaces.Add(interfaceExpression); @@ -505,8 +509,10 @@ private bool ShouldImplementProperty(string name, Type type) foreach (var interfaceType in _typeBuilder.GetInterfaces()) { var typeDefinition = interfaceType; - if(interfaceType.IsGenericType && interfaceType.GenericTypeArguments.Contains(_typeBuilder)) + if (interfaceType.IsGenericType && interfaceType.GenericTypeArguments.Contains(_typeBuilder)) + { typeDefinition = interfaceType.GetGenericTypeDefinition(); + } foreach (var parentInterface in typeDefinition.GetInterfaces()) { @@ -525,7 +531,7 @@ private bool ShouldImplementProperty(string name, Type type) } } - return _interfaceProperties.TryGetValue(name, out Type returnType) && (returnType == type || (returnType.IsGenericParameter)); + return _interfaceProperties.TryGetValue(name, out Type returnType) && (returnType == type || returnType.IsGenericParameter); } public void DefineMembers()