Linker warning IL2050 doesn't get suppressed by adding RequiresUnreferencedCode

[joperezr]

While resolving the linker warnings for Microsoft.VisualBasic.Core I found an instance of warning IL2050 here:

https://github.com/dotnet/runtime/blob/21dcc7385ea41beffe19913f2c6b765728d5ee48/src/libraries/Microsoft.VisualBasic.Core/src/Microsoft/VisualBasic/Helpers/UnsafeNativeMethods.vb#L31-L36

The Linker shows the following message:

Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VariantChangeType(Object&,Object&,Int16,Int16): P/invoke method 'Microsoft.VisualBasic.CompilerServices.UnsafeNativeMethods.VariantChangeType(Object&,Object&,Int16,Int16)' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.

While the warning makes sense, I was surprised that this warning wouldn't be automatically suppressed by adding RequiresUnreferencedCode attribute on that method. In my mind, when resolving Linker warnings there are pretty much 3 different actions to take:

• Rewrite the statement to make it safe so that the linker won't warn.
• Manually Suppress the error with UnconditionallySuppressMessage (maybe along with adding DynamicDependency when appropriate) if the callsite is deemed safe and the linker is warning as a false positive.
• Add RequiresUnreferencedCode attribute so that you can bubble up the warning to your callers.

For IL2050, there doesn't seem to be a way of doing that 3rd action, which there should be one in my opinion (either via RequiresUnreferencedCode or a new attribute).

cc: @MichalStrehovsky @eerhardt @LakshanF @vitek-karas @sbomer

[MichalStrehovsky]

I have mixed feeling about having RequiresUnreferencedCode suppress COM warnings. Conceptually, it's similar, but hitting COM codepaths after trimming leads to a lot more sadness than regular RequiresUnreferencedCode (the failure modes might be a lot more inscrutable - including wrong methods being called with wrong calling conventions). It's bad enough to the point that we should consider IL2050 to be an error.

I wonder if we can somehow connect this to dotnet/runtime#50500 so that if the app is trimmed with default settings, COM would be disabled and VariantChangeType never reached (therefore never generating a warning). If COM would be opted in manually, it's fine to warn from an implementation detail.

[vitek-karas]

Aside from Michal's comment which I mostly agree with, RequiresUnreferencedCode should be added onto the method calling the PInvoke, not on the PInvoke itself. The warning source is the callsite of the PInvoke, and so the "caller of that" should get the annotation - as it's then "bubbling it up". (I didn't try if it actually works though)

@LakshanF - can you please look into somehow tying these warnings into COM feature switch (and maybe turning them into errors by default?)

[LakshanF]

Some observations related to the discussion above,

• Built-in COM is disabled by default for trimming and COM related warnings should not bubble up to the user now and needed to be opted in to use it.
• PInvoke annotation seem to be different than the typical way trimmer bubbles up warnings as this issue indicates. Current annotations on the Marshal class illustrate the issue since caller is close by.

[vitek-karas]

• Built-in COM is disabled by default for trimming and COM related warnings should not bubble up to the user now and needed to be opted in to use it.

Did we try this? Specifically, will the app which has the feature switch turned off actually throw an exception if I try to PInvoke using implicit marshalling of COM interfaces (for example - or VARIANT's).

That said, trimmer should still warn in these cases - actually it's even more important since we know it will fail at runtime (by default at least). So these COM warnings should still be produced regardless of the feature switch.

I added some tests in #2080 around this to understand the current behavior:

• These warnings are not treated as TrimAnalysis warnings unfortunately. At least not fully. They are part of the SuppressTrimAnalysisWarnings MSBuild property which is good, but they are not part of the "RUC suppresses these" group. That feels like a bug.
• They are reported on the PInvoke itself and not on the callsite - I'm not sure about this. Honestly it feels like these should be reported on the callsites instead. It's impossible to reason about potentially suppressing these on the PInvoke itself, it makes much more sense to do so on the callsite.

I think we should:

• Treat these are a problem when the PInvoke is actually used and not with the PInvoke itself.
• Report these in callsites - we should probably handle these in ReflectionMethodBodyScanner instead of MarkStep as it would make the implementation easier and more consistent.
• The above would also implicitly add them to the RUC suppressed category.

It seems we didn't ship this in 5, so this is not a breaking change.

@MichalStrehovsky what do you think?

[MichalStrehovsky]

They are part of the SuppressTrimAnalysisWarnings MSBuild property which is good, but they are not part of the "RUC suppresses these" group. That feels like a bug.

If we can make it so that this is only suppressible if COM is disabled, it works for me. I would be wary of being able to suppress COM warnings if COM is enabled and we trim stuff. This is the kind of stuff that comes as a support call to Aaron...

Yes, reporting at the callsite sounds like an improvement.

[vitek-karas]

If we can make it so that this is only suppressible if COM is disabled, it works for me.

Ideally if COM is disabled then the warnings are effectively errors since the runtime should consistently throw in that case.

If we don't allow to suppress the warnings when COM is enabled then there is literally no workaround, which worries me. So far our approach has been "If you suppress the warning you better know what you're doing" and the failure modes in that case are basically unpredictable. I agree that in case of COM they are especially hard to diagnose, but I don't think it's that different from other cases (for example serialization producing payload with missing data - which doesn't fail anywhere and leads to silent data loss - that one is also very bad and very hard to diagnose).
(There's also the fact that intentionally ignoring explicit warning suppressions is pretty bad - it works everywhere else, why not here).

If we think we should not allow people to suppress these warnings, then I think a better approach would be to simply refuse to trim apps with COM enabled - completely.

[LakshanF]

Disabling built-in COM seem to trim away code that are implicit marshalling of COM interfaces including related VARIANTs as can be see with this recent test change. Direct COM calls as the related Marshal class code referenced above are trimmed away (i.e. BindToMoniker and the related PInvokes). I'll double check with the interop team on the first case to make sure.

Given that we currently strongly discourage using built-in COM support when trimming, I prefer if we allow developers to suppress warnings as proposed above by Vitek if they still want to use built-in COM support.

[vitek-karas]

The test change unfortunately doesn't validate what happens if COM is disabled (it effectively skips those tests). I think it would be valuable to adds the negative test cases for those as well (validate that runtime fails/throws as expected). But given the split it makes sense that it's likely already failing.

[LakshanF]

I validated that they fail with COM disabled. I will add negative tests to this PR.