API Check requires two exclusions for change to interface member

[rynowak]

When making a parameter change to an interface I had to write two exclusions:

  {
        "OldTypeId": "public interface Microsoft.AspNetCore.Mvc.Razor.Compilation.ICompilationService",
        "OldMemberId": "Microsoft.AspNetCore.Mvc.Razor.Compilation.CompilationResult Compile(Microsoft.AspNetCore.Mvc.Razor.Compilation.RelativeFileInfo fileInfo, System.String compilationContent)",
        "NewTypeId": "public interface Microsoft.AspNetCore.Mvc.Razor.Compilation.ICompilationService",
        "NewMemberId": "Microsoft.AspNetCore.Mvc.Razor.Compilation.CompilationResult Compile(Microsoft.AspNetCore.Razor.Evolution.RazorCodeDocument codeDocument, Microsoft.AspNetCore.Razor.Evolution.RazorCSharpDocument cSharpDocument)",
        "Kind": "Modification"
  },
  {
        "OldTypeId": "public interface Microsoft.AspNetCore.Mvc.Razor.Compilation.ICompilationService",
        "NewTypeId": "public interface Microsoft.AspNetCore.Mvc.Razor.Compilation.ICompilationService",
        "NewMemberId": "Microsoft.AspNetCore.Mvc.Razor.Compilation.CompilationResult Compile(Microsoft.AspNetCore.Razor.Evolution.RazorCodeDocument codeDocument, Microsoft.AspNetCore.Razor.Evolution.RazorCSharpDocument cSharpDocument)",
        "Kind": "Addition"
  }

The root cause is here https://github.com/aspnet/BuildTools/blob/dev/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs#L87 - this is fundamentally the wrong approach, just relying on a count is flawed. This code should keep track of two sets (old members, new members) and add/remove items from those sets instead of processing a count.

[dougbu]

@rynowak I agree this is annoying. But, is it worse in some cases i.e. can the bug cause missed changes?

BTW the root cause also relates to the code around https://github.com/aspnet/BuildTools/blob/dev/src/Microsoft.AspNetCore.BuildTools.ApiCheck/ApiListingComparer.cs#L317. A ChangeKind.Modification exception is treated as a single removal (removals are checked first) and not a combination of an addition and a removal.

[Eilon]

The suggestion here is to remove the concept of a "Modification" and instead have only "Add" and "Remove". This eliminates all ambiguities about how similar an old/new member has to be in order to be considered a "Modification".

cc @javiercn FYI

[smitpatel]

@Eilon - Are we thinking to remove "modification" for all types or just interfaces? That would somewhat affect output of #163

[dougbu]

@smitpatel not really. The "modification" concept in API Check doesn't extend beyond reading and using the exceptions. That is, the tool doesn't detect anything but additions and removals. The only special thing about interfaces is additions are always considered breaking changes.

That said, fixing this issue will involve changing exceptions files across Universe repos to change "modification" entries to "removal".

[smitpatel]

That is true except we want to keep "Modification" for normal types and make tool to actually use some logic to arrive at simple case modifications. Using some sort of structural comparison, it will not be too difficult to detect at least method name/return type change or addition of a parameter on method. There is always a limit how smart the tool can be and for some cases, we can never detect modifications at all.

But if we are looking at removing "Modification" as type fully, that would simplify my task a lot at least implementation wise.

For modifying exception files across universe, if #163 is fully done first then it will not be difficult to update exception files.

[dougbu]

we want to keep "Modification" for normal types and make tool to actually use some logic to arrive at simple case modifications

What @Eilon said was we won't keep the concept of modification all up.

if #163 is fully done first then it will not be difficult to update exception files.

The order doesn't really matter because the tool treats modifications as removals.

[Eilon]

Yes, I'd like to get rid of "Modification" entirely. Just "Add" and "Remove".

As far as the ordering of this bug and #163, I guess I could see that once #163 is in it might make things easier, but per @dougbu, I think the difference would be minimal.