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

Do not warn on zero-length allocations in unspecified params parameters in attributes #6114

Merged
merged 8 commits into from
Aug 29, 2022

Conversation

333fred
Copy link
Member

@333fred 333fred commented Aug 17, 2022

Fixes an issue introduced with dotnet/roslyn#59369, as the AvoidZeroLengthAllocations analyzer did not see these params parameters before.

…rs in attributes

Fixes an issue introduced with dotnet/roslyn#59369, as the AvoidZeroLengthAllocations analyzer did not see these params parameters before.
@333fred 333fred requested a review from a team as a code owner August 17, 2022 21:53
{
IArrayCreationOperation arrayCreationExpression = (IArrayCreationOperation)context.Operation;

// We can't replace array allocations in attributes, as they're persisted to metadata
// TODO: Once we have operation walkers, we can replace this syntactic check with an operation-based check.
if (arrayCreationExpression.Syntax.Ancestors().Any(isAttributeSytnax))
if (arrayCreationExpression.Syntax.AncestorsAndSelf().Any(isAttributeSyntax))
Copy link
Member Author

Choose a reason for hiding this comment

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

params parameters have the attribute itself as their syntax.

Note: we could potentially check for an IAttributeOperation itself, but I'm not familiar with the process for updating the reference used by the analyzer itself so it would know about that type.

Copy link
Member

Choose a reason for hiding this comment

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

Note: we could potentially check for an IAttributeOperation itself, but I'm not familiar with the process for updating the reference used by the analyzer itself so it would know about that type.

In case you wanted to go this route, new operations goes into Lightup layer, e.g:

public const OperationKind UsingDeclaration = (OperationKind)0x6c;

internal readonly struct IUsingDeclarationOperationWrapper : IOperationWrapper

We can't yet update the product code to consume latest Roslyn versions.

Then, to consume a Roslyn version in tests that have IAttributeOperation, we update the following:

<MicrosoftCodeAnalysisVersionForTests>4.3.0-2.final</MicrosoftCodeAnalysisVersionForTests>

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 would have to leave this check in the code anyway, so I don't think it's worth it.

Copy link
Member

Choose a reason for hiding this comment

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

params parameters have the attribute itself as their syntax.

Just out of curiosity is that all params parameters or just ones that are zero-length like this?

Copy link
Member Author

@333fred 333fred Aug 18, 2022

Choose a reason for hiding this comment

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

Just the implicit ones. It matches the behavior for use in non-attribute locations (ie, just a regular invocation).

{
IArrayCreationOperation arrayCreationExpression = (IArrayCreationOperation)context.Operation;

// We can't replace array allocations in attributes, as they're persisted to metadata
// TODO: Once we have operation walkers, we can replace this syntactic check with an operation-based check.
if (arrayCreationExpression.Syntax.Ancestors().Any(isAttributeSytnax))
if (arrayCreationExpression.Syntax.AncestorsAndSelf().Any(isAttributeSyntax))
Copy link
Member

Choose a reason for hiding this comment

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

params parameters have the attribute itself as their syntax.

Just out of curiosity is that all params parameters or just ones that are zero-length like this?

@333fred
Copy link
Member Author

333fred commented Aug 18, 2022

@mavasani can you please take a look at the code metrics failures? A quick look told me they were a bit more complicated than fixing up in a minute or two :).

@333fred 333fred enabled auto-merge August 29, 2022 20:37
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #6114 (f8a3c83) into main (dc672ef) will decrease coverage by 0.02%.
The diff coverage is 92.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6114      +/-   ##
==========================================
- Coverage   96.05%   96.02%   -0.03%     
==========================================
  Files        1343     1343              
  Lines      309713   309474     -239     
  Branches     9956     9950       -6     
==========================================
- Hits       297490   297181     -309     
- Misses       9822     9894      +72     
+ Partials     2401     2399       -2     

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.

6 participants