Remove obsolete suppressions when re-generating suppression files

[ViktorHofer]

When an existing suppression file is re-generated, its content is read, deserialized into a set of suppressions and then stored in a list. When then at a later point, the suppressions are serialized to disk (because the ´--generate-suppression-file` flag is provided), the existing deserialized suppressions plus the new ones are written to disk. Therefore, obsolete suppressions which don't apply anymore are preserved and never get trimmed out.

It would be good to offer a flag to trim out such obsolete suppressions, similar to the one that the legacy APICompat tooling already offers: https://github.com/dotnet/arcade/blob/025103bcaefad81506465eeb7bb09b107b20f32d/src/Microsoft.DotNet.ApiCompat/src/ApiCompatRunner.cs#L45

[ghost]

@dotnet/area-infrastructure-libraries a new issue has been filed in the ApiCompat area, please triage

[RussKie]

I'd like to suggest increasing the scope to not just removing the obsolete suppressions but also to detect obsolete suppressions and warn, if those are found.

I'm currently working on a project that had API compat issues. As a starting point we generated a suppression file, then I got to work on fixes. Once I had a fix, I wanted to confirm it addressed the issue by building and packing the project. However, I didn't observe any build warnings with the suppression file still in place. I deleted the suppression file, and still had no warnings... At this point, I was confused and wasn't sure whether the validation is being run at all. And then had to rebuild the whole thing with binlog, and look through it to see that the build target was actually being executed. This is not a good dev experience.

[ViktorHofer]

This is not a good dev experience.

Note that the CSharp compiler doesn't emit an error for obsolete NoWarns either, nor do most other tools. In the case of APICompat, a message indicating that the validation succeeded is not logged on minimum verbosity - by design. If you change the verbosity to normal you will see a validation confirmation message.

While I disagree that this is a bad developer experience, I agree with you on that APICompat should be able to detect obsolete suppressions and indicate them.

If someone would like to try implementing the feature, take a look at the SuppressionEngine type and https://github.com/dotnet/sdk/blob/058e2ae1a2ef6627c9fe3379a21b55ef213864a5/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs#L22 which stores the suppressions.

[RussKie]

While I disagree that this is a bad developer experience

I didn't say it was bad, I said it wasn't good, e.g. not nice :) I should've said "it could be improved" instead. My bad.

In the case of APICompat, a message indicating that the validation succeeded is not logged on minimum verbosity - by design. If you change the verbosity to normal you will see a validation confirmation message.

Good to know, thank you. I'm ofc running with minimal verbosity, so wouldn't know.

[RussKie]

If someone would like to try implementing the feature, take a look at the SuppressionEngine type and

https://github.com/dotnet/sdk/blob/058e2ae1a2ef6627c9fe3379a21b55ef213864a5/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs#L22

which stores the suppressions.

I had a quick look, and I think we may need another set to track which suppressions have been used, i.e., passed into https://github.com/dotnet/sdk/blob/59a1bba0385ec423b465ff967dd3717eed4796e3/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Logging/SuppressionEngine.cs#L52 ...and then reconcile the difference at the end.

Would you say that's somewhere on the right track of thinking?

[ViktorHofer]

Yes, you are on the right track.

I'm currently working on this in a branch and will share it later today. What complicates this scenario is that we allow "global" suppressions that don't exactly match.