Fix obsolete warning for delegate creation#47332
Conversation
b480b3c to
5e32336
Compare
|
The approach I took doesn't seem to work, and also it's very bad even if it works. |
|
I believe you'll want to make a similar change to what I'm doing here: https://github.com/dotnet/roslyn/pull/47311/files#diff-098c83159f29ad4768223015c6455902R1123. I'd prefer if we put this PR on hold until after the We'll also need to update https://github.com/dotnet/roslyn/blob/master/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20post%20VS2019.md, and I'm going to send an email to the compat council to see if this is a break that we'd accept. |
|
Alright @YairHalberstadt, it seems like this is a change we'd accept. Feel free to continue working on it. |
|
Did you mean @Youssef1313? |
|
I did, sorry Yair 😅 |
| if (source is BoundDelegateCreationExpression delegateCreationExpression && | ||
| delegateCreationExpression.MethodOpt is not null) | ||
| { | ||
| ReportDiagnosticsIfObsolete(diagnostics, delegateCreationExpression.MethodOpt, delegateCreationExpression.Argument.Syntax, hasBaseReceiver: false); | ||
| } |
There was a problem hiding this comment.
@333fred I'm unsure of this is how you'd prefer to fix that. Can you take a look?
There was a problem hiding this comment.
I believe it would be better to put this check in MethodGroupConversionHasErrors, like the change to report UnmanagedCallersOnly errors.
In reply to: 490915535 [](ancestors = 490915535)
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 8). @dotnet/roslyn-compiler for a second review of this community PR please.
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 17), other than the whitespace
Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
|
Just in case, I'm going to do a test VS insertion with this change and see if it breaks anything horribly. |
|
Test insertion (Microsoft-internal accessible only) https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/277471 |
|
We've had some infrastructure fires over the past week, so I'm redoing my smoke test. Will update when it's done. |
|
@333fred I'm not sure what the couple of previous comments mean |
|
Nothing you need to do. I'm testing to make sure that this doesn't break the VS build. |
|
Alright, just did another test build and VS continues to work correctly: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/279194. @jaredpar, I'd like sign off from you on this change before I merge. |
|
Talked with Jared offline, we're good. |
|
Thanks @Youssef1313! |
Fixes #47308