Skip to content

Add support for implementing static members in derived interfaces.#61134

Merged
AlekseyTs merged 2 commits intodotnet:features/DefaultInterfaceImplementationfrom
AlekseyTs:DefaultInterfaceImplementation_105
May 10, 2022
Merged

Add support for implementing static members in derived interfaces.#61134
AlekseyTs merged 2 commits intodotnet:features/DefaultInterfaceImplementationfrom
AlekseyTs:DefaultInterfaceImplementation_105

Conversation

@AlekseyTs
Copy link
Copy Markdown
Contributor

https://github.com/dotnet/csharplang/blob/main/proposals/static-abstracts-in-interfaces.md

Test plan: #60968

Tests in DefaultInterfaceImplementationTests.cs are adjusted only for methods. Tests for other members will be adjusted separately.

@AlekseyTs
Copy link
Copy Markdown
Contributor Author

@333fred, @jcouv, @dotnet/roslyn-compiler Please review

Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@AlekseyTs
Copy link
Copy Markdown
Contributor Author

@jcouv, @dotnet/roslyn-compiler For the second review.

}

if (((!hasBody && IsAbstract) || IsVirtual) && IsStatic && !ContainingAssembly.RuntimeSupportsStaticAbstractMembersInInterfaces)
if (((!hasBody && IsAbstract) || IsVirtual) && !isExplicitInterfaceImplementation && IsStatic && !ContainingAssembly.RuntimeSupportsStaticAbstractMembersInInterfaces)
Copy link
Copy Markdown
Member

@jcouv jcouv May 9, 2022

Choose a reason for hiding this comment

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

isExplicitInterfaceImplementation

Do we need the additional parameter given that we have IsExplicitInterfaceImplementation property (used in this method already)? #Pending

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we need the additional parameter given that we have IsExplicitInterfaceImplementation property (used in this method already)?

Good catch, I will follow up on this in a separate PR.

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 2)

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 9, 2022

The spec probably needs updating to reflect decisions as opposed to options: https://github.com/dotnet/csharplang/blob/main/proposals/static-abstracts-in-interfaces.md#default-implementations

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LTGM Thanks (iteration 2). Extra parameter can be removed in later PR

@AlekseyTs AlekseyTs merged commit 4387960 into dotnet:features/DefaultInterfaceImplementation May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants