Add fixer for converting to GeneratedDllImport

[elinor-fung]

• Fixer that offers two options:
  • Convert to GeneratedDllImport in place
  • Convert to GeneratedDllImport under preprocessor (NET right now, which should be .NET 5+)

    // Original
    [DllImport("Lib")]
    public static extern int Method(out int ret);
    
    // Converted in place
    [GeneratedDllImport("Lib")]
    public static partial int Method(out int ret);
    
    // Converted under preprocessor
    #if NET
    [GeneratedDllImport("Lib")]
    public static partial int Method(out int ret);
    #else
    [DllImport("Lib")]
    public static extern int Method(out int ret);
    #endif

• VSIX project to install / use fixer
• Unit tests

Just aiming for helping us switch runtime libraries, so the fixer doesn't try to do anything clever / handle much - just changes the attribute, removes extern, and adds partial. Then, we rely on the generator to error for anything (types, values, etc.) it doesn't support.

cc @AaronRobinsonMSFT @jkoritzinsky

[AaronRobinsonMSFT]

Wouldn't this require the type to already be in use? If so, how is this fixer bootstrapped? This would seem to make the concern about injecting the type rather than having it a part of .NETCoreApp even more relevant.

See dotnet/runtime#46822 (comment)

[elinor-fung]

It would need to be available, but not necessarily in use - e.g. could be in a referenced assembly, but not actually being used.

The fixer has to be installed via VSIX. It will have a chance to provide a fix when diagnostics corresponding to FixableDiagnosticIds are fired.

Your question does bring up the problem that source generators are not VSIX-provided, but analyzers and fixers are. Our analyzers do the same check for the attribute being in the compilation. This would mean that if we put the attribute in NETCoreApp, it would always be there (for NETCoreApp versions after it is added) - and our analyzer (installed through VSIX) would flag things even if the source generator isn't being used. This is mitigated by the fact that rule for migrating to GeneratedDllImport is currently not enabled by default, so the user has to explicitly enable it in their editorconfig.

Injecting the type in the source generator itself would help with that, since the type would only be available when the source generator is used.

[AaronRobinsonMSFT]

Your question does bring up the problem that source generators are not VSIX-provided, but analyzers and fixers are.

This is a good point. Let's add a section in the design document about the fixer/analyzer/source generator scenario. You have answered by question/concern.

@chsienki @stephentoub Is there any guidance on how the fixer/analyzer/source generator scenarios align? How are they installed as a unit? Is a fixer from the command line going to be a thing yet? If I recall it was mentioned in an offline chat I had with someone but I am not sure where that ended up.

[AaronRobinsonMSFT]

I don't think we should do this automatically. If the existing DllImport has setting we don't support we should ignore it and not present a fixer or provide some warning that we doing something different. My concern here is with people simply saying "change everything" in a large code base.

[elinor-fung]

we should ignore it and not present a fixer

As in don't flag it for conversion at all (i.e. don't show a diagnostic)? Or continue flagging it, but just not provide a fixer if the user has BestFitMapping or ThrowOnUnmappableChar set? The analyzer doesn't try to match exactly what the generator supports, so it currently flags everything that has a signature which would require marshalling.

Would you not want a fixer even in the 'easy' case? e.g. this one that is currently dropping the field, since it is explicitly set to the value that corresponds to behaviour with GeneratedDllImport

There are a lot of other configurations we don't support that aren't a simple setting like this. Is your argument that the fixer try to detect/ignore all of those?

At least for the v1 purposes of 'use in NETCoreApp', I didn't want to make the fixer try to be too smart. It currently just relies on the generator to error when it encounters something not supported. (So we can do 'fix all' to convert to GeneratedDllImport and then fix any errors produced by the generator).

provide some warning that we doing something different

I think that's actually a good idea in general, since we have behaviour / compatibility differences (outside of unsupported settings on the attribute). Even for supported scenarios, additional work could be required after applying a fix (e.g. struct marshalling, explicit char/string marshalling).

[AraHaan]

So basically would this apply to libraries like mine that use a NativeMethods class to import things like MiniDumpWriteDump and possibly other Windows only APIs related to mini dumping a process to a file?

Also I would be for having this analizer default the rule to an error (that they can change to warning in their editorconfig), but that is just me probably.

[AaronRobinsonMSFT]

Spoke offline. This was a misunderstanding of the fixer/analyzer relationship and what we can reasonably address.

[elinor-fung]

@AraHaan it would apply to any DllImport that we identify as potentially benefitting from converting to the source-generated approach (currently just based on the return/parameter types and some of the attribute fields). Note that what exactly the analyzer and fixer will flag/do has not fully been decided yet. We are not at point where this is targeting external consumption yet, so the current analyzer and fixer in this prototype are for porting NetCoreApp.