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

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

Merged
merged 8 commits into from
Aug 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7768502
Do not warn on zero-length allocations in unspecified params paramete…
333fred Aug 17, 2022
2746555
Address more test failures from the compiler upgrade.
333fred Aug 18, 2022
2f87fb8
Update CodeMetrics to handle OperationKind.Attribute
mavasani Aug 27, 2022
0b7f443
Fix tests for RegisterActionAnalyzer
mavasani Aug 27, 2022
977ec05
Gracefully bail out of flow analysis for OperationKind.Attribute
mavasani Aug 27, 2022
74a797d
Fix typo
mavasani Aug 27, 2022
931e02d
Merge branch 'main' into fix-zerolength
333fred Aug 29, 2022
f8a3c83
Skip flaky tests.
333fred Aug 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
<!-- Roslyn -->
<MicrosoftCodeAnalysisVersion>3.3.1</MicrosoftCodeAnalysisVersion>
<MicrosoftCodeAnalysisForRoslynDiagnosticsAnalyzersVersion>3.7.0</MicrosoftCodeAnalysisForRoslynDiagnosticsAnalyzersVersion>
<MicrosoftCodeAnalysisVersionForTests>4.3.0-2.final</MicrosoftCodeAnalysisVersionForTests>
<MicrosoftCodeAnalysisVersionForTests>4.4.0-2.22416.9</MicrosoftCodeAnalysisVersionForTests>
<DogfoodAnalyzersVersion>3.3.3</DogfoodAnalyzersVersion>
<DogfoodNetAnalyzersVersion>5.0.4-preview1.21126.5</DogfoodNetAnalyzersVersion>
<MicrosoftCodeAnalysisBannedApiAnalyzersVersion>$(DogfoodAnalyzersVersion)</MicrosoftCodeAnalysisBannedApiAnalyzersVersion>
Expand Down
4 changes: 2 additions & 2 deletions src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/AvoidZeroLengthArrayAllocations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,13 @@ private void AnalyzeOperation(OperationAnalysisContext context, IMethodSymbol ar
AnalyzeOperation(context, arrayEmptyMethodSymbol, IsAttributeSyntax);
}

private static void AnalyzeOperation(OperationAnalysisContext context, IMethodSymbol arrayEmptyMethodSymbol, Func<SyntaxNode, bool> isAttributeSytnax)
private static void AnalyzeOperation(OperationAnalysisContext context, IMethodSymbol arrayEmptyMethodSymbol, Func<SyntaxNode, bool> isAttributeSyntax)
{
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:

roslyn-analyzers/src/Utilities/Compiler/Lightup/OperationKindEx.cs

Line 11 in 793113a

public const OperationKind UsingDeclaration = (OperationKind)0x6c;

roslyn-analyzers/src/Utilities/Compiler/Lightup/IUsingDeclarationOperationWrapper.cs

Line 13 in 793113a

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:

roslyn-analyzers/eng/Versions.props

Line 48 in 793113a

<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 •
edited
Loading

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

{
return;
}
Expand Down
23 changes: 23 additions & 0 deletions ...ers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/AvoidZeroLengthArrayAllocationsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,29 @@ class C
await VerifyCS.VerifyAnalyzerAsync(source);
}

[Fact]
public async Task EmptyArrayCSharp_UsedInAttributeParams_NoDiagnosticsAsync()
{
const string source = @"
using System;

[AttributeUsage(AttributeTargets.All, AllowMultiple = true)]
class CustomAttribute : Attribute
{
public CustomAttribute(params int[] i)
{
}
}

[Custom(new int[0])]
[Custom]
class C
{
}
";
await VerifyCS.VerifyAnalyzerAsync(source);
}

[WorkItem(1298, "https://github.com/dotnet/roslyn-analyzers/issues/1298")]
[Fact]
public async Task EmptyArrayCSharp_FieldOrPropertyInitializerAsync()
Expand Down