Add initial support for specifying explicit base type in the base access. #33281

AlekseyTs · 2019-02-11T04:44:00Z

Relevant LDM notes https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-11-14.md .

Related to #32054.

…ess. Relevant LDM notes https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-11-14.md .

AlekseyTs · 2019-02-11T16:33:49Z

@dotnet/roslyn-compiler Please review. #Closed

AlekseyTs · 2019-02-11T22:53:11Z

@dotnet/roslyn-compiler Please review. #Closed

jaredpar · 2019-02-12T00:04:35Z

src/Test/Utilities/Portable/Assert/ConditionalFactAttribute.cs

+    public class CoreClrOnly : ExecutionCondition
+    {
+        public override bool ShouldSkip => !ExecutionConditionUtil.IsCoreClr;
+        public override string SkipReason => "Test supported only on CoreCLR";


Was a bit surprised we didn't have this type yet. #Resolved

jaredpar · 2019-02-12T00:10:08Z

    System.Console.WriteLine(base(C).F1);

The LDM notes didn't mention base calls to non-interface members that I saw in my reading. Did we leave that out of the notes my mistake? If so we should get @agocke to update #Resolved

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34278 in de15237. [](commit_id = de15237, deletion_comment = False)

jaredpar · 2019-02-12T00:16:26Z

C

Trying to understand the apparent behavior difference between ExplicitBase_011 and this test. In ExplicitBase_011 is C printed both times becuse the member on B is private and thus inaccessible? But here the member on B and C are both accessible hence each can be used? #Resolved

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34522 in de15237. [](commit_id = de15237, deletion_comment = False)

jaredpar · 2019-02-12T00:25:21Z

Have to step away for a bit (through line 35,500 in the test file) #Resolved

AlekseyTs · 2019-02-12T00:26:36Z

    System.Console.WriteLine(base(C).F1);

The LDM notes didn't mention base calls to non-interface members that I saw in my reading. Did we leave that out of the notes my mistake? If so we should get @agocke to update

Existing base access is not limited to virtual members or methods. At LDM we didn't state this explicitly, but I assumed that the new form of base access should have the same property. We'll reconfirm this at LDM.

In reply to: 462547446 [](ancestors = 462547446)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34278 in de15237. [](commit_id = de15237, deletion_comment = False)

AlekseyTs · 2019-02-12T00:30:10Z

C

Trying to understand the apparent behavior difference between ExplicitBase_011 and this test. In ExplicitBase_011 is C printed both times becuse the member on B is private and thus inaccessible? But here the member on B and C are both accessible hence each can be used?

Yes, that is correct.

In reply to: 462550302 [](ancestors = 462550302)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34522 in de15237. [](commit_id = de15237, deletion_comment = False)

jaredpar · 2019-02-12T00:34:19Z

    public void ExplicitBase_028()

Consider a similar case but where A is nested inside B and hence does have access to the property. Possible missed this test or it's coming up but haven't seen the pattern yet. #Resolved

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35038 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-12T00:45:57Z

docs/features/DefaultInterfaceImplementation.md

@@ -43,16 +43,41 @@ class Test1 : I1

 - Declaring types within interfaces. **Protected** and **protected internal** accessibility is not supported.

- Implementing interface methods in derived interfaces by using explicit implementation syntax, accessibility is private, allowed modifiers: **extern** and **async**.
+- Implementing interface methods in derived interfaces by using explicit implementation syntax, accessibility matches accessibility of the implemented member, allowed modifiers: **extern** and **async**.


matches accessibility of the implemented member [](start = 110, length = 47)

This doesn't ensure that the member is accessible to the caller. For example, an internal implementation in one assembly can be implemented in another assembly with internals-visible-to access to the first. A third assembly with internal-visible-to the original interface but not the override's assembly would not have access to that override. To ensure the override is accessible to the caller (and nowhere else), explicit override methods should be given protected access. #Resolved

This doesn't ensure that the member is accessible to the caller.

I am not claiming that it is.

To ensure the override is accessible to the caller (and nowhere else), explicit override methods should be given protected access.

In general, giving protected access would be incorrect because that might expose otherwise inaccessible types.

So far, we disallow declaring protected members in interfaces and haven't defined what protected access means for interface members both for the language and for the runtime.

That is why I am starting with the same accessibility and we will refine this once we discuss design issues at LDM

In reply to: 255757955 [](ancestors = 255757955)

Can you please explain how declaring a protected member in IL (that has an unutterable name) could "expose" an inaccesible type? I'm assuming we would not change the accessibility of types just because we emit symbols that use them.

In reply to: 255762670 [](ancestors = 255762670,255757955)

Can you please explain how declaring a protected member in IL (that has an unutterable name) could "expose" an inaccesible type? I'm assuming we would not change the accessibility of types just because we emit symbols that use them.

The name doesn't matter in my opinion. Original member declaration can be internal and, therefore, refer to internal types. It would be illegal to use those types in a protected member.

In reply to: 255770151 [](ancestors = 255770151,255762670,255757955)

It is possible that protected access would work, if we knew what that means. As I said, this is a topic for an LDM.

In reply to: 255770872 [](ancestors = 255770872,255770151,255762670,255757955)

Yes, this PR updates the speclet and the implementation to match each other, though it produces cases that require that we generate illegal IL.

Could you elaborate, give an example please?

In reply to: 256257520 [](ancestors = 256257520,256257340,256177815,255803908,255803140,255772556,255770872,255770151,255762670,255757955)

Yes, this PR updates the speclet and the implementation to match each other, though it produces cases that require that we generate illegal IL.

Could you elaborate, give an example please?

An example is

// assembly1 [InternalsVisibleTo("assembly2")] [InternalsVisibleTo("assembly3")] public interface I1 { internal void M() {} }

// assembly2 public interface I2 : i1 { void I1.M() {} }

// assembly3 public interface I3: I1, I2 { void I1.M() { // this call must emit an invocation of I2.M // but that is not CLR-accessible in the speclet's translation scheme base(I2).M(); } }

In reply to: 256265573 [](ancestors = 256265573,256257520,256257340,256177815,255803908,255803140,255772556,255770872,255770151,255762670,255757955)

An example is
...

// this call must emit an invocation of I2.M // but that is not CLR-accessible in the speclet's translation scheme base(I2).M();

I think this is misinterpretation of the speclet and misinterpretation of the implementation:

The speclet says: "The most specific implementation must be accessible at the call site." This is a requirement for the base access that should be met, not a guarantee.

According to the above, compiler shouldn't emit a call to the inaccessible method in the example, but should report an error. Unit-test ExplicitBase_152 clearly demonstrates this behavior.

Do you still believe that the speclet "produces cases that require that we generate illegal IL"?

In reply to: 256314854 [](ancestors = 256314854,256265573,256257520,256257340,256177815,255803908,255803140,255772556,255770872,255770151,255762670,255757955)

Do you still believe that the speclet "produces cases that require that we generate illegal IL"?

No, I now believe that the speclet describes an implementation that fails to implement the language specification. The only accessibility requirement on a member accessed through an explicit base is the existing spec constraint that a member found through lookup is accessible. In this case the member found (the original interface definition) is indeed accessible. The constraint "The most specific implementation must be accessible at the call site." is one that doesn't appear in the language specification. It wouldn't even make sense there, as explicit overrides do not have accessibility in the language spec and are not found during name lookup.

In short, unit test ExplicitBase_152 has errors that are not supported by the language.

In reply to: 256494259 [](ancestors = 256494259,256314854,256265573,256257520,256257340,256177815,255803908,255803140,255772556,255770872,255770151,255762670,255757955)

I am glad this is resolved now. And thank you for sharing your thoughts about the feature design.

In reply to: 256640387 [](ancestors = 256640387,256494259,256314854,256265573,256257520,256257340,256177815,255803908,255803140,255772556,255770872,255770151,255762670,255757955)

gafter · 2019-02-12T00:48:51Z

docs/features/DefaultInterfaceImplementation.md


- Implementing interface events in derived interfaces by using explicit implementation syntax, accessibility is private, no allowed modifiers.
+- Implementing interface events in derived interfaces by using explicit implementation syntax, accessibility matches accessibility of the implemented member, no allowed modifiers.

 - Declaring static fields, auto-properties and field-like events (**protected** modifier is not allowed).


protected modifier is not allowed [](start = 66, length = 37)

This constraint is not correct. Just as protected static fields are permitted in classes, if fields et al are permitted at all, then protected versions of them should be permitted in interfaces. However the current spec forbids these categories of symbols. #Closed

This constraint is not correct. Just as protected static fields are permitted in classes, if fields et al are permitted at all, then protected versions of them should be permitted in interfaces. However the current spec forbids these categories of symbols.

So far, we didn't define what protected access means for interface members. This document accurately reflects that. We can revisit this at LDM.

In reply to: 255758478 [](ancestors = 255758478)

gafter · 2019-02-12T00:51:52Z

docs/features/DefaultInterfaceImplementation.md

@@ -43,16 +43,41 @@ class Test1 : I1

 - Declaring types within interfaces. **Protected** and **protected internal** accessibility is not supported.


accessibility is not supported [](start = 78, length = 30)

These constraints are not correct. #Closed

jaredpar · 2019-02-12T00:52:48Z

", verify: VerifyOnCoreClr);

This pattern reminds me I should revive this PR so that it's implicit for changis like this: #32917 #Resolved

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:40378 in de15237. [](commit_id = de15237, deletion_comment = False)

jaredpar · 2019-02-12T00:53:50Z

src/Compilers/CSharp/Portable/Syntax/BaseExpressionSyntax.cs

+    {
+        public BaseExpressionSyntax Update(SyntaxToken token)
+        {
+            return Update(token, null);


Can we make this null a named argument? #Resolved

jaredpar · 2019-02-12T00:56:47Z

    [WorkItem(33256, "https://github.com/dotnet/roslyn/issues/33256")]

Looked at the bug and test and I'm unsure what errors occur on desktop here. The comment was

Observed: An error is reported for compilation1

Can you elaborate on that? #Resolved

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:42061 in de15237. [](commit_id = de15237, deletion_comment = False)

jaredpar · 2019-02-12T01:00:13Z

One test I didn't see was use of compound assignment on a base property / index call where either the get or set was inaccessible. Saw plenty of non-compound operator verification here . #Resolved

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:46046 in de15237. [](commit_id = de15237, deletion_comment = False)

AlekseyTs · 2019-02-12T01:03:02Z

    public void ExplicitBase_028()

Consider a similar case but where A is nested inside B and hence does have access to the property. Possible missed this test or it's coming up but haven't seen the pattern yet.

I am not sure why would nesting make protected member inaccessible. In any case, it is not a goal to test all possible ways why a member might be inaccessible. I just test some limited set to make sure things are not horribly broken.

In reply to: 462555961 [](ancestors = 462555961)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35038 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-12T01:06:46Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

-                    return false;
+                    var baseReference = (BoundBaseReference)receiver;
+
+                    if (!eventSymbol.IsImplementableInterfaceMember() ||


IsImplementableInterfaceMember [](start = 37, length = 30)

Just as you can use a base invocation to a non-virtual member in a class, base invocations to an interface do not require that the symbol found is virtual or abstract. #Closed

Just as you can use a base invocation to a non-virtual member in a class, base invocations to an interface do not require that the symbol found is virtual or abstract.

Completely agree and tests confirm that.

In reply to: 255762096 [](ancestors = 255762096)

jaredpar · 2019-02-12T01:07:36Z

    public void ExplicitBase_028()

Sorry to clarify I meant a case where the private member becomes accessible because the derived type is a nested type.

In reply to: 462564849 [](ancestors = 462564849,462555961)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35038 in de15237. [](commit_id = de15237, deletion_comment = False)

jaredpar · 2019-02-12T01:08:56Z

    public void ExplicitBase_028()

I'll leave it up to you if you think that's warranted or sufficiently covered by the existing cases.

In reply to: 462566187 [](ancestors = 462566187,462564849,462555961)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35038 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-12T01:13:18Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

+                {
+                    if (TypeSymbol.TryFindBaseImplementationInInterface((NamedTypeSymbol)baseReference.ExplicitBaseReferenceOpt.Type, method, diagnostics, node, this) is null)
+                    {
+                        return true;


return true [](start = 24, length = 11)

Should an error be reported before returning true? #Closed

I see that is done in TryFindBaseImplementationInInterface

In reply to: 255764725 [](ancestors = 255764725)

gafter · 2019-02-12T01:18:47Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

+            Symbol conflictingImplementation1 = null;
+            Symbol conflictingImplementation2 = null;
+            Symbol implementingMember = TypeSymbol.FindImplementationInInterface(method, baseInterface) ??
+                                        TypeSymbol.FindMostSpecificImplementation(method, baseInterface,


It isn't clear how this handles reabstraction https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md#reabstraction . I would expect this to be able to return an abstract override if that is most specific. But the caller must be prepared to report that the most specific method is abstract. Similarly, when one branch of a diamond is reabstracted and the other is concrete, there is no most specific override. Do we have tests showing these scenarios in action? #Closed

It isn't clear how this handles reabstraction

We do not support reabstraction. However, for the case of invalid metadata, we still check for an abstract method below:

else if (implementingMember.IsAbstract)

There are tests that test this scenario

In reply to: 255765763 [](ancestors = 255765763)

I see that reabstraction is on the TODO list #17952

In reply to: 255766000 [](ancestors = 255766000,255765763)

I see that reabstraction is on the TODO list #17952

We are not planning to support it because the syntax (an explicit implementation) doesn't allow that. I will make a note in the issue.

In reply to: 255767363 [](ancestors = 255767363,255766000,255765763)

The intent is to extend the syntax to allow abstract void I.M();

In reply to: 255768706 [](ancestors = 255768706,255767363,255766000,255765763)

The intent is to extend the syntax to allow abstract void I.M();

I am not aware of plans to do this for C# 8.

In reply to: 255775466 [](ancestors = 255775466,255768706,255767363,255766000,255765763)

This is required for compatibility with Java, which is one of our principal use cases. I've added an open LDM issue for what syntax change we intend to support this.

In reply to: 255775466 [](ancestors = 255775466,255768706,255767363,255766000,255765763)

AlekseyTs · 2019-02-12T01:18:58Z

    [WorkItem(33256, "https://github.com/dotnet/roslyn/issues/33256")]

Can you elaborate on that?

Look inside the unit-test

            compilation1.VerifyDiagnostics(
                // (2,11): error CS0535: 'A' does not implement interface member 'C.F1'
                // class A : B
                Diagnostic(ErrorCode.ERR_UnimplementedInterfaceMember, "B").WithArguments("A", "C.F1").WithLocation(2, 11)
                );

In reply to: 462562841 [](ancestors = 462562841)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:42061 in de15237. [](commit_id = de15237, deletion_comment = False)

AlekseyTs · 2019-02-12T01:28:15Z

One test I didn't see was use of compound assignment on a base property / index call where either the get or set was inaccessible. Saw plenty of non-compound operator verification here .

I didn't think covering all possible permutations is necessary. Otherwise the test matrix would explode. I think that accessibility for regular get set scenarios is covered and that should be sufficient. Let me know if you disagree.

In reply to: 462563930 [](ancestors = 462563930)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:46046 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-12T04:27:38Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs

+                if (method.IsImplementableInterfaceMember() &&
+                    baseReference.ExplicitBaseReferenceOpt?.Type.IsInterfaceType() == true)
+                {
+                    method = TypeSymbol.TryFindBaseImplementationInInterface((NamedTypeSymbol)baseReference.ExplicitBaseReferenceOpt.Type, method,


TryFindBaseImplementationInInterface [](start = 40, length = 36)

Didn't we do this during binding? It feels like we might have been able to store the result at that time, to ensure the semantics are preserved rather than recomputed. #Closed

Didn't we do this during binding? It feels like we might have been able to store the result at that time, to ensure the semantics are preserved rather than recomputed.

I do not believe re-computing is going to create a problem, we are re-computing accessors for properties and events in general and do not store them in the bound tree. This is not different, just one more step in re-computation.

In reply to: 255799212 [](ancestors = 255799212)

gafter · 2019-02-12T04:48:10Z

    System.Console.WriteLine(base(C).F1);

Added to dotnet/csharplang#406 and queued for LDM resolution in dotnet/csharplang#2215

In reply to: 462553606 [](ancestors = 462553606,462547446)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34278 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:03:02Z

Done with review pass (iteration 1) #Resolved

gafter · 2019-02-13T06:10:29Z

            // (6,17): error CS0572: 'C': cannot reference a type through an expression; try 'B.C' instead

The relevant error appears on base(B).C, none of which is for an event or a field.

In reply to: 463054068 [](ancestors = 463054068,462998426)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35727 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:10:50Z

    public void ExplicitBase_020()

The relevant error appears on base(B).C, none of which is for a property or a field.

In reply to: 463052339 [](ancestors = 463052339,462997039)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34741 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:11:17Z

    public void ExplicitBase_030()

The relevant error appears on base(B).C, none of which is for a property or a field.

In reply to: 463052680 [](ancestors = 463052680,462997080)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35100 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:11:35Z

    public void ExplicitBase_040()

The relevant error appears on base(B).C, none of which is for an indexer or a field.

In reply to: 463052832 [](ancestors = 463052832,462997240)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35463 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:11:49Z

    public void ExplicitBase_054()

The relevant error appears on base(B).C, none of which is for a method or a field.

In reply to: 463053146 [](ancestors = 463053146,462997364)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:36312 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:12:04Z

    public void ExplicitBase_066()

The relevant error appears on base(B).C, none of which is for a property or a field.

In reply to: 463053345 [](ancestors = 463053345,462997487)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:37217 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:12:20Z

    public void ExplicitBase_077()

The relevant error appears on base(B).C, none of which is for an indexer or a field.

In reply to: 463053482 [](ancestors = 463053482,462997667)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:38016 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter · 2019-02-13T06:12:39Z

    public void ExplicitBase_084()

The relevant error appears on base(B).C, none of which is for an event or a field.

In reply to: 463053767 [](ancestors = 463053767,462997777)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:38673 in de15237. [](commit_id = de15237, deletion_comment = False)

AlekseyTs · 2019-02-13T06:16:19Z

I would like to see a test for the scenario discussed in #33281 (comment) whereby an internal interface method in assembly 1 is accessible to the override in assembly 2 due to IVT, but the method in assembly 2 we need to invoke from assembly 3 (which derives from the type in assembly 2) is not accessible to the caller (because it is internal in assembly 2 to which we do not have IVT access). If we will not generate an invocation to a method that is CLR-accessible, then we should produce an error instead.

Added ExplicitBase_151 and ExplicitBase_152.

In reply to: 463001581 [](ancestors = 463001581)

AlekseyTs · 2019-02-13T06:26:51Z

I would like to see a test demonstrating that base(T) is rejected when T is the current type (class and interface).

Added ExplicitBase_153 and ExplicitBase_154

In reply to: 463005290 [](ancestors = 463005290)

AlekseyTs · 2019-02-13T06:28:21Z

Can you please push those changes to the PR?

I am still working on addressing other feedback, and making notes as I go. I'll ping once I push.

In reply to: 463071174 [](ancestors = 463071174,463065586,463005177)

AlekseyTs · 2019-02-13T06:44:40Z

    public void ExplicitBase_028()

Sorry to clarify I meant a case where the private member becomes accessible because the derived type is a nested type.

Added ExplicitBase_155 and ExplicitBase_156

In reply to: 462566187 [](ancestors = 462566187,462564849,462555961)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:35038 in de15237. [](commit_id = de15237, deletion_comment = False)

AlekseyTs · 2019-02-13T06:51:57Z

    public void ExplicitBase_020()

The relevant error appears on base(B).C, none of which is for a property or a field.

I prefer to cover all kinds of members.

In reply to: 463072801 [](ancestors = 463072801,463052339,462997039)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34741 in de15237. [](commit_id = de15237, deletion_comment = False)

AlekseyTs · 2019-02-13T06:53:38Z

    public void ExplicitBase_020()

And I also do find testing for CS0120 useful for each kind of member. Both errors are relevant in my opinion.

In reply to: 463081171 [](ancestors = 463081171,463072801,463052339,462997039)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34741 in de15237. [](commit_id = de15237, deletion_comment = False)

AlekseyTs · 2019-02-13T07:00:10Z

@jaredpar, @gafter Addressed feedback.

gafter · 2019-02-13T09:50:14Z

    public void ExplicitBase_020()

As CS0120 is a cascaded error, it doesn't matter if we produce it or not (and it would be better not to).

In reply to: 463081525 [](ancestors = 463081525,463081171,463072801,463052339,462997039)

Refers to: src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs:34741 in de15237. [](commit_id = de15237, deletion_comment = False)

gafter

As described at #33281 (comment) and confirmed by you at #33281 (comment), and as shown by the unit-test ExplicitBase_152, this speclet and implementation strategy fail to implement the specification at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md#base-interface-invocations . The simplest change to the strategy implemented here to comply with the specification would be to produce a protected implementation method when an interface implements an internal or private protected method in another assembly.

jaredpar · 2019-02-14T16:55:26Z

@gafter

Reading through the spec proposal I don't see any mention of protected here or a detailed discussion of how this scenario should play out across assembly boundaries. It does seem like an area that we need to get a bit of clarity on and possibly adjust the implementation based on that. But I don't think we should be blocking the PR here. Rather I think we should add a PROTOTYPE comment for this, get this on the LDM schedule and continue iterating on this feature.

AlekseyTs · 2019-02-14T17:14:55Z

There is already an issue #32054 that tracks base-access work item.

gafter · 2019-02-15T20:58:33Z

Since this is targeting a production branch, we should not be adding PROTOTYPE comments.

There is already an issue #32054 that tracks base-access work item.

The issue #32054 doesn't say anything about the case that this doesn't implement correctly. Neither does dotnet/csharplang#406. There is no open work item related to it. There are no bugs open for it. There are no LDM/language questions pending. Is this PR not intended to implement #32054?

This is not about implementing the language protection level protected. It is about implementing the language protection level internal, which is well specified and already approved by the LDM. The CLR should already behave as specified for the code generation strategy that would be needed to implement that correctly, so I am not aware of any open issues (other than the possibility that it is not implemented correctly in the CLR currently).

jaredpar · 2019-02-15T21:01:47Z

@gafter

Since this is targeting a production branch, we should not be adding PROTOTYPE comments.

This is a feature branch

There are no LDM/language questions pending.

Mads has agreed to having this discussed in LDM next week.

gafter · 2019-02-15T21:05:36Z

This is a feature branch

In that case a PROTOTYPE comment is fine.

gafter

Looks good modulo a needed PROTOTYPE comment where the accessibility of an explicit override is assigned (to be added in a separate PR). Something like
// PROTOTYPE: this accessibility may not work for internal methods in the face of IVT.

AlekseyTs · 2019-02-15T21:17:13Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

+                    if (!_lazyExplicitInterfaceImplementations.IsDefaultOrEmpty)
+                    {
+                        Debug.Assert(_lazyExplicitInterfaceImplementations.Length == 1);
+                        return _lazyExplicitInterfaceImplementations[0].DeclaredAccessibility;


return _lazyExplicitInterfaceImplementations[0].DeclaredAccessibility; [](start = 24, length = 70)

I am going to add the PROTOTYPE comment here and in other similar places in one of the next PRs.

This is a follow up on #33281.

Add initial support for specifying explicit base type in the base acc…

de15237

…ess. Relevant LDM notes https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-11-14.md .

AlekseyTs added the Area-Compilers label Feb 11, 2019

AlekseyTs requested a review from a team as a code owner February 11, 2019 04:44

AlekseyTs added the Feature - Default Interface Impl Default Interface Implementation label Feb 11, 2019

jaredpar reviewed Feb 12, 2019

View reviewed changes

gafter reviewed Feb 12, 2019

View reviewed changes

jaredpar reviewed Feb 12, 2019

View reviewed changes

gafter reviewed Feb 12, 2019

View reviewed changes

PR feedback

68fdc6b

gafter suggested changes Feb 14, 2019

View reviewed changes

jaredpar approved these changes Feb 15, 2019

View reviewed changes

gafter approved these changes Feb 15, 2019

View reviewed changes

AlekseyTs commented Feb 15, 2019

View reviewed changes

jaredpar merged commit 8b9029a into dotnet:features/DefaultInterfaceImplementation Feb 15, 2019

AlekseyTs mentioned this pull request Feb 15, 2019

Add PROTOTYPE comments to follow up on a previous change. #33440

Merged

AlekseyTs added a commit that referenced this pull request Feb 19, 2019

Add PROTOTYPE comments to follow up on a previous change. (#33440)

1d51d61

This is a follow up on #33281.

		@@ -43,16 +43,41 @@ class Test1 : I1

		- Declaring types within interfaces. Protected and protected internal accessibility is not supported.

Add initial support for specifying explicit base type in the base access. #33281

Add initial support for specifying explicit base type in the base access. #33281

Uh oh!

Conversation

AlekseyTs commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Feb 12, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Feb 12, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Feb 12, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Feb 12, 2019

Uh oh!

AlekseyTs commented Feb 12, 2019

Uh oh!

jaredpar commented Feb 12, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter Feb 12, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

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!

AlekseyTs Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gafter Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Feb 12, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Feb 12, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading

AlekseyTs commented Feb 11, 2019 •

edited

Loading

jaredpar Feb 12, 2019 •

edited

Loading

jaredpar commented Feb 12, 2019 •

edited by AlekseyTs

Loading

jaredpar commented Feb 12, 2019 •

edited by AlekseyTs

Loading

jaredpar commented Feb 12, 2019 •

edited by AlekseyTs

Loading

jaredpar commented Feb 12, 2019 •

edited by AlekseyTs

Loading

gafter Feb 12, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Feb 12, 2019 •

edited

Loading

gafter Feb 13, 2019 •

edited

Loading

AlekseyTs Feb 13, 2019 •

edited

Loading

gafter Feb 12, 2019 •

edited

Loading

gafter Feb 12, 2019 •

edited

Loading

jaredpar commented Feb 12, 2019 •

edited

Loading

jaredpar Feb 12, 2019 •

edited by AlekseyTs

Loading

jaredpar commented Feb 12, 2019 •

edited by AlekseyTs

Loading

jaredpar commented Feb 12, 2019 •

edited by AlekseyTs

Loading

gafter Feb 12, 2019 •

edited

Loading

gafter Feb 12, 2019 •

edited

Loading

gafter Feb 12, 2019 •

edited

Loading

AlekseyTs Feb 12, 2019 •

edited

Loading

gafter Feb 12, 2019 •

edited

Loading