Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 10, 2025

  • Add new diagnostic ID for static keyword changes (CP0022: CannotChangeStatic)
  • Add localizable resource strings
  • Create CannotAddOrRemoveStaticKeyword rule for members and types
  • Create comprehensive tests for static keyword changes
  • Add strict mode support for removing static from types
  • Register the new rule in RuleFactory
  • Test the implementation with the reproduction case
  • Run linter and formatter
  • Address all PR feedback

Latest Changes (this commit)

Split the resource string into two separate strings for proper localization:

  • CannotAddStatic: "Cannot add static keyword to '{0}'."
  • CannotRemoveStatic: "Cannot remove static keyword from '{0}'."

This allows translators to properly localize each message independently, instead of trying to substitute English words ("add"/"remove", "to"/"from") into a template which was not localizable.

The diagnostic ID remains CP0022 (CannotChangeStatic) with two different message formats depending on whether static is being added or removed.

Original prompt

This section details on the original issue you should resolve

<issue_title>APICompat doesn't catch when a member changes static</issue_title>
<issue_description>### Describe the bug
APICompat does not catch the case when a member adds or removes static

To Reproduce

Create a class library that multi-targets and add/remove static on a member.

project.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>net8.0;net10.0</TargetFrameworks>
    <IsPackable>true</IsPackable>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <EnablePackageValidation>true</EnablePackageValidation>
  </PropertyGroup>

</Project>

Class1.cs

public class Class1
{
    #if NET10_0
    public string Foo => "net10.0";
    public static string Bar => "net10.0";
    #else
    public static string Foo => "net8.0";
    public string Bar => "net8.0";
    #endif
}

Expect validation errors for both Foo and Bar since both addition and removal is binary breaking. Removing static is source breaking as well.

We should make sure we include checking for all types of API that might change by static. That should be all members.

Adding static to a type that didn't have it before can be breaking, since static types cannot be used as return types, generic parameters, arguments. We have made such changes in the past - when we had non-constructable types. We should still flag this since we cannot be sure that someone didn't decide to use that in source (even if it wasn't callable).

Removing static from a type is compatible. We'll separately evaluate all members that existed before. </issue_description>

<agent_instructions>Be sure to first reproduce the problem by adding a test to https://github.com/dotnet/sdk/tree/main/test/Microsoft.DotNet.ApiCompatibility.Tests/Rules then fix the issue with a new rule in https://github.com/dotnet/sdk/tree/main/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Fix APICompat to catch changes to static members Add APICompat rule to detect static keyword changes on members and types Dec 10, 2025
Copilot AI requested a review from ericstj December 10, 2025 17:04
Copilot AI requested a review from krwq December 11, 2025 09:19
@krwq krwq self-assigned this Dec 11, 2025
Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

@ericstj I'd appreciate second person looking at this - especially the strict mode warning

@krwq krwq marked this pull request as ready for review December 11, 2025 12:47
@krwq krwq requested a review from a team as a code owner December 11, 2025 12:47
Copilot AI review requested due to automatic review settings December 11, 2025 12:47
Copy link
Contributor Author

Copilot AI commented Dec 11, 2025

@copilot please review failures i.e. https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-sdk-refs-pull-52112-merge-4c6167923cd5459290/Microsoft.NET.Build.Tests.dll.9/3/console.e907141d.log?h...

The log URL appears to be expired or inaccessible. I've verified:

  • All ApiCompatibility tests pass (260 tests)
  • The build succeeds without errors
  • A simple multi-targeting package with EnablePackageValidation builds successfully with the changes

The changes I made:

  1. Added CP0022/CP0023 for adding/removing static from members (always reported)
  2. Added CP0024 for adding static to types (always reported)
  3. Added CP0025 for removing static from types (only in strict mode)

Could you provide more details about the failure? Specifically:

  • What error message appears?
  • Is the test using strict mode?
  • Is there a test asset that has static changing across target frameworks?

The failure might be unrelated to my changes, or there might be a test asset that legitimately changes static and now triggers the new rule.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • helixr1107v0xdeko0k025g8.blob.core.windows.net
    • Triggering command: /usr/bin/curl curl -s REDACTED (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from krwq December 11, 2025 12:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new APICompat rule to detect when the static keyword is added to or removed from members and types, which is a binary breaking change. The implementation follows the established patterns in the codebase for compatibility rules.

  • Implements CannotAddOrRemoveStaticKeyword rule that checks for static modifier changes on methods, properties, fields, events, and types
  • Adds 4 new diagnostic IDs (CP0022-CP0025) with appropriate localization strings
  • Includes comprehensive test coverage for all member types and strict mode behavior
  • Correctly handles the nuance that removing static from types is compatible (members checked separately) but reported in strict mode for developer awareness

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated no comments.

Show a summary per file
File Description
CannotAddOrRemoveStaticKeyword.cs New rule implementation with member and type-level checks for static keyword changes
CannotAddOrRemoveStaticKeywordTests.cs Comprehensive test coverage including reproduction case, all member types, type-level changes, and strict mode
DiagnosticIds.cs Adds four new diagnostic ID constants (CP0022-CP0025)
Resources.resx Adds four new diagnostic message strings
Resources.*.xlf Adds localization entries with state="new" for all supported languages
RuleFactory.cs Registers the new rule to run by default
CannotChangeGenericConstraints.cs Minor whitespace cleanup (trailing whitespace removal and spacing fix)

The implementation is clean, well-tested, and follows the established patterns in the codebase. No issues were found that require changes.

@ericstj
Copy link
Member

ericstj commented Dec 11, 2025

@copilot please address feedback

Copilot AI requested a review from ericstj December 11, 2025 19:34
<value>Visibility of '{0}' reduced from '{1}' to '{2}'.</value>
</data>
<data name="CannotChangeStatic" xml:space="preserve">
<value>Cannot {0} static keyword {1} '{2}'.</value>
Copy link
Member

Choose a reason for hiding this comment

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

This type of substitution is not localizable since it has English words in code. Can you please split this into two resources. Keep a single diagnostic ID but allow it to have two different messages.

Copy link
Member

Choose a reason for hiding this comment

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

@copilot please address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split into two separate resource strings for proper localization:

  • CannotAddStatic: "Cannot add static keyword to '{0}'."
  • CannotRemoveStatic: "Cannot remove static keyword from '{0}'."

The diagnostic ID remains CP0022 but uses the appropriate message based on the operation. Commit: 203993c

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.

APICompat doesn't catch when a member changes static

3 participants