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

[DllImportGenerator] Warn/fail when passing by reference a SafeHandle without an accessible parameterless constructor. #63109

Open
teo-tsirpanis opened this issue Dec 23, 2021 · 4 comments
Labels
area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer source-generator Indicates an issue with a source generator feature
Milestone

Comments

@teo-tsirpanis
Copy link
Contributor

When passing SafeHandles by reference, the DllImportGenerator needs their types to have an accessible parameterless constructor. If they don't, it tries to create them by generating a call to Activator.CreateInstance(typeof(MySafeHandle), nonPublic: true). If they don't have a parameterless constructor at all, the P/Invoke fails at runtime.

An analyzer should be added that helps with this situation. If the SafeHandle descendant in question does not have an accessible parameterless constructor. If it is in the same assembly with the P/Invoke, it should cause an error, directing the user to add one (or even helping them with a fixer). If it is in a different assembly, it should warn the user that the P/Invoke is not guaranteed to work, and suggest them to contact the SafeHandle in question's author.

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

@jkoritzinsky
Copy link
Member

cc: @AaronRobinsonMSFT @elinor-fung @stephentoub this would be a proposal to add back part of the analyzer/code-fix removed in dotnet/roslyn-analyzers#5308 for being too noisy for SafeHandle-derived types used in non-P/Invoke scenarios. As part of #63085, we hit the exact scenario that the removed functionality was designed to assist.

@stephentoub
Copy link
Member

I'm fine with an analyzer that warns on faulty P/Invokes in general: if we can detect a problem in a DllImport at compile time, a warning makes sense. And a SafeHandle returned from a p/invoke but lacking a ctor that makes that feasible is one such case. My pushback on the previous analyzer was that it was triggered for any SafeHandle, regardless of how it was consumed, regardless of whether it was used in P/Invokes.

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Jan 3, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone Jan 3, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT modified the milestones: 7.0.0, Future May 19, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature and removed area-Interop-coreclr labels May 19, 2022
@ghost
Copy link

ghost commented May 19, 2022

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

Issue Details

When passing SafeHandles by reference, the DllImportGenerator needs their types to have an accessible parameterless constructor. If they don't, it tries to create them by generating a call to Activator.CreateInstance(typeof(MySafeHandle), nonPublic: true). If they don't have a parameterless constructor at all, the P/Invoke fails at runtime.

An analyzer should be added that helps with this situation. If the SafeHandle descendant in question does not have an accessible parameterless constructor. If it is in the same assembly with the P/Invoke, it should cause an error, directing the user to add one (or even helping them with a fixer). If it is in a different assembly, it should warn the user that the P/Invoke is not guaranteed to work, and suggest them to contact the SafeHandle in question's author.

Author: teo-tsirpanis
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: Future

@teo-tsirpanis teo-tsirpanis added the code-analyzer Marks an issue that suggests a Roslyn analyzer label May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime.InteropServices code-analyzer Marks an issue that suggests a Roslyn analyzer source-generator Indicates an issue with a source generator feature
Projects
Status: No status
Development

No branches or pull requests

5 participants