Proposal to provide API Compat tools #177
Conversation
|
|
||
| ## Compatibility Rules | ||
|
|
||
| After looking at the existing tools, these is an overview of the proposed rules for the first version of the tool and that will run by default. |
There was a problem hiding this comment.
When in servicing, almost any change to public types and members is a breaking change. Hope narrowing errors or warnings to breaking changes can be disabled i.e. we have the ability to get the tool to scream about any change.
| 1. All analyzer's disable mechanism when using the tool as an analyzer. | ||
| 2. A txt file that lists the errors to baseline (maybe we could support this for the analyzer scenario as well). | ||
|
|
||
| Configuring rules involves settings like, ignoring certain attributes as describe in the attribute compat rule, rules number 8 in the document, etc. I think these could be part of the `.editorconfig` with configuration for rules' severity. |
There was a problem hiding this comment.
Sorry if this is a silly question, but why dont we use the project files that already contain all project configuration for this? Why a separate .editorconfig file (which also seems a bit surprising because this isn't about configuring the editor, it's for analyzers).
There was a problem hiding this comment.
For the analyzer scenario .editorconfig is the best way to set this rules as project configurations don't make it to the analyzer because of how the analyzers are invoked as part of the build. So if you already have an .editorconfig with these rules and want to run the tool as a dotnet tool for some reason, I thought it might be useful to accept that .editorconfig as an input rather than having people learn about multiple command line arguments to flow those rule settings.
However, it seems like something that is not critical. We could leave the .editorconfig support only for the analyzer scenario.
There was a problem hiding this comment.
After chatting with @ericstj extensibly we believe that the analyzer scenario is not the highest priority at the moment so we shouldn't design based on the analyzer scenario, the analyzer scenario can come after.
| 2. Attribute arguments does not match in both the contract and implementation. | ||
| 2.1 For flag arguments (i.e AtrributeTargets) we should decide whether expanding or removing a flag is breaking or not. | ||
|
|
||
| We should allow a Rule setting to disable attribute diffing for a list of attributes where libraries can add attributes they don't care about compat, i.e `EditorBrowsableAttribute`. |
There was a problem hiding this comment.
Would we be opinionated by default, e.g. with a built-in default list of attributes we believe shouldn't impact compatibility? Or would we require every developer to build that list up themselves?
There was a problem hiding this comment.
Yeah, I think we should have a default list of attributes that shouldn't impact compatibility, like compiler generated attributes, obsolete attributes, editor browsable, etc. Will add a note about that here.
| ### 13. Nullable annotations should match | ||
| This rule should make sure that nullable annotations match in between the implementation and the contract. | ||
|
|
||
| Also, this rule should allow differences in nullable annotations that are not breaking, for example, relaxing a nullable reference type annotation on an in parameter (i.e `public string Foo(string a) -> public string Foo(string? a)`). However, this should be configurable via rule settings to disable this as in some cases where you are comparing a reference assembly vs it's implementation you want them to match in all cases. |
There was a problem hiding this comment.
Does the above list cover everything relevant called out in the breaking change guidance we have in dotnet/runtime?
There was a problem hiding this comment.
Also, are there any differences between this and what apicompat protects in dotnet/runtime?
There was a problem hiding this comment.
Yes, APICompat doesn't yet support nullable. I think we should be calling out all the rules which APICompat has today as well as those which we deem are needed.
There was a problem hiding this comment.
I doubled checked and this list covers everything we have in dotnet/runtime. It is a super set as @ericstj mentioned we don't yet support nullable in APICompat.
|
|
||
| ## Open questions | ||
|
|
||
| 1. Where should these tools live? Maybe [dotnet/roslyn-analyzers](https://github.com/dotnet/roslyn-analyzers)? |
There was a problem hiding this comment.
What is the distribution mechanism? Are the tools part of the .NET SDK? Distributed separately as a nuget package?
There was a problem hiding this comment.
I'll add that as an open question, but I think it should be distributed separately as a nuget package.
Co-authored-by: Jan Kotas <[email protected]>
|
|
||
| ### MSBuild support | ||
|
|
||
| We should provide MSBuild first class configurability via properties so that costumers can just add a `PackageReference` and set properties on how they want to consume the tool (via MSBuild Task, Analyzer, etc), what the inputs are going to be, where the baseline errors file can be found, etc. This would make it very simple for customers to include in their libraries and configure without having to write complicated MSBuild targets to run the tool on an MSBuild process if that is their best way to hook it up into their build system. |
There was a problem hiding this comment.
Roslyn analyzer a dotnet tool and potentially an MSBuild Task,
Could you say a little more about the scenarios for each? I guess Roslyn analyzers don't get handed the list of references, or we wouldn't need an MSBuild task also? When would we use each of these -- and do we need all three in this release?
There was a problem hiding this comment.
Maybe this goes together with your list of different inputs.
There was a problem hiding this comment.
Not to belabor the point, -- but I think it would be helpful to walk through from a customer point of view -- they have an existing library they're building and shipping, and want to be sure they don't unintentionally break the surface -- they heard about this feature -- what do they do now? Other scenarios might include -- understanding whether their dependencies changed ? Baselining a known break? Baselines that differ according to target framework? Etc. There may be other scenarios.
There was a problem hiding this comment.
Sure, makes sense. I will also add priority of each one, user scenarios, pros and cons.
| - Constructor | ||
| - Generic Parameters | ||
|
|
||
| Should we check for assembly attributes maybe as opt-in? |
There was a problem hiding this comment.
The concern here is that most of them don't relate to compat?
There was a problem hiding this comment.
Attributes are all use case specific if they impact compatibility or not. You need to understand the purpose of the attribute to know if it matters. This is not only limited to assembly attributes.
IMHO a public tool should error on the side of minimizing false positives by default. We should have rules for attributes we know impact compatibility, and then define the comparisons for those attributes. We could allow a user to enable more checking if we think its important, perhaps by enabling user-specified attributes and assuming a default compatibility rule (exact match).
|
|
||
| In an effort to provide tools to help customers write long term compatible libraries there are different tools. Currently we have [`Microsoft.DotNet.ApiCompat`](https://github.com/dotnet/arcade/tree/0a67fb1cd008b0d30577f53d1633ed00db8bc1e6/src/Microsoft.DotNet.ApiCompat#microsoftdotnetapicompat) and [`Microsoft.CodeAnalysis.PublicApiAnalyzers`](https://github.com/dotnet/roslyn-analyzers/blob/master/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md). In between these two there is a huge difference on how these work, what rules they have and how to integrate it to libraries. | ||
|
|
||
| `Microsoft.DotNet.ApiCompat` is heavily used in `ASP.NET`, `dotnet/runtime`, `ML.NET` and a few other products across the `dotnet` orgs. While `ApiCompat` has a lot of useful rules and functionality, it uses `CCI` as its metadata object model which brings some limitations on maintainibility and extensibility when new language features are added because `CCI` doesn't know about them and we have to implement those ourselves (i.e ref structs, readonly, nullable annotations, etc). |
There was a problem hiding this comment.
With this effort, could we delete Microsoft.DotNet.ApiCompat everywhere? What work would be necessary in each repo to change over?
There was a problem hiding this comment.
Yeah, I think with this work Microsoft.DotNet.ApiCompat should be obsoleted and each repo should change over.
Changing over will be, consuming the new package/tool, deciding what's the best scenario for each repo and adjusting based on the new support, MSBuild properties/targets, etc.
There was a problem hiding this comment.
nit / FYI: dotnet/aspnetcore does not use Microsoft.DotNet.ApiCompat. We considered using it but enough bug fixes weren't back-ported to Arcade's release/3.x branch that we decided just to be extra careful when reviewing 2.1 and 3.1 code changes.
|
|
||
| Instead of binaries or text files, it might be helpful to capture what the contract is as a C# minimal file (somewhat like a native header file) that contains the API definitions and metadata. For this we would provide a separate tool, aka `GenAPI` that given an binary input produces minimal C# code. This tool will also be Roslyn based, protype can be found here: https://github.com/safern/roslyn-genapi. This provides a contract that is "easier" to read and is also buildable and produces a minimal reference assembly, currently what `dotnet/runtime` uses as a contract. | ||
|
|
||
| ## Compatibility Rules |
There was a problem hiding this comment.
It might be easier to just pull out all these rules into a separate doc since that's really a separate set of decisions and then this doc can focus on how the tool will be used, how we will write it, etc.
|
|
||
| The proposal in this document is to create a tool that will help customers build compatible libraries by helping them identify breaking changes as part of their builds. This tool, will make sure that the public APIs in between an implementation and a contract (which can be a previous version or a current version of the library) are compatible to avoid introducing an accidental breaking change. | ||
|
|
||
| ## How is this tool going to be consumed |
There was a problem hiding this comment.
Does this tool understand anything about assembly versioning eg semver? I assume not.
There was a problem hiding this comment.
I don't think that is in the scope of this tool but it might come into play when thinking on package validation which will land into the same area/tool as we want to make it less expensive so having different tools for binary compatibility and package compat would defeat that purpose.
|
|
||
| ### 3. Source inputs | ||
|
|
||
| Instead of binaries or text files, it might be helpful to capture what the contract is as a C# minimal file (somewhat like a native header file) that contains the API definitions and metadata. For this we would provide a separate tool, aka `GenAPI` that given an binary input produces minimal C# code. This tool will also be Roslyn based, protype can be found here: https://github.com/safern/roslyn-genapi. This provides a contract that is "easier" to read and is also buildable and produces a minimal reference assembly, currently what `dotnet/runtime` uses as a contract. |
There was a problem hiding this comment.
Could you explain why it might be helpful (to customers?)
There was a problem hiding this comment.
Sure, I'll elaborate more on each input.
|
I was wondering today whether I could reuse some of the tooling I know from dotnet repos in my own libraries. @ViktorHofer pointed me to this design. I see many things I'd like in the design doc. This is how I imagine using them: After adding something in csproj, |
|
See also dotnet/roslyn-analyzers#3047 for some places where the Roslyn PublicApiAnalyzer could have been extended. |
|
|
||
| In an effort to provide tools to help customers write long term compatible libraries there are different tools. Currently we have [`Microsoft.DotNet.ApiCompat`](https://github.com/dotnet/arcade/tree/0a67fb1cd008b0d30577f53d1633ed00db8bc1e6/src/Microsoft.DotNet.ApiCompat#microsoftdotnetapicompat) and [`Microsoft.CodeAnalysis.PublicApiAnalyzers`](https://github.com/dotnet/roslyn-analyzers/blob/master/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md). In between these two there is a huge difference on how these work, what rules they have and how to integrate it to libraries. | ||
|
|
||
| `Microsoft.DotNet.ApiCompat` is heavily used in `ASP.NET`, `dotnet/runtime`, `ML.NET` and a few other products across the `dotnet` orgs. While `ApiCompat` has a lot of useful rules and functionality, it uses `CCI` as its metadata object model which brings some limitations on maintainibility and extensibility when new language features are added because `CCI` doesn't know about them and we have to implement those ourselves (i.e ref structs, readonly, nullable annotations, etc). |
There was a problem hiding this comment.
nit / FYI: dotnet/aspnetcore does not use Microsoft.DotNet.ApiCompat. We considered using it but enough bug fixes weren't back-ported to Arcade's release/3.x branch that we decided just to be extra careful when reviewing 2.1 and 3.1 code changes.
|
|
||
| `Microsoft.DotNet.ApiCompat` is heavily used in `ASP.NET`, `dotnet/runtime`, `ML.NET` and a few other products across the `dotnet` orgs. While `ApiCompat` has a lot of useful rules and functionality, it uses `CCI` as its metadata object model which brings some limitations on maintainibility and extensibility when new language features are added because `CCI` doesn't know about them and we have to implement those ourselves (i.e ref structs, readonly, nullable annotations, etc). | ||
|
|
||
| `Microsoft.CodeAnalysis.PublicAnalyzers` is used in `dotnet/roslyn` and it is a pure roslyn analyzer. |
There was a problem hiding this comment.
FYI dotnet/aspnetcore uses Microsoft.CodeAnalysis.PublicApiAnalyzers in our release/5.0 and main branches
I assume you mean
| `Microsoft.CodeAnalysis.PublicAnalyzers` is used in `dotnet/roslyn` and it is a pure roslyn analyzer. | |
| `Microsoft.CodeAnalysis.PublicApiAnalyzers` is used in `dotnet/roslyn` and it is a pure roslyn analyzer. |
|
|
||
| We are proposing a tool that can be consumed as a Roslyn analyzer and also providing a dotnet tool that people can integrate to their build systems as an out of proc tool or run directly from the console. | ||
|
|
||
| Something to consider would be adding an MSBuild Task entrypoint so that libraries that need to run it in their build, they don't need to shell out a process to invoke it and just run in-proc. |
There was a problem hiding this comment.
- So, a Roslyn analyzer for in-proc use and a command-line tool to wrap it❔
- What about fixers usable from command line, VS, VS for Mac, and VS Code❔
There was a problem hiding this comment.
We still need to figure out the fixer's scenario. From talking with @ericstj we think that running in-proc as an MSBuild Task is the most important scenario to tackle as that is what drives compat from one release to another.
The analyzer scenario is very valuable in this case, specifically for the code fixers (I have a compat warning, help me fix it), however, we think that can come afterwards.
There was a problem hiding this comment.
however, we think that can come afterwards
Could you please expand on the reasoning here❔
My thought is that without automation to correct the API baselines, ASP.NET Core just has too much surface area to handle. That means we can't use new tooling without reliable and usable fixers. It's currently a pain point that the PublicAPIAnalyzers fixers work only in Visual Studio. Requiring manual API baseline generation and updates is a non-starter.
There was a problem hiding this comment.
My thought is that without automation to correct the API baselines, ASP.NET Core just has too much surface area to handle. That means we can't use new tooling without reliable and usable fixers. It's currently a pain point that the PublicAPIAnalyzers fixers work only in Visual Studio. Requiring manual API baseline generation and updates is a non-starter.
Oh, we do plan to support baselining errors automatically regardless of having the analyzer scenario or not... I still need to discuss with @terrajobst the user experience for baselining errors. Cause with analyzers you can disable on the code or via .editorconfig, so we might want to be able to do similar stuff like a global "NoWarn" and also baseline errors via a baseline file.
But we think that the MSBuild Task can generate this for you if you run it with a flag (which is essentially the same thing as running a codefixer). Does that make sense?
By the way this is great input and that is why I wanted to publish the doc early to gather feedback, no I'm working tomorrow with @terrajobst to polish this and put it in a final state where we can say, this is how everything is going to work on v1 and this is what will come in future releases, etc.
There was a problem hiding this comment.
MSBuild Task can generate this for you if you run it with a flag (which is essentially the same thing as running a codefixer). Does that make sense?
I'm not sure because my expectation is we need to fix both product code (possibly with suppressions) and the baselines. The tool you described only generates baselines, right❔
|
|
||
| `Microsoft.CodeAnalysis.PublicAnalyzers` is used in `dotnet/roslyn` and it is a pure roslyn analyzer. | ||
|
|
||
| The biggest difference in between these is the inputs they take for the contract to know if the current implementation is a breaking change. `PublicAnalyzers` looks for two files in the project, `PublicAPI.Shipped.txt` to check for compat against previous versions of the library and `PublicAPI.Unshipped.txt` to check for compat on the current release. Wheareas `ApiCompat` takes two binaries, one for the contract and the other for the implementation and checks for the differences in between those two. These two different inputs are useful in different ways and customers have their own preference. |
There was a problem hiding this comment.
| The biggest difference in between these is the inputs they take for the contract to know if the current implementation is a breaking change. `PublicAnalyzers` looks for two files in the project, `PublicAPI.Shipped.txt` to check for compat against previous versions of the library and `PublicAPI.Unshipped.txt` to check for compat on the current release. Wheareas `ApiCompat` takes two binaries, one for the contract and the other for the implementation and checks for the differences in between those two. These two different inputs are useful in different ways and customers have their own preference. | |
| The biggest difference in between these is the inputs they take for the contract to know if the current implementation is a breaking change. `PublicApiAnalyzers` looks for two files in the project, `PublicAPI.Shipped.txt` to check for compat against previous versions of the library and `PublicAPI.Unshipped.txt` to check for compat on the current release. Whereas `ApiCompat` takes two binaries, one for the contract and the other for the implementation and checks for the differences in between those two. These two different inputs are useful in different ways and customers have their own preference. |
|
|
||
| ### 1. Text files | ||
|
|
||
| Just as `Microsoft.CodeAnalysis.PublicAnalyzers` currently accepts a `PublicAPI.Unshipped.txt` and `Public.Shipped.txt` files as inputs, we should preserve this behavior, but extend some of the rules and rule settings that we support so that all inputs have equal support. |
There was a problem hiding this comment.
| Just as `Microsoft.CodeAnalysis.PublicAnalyzers` currently accepts a `PublicAPI.Unshipped.txt` and `Public.Shipped.txt` files as inputs, we should preserve this behavior, but extend some of the rules and rule settings that we support so that all inputs have equal support. | |
| Just as `Microsoft.CodeAnalysis.PublicApiAnalyzers` currently accepts a `PublicAPI.Unshipped.txt` and `Public.Shipped.txt` files as inputs, we should preserve this behavior, but extend some of the rules and rule settings that we support so that all inputs have equal support. |
| ### 2. Cannot remove abstract members | ||
| This rules check for abstract members in types. This rule validates that a type has the same abstract members in both the contract and implementation when the type can be used as base type: | ||
| 1. Is marked as sealed. | ||
| 2. Has no non static visible constructors outside the assembly. |
There was a problem hiding this comment.
Adding an abstract member is also a breaking change
| 3. Type is not sealed. | ||
|
|
||
| ### 4. Cannot remove base type or interface | ||
| This rule should check that the base types and interfaces match on both the implementation and in the contract. |
There was a problem hiding this comment.
Important to handle the special case of moving an interface declaration to the base type. That's not a breaking change.
There was a problem hiding this comment.
Yeah, this should cover the interfaces from the base types.
|
|
||
| ## Compatibility Rules | ||
|
|
||
| After looking at the existing tools, these is an overview of the proposed rules for the first version of the tool and that will run by default. |
There was a problem hiding this comment.
When in servicing, almost any change to public types and members is a breaking change. Hope narrowing errors or warnings to breaking changes can be disabled i.e. we have the ability to get the tool to scream about any change.
Thanks @tmds for your input and interest on this. I will push an updated doc after the feedback and your input and will describe more extensively the scenarios and how customers would use it.
Thanks, @drewnoakes, will make sure to include all of them in the list of rules. I believe most of them are already covered, but I need to confirm. Adding |
|
@safern, what do we need to do to move this forward? |
|
I'll update it to what we decided at the end and clean it up. Thanks! |
Design doc for: dotnet/core#5701
Contributes to: dotnet/core#5700
Relates to: dotnet/roslyn-analyzers#3901
cc: @terrajobst @danmoseley @ericstj @Anipik @sharwell @ViktorHofer @stephentoub
Feel free to loop in other people that might be interested on this.