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

Support VB for ImmutableObjectMethodAnalyzer #6321

Merged
merged 4 commits into from
Jan 4, 2023

Conversation

Youssef1313
Copy link
Member

Write the analyzer using IOperation for easy VB support.

Also fixes #1128

@mavasani I'm not sure whether using IOperation improves performance, doesn't affect it, or make it worse, but if you think this should remain using syntax node actions, let me know and I'll add VB support using syntax node actions.

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 10, 2022 11:53
Comment on lines 91 to 99
INamedTypeSymbol? type = null;
if (invocation.Instance is not null)
{
type = invocation.Instance.Type as INamedTypeSymbol;
}
else if (invocation.TargetMethod.IsExtensionMethod && invocation.TargetMethod.Parameters is [{ Type: var parameterType }, ..])
{
type = parameterType as INamedTypeSymbol;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@mavasani Does this logic sound correct? I'm surprised there is no property on IInvocationOperation to get this info directly (or at least I couldn't find it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you instead use

/// <summary>
/// Gets the receiver type for an invocation expression (i.e. type of 'A' in invocation 'A.B()')
/// If the invocation actually involves a conversion from A to some other type, say 'C', on which B is invoked,
/// then this method returns type A if <paramref name="beforeConversion"/> is true, and C if false.
/// </summary>
public static INamedTypeSymbol? GetReceiverType(this IInvocationOperation invocation, Compilation compilation, bool beforeConversion, CancellationToken cancellationToken)

Copy link
Member Author

@Youssef1313 Youssef1313 Jan 3, 2023

Choose a reason for hiding this comment

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

It looks like GetReceiverType implementation is incorrect when beforeConversion is false.

firstArg.Type is always null. I'll use this extension and fix its implementation.

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #6321 (c4909ec) into main (7e5162c) will increase coverage by 0.04%.
The diff coverage is 99.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6321      +/-   ##
==========================================
+ Coverage   96.07%   96.12%   +0.04%     
==========================================
  Files        1367     1361       -6     
  Lines      315348   316084     +736     
  Branches    10187    10183       -4     
==========================================
+ Hits       302985   303841     +856     
+ Misses       9930     9814     -116     
+ Partials     2433     2429       -4     

@mavasani mavasani merged commit 2e06d13 into dotnet:main Jan 4, 2023
@github-actions github-actions bot added this to the vNext milestone Jan 4, 2023
@Youssef1313 Youssef1313 deleted the immutable-analyzer branch January 4, 2023 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSharpImmutableObjectMethodAnalyzer does not work when MS.CA.Workspaces is not installed
2 participants