search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

API compat should be run during build #9126

Open davkean opened 8 years ago

davkean commented 8 years ago

We should be running API compat above and beyond what the API analyzer is catching, to ensure that we can't break it just updating a text file. For example: https://github.com/dotnet/roslyn/issues/9047.

Instead, we should be comparing the current build against against our previously shipped version (maybe pull down a package from NuGet?) and fail the build if we break it.

We can use the existing ApiCompat tool that .NET is using.

davkean commented 8 years ago

tag @jaredpar

jaredpar commented 8 years ago

Issue #9047 happened because we were slow to update our API files. They were not updated for update 1 until very late in our update 2 cycle. Hence this API appeared as new when in fact it was shipped. That is why the change slipped through unnoticed.

I'm okay with running API compat but to my understanding it has not been updated for the compat that Roslyn uses. In particular it doesn't understand our requirements around versioning named / optional parameters. If this is still the case is there a plan to update it?

Also is this tool public?

davkean commented 8 years ago

API compat doesn't apply C# overload resolution to its compat rules, so it won't catch breaks that only occur at compile. It will catch the case, however, when you add additional (optional) parameters to a method, though.

The .NET repro is using, so either it's public or available in a downloadable tool form.

jaredpar commented 8 years ago

I'm not concerned with C# overload resolution here. That's not a problem API Compat should be concerned with. I'm concerned with API compat's ability to correctly understand how we currently version methods with named / optional parameters.

davkean commented 8 years ago

Can you be specific exactly? ApiCompat catches all binary breaking changes, what exactly are you concerned about? Can you give me a specific example?

jaredpar commented 8 years ago

Specific example:

// Version 1 
void M(int x = 42); 

// Version 2 
void M(int x); 
void M(int x = 42, int y = 13);

This is not a binary break but in the past it didn't meet the guidelines of the CoreFX team. It's unclear to me if the APICompat tool is written towards binary compat or the compat rules of CoreFX.

davkean commented 8 years ago

Ah, I see your concern.

We never considered that a breaking change. We just never allowed optional parameters. ApiCompat will not flag that.