Use Microsoft.CodeAnalysis.PublicApiAnalyzers to track public API surface

[RussKie]

ℹ️ Review commit by commit

• The first commit brings the API base from Preview 6.
• The second commit is the API delta as changed since Preview 6.

Microsoft Reviewers: Open in CodeFlow

[rafal-mz]

Why? We've used it and it didn't work for us.

[RussKie]

Why? We've used it and it didn't work for us.

This is a standard way of tracking the changes to the public API surface (e.g., https://github.com/dotnet/aspnetcore, https://github.com/dotnet/winforms, https://github.com/dotnet/roslyn, etc.).
I'm aware of the inhouse solution that has been used internally, which is useful to tracking the experimental API, and it can be complimentary to this. One of the reasons, the internal solution doesn't track all API violations, e.g. RS0026 and RS0027.
Also, at some point we should expect the Roslyn analyzers to support the experimental attribute as well since it is now supported by the compiler (dotnet/roslyn#68702).

[rafal-mz]

I think experience won't be great if we use PublicApiAnalyzers with experimental flow. Those analyzers require us to add API to .txt file as we write it. Then, when API is experimental and evolves, it would mean need developer to fiddle with text files manually to make changes.

IMHO not worth for RS00026 and RS00027. We can add support in API Chief for it if needed. It was deliberate choice to allow adding default params, as it does not spoil call site. The same as you can add new properties to a class, adding new default params should be harmless (or I miss something?).

I am not strongly opinionated to leave APIChief, but I wouldn't mix those two - dev experience will be bad having both.

[RussKie]

IMHO not worth for RS00026 and RS00027. We can add support in API Chief for it if needed.

I'm not an expert in this area, but I think there are reasons for RS00026 and RS00027 being reported.

It was deliberate choice to allow adding default params, as it does not spoil call site. The same as you can add new properties to a class, adding new default params should be harmless (or I miss something?).

Additive changes are generally allowed, however, some other changes may result in ABI breaking changes.
.NET runtime has several documents describing breaking changes, e.g. https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-definitions.md, https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-changes.md and https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md.

[mavasani]

I'm not an expert in this area, but I think there are reasons for RS00026 and RS00027 being reported.

Note that RS0026 and RS0027 are pretty opinionated rules, which are turned off in many repos. I wouldn't be too concerned if you decide to keep them disabled, it's an API design preference.

[geeknoid]

We don't want this analyzer in this repo, we have a better solution that accounts for experimental APIs.

[sharwell]

@geeknoid I'd be interested in learning more about the better solution. Currently many teams are using many different solutions, with various benefits and drawbacks. It would be good to consolidate on one tool that is publicly maintained to address the needs of the ecosystem.

[RussKie]

I started a discussion about the support of the experimental attribute in dotnet/roslyn-analyzers#6759.

[RussKie]

We don't want this analyzer in this repo, we have a better solution that accounts for experimental APIs.

Right now, we don't have any API tracking solution in this repository, which means we don't have any visibility of how API change from release to release. For example, we made a number of API breaking changes since Preview 6, which normally would not be allowed.
We can revisit this topic and remove PublicApiAnalyzers when we have your custom solution working here, and if it is indeed better. Do you have any objections?

Those analyzers require us to add API to .txt file as we write it. Then, when API is experimental and evolves, it would mean need developer to fiddle with text files manually to make changes.

That's the price to pay for changing the public API surface. Many developers do not appreciate the risks associated with such changes. E.g., a rename of an argument may seem like a trivial thing, however, it is a binary breaking change (we've done this in FakeTimeProvider).
These analyzers make such changes obvious and alert the reviewers.

[rafal-mz]

That's the price to pay for changing the public API surface.

I disagree. In this case it is limitation of the tool, that is aware only of two API stages: 'new' and 'old'. Its OK to flag all issues when you change API surface, but NOT when you are prototyping. Having tooling that reports false-positive for each code change is counter-productive. This is why we invested in API chief at first, because those analyzers were not working for us.

[RussKie]

@rafal-mz please allow me to reiterate the problem at hand.

1. We currently have no working public API tracking solution in this repo at all.
2. We have already made a number of API breaking changes since Preview 6, and not all affected API are marked as experimental.
3. We have to snap for Preview 7 in few days, and we need to know what API we ship in Preview 7.

We can remove PublicApiAnalyzers when we have your custom solution working here.

In this case it is limitation of the tool, that is aware only of two API stages: 'new' and 'old'. Its OK to flag all issues when you change API surface, but NOT when you are prototyping. Having tooling that reports false-positive for each code change is counter-productive. This is why we invested in API chief at first, because those analyzers were not working for us.

Since the experimental attribute is now part of the .NET platform and is supported by the compiler, the problem you're describing is now universal, and it will be felt by others. The issue needs to be addressed generically and for everyone.

[geeknoid]

The API tracking solution has been in place in this repo for a month or two. You'll see .json files in the repo that capture the public API surface, and a corresponding analyzer that understand these files and updates them as needed.