dotnet / arcade

Tools that provide common build infrastructure for multiple .NET Foundation projects.
MIT License
670 stars 347 forks source link

ApiCompat: CannotChangeAttribute raised when IteratorStateMachineAttribute's auto-generated type name changed #7896

Open airbreather opened 3 years ago

airbreather commented 3 years ago

The PR validation build for NetTopologySuite/NetTopologySuite#548 initially failed (https://github.com/NetTopologySuite/NetTopologySuite/runs/3599391887) with the following error:

/home/runner/.nuget/packages/microsoft.dotnet.apicompat/6.0.0-beta.21159.11/build/Microsoft.DotNet.ApiCompat.targets(82,5): error : CannotChangeAttribute : Attribute 'System.Runtime.CompilerServices.IteratorStateMachineAttribute' on 'RTools_NTS.Util.StreamTokenizer.GetEnumerator()' changed from '[IteratorStateMachineAttribute(typeof(StreamTokenizer.<GetEnumerator>d__33))]' in the contract to '[IteratorStateMachineAttribute(typeof(StreamTokenizer.<GetEnumerator>d__34))]' in the implementation. [/home/runner/work/NetTopologySuite/NetTopologySuite/src/NetTopologySuite/NetTopologySuite.csproj]
/home/runner/.nuget/packages/microsoft.dotnet.apicompat/6.0.0-beta.21159.11/build/Microsoft.DotNet.ApiCompat.targets(96,5): error : ApiCompat failed for '/home/runner/work/NetTopologySuite/NetTopologySuite/src/NetTopologySuite/bin/Release/netstandard2.0/NetTopologySuite.dll' [/home/runner/work/NetTopologySuite/NetTopologySuite/src/NetTopologySuite/NetTopologySuite.csproj]

Updating the PackageReference version to 7.0.0-beta.21463.6 does not make this error go away (edit: on my machine).

I understand and appreciate that this rule is going to be a bit on the sensitive side, and I know that we can baseline issues like these that do not, in my judgment, signal that there are actual compatibility issues.

Two questions:

  1. Do you agree that this is a perfectly non-breaking change?
  2. If you agree that this is perfectly non-breaking, do you think that this might merit a change to ApiCompat to have the tool include that wisdom as one or more special-cases?
ViktorHofer commented 3 years ago

cc @joperezr @safern @ericstj

safern commented 3 years ago

If you agree that this is perfectly non-breaking, do you think that this might merit a change to ApiCompat to have the tool include that wisdom as one or more special-cases?

I completely agree that API Compat should be smarter for the way it handles attributes. Currently it is a pretty "simple" implementation of comparing left vs right value of the attribute and its constructor arguments.

It also, doesn't filter compiler generated attributes, however we don't want to filter all of them, i.e ReadOnlyAttribute is the way the readonly keyword is expressed on metadata.

Anyway, we started rewriting API Compat as part of the 6.0 release and it is part of the SDK. However it is not complete and is lacking some rules (this one included), but when we add this rule we plan on doing it in a smarter way to consider these kind of scenarios. You can follow our progress on that here: https://github.com/dotnet/sdk/issues/18678

In the meantime as a workaround, you can use a file to exclude certain attributes you don't care on your API Compat runs. i.e: https://github.com/dotnet/runtime/blob/main/eng/DefaultGenApiDocIds.txt

Then you can set an MSBuild property <ApiCompatExcludeAttributeList> pointing to the path of the file containing the attributes to ignore, and API Compat should no longer consider those attributes as part of your compatibility run.