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

Allow static virtual members in CA1000 #7142

Merged
merged 7 commits into from
Jan 16, 2024
Merged

Conversation

RenderMichael
Copy link
Contributor

Fixes #7126

@RenderMichael RenderMichael requested a review from a team as a code owner January 13, 2024 08:29
@RenderMichael
Copy link
Contributor Author

RenderMichael commented Jan 13, 2024

I wasn't sure how to specify the test as C# 11+ and .NET 7+, so I just set those diagnostics as expected.

Let me know if I should do it differently.

edit: I found a better way

Copy link

codecov bot commented Jan 13, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (8d8ac7d) 96.44% compared to head (9d0b999) 96.44%.
Report is 8 commits behind head on main.

❗ Current head 9d0b999 differs from pull request most recent head 9358402. Consider uploading reports for the commit 9358402 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7142   +/-   ##
=======================================
  Coverage   96.44%   96.44%           
=======================================
  Files        1413     1413           
  Lines      337719   337737   +18     
  Branches    11177    11178    +1     
=======================================
+ Hits       325700   325724   +24     
+ Misses       9200     9194    -6     
  Partials     2819     2819           

public async Task CSharp_CA1000_ShouldNotGenerate_VirtualMember()
{
await VerifyCS.VerifyAnalyzerAsync(@"
#if NET7_OR_GREATER
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we condition the entire test method on NET7_OR_GREATER?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although the test suite itself runs in .NET 8, when I run the test without the #if I get diagnostics about the C# and .NET versions being insufficient

Diagnostics:
// /0/Test0.cs(5,23): hidden CA1000: Do not declare static members on generic types
VerifyCS.Diagnostic().WithSpan(5, 23, 5, 37).WithArguments("AbstractMember"),
// /0/Test0.cs(5,40): error CS8919: Target runtime doesn't support static abstract members in interfaces.
DiagnosticResult.CompilerError("CS8919").WithSpan(5, 40, 5, 43),
// /0/Test0.cs(7,27): hidden CA1000: Do not declare static members on generic types
VerifyCS.Diagnostic().WithSpan(7, 27, 7, 40).WithArguments("VirtualMember"),
// /0/Test0.cs(7,44): error CS8919: Target runtime doesn't support static abstract members in interfaces.
DiagnosticResult.CompilerError("CS8919").WithSpan(7, 44, 7, 46),

I know we can specify the C# version to test against but I could not figure out if there's a way to set the .NET version. I didn't bother setting the C# version because .NET 7 implies C# 11.

Copy link
Contributor Author

@RenderMichael RenderMichael Jan 16, 2024

Choose a reason for hiding this comment

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

It turns out the #if was causing nothing to be generated (since the .NET version we're testing with is less than 7).

So I specified C# 11 and added expected diagnostics about .NET 7 features. It doesn't look very pleasant, if there's a more elegant way I'd be happy to improve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I figured out how to specify .NET 6. Thank you for bearing with my learning, I'm fairly new to this codebase.

@mavasani mavasani merged commit fe0b19a into dotnet:main Jan 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CA1000 hasn't been reconsidered with static abstracts in mind
2 participants