Implement DisableRuntimeMarshalling analyzer and code fixer #5822

jkoritzinsky · 2022-01-28T01:11:09Z

Fixes dotnet/runtime#63714

jkoritzinsky · 2022-01-28T01:12:05Z

I meant to open this as a draft PR since it's not totally ready yet. My mistake.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs

Youssef1313 · 2022-01-28T09:22:09Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs

+        int sizePassedInType = {|CA1421:Marshal.SizeOf(type)|};
+        int sizePassedInGeneric = {|CA1421:Marshal.SizeOf<T>()|};
+        int sizePassedInGenericUnmanaged = {|CA1421:Marshal.SizeOf<U>()|};
+    }


This tests whether we'll crash on accessing the first argument or not.

"SizeOf" => invocation.TargetMethod.IsGenericMethod || invocation.Arguments[0].Value is ITypeOfOperation { TypeOperand.IsUnmanagedType: true },

Suggested change

}

int nonGenericNoParameter = Marshal.SizeOf();

}

Consider also adding the test for StructureToPtr

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Youssef1313 · 2022-01-28T09:54:24Z

.../CSharp/Microsoft.NetCore.Analyzers/InteropServices/CSharpDisableRuntimeMarshalling.Fixer.cs

+                else if (operation.Arguments[0].Value is ITypeOfOperation { TypeOperand.IsUnmanagedType: true } typeOf)
+                {
+                    addUnsafeToEnclosingMethod = true;
+                    editor.ReplaceNode(syntax, SyntaxFactory.SizeOfExpression(GetTypeOfTypeSyntax((TypeOfExpressionSyntax)typeOf.Syntax)));


Consider handling trivia.

.../CSharp/Microsoft.NetCore.Analyzers/InteropServices/CSharpDisableRuntimeMarshalling.Fixer.cs

…llable PtrToStructure cases.

…CoreAnalyzersResources.resx Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

…eruntimemarshalling-analyzer

…tzinsky/roslyn-analyzers into disableruntimemarshalling-analyzer

mavasani · 2022-01-31T09:15:21Z

I meant to open this as a draft PR since it's not totally ready yet. My mistake.

@jkoritzinsky Is the PR ready for review after your latest commits? If not, can you please convert it to a draft? Thanks.

…re testing against.

codecov · 2022-01-31T23:49:53Z

Codecov Report

Merging #5822 (e1cd121) into main (9228fb6) will increase coverage by 0.00%.
The diff coverage is 96.42%.

@@            Coverage Diff            @@
##             main    #5822     +/-   ##
=========================================
  Coverage   96.02%   96.03%             
=========================================
  Files        1310     1318      +8     
  Lines      302825   304802   +1977     
  Branches     9572     9657     +85     
=========================================
+ Hits       290799   292718   +1919     
- Misses       9740     9768     +28     
- Partials     2286     2316     +30

jkoritzinsky · 2022-02-01T02:00:12Z

@mavasani this is ready for review.

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs

jkoritzinsky · 2022-03-17T22:01:12Z

@mavasani any more feedback on this PR?

.../CSharp/Microsoft.NetCore.Analyzers/InteropServices/CSharpDisableRuntimeMarshalling.Fixer.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs

mavasani · 2022-03-18T11:02:58Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs

+                // The first run fixes all the fixable diagnostics.
+                // Since there are still some (unfixable) diagnostics, the test infrastructure decides to run the fix-all provider again.
+                // The second run doesn't do anything, since the remaining diagnostics are unfixable.
+                // Setting NumberOfFixAllIterations to -2 specifies that the fix-all provider can be run up to 2 times as part of a test run.


Thank you for the comment!

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs

mavasani · 2022-03-18T11:06:48Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs

@@ -1382,6 +1382,7 @@ private static async Task VerifyCSCodeFixAsync(string source, string codeFix, bo

            // Verify that there are some instances of the diagnostic that we can't fix.
            test.FixedState.MarkupHandling = MarkupMode.Allow;
+            test.MarkupOptions = MarkupOptions.UseFirstDescriptor;


This is good to know!

mavasani

Overall LGTM. Have added minor comments to address. Feel free to merge after addressing/responding to them. Thanks.

Youssef1313 · 2022-03-18T11:08:00Z

.../CSharp/Microsoft.NetCore.Analyzers/InteropServices/CSharpDisableRuntimeMarshalling.Fixer.cs

+                if (document.Project.CompilationOptions is CSharpCompilationOptions { AllowUnsafe: false })
+                {
+                    // We can't code fix if unsafe code isn't allowed.
+                    return await document.GetSyntaxRootAsync(fixAllContext.CancellationToken);
+                }


Is this redundant? This was already checked in RegisterCodeFixesAsync

I tried turning this into an assert and it tripped on a few tests, so I guess it's not redundant.

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs

Youssef1313 · 2022-03-18T11:37:19Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs

+            {
+                IMethodSymbol method = (IMethodSymbol)context.Symbol;
+
+                DllImportData? dllImportData = method.GetDllImportData();


I'm surprised this didn't cause a crash due to:

https://github.com/dotnet/roslyn/blob/7e2966e180eef601cb0fc4a029d4f2093d22b639/src/Compilers/CSharp/Portable/Symbols/FunctionPointers/FunctionPointerMethodSymbol.cs#L829

Also surprised on roslyn's side there is an exception thrown in this code path instead of simply returning null. cc @333fred

We don't go down this path for function pointers. We go from AnalyzeFunctionPointerCall to AnalyzeMethodSignature directly.

FunctionPointerMethodSymbol is a method symbol. Shouldn't we get callbacks for it from context.RegisterSymbolAction(AnalyzeMethod, SymbolKind.Method);? then we meet method.GetDllImportData() unconditionally. I know this is likely not happening, otherwise a test would have crashed. But I don't yet see the reason

I think we don't get callbacks for it for the same reason that we don't get callbacks for SymbolKind.Method for local functions (which I just added code to handle in my most recent commit). It seems that symbols that are introduced due to an IOperation are exposed through that operation in RegisterOperationAction instead of through RegisterSymbolAction.

Good to know! Thanks!

Youssef1313

Do we want to report a diagnostic for properties?

e.g:

public static extern string P { [DllImport("abc")] get; }

I don't know if there will already be a diagnostic or not.

jkoritzinsky · 2022-03-18T18:26:52Z

I didn't know that was even valid...

Yes, we should add the diagnostic there too.

Youssef1313 · 2022-03-18T18:31:10Z

@jkoritzinsky Consider also:

using System;
using System.Runtime.InteropServices;

public class C
{
    [DllImport("abc")]
    public static extern int operator +(C a, C b);
    
    [method: DllImport("abc")]
    public extern static event Action G;
}

Note: those edge cases are being stolen from Roslyn tests 😄

jkoritzinsky · 2022-03-18T18:51:55Z

This is so much sadness right now 😢

The way the event test case compiles down also makes me sad, but I guess I should add tests to validate behavior.

jkoritzinsky · 2022-03-18T22:33:01Z

@mavasani I don't have permissions to merge, so can you hit the button for me?

Implement DisableRuntimeMarshalling analyzer and partial code fixer.

ac9e294

jkoritzinsky requested a review from a team as a code owner January 28, 2022 01:11

jkoritzinsky mentioned this pull request Jan 28, 2022

FixAllProvider doesn't support more than two fixes with some identical insertions dotnet/roslyn#59130

Closed

Youssef1313 reviewed Jan 28, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 28, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 28, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 28, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 28, 2022

View reviewed changes

.../CSharp/Microsoft.NetCore.Analyzers/InteropServices/CSharpDisableRuntimeMarshalling.Fixer.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 28, 2022

View reviewed changes

.../CSharp/Microsoft.NetCore.Analyzers/InteropServices/CSharpDisableRuntimeMarshalling.Fixer.cs Outdated Show resolved Hide resolved

jkoritzinsky and others added 6 commits January 28, 2022 10:58

Implement custom FixAllProvider and implement the code fix for non-nu…

27cc464

…llable PtrToStructure cases.

Implement nullable PtrToStructure code fix

3129529

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNet…

2584bab

…CoreAnalyzersResources.resx Co-authored-by: Youssef Victor <youssefvictor00@gmail.com>

Merge branch 'main' of github.com:dotnet/roslyn-analyzers into disabl…

16dafc4

…eruntimemarshalling-analyzer

Merge branch 'disableruntimemarshalling-analyzer' of github.com:jkori…

276efc7

…tzinsky/roslyn-analyzers into disableruntimemarshalling-analyzer

Get the build clean locally.

988816f

jkoritzinsky marked this pull request as draft January 31, 2022 18:02

jkoritzinsky added 2 commits January 31, 2022 14:11

Use a RWLock to make the auto-layout cache thread-safe

329a8b2

Update tests/analyzers that were failing with the new Roslyn that we'…

fb177a5

…re testing against.

jkoritzinsky marked this pull request as ready for review February 1, 2022 01:58

jkoritzinsky added 2 commits February 1, 2022 16:39

Merge branch 'main' into disableruntimemarshalling-analyzer

217a5de

Merge branch 'main' into disableruntimemarshalling-analyzer

b02f9f8

Youssef1313 reviewed Feb 5, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 5, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Feb 5, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

jkoritzinsky requested review from mavasani and stephentoub March 17, 2022 22:01

mavasani reviewed Mar 18, 2022

View reviewed changes

.../CSharp/Microsoft.NetCore.Analyzers/InteropServices/CSharpDisableRuntimeMarshalling.Fixer.cs Show resolved Hide resolved

mavasani reviewed Mar 18, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx Outdated Show resolved Hide resolved

mavasani reviewed Mar 18, 2022

View reviewed changes

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs Outdated Show resolved Hide resolved

mavasani reviewed Mar 18, 2022

View reviewed changes

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs Outdated Show resolved Hide resolved

mavasani reviewed Mar 18, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

mavasani reviewed Mar 18, 2022

View reviewed changes

mavasani approved these changes Mar 18, 2022

View reviewed changes

Youssef1313 reviewed Mar 18, 2022

View reviewed changes

...zers/UnitTests/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshallingTests.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Mar 18, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Mar 18, 2022

View reviewed changes

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/DisableRuntimeMarshalling.cs Show resolved Hide resolved

Youssef1313 reviewed Mar 18, 2022

View reviewed changes

PR feedback and add tests for local functions

8a29a96

Youssef1313 reviewed Mar 18, 2022

View reviewed changes

Add some tests of sadness

0cc8679

Fix test failures

e1cd121

jmarolf merged commit 0495618 into dotnet:main Mar 18, 2022

github-actions bot added this to the vNext milestone Mar 18, 2022

jkoritzinsky deleted the disableruntimemarshalling-analyzer branch March 18, 2022 23:15

Youssef1313 mentioned this pull request May 3, 2022

Missing CAxxxx documentation dotnet/docs#29264

Closed

10 tasks

jmarolf modified the milestones: vNext, .NET 7.x Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement DisableRuntimeMarshalling analyzer and code fixer #5822

Implement DisableRuntimeMarshalling analyzer and code fixer #5822

jkoritzinsky commented Jan 28, 2022

jkoritzinsky commented Jan 28, 2022

Youssef1313 Jan 28, 2022

Youssef1313 Jan 28, 2022

mavasani commented Jan 31, 2022

codecov bot commented Jan 31, 2022 •

edited

Loading

jkoritzinsky commented Feb 1, 2022

jkoritzinsky commented Mar 17, 2022

mavasani Mar 18, 2022

mavasani Mar 18, 2022

mavasani left a comment •

edited

Loading

Youssef1313 Mar 18, 2022

jkoritzinsky Mar 18, 2022

Youssef1313 Mar 18, 2022

jkoritzinsky Mar 18, 2022

Youssef1313 Mar 18, 2022

jkoritzinsky Mar 18, 2022

Youssef1313 Mar 18, 2022

Youssef1313 left a comment

jkoritzinsky commented Mar 18, 2022

Youssef1313 commented Mar 18, 2022 •

edited

Loading

jkoritzinsky commented Mar 18, 2022

jkoritzinsky commented Mar 18, 2022

Implement DisableRuntimeMarshalling analyzer and code fixer #5822

Implement DisableRuntimeMarshalling analyzer and code fixer #5822

Conversation

jkoritzinsky commented Jan 28, 2022

jkoritzinsky commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Jan 31, 2022

codecov bot commented Jan 31, 2022 • edited Loading

Codecov Report

jkoritzinsky commented Feb 1, 2022

jkoritzinsky commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment • edited Loading

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

Choose a reason for hiding this comment

Youssef1313 left a comment

Choose a reason for hiding this comment

jkoritzinsky commented Mar 18, 2022

Youssef1313 commented Mar 18, 2022 • edited Loading

jkoritzinsky commented Mar 18, 2022

jkoritzinsky commented Mar 18, 2022

codecov bot commented Jan 31, 2022 •

edited

Loading

mavasani left a comment •

edited

Loading

Youssef1313 commented Mar 18, 2022 •

edited

Loading