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

Implement Analyzer and best-effort Code Fix for SafeHandle Public Parameterless Constructor guidance. #5043

Merged
merged 18 commits into from
May 24, 2021
Merged

Implement Analyzer and best-effort Code Fix for SafeHandle Public Parameterless Constructor guidance. #5043

merged 18 commits into from
May 24, 2021

Conversation

jkoritzinsky
Copy link
Member

As part of dotnet/runtime#46830, we updated the guidance for concrete SafeHandle-derived types to request that all SafeHandle-derived types provide a public parameterless constructor to aid in source-generated interop scenarios.

This PR adds a code analyzer to detect when a public parameterless constructor is not present on a concrete SafeHandle-derived type.

It also adds a code fix for the cases where there is a non-public parameterless constructor (where it changes the visibility to public) and when the base class has an accessible parameterless constructor (where it adds a public parameterless constructor).

cc: @AaronRobinsonMSFT @elinor-fung

Fixes dotnet/runtime#46830

@jkoritzinsky jkoritzinsky requested a review from a team as a code owner April 20, 2021 16:36
@AaronRobinsonMSFT
Copy link
Member

LGTM. I defer to the analyzer experts to sign off.

Youssef1313
Youssef1313 reviewed Apr 21, 2021
View reviewed changes
...t.NetCore.Analyzers/InteropServices/ProvidePublicParameterlessSafeHandleConstructor.Fixer.cs
SyntaxGenerator generator = SyntaxGenerator.GetGenerator(context.Document);
SyntaxNode root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);

SyntaxNode enclosingNode = root.FindNode(context.Span);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to pass true for getInnermostNodeForTie? If so, we should add a test too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what that flag does, so I don't know if I need to pass it or what a test case would look like.

Copy link
Member

Choose a reason for hiding this comment

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

@jkoritzinsky The flag is when two nodes have the same span, which one to get?
I don't think this can happen for the case of this analyzer, but will let @mavasani confirm in case there is any corner case I'm missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe that can happen with the nodes we're looking at in this analyzer, so I don't think we need to worry about it.

Youssef1313
Youssef1313 reviewed Apr 21, 2021
View reviewed changes
...t.NetCore.Analyzers/InteropServices/ProvidePublicParameterlessSafeHandleConstructor.Fixer.cs
SyntaxGenerator generator = SyntaxGenerator.GetGenerator(context.Document);
SyntaxNode root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);

SyntaxNode enclosingNode = root.FindNode(context.Span);
Copy link
Member

Choose a reason for hiding this comment

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

@mavasani I'm quite confused regarding CodeFixContext when it has multiple diagnostics, which diagnostic will the value of context.Span belong to?

Copy link
Contributor

Choose a reason for hiding this comment

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

All diagnostics in the code fix context should have the same ID and span.

Youssef1313
Youssef1313 reviewed Apr 21, 2021
View reviewed changes
...t.NetCore.Analyzers/InteropServices/ProvidePublicParameterlessSafeHandleConstructor.Fixer.cs Outdated
return;
}

SemanticModel model = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the semantic model is used to calculate things that are already calculated in the analyzer? Can we use additionalProperties to avoid re-calculating that?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't check what constructors the base class has in the analyzer. That's only checked in the fixer.

Copy link
Member

Choose a reason for hiding this comment

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

This is going to block the light bulb on semantic evaluation. It's better to quickly show a code fix that ends up taking no action than to slowly show no code fix at all.

@codecov
Copy link

codecov bot commented Apr 21, 2021 •
edited
Loading

Codecov Report

Merging #5043 (483e77e) into release/6.0.1xx (d464b5f) will increase coverage by 0.00%.
The diff coverage is 98.72%.

@@                Coverage Diff                @@
##           release/6.0.1xx    #5043    +/-   ##
=================================================
  Coverage            95.58%   95.59%            
=================================================
  Files                 1223     1226     +3     
  Lines               280320   280792   +472     
  Branches             16841    16867    +26     
=================================================
+ Hits                267955   268427   +472     
- Misses               10092    10096     +4     
+ Partials              2273     2269     -4

@Youssef1313
Copy link
Member

@jkoritzinsky New analyzers should target the latest preview branch, can you rebase?

This was referenced Apr 22, 2021
Closed
Closed
@jkoritzinsky
Copy link
Member Author

I believe I've addressed all of the feedback.

@jkoritzinsky
Copy link
Member Author

@dotnet/roslyn-analysis this PR is ready for review.

@mavasani
Copy link
Contributor

@jkoritzinsky Please retarget the PR to release\6.0.1xx. Thanks!

@jkoritzinsky jkoritzinsky changed the base branch from release/6.0.1xx-preview4 to release/6.0.1xx May 17, 2021 16:27
@jkoritzinsky
Copy link
Member Author

@mavasani I've retargeted the PR.

@jkoritzinsky
Copy link
Member Author

@mavasani do you have any other feedback?

sharwell
sharwell requested changes May 21, 2021
View reviewed changes
Copy link
Member

@sharwell sharwell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

❗ The data necessary to separate MakeParameterlessConstructorPublic from AddPublicParameterlessConstructor should be placed in the diagnostic properties, or determined from syntax alone (this should be easy). The data necessary to further disable showing a code fix for cases where no base constructor is available should be either placed in the property bag of the diagnostic, or moved into the callback AddParameterlessConstructor. This will ensure the light bulb is not delayed for a semantic evaluation.

If you end up showing AddPublicParameterlessConstructor for a case where no constructor is added, this is fine and the code fix should simply take no action. In the future, it may be beneficial to detect analyzer execution in an environment that supports code fixes, such that it can perform additional work in the analyzer to provide accurate data to the code fix with minimal overhead.

@jkoritzinsky
Copy link
Member Author

@sharwell can we write down these policies (ie. not getting the SemanticModel object during the RegisterCodeFixesAsync method) for newer contributors to the repo like myself? I've just been following the patterns I've seen in other analyzers/code-fixers as I've been contributing here.

@jkoritzinsky
Copy link
Member Author

Done! Once CI is green, this'll be ready for you to merge (I don't have permissions).

@sharwell
Copy link
Member

sharwell commented May 21, 2021 •
edited
Loading

@mavasani this LGTM reassigning back so you can determine the correct merge point

@sharwell sharwell assigned mavasani and unassigned sharwell May 21, 2021
@mavasani
Copy link
Contributor

@jkoritzinsky Can you please resolve merge conflicts?

mavasani
mavasani reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

I can merge once you resolve the conflicts and my question in the review comments. Thanks for the contribution!

@mavasani mavasani merged commit a43657b into dotnet:release/6.0.1xx May 24, 2021
@jkoritzinsky jkoritzinsky deleted the safehandle-analyzer-and-fixer branch May 24, 2021 18:48
This was referenced Jul 10, 2021
Closed
Closed
Closed
@jeffhandley jeffhandley mentioned this pull request Aug 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavasani mavasani mavasani left review comments

@Youssef1313 Youssef1313 Youssef1313 left review comments

@elinor-fung elinor-fung elinor-fung left review comments

@sharwell sharwell sharwell approved these changes

Assignees

@carlossanlop carlossanlop

@mavasani mavasani

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose interop manipulation of SafeHandle
7 participants
@jkoritzinsky @AaronRobinsonMSFT @Youssef1313 @mavasani @sharwell @elinor-fung @carlossanlop