Skip to content

WIP: Allow self-referential generic type parameters in interface list#12863

Closed
IISResetMe wants to merge 3 commits intoPowerShell:masterfrom
IISResetMe:fix/type-param-self
Closed

WIP: Allow self-referential generic type parameters in interface list#12863
IISResetMe wants to merge 3 commits intoPowerShell:masterfrom
IISResetMe:fix/type-param-self

Conversation

@IISResetMe
Copy link
Copy Markdown
Collaborator

PR Summary

Delay type parameter-binding of unclosed (self-referential) type references in generic interface declarations

PR Context

We currently support implementing typed generic interfaces, but only if the type parameters can be resolved in the defining scope prior to defining the new type.

This makes the following implementation impossible in PowerShell:

    class SortableThing : IComparable, IComparable[SortableThing]
    {
        [int]Value

        [int]
        CompareTo([SortableThing]$obj)
        {
            return $this.Value.CompareTo($obj.Value)
        }

        [int]
        CompareTo($obj)
        {
            if($obj -isnot [SortableThing]){
                throw [ArgumentException]::new()
            }
            return $this.CompareTo([SortableThing]$obj)
        }
    }

To this end, the following changes have been made to the DefineTypeHelper in PSType:

  • For declared interfaces, GetBaseTypes now outputs a list of late-binding InterfaceExpression's rather than a list of Type instances describing concrete interfaces
  • We add the interfaces to the new type after defining the type builder, allowing us to resolve self-referential type parameters against the still-unclosed-type instead of throwing TypeNotFound

At some point we will need to re-think how we resolve generic type arguments, perhaps refactor GenericTypeName completely to natively support late binding against unclosed types, but I'll leave that an exercise for the future for now.

PR Checklist

@IISResetMe IISResetMe requested a review from daxian-dbw as a code owner June 1, 2020 13:06
@ghost ghost assigned TravisEz13 Jun 1, 2020
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@IISResetMe, your last commit had 4 failures in PowerShell-CI-linux
Error occurred in test script '/home/vsts/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /home/vsts/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Error occurred in test script '/home/vsts/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /home/vsts/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Error occurred in test script '/home/vsts/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /home/vsts/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Error occurred in test script '/home/vsts/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /home/vsts/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@IISResetMe, your last commit had 4 failures in PowerShell-CI-macos
Error occurred in test script '/Users/runner/runners/2.169.1/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /Users/runner/runners/2.169.1/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Error occurred in test script '/Users/runner/runners/2.169.1/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /Users/runner/runners/2.169.1/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Error occurred in test script '/Users/runner/runners/2.169.1/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /Users/runner/runners/2.169.1/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Error occurred in test script '/Users/runner/runners/2.169.1/work/1/s/test/powershell/Language/Classes/scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, /Users/runner/runners/2.169.1/work/1/a/bins/publish/Modules/Pester/4.10.1/Pester.psm1: line 1111

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@IISResetMe, your last commit had 4 failures in PowerShell-CI-windows
Error occurred in test script 'D:\a\1\s\test\powershell\Language\Classes\scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, D:\a\1\s\src\powershell-win-core\bin\Release\net5.0\win7-x64\publish\Modules\Pester\4.10.1\Pester.psm1: line 1111

Error occurred in test script 'D:\a\1\s\test\powershell\Language\Classes\scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, D:\a\1\s\src\powershell-win-core\bin\Release\net5.0\win7-x64\publish\Modules\Pester\4.10.1\Pester.psm1: line 1111

Error occurred in test script 'D:\a\1\s\test\powershell\Language\Classes\scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, D:\a\1\s\src\powershell-win-core\bin\Release\net5.0\win7-x64\publish\Modules\Pester\4.10.1\Pester.psm1: line 1111

Error occurred in test script 'D:\a\1\s\test\powershell\Language\Classes\scripting.Classes.inheritance.tests.ps1'

Object reference not set to an instance of an object.
at <ScriptBlock>, D:\a\1\s\src\powershell-win-core\bin\Release\net5.0\win7-x64\publish\Modules\Pester\4.10.1\Pester.psm1: line 1111

@IISResetMe
Copy link
Copy Markdown
Collaborator Author

Although some of there errors are surely mine, HEAD of master seems broken at the moment, please advice on how to proceed.

Build from clean clone of master at e93381e
image

@vexx32
Copy link
Copy Markdown
Collaborator

vexx32 commented Jun 1, 2020

You should be able to build in release mode in the meantime to bypass the debug assert and test things, but we do need #12840 fixed so we don't need to do that. 🙂

@TravisEz13 TravisEz13 marked this pull request as draft June 1, 2020 19:52
@ghost ghost added the Review - Needed The PR is being reviewed label Jun 9, 2020
@ghost
Copy link
Copy Markdown

ghost commented Jun 9, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@TravisEz13 TravisEz13 removed the Review - Needed The PR is being reviewed label Jun 9, 2020
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.
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,
@IISResetMe IISResetMe force-pushed the fix/type-param-self branch from c8137a0 to 6f4fc21 Compare June 11, 2020 22:01
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jun 15, 2020
@ghost ghost added the Stale label Jun 30, 2020
@ghost
Copy link
Copy Markdown

ghost commented Jun 30, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@ghost ghost closed this Jul 10, 2020
@ghost ghost removed the Stale label Jul 14, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants