Skip to content
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

ECMA spec addendum dealing with static interface methods #49558

Merged
merged 16 commits into from
May 14, 2021

Conversation

trylek
Copy link
Member

@trylek trylek commented Mar 12, 2021

Opening this review to discuss ECMA spec amendments related to static interface methods.

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

This is my first pass at this. I think we'll need a longer writeup for the IL opcode(s), but its a good first step here.

docs/design/specs/Ecma-335-Augments.md Outdated Show resolved Hide resolved
docs/design/specs/Ecma-335-Augments.md Outdated Show resolved Hide resolved
docs/design/specs/Ecma-335-Augments.md Show resolved Hide resolved
docs/design/specs/Ecma-335-Augments.md Outdated Show resolved Hide resolved
docs/design/specs/Ecma-335-Augments.md Show resolved Hide resolved
docs/design/specs/Ecma-335-Augments.md Outdated Show resolved Hide resolved
@davidwrighton
Copy link
Member

I've spoken to Mads and the rest of the static interfaces group, and there is general agreement that we'll need delegates and possibly function pointers.

@davidwrighton
Copy link
Member

Continuing suggestions

Continuing suggestions for spec changes.

[II.22.26] MethodDef
Replace 7.a "Static | Virtual" in the metadata validation rules with "Static | Virtual | Abstract"

Add a new metadata validation rule
41. If the owner of this method is not an interface, then if Flags.Static is 1 then Flags.Virtual must be 0.

[II.22.27] MethodImpl
Change metadata validation rule 7.
Replace "The method indexed by MethodBody shall be virtual" with "The method indexed by MethodBody shall be non-virtual if the method indexed by MethodDeclaration is static. Otherwise it shall be virtual."

[III.2.1] constrained prefix
Adjust stack transition diagram into a table.

| Prefix and instruction pair | Stack Transition
| constrained. thisType callvirt method | ..., ptr, arg1, ... argN -> ..., ptr, arg1, ... argN
| constrained. implementorType call method | ..., arg1, ... argN -> ..., arg1, ... argN
| constrained. implementorType ldftn method | ..., ftn -> ..., ftn

Replace

"The constrained. prefix is permitted only on a callvirt instruction. The" with

"The constrained. prefix is permitted on the callvirt, call and ldftn instructions.

When followed by the callvirt instruction, the"

Then at the end of the description add the following text.

"When followed by the call instruction or the ldftn instruction, method must refer to a virtual static method defined on an interface. The behavior of the constrained. prefix is to change the method that the call or ldftn instruction refer to to be the method on implementorType which implements the virtual method described by method as defined in partition II, Introducing and overriding virtual methods.

In the correctness section replace "callvirt instruction" with "callvirt, call, or ldftn instruction"

In the verifiability section add "The implementorType must be constrained to implement the interface that is the owning type of method. If the constrained. prefix is applied to a call or ldftn instruction, method must be a virtual static method."

@trylek
Copy link
Member Author

trylek commented Mar 20, 2021

@davidwrighton - I am somewhat confused about your comment

[II.22.26] MethodDef
Replace 7.a "Static | Virtual" in the metadata validation rules with "Static | Virtual | Abstract"

7.a is "static | final" and 7.b is "static | virtual". Assuming you suggest to modify the section 7.b that seems to better match your quotation, does that mean that "static virtual abstract" methods are invalid? Please note that the section 7 refers to invalid flag combinations. I suspect we probably need something like Static | Virtual | !Abstract but I'm not sure how to express this.

@trylek trylek force-pushed the StaticInterfacesEcmaAddendum branch 3 times, most recently from ff50b0f to 2bd775e Compare March 20, 2021 23:02
@trylek
Copy link
Member Author

trylek commented Mar 20, 2021

@davidwrighton - I believe I have addressed all your PR feedback, can you please take another look when you have a chance? From the E2E perspective, I only have a bunch of open questions and I haven't yet populated the "examples" section, otherwise I believe I've covered all relevant sections that you and I found in the ECMA spec.

@trylek trylek force-pushed the StaticInterfacesEcmaAddendum branch 2 times, most recently from 56564d1 to fe12ad5 Compare March 21, 2021 00:09
@davidwrighton
Copy link
Member

@trylek, I think this is close enough for us to start working on runtime test cases/implementation.

@AlekseyTs, I'd like you to take a look through the changes and see if they seem appropriate to you as well.

(Add second paragraph)

Static interface methods may be marked as virtual. Valid object types implementing such interfaces shall provide implementations
for these methods by means of Method Implementations (II.15.1.4). Polymorphic behavior of calls to these methods is facilitated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think virtual methods don't have to be implemented. They can be, but don't have to be.

@trylek
Copy link
Member Author

trylek commented Mar 23, 2021

@davidwrighton - I have added an initial attempt at static interface method examples; I apologize to the C# team for tons of bugs I have certainly made in the syntax, writing valid code that doesn't compile is hard. Cc @tannergooding as the first examples I tried to provide are mostly based on the polymorphic math. I'll welcome any feedback as to how to steer this section of the proposed spec augment.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 23, 2021

Should we reinstate variance safety requirement for signatures of virtual static interface methods? Recently we removed that requirement for static methods since they couldn't be virtual and were not subject to variance.

@davidwrighton
Copy link
Member

@AlekseyTs I'm not familiar with the details of the variance safety work. Could you provide a reference? At first blush I don't think we need any adjustment, as this form of variant dispatch is not effected by the instance type of one of the parameters, but I'd need more details to respond with confidence.

@davidwrighton
Copy link
Member

@trylek With respect to the static method examples, I believe they are roughly correct and good enough for now, but we'll need to adjust them to be proper C# once the design is finalized. As compared to the BCL design that @tannergooding is working on, I'm not sure that we need them to match up at all. although we might want the example to use the curiously recurring generic pattern.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Mar 24, 2021

@davidwrighton

I'm not familiar with the details of the variance safety work. Could you provide a reference? At first blush I don't think we need any adjustment, as this form of variant dispatch is not effected by the instance type of one of the parameters, but I'd need more details to respond with confidence.

See #40152. It feels like the "instance type" (not a parameter type) can actually implement variant "compatible" interface and, since virtual static methods is now a subject for a virtual dispatch, variance safety rules should be enforced for them.

A generic parameter declared on a generic class or generic method can be *constrained* by one or more
type (for encoding, see *GenericParamConstraint* table in paragraph II.22.21) and by one or more special
constraints (paragraph II.10.1.7). Generic parameters can be instantiated only with generic arguments that are
*assignable-to* (paragraph I.8.7.3) (when boxed) and *implements-all-static-interface-methods-of* (**paragraph
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to use wording used in proposed language specification at https://github.com/dotnet/csharplang/blob/main/proposals/statics-in-interfaces.md#interface-constraints-with-static-abstract-members? Specifically: "When I has static abstract members this needs to be further restricted so that T cannot itself be an interface."

Copy link
Member

Choose a reason for hiding this comment

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

That would be an additional restriction on top of this one. This is designed to cover the case where abstract classes can be used as generic parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be an additional restriction on top of this one.

Could you elaborate please?

This is designed to cover the case where abstract classes can be used as generic parameters.

Abstract classes are not interfaces. Right?

Copy link
Member

Choose a reason for hiding this comment

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

So, we need to restrict the generic to fully implement the constraint. Normally, this is done by requiring that the type fully implement the interface through one of two mechanisms.

  1. The type must be a reference type, in which case any actual object instance must be a non-abstract type and therefore have a full implementation of the interfaces it and its base types claim to implement.
  2. The type must be a valuetype which must by definition cannot be abstract type, and thus satisfy the rule above.

With the addition of the static interfaces feature, instantiations over abstract classes bring along the possibility that the type may not fully implement the interface. Thus we need to protect against that.

Now, as you note, the C# language proposal suggests that we simply prevent any interface from being used as a generic for a static interface. I would prefer to word this restriction in a different way. My preference would be to requite that the type which is used as the generic parameter to provide a full set of implementations, and to then restrict the ability to use a MethodImpl on an interface type to work with these static methods.

@trylek trylek force-pushed the StaticInterfacesEcmaAddendum branch from d547b9c to d596870 Compare April 20, 2021 12:40
AlekseyTs added a commit to dotnet/roslyn that referenced this pull request Apr 30, 2021
…in classes and structures (#52969)

In metadata an implementation should be a static, not newslot, not final, not virtual, not abstract method. Type should have a MethodImpl entry pointing to the ”body” method with a MethodDef index. The “body” must be a method declared within the type. There is no concept of an implicit interface implementation for static methods in metadata. I.e., if there is no corresponding MethodImpl entry, the method isn’t an implementation from runtime point of view.

Language supports both implicit and explicit interface implementation.

Relevant ECMA-335 changes PR - dotnet/runtime#49558
Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
@trylek trylek merged commit 0979434 into dotnet:main May 14, 2021
@trylek trylek deleted the StaticInterfacesEcmaAddendum branch May 14, 2021 22:14
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants