New analyzer: Do not call BeginInvoke on a delegate for .NET Core #2807 #2972

duracellko · 2019-10-24T07:14:46Z

Implements #2807

New analyzer: CA2069:DoNotCallBeginInvokeOnDelegate

Analyzer reports warning on a call of BeginInvoke method on Delegate. It reports only on compilation targeting .NET Standard or .NET Core, because BeginInvoke is not implemented in .NET Core.

mavasani · 2019-10-24T08:13:28Z

@sharwell for review

sharwell

Need to update the detection of .NET Standard scenarios

src/Microsoft.NetCore.Analyzers/Core/Tasks/DoNotCallBeginInvokeOnDelegate.cs

src/Microsoft.NetCore.Analyzers/UnitTests/Tasks/DoNotCallBeginInvokeOnDelegateTests.cs

stephentoub · 2019-10-25T15:21:54Z

How are we thinking about analyzers like this vs https://github.com/microsoft/dotnet-apiport/tree/dev/docs/VSExtension and https://docs.microsoft.com/en-us/dotnet/standard/analyzers/api-analyzer ?

There are a bunch of cases of APIs that aren't 100% portable across either version of .NET or OS platform, and I'm not sure we want to be adding a new rule for each. Shouldn't we instead be investing in the aforementioned existing analyzers?

cc: @terrajobst

duracellko · 2019-10-26T03:56:27Z

I forgot to add my open questions:

Is Rule ID correct?
Is Role category and analyzer class namespace correct?
Is there need to add the Rule into any other file in repository for documentation?

mavasani · 2019-10-26T04:22:30Z

@duracellko Thanks for your PR. However, as per @stephentoub's comment, I think we need to first revisit if this is the correct repo for this analyzer and whether this is the space we want to invest in right now as there are large number of such APIs.

src/Test.Utilities/VisualBasicCodeFixVerifier`2.cs

src/Microsoft.NetCore.Analyzers/UnitTests/Tasks/DoNotCallBeginInvokeOnDelegateTests.cs

sharwell · 2019-10-26T05:30:19Z

src/Utilities/Compiler/Extensions/CompilationExtensions.cs

+            // This condition should be removed in future.
+            // It only ensures that tests are successful, because tests compilate to .NET Framework.
+            if (compilation.ReferencedAssemblyNames.Any(identity => string.Equals(identity.Name, "netstandard", StringComparison.OrdinalIgnoreCase)))
+            {
+                return false;
+            }


💭 I think we should remove this from the start. With a small amount of work to complete dotnet/roslyn-sdk#329 we should be able to get this working properly.

sharwell · 2019-10-26T17:58:48Z

Marking as Blocked pending decision above

mavasani · 2021-05-20T17:09:45Z

Is this PR still active? If not, can we close out?

sharwell · 2021-05-20T19:14:02Z

src/Microsoft.NetCore.Analyzers/Core/Tasks/DoNotCallBeginInvokeOnDelegate.cs

+        internal static DiagnosticDescriptor Rule = new DiagnosticDescriptor(RuleId,
+                                                                             s_localizableTitle,
+                                                                             s_localizableMessage,
+                                                                             DiagnosticCategory.Design,


❓ Should this be in Interoperability instead?

sharwell · 2021-05-20T19:14:50Z

src/Microsoft.NetCore.Analyzers/Core/Tasks/DoNotCallBeginInvokeOnDelegate.cs

+    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+    public sealed class DoNotCallBeginInvokeOnDelegate : DiagnosticAnalyzer
+    {
+        internal const string RuleId = "CA1069";


❓ Why would this not be an integral part of PlatformCompatibilityAnalyzer?

sharwell · 2021-05-20T19:16:51Z

src/Microsoft.NetCore.Analyzers/UnitTests/Tasks/DoNotCallBeginInvokeOnDelegateTests.cs

+        private static DiagnosticResult GetCSResultAt(int line, int column)
+        {
+            return VerifyCS.Diagnostic(DoNotCallBeginInvokeOnDelegate.RuleId)
+                .WithMessage(MicrosoftNetCoreAnalyzersResources.DoNotCallBeginInvokeOnDelegateMessage)
+                .WithLocation(line, column);
+        }
+
+        private static DiagnosticResult GetVBResultAt(int line, int column)
+        {
+            return VerifyVB.Diagnostic(DoNotCallBeginInvokeOnDelegate.RuleId)
+                .WithMessage(MicrosoftNetCoreAnalyzersResources.DoNotCallBeginInvokeOnDelegateMessage)
+                .WithLocation(line, column);
+        }


💡 These helpers can now be replaced with markup syntax ([|...|])

sharwell · 2021-05-20T19:18:01Z

src/Microsoft.NetCore.Analyzers/UnitTests/Tasks/DoNotCallBeginInvokeOnDelegateTests.cs

+using Microsoft.CodeAnalysis.Testing;
+using Xunit;
+
+using VerifyCS = Test.Utilities.CSharpSecurityCodeFixVerifier<


❕ These are not security analyzers

Suggested change

using VerifyCS = Test.Utilities.CSharpSecurityCodeFixVerifier<

using VerifyCS = Test.Utilities.CSharpCodeFixVerifier<

📝 Previously, CSharpSecurityCodeFixVerifier was used due to a limitation in the test library. This limitation no longer applies and CSharpCodeFixVerifier will now work without the need for other workarounds.

sharwell · 2021-05-20T19:18:08Z

src/Microsoft.NetCore.Analyzers/UnitTests/Tasks/DoNotCallBeginInvokeOnDelegateTests.cs

+    Microsoft.NetCore.Analyzers.Tasks.DoNotCallBeginInvokeOnDelegate,
+    Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;
+
+using VerifyVB = Test.Utilities.VisualBasicSecurityCodeFixVerifier<


Suggested change

using VerifyVB = Test.Utilities.VisualBasicSecurityCodeFixVerifier<

using VerifyVB = Test.Utilities.VisualBasicCodeFixVerifier<

sharwell · 2021-05-20T19:21:40Z

src/Microsoft.NetCore.Analyzers/UnitTests/Tasks/DoNotCallBeginInvokeOnDelegateTests.cs

+    End Sub
+End Class
+";
+            await VerifyVB.VerifyAnalyzerAsync(code, GetVBResultAt(7, 9));


After the conversion to markup, it would be good to expand these tests:

await new VerifyVB.Test { ReferenceAssemblies = ReferenceAssemblies.NetStandard.NetStandard10, TestCode = code, }.RunAsync(); await new VerifyVB.Test { ReferenceAssemblies = ReferenceAssemblies.NetStandard.NetStandard20, TestCode = code, }.RunAsync(); await new VerifyVB.Test { ReferenceAssemblies = ReferenceAssemblies.NetCore.NetCoreApp31, TestCode = code, }.RunAsync(); await new VerifyVB.Test { ReferenceAssemblies = ReferenceAssemblies.Net.Net50, TestCode = code, }.RunAsync(); await new VerifyVB.Test { ReferenceAssemblies = ReferenceAssemblies.NetFramework.NetFramework472.Default, TestState = { Sources = { code }, // Ignore markup; no diagnostics will be reported for .NET Framework MarkupMode = MarkupMode.Ignore, }, }.RunAsync();

stephentoub · 2023-01-04T20:19:44Z

Is this PR still active? If not, can we close out?

Seems like there hasn't been any progress or response in a year and a half. I'll close it for now.

Thanks for the efforts.

duracellko added 5 commits October 24, 2019 15:09

Add rule CA1069:DoNotCallBeginInvokeOnDelegate

5af4a25

DoNotCallBeginInvokeOnDelegate - CSharp tests without diagnostic

ad11c88

DoNotCallBeginInvokeOnDelegate - Basic tests

a38dd7d

DoNotCallBeginInvokeOnDelegate - translations

bc4dfea

DoNotCallBeginInvokeOnDelegate run on .NET Standard and .NET Core only

c8bfead

sharwell requested changes Oct 25, 2019

View reviewed changes

DoNotCallBeginInvokeOnDelegate update tests to use VerifyCS and VerifyVB

40333d0

sharwell reviewed Oct 26, 2019

View reviewed changes

DoNotCallBeginInvokeOnDelegate - report in generated code

b0b9d71

duracellko force-pushed the begin-invoke branch from 3b2021d to b0b9d71 Compare October 26, 2019 06:52

Update condition to detect .NET Framework target

6144be0

sharwell added the Blocked label Oct 26, 2019

sharwell mentioned this pull request Nov 6, 2019

Standardize detection of .NET Standard #3017

Closed

Evangelink mentioned this pull request Feb 25, 2020

Create and use Netstandard detection helper #3319

Merged

Base automatically changed from master to main March 5, 2021 02:20

jinujoseph requested a review from a team as a code owner March 5, 2021 02:20

mavasani assigned sharwell May 20, 2021

sharwell requested changes May 20, 2021

View reviewed changes

stephentoub closed this Jan 4, 2023

jkotas mentioned this pull request Apr 11, 2023

MulticastDelegate, BeginInvoke throws NotSupportedException dotnet/runtime#16312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New analyzer: Do not call BeginInvoke on a delegate for .NET Core #2807 #2972

New analyzer: Do not call BeginInvoke on a delegate for .NET Core #2807 #2972

duracellko commented Oct 24, 2019

mavasani commented Oct 24, 2019

sharwell left a comment

stephentoub commented Oct 25, 2019

duracellko commented Oct 26, 2019

mavasani commented Oct 26, 2019

sharwell Oct 26, 2019

sharwell commented Oct 26, 2019

mavasani commented May 20, 2021

sharwell May 20, 2021

sharwell May 20, 2021

sharwell May 20, 2021

sharwell May 20, 2021

sharwell May 20, 2021

sharwell May 20, 2021

sharwell May 20, 2021

stephentoub commented Jan 4, 2023

	using VerifyCS = Test.Utilities.CSharpSecurityCodeFixVerifier<
	using VerifyCS = Test.Utilities.CSharpCodeFixVerifier<

	using VerifyVB = Test.Utilities.VisualBasicSecurityCodeFixVerifier<
	using VerifyVB = Test.Utilities.VisualBasicCodeFixVerifier<

New analyzer: Do not call BeginInvoke on a delegate for .NET Core #2807 #2972

New analyzer: Do not call BeginInvoke on a delegate for .NET Core #2807 #2972

Conversation

duracellko commented Oct 24, 2019

mavasani commented Oct 24, 2019

sharwell left a comment

Choose a reason for hiding this comment

stephentoub commented Oct 25, 2019

duracellko commented Oct 26, 2019

mavasani commented Oct 26, 2019

Choose a reason for hiding this comment

sharwell commented Oct 26, 2019

mavasani commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jan 4, 2023