Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jnm2 opened this issue Jul 21, 2021 · 8 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented Jul 21, 2021

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
Copy link
Contributor

AArnott commented Jul 21, 2021

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
Copy link
Contributor Author

jnm2 commented Jul 21, 2021

That will give "CS0267: The 'partial' modifier can only appear immediately before 'class', 'record', 'struct', 'interface', or a method return type." (dotnet/csharplang#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
Copy link
Member

jaredpar commented Aug 2, 2021

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.

@jaredpar jaredpar transferred this issue from dotnet/roslyn Aug 2, 2021
@mavasani
Copy link

Needs to be triaged/approved by dotnet/runtime.

@mavasani mavasani transferred this issue from dotnet/roslyn-analyzers Sep 13, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Sep 13, 2021
@dotnet-issue-labeler
Copy link

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.

@mavasani mavasani added the code-analyzer Marks an issue that suggests a Roslyn analyzer label Sep 13, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Sep 13, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone Sep 13, 2021
@carlossanlop carlossanlop modified the milestones: 7.0.0, 8.0.0 Jul 6, 2022
@elachlan
Copy link
Contributor

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

CC: @JeremyKuhne, @lonitra

@elachlan
Copy link
Contributor

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

@AArnott
Copy link
Contributor

AArnott commented Aug 19, 2022

@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.

@buyaa-n buyaa-n added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 9, 2022
@jkoritzinsky jkoritzinsky modified the milestones: 8.0.0, 9.0.0 Jul 10, 2023
@jkoritzinsky jkoritzinsky modified the milestones: 9.0.0, 10.0.0 Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
Status: No status
Development

No branches or pull requests

10 participants