Enable a default implementation of an interface event to be provided as part of its declaration. #18183
Conversation
…as part of its declaration.
AlekseyTs
added
Area-Compilers
Language-C#
Feature - Default Interface Impl
|
@dotnet/roslyn-compiler Please review. |
|
|
||
| Validate2(compilation2.SourceModule); | ||
| compilation2.VerifyDiagnostics(); | ||
| CompileAndVerify(compilation2, verify: false, symbolValidator: Validate2); |
There was a problem hiding this comment.
I don't remember if I asked already: should we track updating PEVerify to understand the newly produced assemblies?
There was a problem hiding this comment.
PEVerify doesn't even handle C# 7 constructs. We should track that work, but I don't expect it will be in scope of completing and shipping this language feature.
There was a problem hiding this comment.
This tracked in #17952.
| ); | ||
|
|
||
| ValidateEventImplementation_501(compilation1.SourceModule, "Test1"); | ||
| } |
There was a problem hiding this comment.
Maybe I missed it, but I didn't see the happy path run end-to-end (CompileAndVerify with expectedOutput).
There was a problem hiding this comment.
No you didn't miss it. At the moment tests cannot target runtime that supports the feature. Tracked in #17952 as well. We verify symbols for now.
| Assert.Equal("I1", test2Result.Interfaces.Single().ToTestDisplayString()); | ||
| ValidateEventImplementation_501(m, "Test2"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I remember there is a special kind of events for windows runtime. Is it worth covering too?
There was a problem hiding this comment.
I plan to cover them eventually, tracked in #17952.
|
|
||
| compilation1.VerifyDiagnostics( | ||
| // (6,12): error CS0073: An add or remove accessor must have a body | ||
| // add; |
There was a problem hiding this comment.
Feels fine to me and is not caused by the change. Abstract events must omit accessors list. Non-abstract custom events must provide bodies for both accessors. This behavior feels reasonable to me.
There was a problem hiding this comment.
The diagnostic seems to complain about the add;, but you're saying it complains about the remove.
There was a problem hiding this comment.
I see what you mean now, yes the comment is out of sync with the test scenario. I'll adjust it.
| Assert.True(compilation1.Assembly.RuntimeSupportsDefaultInterfaceImplementation); | ||
|
|
||
| compilation1.VerifyDiagnostics( | ||
| // (4,32): error CS0106: The modifier 'static' is not valid for this item |
There was a problem hiding this comment.
I know it was not your intent to support it yet, but the appropriate diagnostic here should be that 'static' in an interface is not supported in language version '7'; use language version '7.1'. Please add a PROTOTYPE comment noting that will need to change.
There was a problem hiding this comment.
Feature MD file clearly states that static members in interfaces are not supported (by explicitly listing what is supported). I do not think there is a good reason to add a PROTOTYPE comment to duplicate that.
There was a problem hiding this comment.
Let me put it this way: this test contradicts the feature specification. You should not add tests that ensure that the feature implementation behaves in a way contrary to the feature specification.
There was a problem hiding this comment.
I am adding tests to test the current state of the feature, my expectations. What spec says is irrelevant, until I claim that the relevant portion of the spec is implemented.
@dotnet/roslyn-compiler Please review.