Inaccessible extension methods should not be considered eligible candidates. #26345
Conversation
| @@ -11484,7 +11483,7 @@ static void Main(string[] args) | |||
| Assert.Equal(ConversionKind.Identity, semanticInfo.ImplicitConversion.Kind); | |||
|
|
|||
| Assert.Null(semanticInfo.Symbol); | |||
| Assert.Equal(CandidateReason.Inaccessible, semanticInfo.CandidateReason); | |||
| Assert.Equal(CandidateReason.OverloadResolutionFailure, semanticInfo.CandidateReason); | |||
| @@ -6084,7 +6084,7 @@ private static void CombineExtensionMethodArguments(BoundExpression receiver, An | |||
| this.LookupExtensionMethodsInSingleBinder(scope, lookupResult, rightName, arity, options, ref useSiteDiagnostics); | |||
| diagnostics.Add(node, useSiteDiagnostics); | |||
|
|
|||
| if (!lookupResult.IsClear) | |||
| if (lookupResult.IsMultiViable) | |||
| @@ -88,7 +88,7 @@ public bool IsExtensionMethodGroup | |||
| } | |||
|
|
|||
| public bool IsLocalFunctionInvocation => | |||
| MethodGroup.Methods.Count == 1 && // Local functions cannot be overloaded | |||
| MethodGroup?.Methods.Count == 1 && // Local functions cannot be overloaded | |||
There was a problem hiding this comment.
This is another little bug fix while I was in the area. It prevents the debugger from displaying exceptions for this property when viewing the members of a MethodGroupResolution.
There was a problem hiding this comment.
Were you able to cover this previous crash case with a test?
In reply to: 183521116 [](ancestors = 183521116)
There was a problem hiding this comment.
No, because there is no crash. It is just very poor experience in the debugger that I am fixing.
|
@dotnet/roslyn-compiler Please review this small bug fix. |
|
Since this PR is targeting |
| @@ -783,9 +783,9 @@ public static void M(string s) | |||
| // (4,35): error CS1057: 'Extensions.SomeExtension(string)': static classes cannot contain protected members | |||
| // static private protected void SomeExtension(this string s) { } // error: no pp in static class | |||
| Diagnostic(ErrorCode.ERR_ProtectedInStatic, "SomeExtension").WithArguments("Extensions.SomeExtension(string)").WithLocation(4, 35), | |||
| // (11,11): error CS0122: 'Extensions.SomeExtension(string)' is inaccessible due to its protection level | |||
| // (11,11): error CS1061: 'string' does not contain a definition for 'SomeExtension' and no extension method 'SomeExtension' accepting a first argument of type 'string' could be found (are you missing a using directive or an assembly reference?) | |||
There was a problem hiding this comment.
no extension [](start = 105, length = 12)
Should the error message be updated to mention accessibility? "... no accessible extension method ..." #Closed
There was a problem hiding this comment.
| // fixed (int* p = new Fixable(1)) | ||
| Diagnostic(ErrorCode.ERR_ExprCannotBeFixed, "new Fixable(1)").WithLocation(6, 25), | ||
| // (6,25): error CS0122: 'FixableExt.GetPinnableReference(Fixable)' is inaccessible due to its protection level | ||
| // (6,25): error CS8385: The given expression cannot be used in a fixed statement |
There was a problem hiding this comment.
Same here, maybe the message could be tweaked to give hints.
There was a problem hiding this comment.
The problem is it currently gives no hints. Should this diagnostic also mention that the method GetPinnableReference could be called if it were found, but that it must be non-static, what its signature should be, and that it could be either an instance or extension method? I think this PR is the wrong place to start educating users about the new feature.
|
I've adjusted the diagnostic as suggested (one of the two). Please let me know if there is anything else. |
| @@ -11145,5 +11145,33 @@ static void Main() | |||
| Diagnostic(ErrorCode.ERR_BadRetType, "F").WithArguments("Program.F(in System.DateTime)", "string").WithLocation(16, 15) | |||
| ); | |||
| } | |||
|
|
|||
| [Fact, WorkItem(25813, "https://github.com/dotnet/roslyn/issues/25813")] | |||
| public void InaccessibleExtensionMethod() | |||
There was a problem hiding this comment.
InaccessibleExtensionMethod [](start = 20, length = 27)
It would be interesting to test scenario when there are instance candidates and extension candidates and all of them are inaccessible. What would be the error message and candidates returned by SemanticModel?
|
test windows_debug_spanish_unit32_prtest please |
|
test windows_release_vs-integration_prtest please |
Fixes #25813
@dotnet/roslyn-compiler Please review this small bug fix.
Note that there is a slight regression in the quality of our diagnostics for inaccessible extension methods. I think this is unavoidable unless we go to the two-pass technique used to diagnose problems with non-extension methods (first look for viable methods, then if that fails try again looking for anything, viable or not).
There is also a slight regression in one
SemanticModelcase caused bySemanticModelattempting to compensate for the fact that we don't take a two-pass approach. I've filed #26344 for that, as fixing it probably requires some deep refactoring of theSemanticModeland binding of overload resolution. Since the cost and risk of that is high and the benefits small, I do not consider that worth doing in this bug fix.