-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Add initial support for specifying explicit base type in the base access. #33281
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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**. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This doesn't ensure that the member is accessible to the caller. For example, an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am not claiming that it is.
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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you elaborate, give an example please? In reply to: 256257520 [](ancestors = 256257520,256257340,256177815,255803908,255803140,255772556,255770872,255770151,255762670,255757955) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think this is misinterpretation of the speclet and misinterpretation of the implementation:
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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 In reply to: 256494259 [](ancestors = 256494259,256314854,256265573,256257520,256257340,256177815,255803908,255803140,255772556,255770872,255770151,255762670,255757955) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
||
- Implementing interface properties and indexers in derived interfaces by using explicit implementation syntax, accessibility is private, allowed modifiers: **extern**. | ||
- Implementing interface properties and indexers in derived interfaces by using explicit implementation syntax, accessibility matches accessibility of the implemented member, allowed modifiers: **extern**. | ||
|
||
- 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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This constraint is not correct. Just as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) |
||
|
||
- Declaring operators ```+ - ! ~ ++ -- true false * / % & | ^ << >> > < >= <=``` in interfaces. | ||
|
||
- Base access | ||
The following forms of base-access are added (https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-11-14.md) | ||
``` | ||
base ( <type-syntax> ) . identifier | ||
base ( <type-syntax> ) [ argument-list ] | ||
``` | ||
|
||
The type-syntax can refer to one of the base classes of the containing type, or one of the interfaces implemented or inherited by the containing type. | ||
|
||
When the type-syntax refers to a class, the member lookup rules, overload resolution rules and IL emit match the rules for the 7.3 supported | ||
forms of base-access. The only difference is the specified base class is used instead of the immediate base class, the most derived implementation is | ||
found in that class, etc. | ||
|
||
When the type-syntax refers to an interface: | ||
1. The member lookup is performed in that interface, using the regular member lookup rules within interfaces, with an exception that members of | ||
System.Object do not participate in the lookup. | ||
2. Regular overload resolution is performed for members returned by the lookup process, virtual or abstract members are not replaced with most | ||
derived implementations at this step (unlike the case when the type-syntax refers to a class). If result of overload resolution is a virtual | ||
or abstract method, it must have a most specific implementation within the interface type, an error is reported otherwise. The most specific | ||
implementation must be accessible at the call site. | ||
3. During IL emit a **call** (non-virtual call) instruction is used to invoke methods. If result of overload resolution on the previous step is | ||
virtual or abstract method, the most specific implementation of the method is used as the target for the instruction. | ||
|
||
Given the accessibility requirements for the most specific interface implementation, accessibility of implementations provided in derived interfaces | ||
is changed to match accessibility of implemented members, prior to this change the accessibility was private. | ||
|
||
**Open issues and work items** are tracked in https://github.com/dotnet/roslyn/issues/17952. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -773,10 +773,17 @@ private bool CheckEventValueKind(BoundEventAccess boundEvent, BindValueKind valu | |
|
||
if (receiver?.Kind == BoundKind.BaseReference && eventSymbol.IsAbstract) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_AbstractBaseCall, boundEvent.Syntax, eventSymbol); | ||
return false; | ||
var baseReference = (BoundBaseReference)receiver; | ||
|
||
if (!eventSymbol.IsImplementableInterfaceMember() || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Completely agree and tests confirm that. In reply to: 255762096 [](ancestors = 255762096) |
||
baseReference.ExplicitBaseReferenceOpt?.Type.IsInterfaceType() != true) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_AbstractBaseCall, boundEvent.Syntax, eventSymbol); | ||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this error is triggered (though it should be) for an abstract event in an interface that is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This code path is for legacy scenarios, not when an abstract event is in an interface. Interface case is handled in In reply to: 255806868 [](ancestors = 255806868) |
||
} | ||
} | ||
else if (ReportUseSiteDiagnostics(eventSymbol, diagnostics, eventSyntax)) | ||
|
||
if (ReportUseSiteDiagnostics(eventSymbol, diagnostics, eventSyntax)) | ||
{ | ||
// NOTE: BindEventAssignment checks use site errors on the specific accessor | ||
// (since we don't know which is being used). | ||
|
@@ -952,15 +959,6 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV | |
return false; | ||
} | ||
} | ||
else if (receiver?.Kind == BoundKind.BaseReference && setMethod.IsAbstract) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_AbstractBaseCall, node, propertySymbol); | ||
return false; | ||
} | ||
else if (!object.Equals(setMethod.GetUseSiteDiagnostic(), propertySymbol.GetUseSiteDiagnostic()) && ReportUseSiteDiagnostics(setMethod, diagnostics, propertySyntax)) | ||
{ | ||
return false; | ||
} | ||
else | ||
{ | ||
var accessThroughType = this.GetAccessThroughType(receiver); | ||
|
@@ -988,6 +986,12 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV | |
{ | ||
return false; | ||
} | ||
|
||
if (IsBadBaseAccess(node, receiver, setMethod, diagnostics, propertySymbol) || | ||
(!object.Equals(setMethod.GetUseSiteDiagnostic(), propertySymbol.GetUseSiteDiagnostic()) && ReportUseSiteDiagnostics(setMethod, diagnostics, propertySyntax))) | ||
{ | ||
return false; | ||
} | ||
} | ||
} | ||
|
||
|
@@ -1001,15 +1005,6 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV | |
Error(diagnostics, ErrorCode.ERR_PropertyLacksGet, node, propertySymbol); | ||
return false; | ||
} | ||
else if (receiver?.Kind == BoundKind.BaseReference && getMethod.IsAbstract) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_AbstractBaseCall, node, propertySymbol); | ||
return false; | ||
} | ||
else if (!object.Equals(getMethod.GetUseSiteDiagnostic(), propertySymbol.GetUseSiteDiagnostic()) && ReportUseSiteDiagnostics(getMethod, diagnostics, propertySyntax)) | ||
{ | ||
return false; | ||
} | ||
else | ||
{ | ||
var accessThroughType = this.GetAccessThroughType(receiver); | ||
|
@@ -1032,6 +1027,12 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV | |
} | ||
|
||
ReportDiagnosticsIfObsolete(diagnostics, getMethod, node, receiver?.Kind == BoundKind.BaseReference); | ||
|
||
if (IsBadBaseAccess(node, receiver, getMethod, diagnostics, propertySymbol) || | ||
(!object.Equals(getMethod.GetUseSiteDiagnostic(), propertySymbol.GetUseSiteDiagnostic()) && ReportUseSiteDiagnostics(getMethod, diagnostics, propertySyntax))) | ||
{ | ||
return false; | ||
} | ||
} | ||
} | ||
|
||
|
@@ -1044,6 +1045,32 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV | |
return true; | ||
} | ||
|
||
private bool IsBadBaseAccess(SyntaxNode node, BoundExpression receiverOpt, MethodSymbol method, DiagnosticBag diagnostics, | ||
PropertySymbol propertySymbolOpt = null, bool checkOnlyAccessThroughInterface = false) | ||
{ | ||
if (receiverOpt?.Kind == BoundKind.BaseReference) | ||
{ | ||
var baseReference = (BoundBaseReference)receiverOpt; | ||
|
||
if (method.IsImplementableInterfaceMember() && | ||
baseReference.ExplicitBaseReferenceOpt?.Type.IsInterfaceType() == true) | ||
{ | ||
if (TypeSymbol.TryFindBaseImplementationInInterface((NamedTypeSymbol)baseReference.ExplicitBaseReferenceOpt.Type, method, diagnostics, node, this) is null) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth asserting that |
||
Debug.Assert(!diagnostics.IsEmptyWithoutResolution); | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should an error be reported before returning true? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that is done in In reply to: 255764725 [](ancestors = 255764725) |
||
} | ||
} | ||
else if (!checkOnlyAccessThroughInterface && method.IsAbstract) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_AbstractBaseCall, node, propertySymbolOpt ?? (Symbol)method); | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/// <summary> | ||
/// Computes the scope to which the given invocation can escape | ||
/// NOTE: the escape scope for ref and val escapes is the same for invocations except for trivial cases (ordinary type returned by val) | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constraints are not correct. #Closed