-
Notifications
You must be signed in to change notification settings - Fork 132
Add RFC: Native support for interface declarations #152
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.
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
Conversation
The semantics section seems to talk a lot about the AST representation but not about the semantics and uses of interfaces. I think interfaces would be awesome in a language like PowerShell if they work like in TypeScript (duck typed, not like in C#). PowerShell works a lot with JSON from APIs or PSCustomObject literals. Ducktyped interfaces would allow to validate input objects without imperative type checks and declare output types of functions that return API JSON objects (for autocompletion). |
@felixfbecker Thanks for your feedback, I will expand on use cases in the RFC draft. With regards to JSON, it sounds more like we need better support for JSON deserialization and schema validation. While I agree, I fail to see how it relates to the contents of this RFC |
|
||
# interface declaration with a method | ||
interface IMyInterfaceWithMethod { | ||
[void] DoSomething([int]$IntParam) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the [void] is unnecessary as it is likewise unnecessary in classes. You might want to include such an example.
|
||
```powershell | ||
# interface declaration with no members | ||
interface IMyInterface { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is a breaking change with a reasonably high likelihood of colliding with a command named "interface". Not saying we shouldn't do it, just that we should point this out in the proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JamesWTruher and I agree that breaking is undesirable, and we did find this one instance of a real-world module that would break, but we may still decide to do this via an experimental flag and message heavily around the breakage if we decide the value is high enough
} | ||
``` | ||
|
||
*Note: [C# 8 is slated to support default implementations](https://blogs.msdn.microsoft.com/dotnet/2018/11/12/building-c-8-0/) for interface methods, something we might want to consider as well* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had exactly the same thought!
|
||
Currently, when the parser recognizes a `class` or `enum` declaration, it produces a `TypeDefinitionAst` made up of `PropertyMemberAst` and `FunctionMemberAst` children. The type definition AST is then dispatched to either TypeDefiner.DefineTypeHelper or TypeDefiner.DefineEnumHelper for creating and emitting the corresponding runtime type. | ||
|
||
We can either add new `AbstractMemberAst` types specifically for the purpose of abstract type definitions, or modify the existing `MemberAst` types to account for abstract member constraints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on this? And perhaps add examples to the previous section? There is a strong preference at the Committee level for lots of illustrative (practical) examples.
|
||
Going with separate AST types for abstract type members would reduce the number of branches and special cases required in the rest of the code base, but the obvious benefit of reusing existing AST types for member declarations is that we can do not have to extend the existing AST visitor surface (ie. no `IAstVisitor3 : IAstVisitor2`, `AstVisitor3 : AstVisitor2, IAstVisitor3` etc.). This may present hidden parsing issues for tooling developers though. | ||
|
||
Regardless of design changes to the AST surface, we can either define a new `TypeDefiner.DefineInterfaceHelper` for emitting interface types, or we can modify the existing `DefineTypeHelper` definition to take the `Interface` attribute into account and modify all the other emitting functions being called by it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that there would be enough common code that it makes sense to generalize the DefineTypeHelper
. Another thing to consider is if it makes sense to handle abstract classes in the process.
There are a couple of open questions around the design and implementation of interface declaration support: | ||
|
||
- Should we allow control over getter/setter declarations? | ||
- If so, should we replicate the syntax from C#? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say yes. As far as C# syntax goes, there's the traditional stuff but there are also expression-valued getters (which are cool.) to consider. However I suspect that conservative might be the best approach.
- Should we allow control over getter/setter declarations? | ||
- If so, should we replicate the syntax from C#? | ||
- Existing class definitions need to [mark interface member implementations virtual](https://github.com/PowerShell/PowerShell/issues/8302) | ||
- Depending on community feedback and envisioned use cases, are interfaces sufficient or might we consider `abstract` classes as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😁
## Motivation | ||
|
||
As a module author, | ||
I can leverage common OO design patterns that require abstract type definitions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how about "I can interoperate with .NET frameworks that require implementing interfaces'
|
||
# Native Support for .NET Interface Declarations | ||
|
||
The PowerShell Classes feature introduced in PowerShell 5.0 allows for type definitions at runtime. Since open sourcing PowerShell Core, one of the most oft-repeated feature request I've heard from community members is the ability to also declare and define interfaces at runtime, usually for ease of implementing common software design pattern such as the Builder pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include a small illustrative example of what you want to achieve?
@daxian-dbw @rjmholt are going to take a look at the prototype implementation here. @PowerShell/powershell-committee agrees we should merge this as experimental sooner rather than later to understand how folks like this might break. We realized in playing with the existing reserved keywords (
To that end, we can potentially mitigate the impact of the breakage by introducing a runtime warning or PSSA rule that gets thrown when a user defines a function called interface, and recommending that they use the call operator when invoking that function. |
A possible workaround if it's the Keyword "Interface" that's holding this up
Also I think someone asked for an implementation example.
|
@PowerShell/powershell-committee definitely agrees on the usefulness of this PR (thank you for the additional scenario, @Xpyder), and we want to eventually pursue it. However, we're concerned that we don't have the runway to ensure that this can be reviewed and sufficiently tested in real-world scenarios prior to the release of 7.2 by the end of this year. Furthermore, while it should be checked in as an experimental feature first, we don't believe that it will be easy to completely isolate the changes within an experimental feature that can be turned off. Given that 7.2 is an LTS release, we're worried that this lack of isolation may result in behaviors that we haven't fully agreed upon "leaking" out into a GA release. We should also try our best to look at other class work (like that in PowerShell/PowerShell#6652) holistically with this proposal, so that classes are designed consistently within PowerShell. However, we recognize that we've had difficulties making clear progress in that area, and this may be still be something we want to do as a standalone feature. The next step would be for @IISResetMe (or @vexx32, per his comment in PowerShell/PowerShell#2223) to open a code PR with the prototype design fenced as much as possible by an experimental feature. (Even if fencing is completely impossible, there should still be an experimental feature entry added as notation.) Community members can start commenting/reviewing on that immediately, and the expectation is that the Language Working Group could potentially do a formal review for the 7.3 timeframe at the earliest. (In my own opinion as well, I think this could be useful for the new PS7 DSC scenarios around which we're leveraging classes.) |
Thanks for the update @joeyaiello! I'm happy to update the original reference implementation so people can start testing, but I would really appreciate some guidance on the AST changes - is introducting a new set of The reason I say "break" is that, in lieu of new syntax types for abstract members, I need to introduce some mutation to |
Great question, but for the aforementioned reasons for pushing this out to 7.3, that's probably not something the experts on our team can answer in the short-term. It might be worthwhile to play with both implementations to understand the pros/cons, especially given the timeframe on us seriously looking at this. |
I'm not waiting another year for this |
RFC for new interface declaration feature, working prototype available as of IISResetMe/PowerShell@f98fb2b