Unsafe evolution: handle remaining member kinds#82043
Conversation
| public readonly void ReportDiagnosticsIfUnsafeMemberAccess(Binder binder, SyntaxNodeOrToken node, SyntaxNode syntax, BindingDiagnosticBag diagnostics) | ||
| { | ||
| var getEnumeratorMethod = this.GetEnumeratorInfo?.Method; | ||
| if (getEnumeratorMethod != null) binder.ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, getEnumeratorMethod, node); |
There was a problem hiding this comment.
I'm actually surprised this compiled, I thought we obsoleted these.
There was a problem hiding this comment.
I'm actually surprised this compiled, I thought we obsoleted these.
Only the operator on TypeSymbol is obsolete (because it can't take TypeCompareKind parameter I think).
For symbols, I believe this comparison will actually call the user-defined equality operator. Is that intentional?
I think it's functionally equivalent, but can use is null to make it clearer.
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (commit 57)
333fred
left a comment
There was a problem hiding this comment.
Done review pass. Did not review tests yet.
| } | ||
|
|
||
| static void reportObsoleteDiagnostics(Binder @this, BindingDiagnosticBag diagnostics, MethodSymbol method, SyntaxNode syntax) | ||
| static void reportUseSiteDiagnostics(Binder @this, BindingDiagnosticBag diagnostics, MethodSymbol method, SyntaxNode syntax) |
There was a problem hiding this comment.
These aren't use site diagnostics (or at least, I don't see any determination of the use site diagnostics of the symbol involved). This should probably be named reportObsoleteAndUnsafeDiagnostics. #Closed
| if (symbol.Kind is not (SymbolKind.Event or SymbolKind.Property)) | ||
| { | ||
| ReportDiagnosticsIfObsolete(diagnostics, symbol, node, hasBaseReceiver: false); | ||
| AssertNotUnsafeMemberAccess(symbol); |
There was a problem hiding this comment.
Prototype, since we may need unsafe fields? #Closed
| public readonly void ReportDiagnosticsIfUnsafeMemberAccess(Binder binder, SyntaxNodeOrToken node, SyntaxNode syntax, BindingDiagnosticBag diagnostics) | ||
| { | ||
| var getEnumeratorMethod = this.GetEnumeratorInfo?.Method; | ||
| if (getEnumeratorMethod != null) binder.ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, getEnumeratorMethod, node); |
There was a problem hiding this comment.
I'm actually surprised this compiled, I thought we obsoleted these.
| /// </summary> | ||
| internal abstract bool IsRequired { get; } | ||
|
|
||
| internal sealed override CallerUnsafeMode CallerUnsafeMode => CallerUnsafeMode.None; |
There was a problem hiding this comment.
Maybe leave a PROTOTYPE? #Resolved
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (commit 61). I'll add a follow-up note to test plan regarding well-known members in general
Test plan: #81207
OHI not handled as part of this PR.
Delegates handled according to this open question: https://github.com/dotnet/csharplang/blob/79da45137622071d3c80ebe0a1296387af67afb9/proposals/unsafe-evolution.md#delegate-type-unsafety (i.e., it's not possible to mark delegates as caller-unsafe - I'd expect this to be approved and even if not, it seems like a good starting point - see also comments by jkotas under dotnet/csharplang#9831).
If you want to see what location is covered by which test, the individual commits should show that (although they are not super clean, i.e., there are some fixups later, etc.).