Import all custom modifiers and move their validation to compilers. #45220
Conversation
|
@jcouv, @cston, @RikkiGibson, @dotnet/roslyn-compiler Please review |
| DeriveUseSiteDiagnosticFromCustomModifiers(ref result, param.RefCustomModifiers); | ||
| return DeriveUseSiteDiagnosticFromType(ref result, param.TypeWithAnnotations, AllowedRequiredModifierType.None) || | ||
| DeriveUseSiteDiagnosticFromCustomModifiers(ref result, param.RefCustomModifiers, | ||
| this is MethodSymbol method && method.MethodKind == MethodKind.FunctionPointerSignature ? |
There was a problem hiding this comment.
It feels like this would be better as a parameter to DeriveUseSiteDiagnosticFromParameter, rather than calculated internally here. #Resolved
There was a problem hiding this comment.
It feels like this would be better as a parameter to DeriveUseSiteDiagnosticFromParameter, rather than calculated internally here.
I do not think so. We have all information to reliably make the decision here, why would we want the consumer to to do extra work, given that the condition is still going to be pretty much the same.
In reply to: 441015421 [](ancestors = 441015421)
There was a problem hiding this comment.
We have all information to reliably make the decision here, why would we want the consumer to to do extra work, given that the condition is still going to be pretty much the same.
Because it feels like this is the wrong place to make that decision. We can enumerate the possibilities here today, but that feels fragile. It seems like the best place to state the required modifiers is the component that actually cares about that requirement. I'm not going to block this PR on it as it's minor, but it really doesn't feel like the appropriate location.
In reply to: 441020808 [](ancestors = 441020808,441015421)
There was a problem hiding this comment.
Because it feels like this is the wrong place to make that decision. We can enumerate the possibilities here today, but that feels fragile. It seems like the best place to state the required modifiers is the component that actually cares about that requirement. I'm not going to block this PR on it as it's minor, but it really doesn't feel like the appropriate location.
Even if we assume that the code is fragile, which I do not agree with. I am failing to see how duplication of the same logic in many places makes the code less fragile.
In reply to: 441029178 [](ancestors = 441029178,441020808,441015421)
|
@jcouv, @cston, @RikkiGibson, @dotnet/roslyn-compiler Please review, need a second sign-off |
| @@ -334,7 +334,7 @@ private FunctionPointerMethodSymbol(CallingConvention callingConvention, Immutab | |||
| RefCustomModifiers = CSharpCustomModifier.Convert(retInfo.RefCustomModifiers); | |||
| CallingConvention = callingConvention; | |||
| ReturnTypeWithAnnotations = returnType; | |||
| RefKind = getRefKind(retInfo, RefCustomModifiers, RefKind.RefReadOnly); | |||
| RefKind = getRefKind(retInfo, RefCustomModifiers, RefKind.RefReadOnly, RefKind.Ref); | |||
There was a problem hiding this comment.
RefKind.Ref); [](start = 83, length = 13)
I didn't understand this. Why are we mapping OutAttribute to RefKind.Ref?
There was a problem hiding this comment.
I didn't understand this. Why are we mapping
OutAttributetoRefKind.Ref?
What would you expect us to do here?
In reply to: 441161001 [](ancestors = 441161001)
There was a problem hiding this comment.
I'm not sure, that's why I'm asking for clarification. Why are we mapping OutAttribute to RefKind.Ref?
There was a problem hiding this comment.
I'm not sure, that's why I'm asking for clarification. Why are we mapping OutAttribute to RefKind.Ref?
What do you think is a better option?
In reply to: 441164767 [](ancestors = 441164767)
| @@ -351,9 +351,9 @@ class CBar : IFoo // CS0535 * 2 | |||
| // (7,14): error CS0535: 'CBar' does not implement interface member 'IFoo.M<T>(T)' | |||
| // class CBar : IFoo // CS0535 * 2 | |||
| Diagnostic(ErrorCode.ERR_UnimplementedInterfaceMember, "IFoo").WithArguments("CBar", "Metadata.IFoo.M<T>(T)").WithLocation(7, 14), | |||
| // (2,21): error CS0570: 'IFooAmbiguous<T, R>.M(?)' is not supported by the language | |||
There was a problem hiding this comment.
? [](start = 52, length = 1)
nit: I like that we load the type even if there is a modifier problem. But it feels like the change in diagnostics is making troubleshooting worse, as the ? would at least point users closer to the source of the problem. We can tweak if we get feedback... #Resolved
There was a problem hiding this comment.
it feels like the change in diagnostics is making troubleshooting worse, as the ? would at least point users closer to the source of the problem. We can tweak if we get feedback...
I don't think a "?" actually points to any specific source of the problem
In reply to: 441162316 [](ancestors = 441162316)
There was a problem hiding this comment.
I think the error is actually more informative now because you can use the info to locate the member in metadata. The signature is not mangled
In reply to: 441163418 [](ancestors = 441163418,441162316)
ghost
added this to the 
Closes #44671.