Unsafe evolution: handle new() constraint#82647
Conversation
b6c8be3 to
9175dcc
Compare
| { | ||
| awaitableType = originalBinder.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask); | ||
| if (awaitableType is not null) AssertNotUnsafeMemberAccess(awaitableType); | ||
| if (awaitableType is not null) originalBinder.ReportDiagnosticsIfUnsafeMemberAccess(diagnostics, awaitableType, syntax); |
There was a problem hiding this comment.
Is this change useful? I think we should remove it.
There was a problem hiding this comment.
nit: it's a bit odd to keep the assertion for just this one well-known type. I'd either remove the assertion of push it up in GetWellKnownType
There was a problem hiding this comment.
I will remove it, thanks.
| diagnostics.DiagnosticBag is { } bag && | ||
| alias.Alias.TryGetFirstLocation() is { } location) | ||
| { | ||
| compilation.GetBinder(alias.UsingDirective.NamespaceOrType).ReportDiagnosticsIfUnsafeMemberAccess(bag, immediateTarget, location); |
There was a problem hiding this comment.
We've been checking for unsafe member access at the same time that we check for Obsolete. Why is this different? Are we missing an obsolete check? #Closed
There was a problem hiding this comment.
Thanks. nit: consider commenting/linking
There was a problem hiding this comment.
I have linked this PR from that second tracking issue
| """, | ||
| caller: """ | ||
| using static D1<C>; | ||
| using static unsafe D2<C>; |
There was a problem hiding this comment.
This test shows that unsafe on using is still meaningful. We may want to tweak the spec:
Since pointer types are now safe, an unsafe modifier on declarations without bodies does not have a meaning anymore. Hence unsafe on the following declarations will produce a warning:
using static,
using alias.
``` #Pending
There was a problem hiding this comment.
Yes, that's already mentioned in this open question: https://github.com/dotnet/csharplang/blob/909fd31b4b51cc4df6dba8c616b8b7440b5a7a07/proposals/unsafe-evolution.md#new-constraint-and-usings
I will change the main body of the speclet once this PR is merged to match the implementation.
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (commit 6)
Test plan: #81207
This should resolve the following items from the test plan: