Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

I'm all for this. @stephentoub Do you have any concerns on this front?

Does it have any / many false positives?

Is there any significant performance impact to the analyzer itself?

And I assume this maps to our guidance moving forward which is that every use of DllImport in C# should be changed to LibraryImport unless there's a technical reason it can't (in which case the analyzer also won't fire)?

Does it have any / many false positives?

Depends on your definition of false positive.

If your definition is "will the build break after running the fix", then the answer is "sometimes". If the user doesn't have AllowUnsafeBlocks set to true, there will be an error for that, but there will be a code fix on the type that contains the LibraryImport to add the AllowUnsafeBlocks property to the project file (Jeff Schwartz and I tested out the whole workflow yesterday, which is what prompted this PR). Also, if the user has any non-blittable structures, the user will have to write a custom marshaller for the structure to get their code compiling again. However, we will also have diagnostics that will explicitly state this to the user, as well as documentation.

If your definition is "will the code-fix convert any P/Invokes using custom MarshalAs marshalling that is not supported by LibraryImport" (aka the special rules for built-in types that we got rid of like SafeArray or CustomMarshaler), then the answer is no. We do not have any false positives in this area. The only places where the user will have to change anything afterwards to get their code building is the above cases.

Is there any significant performance impact to the analyzer itself?

No

And I assume this maps to our guidance moving forward which is that every use of DllImport in C# should be changed to LibraryImport unless there's a technical reason it can't (in which case the analyzer also won't fire)?

Yes, this maps to our guidance.

Thanks.

There are three levels here. For any given location:
a) Don't trigger the analyzer
b) Trigger the analyzer but don't offer a fixer
c) Trigger the analyzer and offer a fixer

It seems like it's technically possible for every DllImport to be translated to a LibraryImport with enough work on the user's part, so (a) seems unnecessary to consider. But I'm very much against offering a fixer that results in the user's code breaking / failing to build. There are multiple ways to achieve that (e.g. multiple diagnostic IDs to differentiate), and if this is going to start making such suggestions to developers, and especially if there's a bulk auto-fix available (which would be nice to enable and looks like exists), we should ensure we're not causing the developer undue pain. For the AllowUnsafeBlocks not set case, the auto-fixer offered in that situation could even handle enabling it.

Also, if the user has any non-blittable structures, the user will have to write a custom marshaller for the structure to get their code compiling again. However, we will also have diagnostics that will explicitly state this to the user, as well as documentation.

This does seem to have negative impact. The issue is suppressions that get added and then never reviewed, which is very common. The issue is a user upgrades to NET 7 and sees a bunch of issues/noise. They then say, "not right now" and add a suppression for LibraryImport. This means the user could just move forward and forget the suppression and not benefit.

I'm leery of pushing something that could produce a bad first experience. Perhaps we should wait for turning this on by default.

I agree with Stephen that option a) is not the best choice. If we don't have the analyzer on by default, people won't ever even notice that converting to LibraryImport unless they personally read the release notes for .NET 7. I strongly believe this will lead to a significant reduction in uptake of LibraryImport usage.

The diagnostic from the analyzer doesn't break the build and doesn't show up in a command line build (only in VS and as a message), so it's quite non-intrusive if a user chooses not to use the code fix.

I'm okay updating the code fix to automatically add the <AllowUnsafeBlocks>true</AllowUnsafeBlocks> designation to the project file if it is not present (also telling the user that we will do so as part of the fix title). I found how the one provided by Roslyn works, so we can copy how that one adds it and update our fixer to do the same.

We can also update the analyzer/fixer to conditionally offer the fix so when the user is using a non-blittable user-defined type, we don't offer it (so option b).

Also, we could add a warning annotation to the code-fix so the user can see which parameters they'll need to manually add marshalling support for after converting to LibraryImport (a variant on option c).

The diagnostic from the analyzer doesn't break the build and doesn't show up in a command line build (only in VS and as a message), so it's quite non-intrusive if a user chooses not to use the code fix.

What about warnings as errors? Is this the same thing or is a suggestion different?

info/suggestion are the blue squiggles in VS. Info-level analyzers are design-time and aren't run as part of actual builds. They're a lower severity than warnings.

I strongly believe this will lead to a significant reduction in uptake of LibraryImport usage.

If we can measure that sure, but the argument "it will hurt usage" with no way to measure it doesn't seem valid.

info/suggestion are the blue squiggles in VS. Info-level analyzers are design-time and aren't run as part of actual builds. They're a lower severity than warnings.

Perfect. I'm fine with this then.

I'd like to merge this in as-is and and open an issue for the following follow-up items:

• Add <AllowUnsafeBlocks>true</AllowUnsafeBlocks> as part of the code fix and tell the user we will do so as part of the code fix title.
• Split the code fix so that we can differentiate between the "won't break anything" cases and the "user will need to add marshalling" cases and the fix-all provider won't auto-fix the "user will need to add marshalling" cases if they're auto-fixing a "won't break anything" case.
• Provide a warning annotation to the user that there will be required follow-up work for the "user will need to add marshalling" cases.

@stephentoub what do you think of this plan?

what do you think of this plan?

Can we get to those follow-ups, and in particular the second one (avoid breaking), as part of the same RC release as this PR will be in? If so, sounds good.

Yes, we can get these follow ups in for RC1. We have enough time until the cut-off to get it in.