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

Avoid reporting CA1823 for inline arrays #6790

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Conversation

sharwell
Copy link
Member

Fixes #6789

@sharwell sharwell requested a review from a team as a code owner July 21, 2023 14:23
"net8.0",
new PackageIdentity(
"Microsoft.NETCore.App.Ref",
"8.0.0-preview.6.23329.7"),
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be updated whenever the repo updates the version it depends on preview to preview?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. Normally the signatures in the reference assemblies don't change to the point of affecting the unit tests so they just keep working. Plus, it'll end up in dotnet/roslyn-sdk soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another potential approach we take for tests is to add an additional source file to the test with custom stub implementation of the newer APIs added to the Framework which are not yet available via the currently referenced assemblies. We can do a sweeping cleanup later once we move the tests to reference packages with required APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another potential approach we take for tests is to add an additional source file to the test with custom stub implementation of the newer APIs

This is necessary when the APIs haven't been published, but in this case we can reference the real APIs and avoid the risk involved in cases where a deviation in our test implementation impacts the behavior of the analyzer.

@@ -659,6 +659,20 @@ public static bool HasAnyAttribute(this ISymbol symbol, params INamedTypeSymbol?
return symbol.GetAttributes(attributeTypesToMatch).Any();
}

public static bool HasAnyAttribute(this ISymbol symbol, [NotNullWhen(true)] INamedTypeSymbol? attributeToMatch)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between this extension, and the one right below?

/// <summary>
/// Returns a value indicating whether the specified symbol has the specified
/// attribute.
/// </summary>
/// <param name="symbol">
/// The symbol being examined.
/// </param>
/// <param name="attribute">
/// The attribute in question.
/// </param>
/// <returns>
/// <see langword="true"/> if <paramref name="symbol"/> has an attribute of type
/// <paramref name="attribute"/>; otherwise <see langword="false"/>.
/// </returns>
/// <remarks>
/// If <paramref name="symbol"/> is a type, this method does not find attributes
/// on its base types.
/// </remarks>
public static bool HasAttribute(this ISymbol symbol, [NotNullWhen(returnValue: true)] INamedTypeSymbol? attribute)

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'll send a follow-up to address this.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ To answer the question, the previous HasAttribute method was delegating to the params form of HasAnyAttribute. The new method avoids the allocation that all calls to HasAttribute were facing.

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.

Changes LGTM, though we should consolidate all the HasXXXAttribute helpers instead of adding a new one.

@sharwell sharwell merged commit a928d7b into dotnet:main Jul 31, 2023
@sharwell sharwell deleted the inline-array branch July 31, 2023 13:37
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.

Suppress CA1823 and related analyzers in [InlineArray] types
3 participants