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.

Sign up for GitHub

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

Jump to bottom

Create analyzer in ComInterfaceGenerator to enforce the generator is used with IUnknown only #78189

Merged
merged 24 commits into from
Jan 12, 2023
Merged

Create analyzer in ComInterfaceGenerator to enforce the generator is used with IUnknown only #78189

merged 24 commits into from
Jan 12, 2023

Conversation

jtschuster
Copy link
Member

Creates an analyzer that warns if the GeneratedComInterfaceAttribute is
used with InterfaceTypeAttribute that has an argument that is not
'InterfaceIsIUnknown'

@ghost ghost assigned jtschuster Nov 10, 2022
@ghost
Copy link

ghost commented Nov 10, 2022

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

Issue Details

Creates an analyzer that warns if the GeneratedComInterfaceAttribute is
used with InterfaceTypeAttribute that has an argument that is not
'InterfaceIsIUnknown'

Author: jtschuster
Assignees: jtschuster
Labels:

area-System.Runtime.InteropServices
Milestone: -

@jtschuster jtschuster changed the title Create analyzer in ComInterfaceGenerator to enforce Generator is used with IUnknown only Create analyzer in ComInterfaceGenerator to enforce the generator is used with IUnknown only Nov 10, 2022
@jtschuster
Create analyzer to enforce Generator is used with IUnknown only
0cdeb34 
Creates an analyzer that warns if the GeneratedComInterfaceAttribute is
used with InterfaceTypeAttribute that has an argument that is not
'InterfaceIsIUnknown'
jtschuster and others added 4 commits November 10, 2022 12:37
@jtschuster @jkoritzinsky
Update src/libraries/System.Runtime.InteropServices/gen/ComInterfaceG…
eedf06e 
…enerator/GeneratorDiagnostics.cs

Co-authored-by: Jeremy Koritzinsky <[email protected]>
@jtschuster
PR Feedback
ad8429e 
- Use test harness from LibraryImportGenerator.Unit.Tests
- Don't check for assembly name when matching attribute
- Undo changes from TestUtils
@jtschuster
Undo auto formatting
e5cbbb7
@jtschuster
Checkout System.Private.CoreLib.Shared.projitems from upstream/main
e8a3c08
This was referenced Nov 11, 2022
Closed
Closed
Closed
@jtschuster jtschuster marked this pull request as ready for review November 11, 2022 17:48
elinor-fung
elinor-fung reviewed Nov 11, 2022
View reviewed changes
Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected behaviour of the generator if it encounters this case? Will it just generate code for IUnknown / completely ignore InterfaceType? If we might put the user in an unexpectedly bad state, would it make sense to be an error diagnostic in the generator instead?

src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratorDiagnostics.cs Outdated Show resolved Hide resolved
src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/Resources/Strings.resx Outdated Show resolved Hide resolved
src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/Resources/Strings.resx Outdated Show resolved Hide resolved
src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratorDiagnostics.cs Outdated Show resolved Hide resolved
....Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratedComInterfaceAttributeAnalyzer.cs Outdated Show resolved Hide resolved
....Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratedComInterfaceAttributeAnalyzer.cs Outdated Show resolved Hide resolved
src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratorDiagnostics.cs Outdated Show resolved Hide resolved
....Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratedComInterfaceAttributeAnalyzer.cs Outdated Show resolved Hide resolved
....Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratedComInterfaceAttributeAnalyzer.cs Outdated Show resolved Hide resolved
....Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratedComInterfaceAttributeAnalyzer.cs Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request Nov 11, 2022
Open
@jkoritzinsky
Copy link
Member

What is the expected behaviour of the generator if it encounters this case? Will it just generate code for IUnknown / completely ignore InterfaceType? If we might put the user in an unexpectedly bad state, would it make sense to be an error diagnostic in the generator instead?

The InterfaceType attribute basically allows developers to specify the "base" interface type for their COM interface in the built-in system. Users can pick, IUnknown, IDispatch, or IInspectable (there's also a Dual option which means IDispatch + actually generate vtable members for the methods).

This attribute would allow users to explicitly state that they want an IUnknown-based interface, which will be our default with the source generator and would block users from trying to say they want a different base interface that we don't support.

@elinor-fung
Copy link
Member

If a project has an interface with [GeneratedComInterface(...)] and [InterfaceType(ComInterfaceType.IInspectable)], but has analyzers turned off and uses the generator, what is the user's experience? Would the generator:

  1. Just generate what we support, effectively ignoring InterfaceType
    • This would leave the user in a bad/unsupported state
  2. Actually block/fail
    • If the generator does this, I don't know that we need an analyzer to also emit a diagnostic

I know we hit the 'analyzers disabled, generator enabled' case with LibraryImport and moved our invalid attribute usage diagnostics to the generator instead of analyzers.

@jkoritzinsky
Copy link
Member

I think the generator would just skip generating what is not supported.

I know that @CyrusNajmabadi was pushing for diagnostics like these to be moved to analyzers for better performance in the IDE. I'd be curious to hear his thoughts on the "analyzers disabled, generator enabled" scenario.

@build-analysis build-analysis bot mentioned this pull request Nov 17, 2022
Closed
@jtschuster
Merge branch 'GenComIntAnalyzer' of https://github.com/jtschuster/run…
9d907a2 
…time into GenComIntAnalyzer
@jtschuster
PR feedback
76dc0c9 
Move Analyzer and related diagnostics to Analayzers/ folder
Change diagnostic to "InvalidUse of GeneratedComInterfaceAttribute"
Add diagnostic to list-of-diagnostics.md
Add and organize tests using all variations of ComInterfaceType
This was referenced Dec 9, 2022
Open
Closed
@build-analysis build-analysis bot mentioned this pull request Dec 21, 2022
Closed
@jkoritzinsky
Copy link
Member

Failure was a known issue with our usage of homebrew. This is ready to be merged.

@jtschuster jtschuster merged commit 1630725 into dotnet:main Jan 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@elinor-fung elinor-fung elinor-fung approved these changes

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@jeffhandley jeffhandley Awaiting requested review from jeffhandley jeffhandley is a code owner

Assignees

@jtschuster jtschuster

Labels
area-System.Runtime.InteropServices
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jtschuster @jkoritzinsky @elinor-fung @CyrusNajmabadi