dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.24k stars 4.72k forks source link

Warning for partial ComImport types with members in more than one type part #59013

Open jnm2 opened 3 years ago

jnm2 commented 3 years ago

The same reasoning applies as the reasoning for why partial enums wouldn't make sense. The meaning of a member in a ComImport type changes based on its order within the type.

There is currently no warning for a pair of source files like this, but there should be:

[ComImport]
partial interface ISomeInterface { void Foo(); }
partial interface ISomeInterface { void Bar(); }

There should be no warning for a case like this:

[ComImport]
partial interface ISomeInterface { void Foo(); }
[SomeInterface]
partial interface ISomeInterface
{
    public struct DoesNotAffectVTable { }
}

Why not warn for partial ComImport interfaces in the first place?

The source generator https://github.com/microsoft/CsWin32 is a new official vehicle for consuming Windows platform APIs. It generates most types as partial so that you can add customizations, but it doesn't do this for ComImport interfaces. @AArnott cited the danger described at the top. The use case for a partial ComImport interface was to add attributes:

namespace Windows.Win32.UI.Shell
{
    [CoClass(typeof(ShellLink))]
    internal partial interface IShellLinkW { }
}

This is safe, but adding a member would not be safe. Therefore the warning should only apply if more than one type part has members.

AArnott commented 3 years ago

The same reasoning applies as the reasoning for why partial enums wouldn't make sense.

partial enums are fine if the first enum value declared in each partial explicitly prescribes the value. For example, there's no problem here:

partial enum Foo
{
    A = 1,
    B,
}

partial enum Foo
{
    C = 3,
    D,
}
jnm2 commented 3 years ago

That will give "CS0267: The 'partial' modifier can only appear immediately before 'class', 'record', 'struct', 'interface', or a method return type." (https://github.com/dotnet/csharplang/discussions/2669) Even if partial enums with explicit ordering were allowed in the future, you'd want errors or warnings if the order wasn't explicitly given. That error/warning would be analogous to what we're asking for in ComImport types.

jaredpar commented 3 years ago

Moving to roslyn-analyzers as this feels more like an analyzer issue vs. a core compiler one. The reasoning being that [ComImport] isn't really a concept that is understood by the compiler. It exists outside of it. That's very much in analyzer territory.

mavasani commented 3 years ago

Needs to be triaged/approved by dotnet/runtime.

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

elachlan commented 2 years ago

This is a potential blocker for winforms as we attempt to migrate interop code to CsWin32.

CC: @JeremyKuhne, @lonitra

elachlan commented 2 years ago

@danmoseley can you please see if you can get this unstuck?

AArnott commented 2 years ago

@elachlan Can you elaborate on how lack of an analyzer would be a blocker for WinForms? At best it seems to me it would just help you write correct code. But when would WinForms define partial COM interfaces? CsWin32 is being used in WinForms without marshaling, so interfaces aren't even being generated.