Enable implicit “this” in default interface implementations. #18393
Conversation
| I2.set_P3 | ||
| I2.add_E3 | ||
| I2.remove_E3 | ||
| I2.get_P2 |
There was a problem hiding this comment.
Consider some additional formatting to improve readability. Maybe add "## " on the "headers".
|
|
||
| CompileAndVerify(compilation1, verify: false); | ||
|
|
||
| /* Expected output |
There was a problem hiding this comment.
Running success scenarios is tracked in #17952.
|
@dotnet/roslyn-compiler Please review, need a second sign-off. |
| @@ -1319,7 +1319,7 @@ private BoundExpression SynthesizeMethodGroupReceiver(CSharpSyntaxNode syntax, A | |||
| var declaringType = members[0].ContainingType; | |||
|
|
|||
| HashSet<DiagnosticInfo> unused = null; | |||
| if (currentType.IsEqualToOrDerivedFrom(declaringType, TypeCompareKind.ConsiderEverything, useSiteDiagnostics: ref unused)) | |||
| if (currentType.IsInterface || currentType.IsEqualToOrDerivedFrom(declaringType, TypeCompareKind.ConsiderEverything, useSiteDiagnostics: ref unused)) | |||
There was a problem hiding this comment.
I don't think this is correct for an interface nested within a class that does not implement the interface. If we find a method invocation of an instance method of the enclosing class, this would appear to return an incompatible "this". The design of this method would appear to require that null be returned rather than an incompatible receiver. Here is the scenario:
class A
{
public void M() {}
interface I
{
void N() { M(); } // what `this` here?
}
}I suspect the correct fix is to use something other than (or modify) IsEqualToOrDerivedFrom, which considers only non-interface inheritance.
The alternative is to remove the test for reference-relatedness from this method entirely and rely on a mechanism for that to be checked elsewhere, if you believe that such a larger refactoring has other benefits.
Please add tests for nested types, and specifically implicit receivers for invocations of names found on or inherited into enclosing types.
| @@ -1538,7 +1538,7 @@ private BoundExpression SynthesizeReceiver(CSharpSyntaxNode node, Symbol member, | |||
| var currentType = this.ContainingType; | |||
| HashSet<DiagnosticInfo> useSiteDiagnostics = null; | |||
|
|
|||
| if (currentType.IsEqualToOrDerivedFrom(member.ContainingType, TypeCompareKind.ConsiderEverything, useSiteDiagnostics: ref useSiteDiagnostics)) | |||
| if (currentType.IsInterface || currentType.IsEqualToOrDerivedFrom(member.ContainingType, TypeCompareKind.ConsiderEverything, useSiteDiagnostics: ref useSiteDiagnostics)) | |||
|
@gafter I believe I've addressed your feedback, please take a look. |
|
@dotnet/roslyn-compiler Please review, need a second sign-off. |
| @@ -1319,7 +1319,8 @@ private BoundExpression SynthesizeMethodGroupReceiver(CSharpSyntaxNode syntax, A | |||
| var declaringType = members[0].ContainingType; | |||
|
|
|||
| HashSet<DiagnosticInfo> unused = null; | |||
| if (currentType.IsEqualToOrDerivedFrom(declaringType, TypeCompareKind.ConsiderEverything, useSiteDiagnostics: ref unused)) | |||
| if (currentType.IsEqualToOrDerivedFrom(declaringType, TypeCompareKind.ConsiderEverything, useSiteDiagnostics: ref unused) || | |||
| (currentType.IsInterface && (declaringType.IsObjectType() || currentType.AllInterfacesNoUseSiteDiagnostics.Contains(declaringType)))) | |||
There was a problem hiding this comment.
( [](start = 16, length = 1)
This is not correct for an invocation of an interface method from a class nested within the interface. Although you have not implemented classes declared within interfaces yet, I do not think it makes sense to add code here that will just have to be changed again.
There was a problem hiding this comment.
Are there tests that distinguish this if statement's condition from (currentType == declaringType) thereby illustrating the importance of the test for related types here?
There was a problem hiding this comment.
Oh, I see declaringType has nothing to do with the T quoted in the spec, above. That means that the "If T is the instance type of the immediately enclosing class or struct type..." part of the quoted spec isn't implemented. Anywhere. This relatedness test appears intended to imitate that test, but it does not do so correctly in cases involving hiding. As a consequence, this implementation would appear to get the hiding cases wrong.
@dotnet/roslyn-compiler Please review.