Covariant return updates to ecma augments by davidwrighton · Pull Request #56496 · dotnet/runtime

davidwrighton · 2021-07-28T22:02:49Z

Update the ECMA augments file to reflect changes made in .NET 5.0 for covariant returns.

janvorli · 2021-07-28T22:08:42Z

+| C | C::VirtualFunction() | C::VirtualFunction |
+"
+
+Add a new paragraph "When a rules above result in the implementation of virtual method not satisfying the requirement that the signature of the implementor of a virtual method is not *covariant-return-compatible-with* (§I.8.7.1) the program is not considered well formed. As an example, this can happen if a virtual method is overriden with a .override directive(A) paired with a `PreserveBaseOverridesAttribute` with a *covariant-return-compatible-with* signature. and then the initial virtual method is subsequently overridden with a method which is *covariant-return-compatible-with* the initial virtual method, but not with the method body associated with .override directive(A)."


A nit "When a rules" -> "When rules"

(or maybe the rules)

(or a rule even though I'm not sure whether there's a real difference between the singular and plural in this case)

Actually, I think there's also a confusing double negative in the sentence unless I'm reading it wrong - "something results in the implementation of virtual method not satisfying that the signature of something is not covariant compatible", at the very least it's quite a bit to chew on which is the traditional hell of all specs :-).

janvorli · 2021-07-28T22:09:18Z

+Add a new paragraph "When a rules above result in the implementation of virtual method not satisfying the requirement that the signature of the implementor of a virtual method is not *covariant-return-compatible-with* (§I.8.7.1) the program is not considered well formed. As an example, this can happen if a virtual method is overriden with a .override directive(A) paired with a `PreserveBaseOverridesAttribute` with a *covariant-return-compatible-with* signature. and then the initial virtual method is subsequently overridden with a method which is *covariant-return-compatible-with* the initial virtual method, but not with the method body associated with .override directive(A)."
+
+### II.22.27
+Edit rule 12 to specify that "The method isgnature defined by *MethodBody* shall match those defined by *MethodDeclaration* exactly if *MethodDeclaration* defines a method on an interface or be covariant-return-compatible-with* (§I.8.7.1) if *MethodDeclaration* represents a method on a class."


isgnature -> signature

Missing * in front of the covariant-return-compatible-with

janvorli · 2021-07-28T22:11:10Z

+}
+```
+
+Covariant return methods can only be described through MethodImpl records, and as are only be applicable to methods on reference types. Methods on interfaces and value types are not supported. In addition the covariant override is only applicable for overriding methods also defined on a reference type.


"and as are only ..." - was it meant to be "and as such are only ..."?

trylek

Looks great to me modulo a few nits, thanks David!

trylek · 2021-07-28T22:30:54Z

+
+### II.10.3.2 The .override directive (page 147)
+
+Edit the first paragraph "The .override directive specifies that a virtual method shall be implemented (overridden), in this type, by a virtual method with a different name, but with a *covariant-return-compatible-with* signature (§I.8.7.1). This directive can be used to provide an implementation for a virtual method inherited from a base class, or a virtual method specified in an interface implemented by this type and all other. If not used to provide an implementation for a virtual method inherited from a base class the signature must be the identical."


Nit - "the signature must be the identical" - as you know I'm not a native English speaker but should this rather be the signature must be identical or alternatively the signature must be the same?

Sigh. My reading tends to gloss over that sort of issue, which is why a non-English speaker is actually quite helpful in this kind of review as you don't do that. Yes. the signature must be identical sounds right to me.

trylek · 2021-07-28T22:34:01Z

+
+### II.10.3.4 Impact of overrides on derived classes
+Add a third bullet
+"- If a virtual method is overridden via a .override directive and virtual method in parent class was overriden with a .override directive where the method body of that second .override directive is decorated with `System.Runtime.CompilerServices.PreserveBaseOverridesAttribute` then the virtual method override shall apply to the method body of the second .override directive as well.


Nit - "an .override directive"?

trylek · 2021-07-28T22:35:15Z

+| C | C::VirtualFunction() | C::VirtualFunction |
+"
+
+Add a new paragraph "When a rules above result in the implementation of virtual method not satisfying the requirement that the signature of the implementor of a virtual method is not *covariant-return-compatible-with* (§I.8.7.1) the program is not considered well formed. As an example, this can happen if a virtual method is overriden with a .override directive(A) paired with a `PreserveBaseOverridesAttribute` with a *covariant-return-compatible-with* signature. and then the initial virtual method is subsequently overridden with a method which is *covariant-return-compatible-with* the initial virtual method, but not with the method body associated with .override directive(A)."


(or maybe the rules)

trylek · 2021-07-28T22:36:27Z

+| C | C::VirtualFunction() | C::VirtualFunction |
+"
+
+Add a new paragraph "When a rules above result in the implementation of virtual method not satisfying the requirement that the signature of the implementor of a virtual method is not *covariant-return-compatible-with* (§I.8.7.1) the program is not considered well formed. As an example, this can happen if a virtual method is overriden with a .override directive(A) paired with a `PreserveBaseOverridesAttribute` with a *covariant-return-compatible-with* signature. and then the initial virtual method is subsequently overridden with a method which is *covariant-return-compatible-with* the initial virtual method, but not with the method body associated with .override directive(A)."


(or a rule even though I'm not sure whether there's a real difference between the singular and plural in this case)

trylek · 2021-07-28T22:40:48Z

+| C | C::VirtualFunction() | C::VirtualFunction |
+"
+
+Add a new paragraph "When a rules above result in the implementation of virtual method not satisfying the requirement that the signature of the implementor of a virtual method is not *covariant-return-compatible-with* (§I.8.7.1) the program is not considered well formed. As an example, this can happen if a virtual method is overriden with a .override directive(A) paired with a `PreserveBaseOverridesAttribute` with a *covariant-return-compatible-with* signature. and then the initial virtual method is subsequently overridden with a method which is *covariant-return-compatible-with* the initial virtual method, but not with the method body associated with .override directive(A)."


Actually, I think there's also a confusing double negative in the sentence unless I'm reading it wrong - "something results in the implementation of virtual method not satisfying that the signature of something is not covariant compatible", at the very least it's quite a bit to chew on which is the traditional hell of all specs :-).

janvorli · 2021-07-28T23:03:02Z

+}
+```
+
+Covariant return methods can only be described through MethodImpl records, and are only be applicable to methods on non-interface reference types. In addition the covariant override is only applicable for overriding methods also defined on a non-interface reference type.


One last: "are only be applicable" -> "are only applicable"

Nice, I missed that one :D.

davidwrighton · 2021-07-28T23:25:34Z

@trylek @janvorli I rewrote the error case to be part of the bullet point and added an example. I think that makes it a fair bit clearer.

janvorli

LGTM, thank you!

trylek · 2021-07-28T23:29:48Z

+| C | C::VirtualFunction() | C::VirtualFunction | |
+| D | B::VirtualFunction() | ERROR | A program containing type D is not valid, as B::VirtualFunction would be implemented by D::VirtualFunction which is not *covariant-return-compatible-with* (§I.8.7.1) B::VirtualFunction |
 "
-


Wow, I didn't expect such a profound effect on the wording, I really appreciate your diligence towards polishing the spec!

Covariant return updates to ecma augments

4909f56

davidwrighton requested review from janvorli, mangod9 and trylek July 28, 2021 22:02

ghost added the area-TypeSystem-coreclr label Jul 28, 2021

Fix linter issues.

4ccdd73

janvorli reviewed Jul 28, 2021

View reviewed changes

Feedback

e3ec229

trylek approved these changes Jul 28, 2021

View reviewed changes

janvorli reviewed Jul 28, 2021

View reviewed changes

Make error scenario clearer, and fix more typos

b7074a2

janvorli approved these changes Jul 28, 2021

View reviewed changes

trylek reviewed Jul 28, 2021

View reviewed changes

davidwrighton merged commit 5d4d913 into dotnet:main Jul 29, 2021

ghost locked as resolved and limited conversation to collaborators Aug 28, 2021

davidwrighton deleted the ecma_augments branch April 13, 2023 18:49


		### II.10.3.2 The .override directive (page 147)

		Edit the first paragraph "The .override directive specifies that a virtual method shall be implemented (overridden), in this type, by a virtual method with a different name, but with a covariant-return-compatible-with signature (§I.8.7.1). This directive can be used to provide an implementation for a virtual method inherited from a base class, or a virtual method specified in an interface implemented by this type and all other. If not used to provide an implementation for a virtual method inherited from a base class the signature must be the identical."

Conversation

davidwrighton commented Jul 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented Jul 28, 2021

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants