-
Notifications
You must be signed in to change notification settings - Fork 18
feat: allow covariant overrides in classes #2324
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
base: main
Are you sure you want to change the base?
Conversation
df6761e
to
83e56ae
Compare
079dca9
to
bd3a0e0
Compare
bd3a0e0
to
a419c49
Compare
label: string, | ||
action: string, | ||
opts: { | ||
allowCovariance: boolean; |
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.
Return type covariance.
By the way, does that mean we also have argument type contravariance? Or no?
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.
It's only return types and readonly properties. Argument type covariance is also still not allowed.
return true; | ||
} | ||
|
||
if (!spec.isNamedTypeReference(candidateType) || !spec.isNamedTypeReference(expectedType)) { |
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.
For collections't, won't A ⊑ B ⇒ List<A> ⊑ List<B>
?
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've currently excluded lists. Will see if that's supported in C#.
// Handle class-to-class inheritance | ||
if (spec.isClassType(candidateTypeSpec) && spec.isClassType(expectedTypeSpec)) { | ||
// Check if candidateType extends expectedType (directly or indirectly) |
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.
Shouldn't we also call _interfaceExtendsInterface
here?
Co-authored-by: Rico Hermans <rix0rrr@gmail.com> Signed-off-by: Momo Kornher <mail@moritzkornher.de>
Relaxes the the restriction on covariant overrides as currently documented here:
https://aws.github.io/jsii/user-guides/lib-author/typescript-restrictions/#covariant-overrides-parameter-list-changes
Use of this new feature is indicated by the
class-covariant-overrides
feature in the produced jsii assembly.What is the change?
It is now allowed to override the type of readonly class properties and the return type of class methods in class inheritance, as long as the changes are covariant to the superclass. Importantly, we still don't allow covariant overrides when implementing interfaces.
✅ now possible
❌ still prohibited
Why is this safe now?
We can make these changes now, because C# 9 has added the necessary support, and this version of C# has been introduced in .NET 5.
In jsii we have now made the necessary change to increase the target framework to
net6.0
.C# references:
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/covariant-returns
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/override
https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/knowing-when-to-use-override-and-new-keywords
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.