Api baselines #25386

Pilchie · 2020-08-28T23:57:29Z

Resolves part of #24347

Pilchie · 2020-08-28T23:58:17Z

src/Analyzers/Analyzers/src/PublicAPI.Unshipped.txt

@@ -0,0 +1 @@
+#nullable enable


@dougbu Pretty sure this project has public API, but the analyzer isn't running on it. Thoughts on why not?

I specifically include only projects with '$(IsImplementationProject)' == 'true' and that excludes analyzer projects. From a compatibility POV, adding or removing public surface in these projects shouldn't matter. So, these files can be removed.

Side note: I also excluded all projects in the src/Tools/ directory (well, anything w/ a relative path that includes "Tools") for similar reasons. Might need to exclude msbuild tasks too (for the Razor SDK) but I didn't get there.

BTW I wasn't sure whether the analyzers folder contained anything w/ important public API e.g. a base package when I wrote up #24347. Sorry.

Pilchie · 2020-08-29T00:00:17Z

src/FileProviders/Manifest.MSBuildTask/src/PublicAPI.Unshipped.txt

@@ -0,0 +1 @@
+#nullable enable


Another example of a project that has public API, but the analyzer isn't running on.

Project file contains <IsImplementationProject>false</IsImplementationProject> so it's excluded from the analysis

Pilchie · 2020-08-29T00:01:48Z

src/Analyzers/Analyzers/src/PublicAPI.Shipped.txt

@@ -0,0 +1 @@
+#nullable enable


@mavasani Regarding this, how much does it matter whether we've actually started annotating these libraries here?

@jcouv would be a better person to answer this.

Based on the lack of errors about this, I suggest we include #nullable enable everywhere. Makes life simpler.

Pilchie · 2020-08-29T00:15:22Z

Hmm. VS had no errors, but the commandline is definitely unhappy 😢

AspNetCore.sln

dougbu · 2020-08-29T00:33:07Z

@Pilchie on the failures, it looks like you're hitting a conflict with 00bbb78 aka @pranavkm's #25275 fix.

dougbu · 2020-08-29T00:33:57Z

Sorry, unintended the close

src/HealthChecks/HealthChecks/src/DependencyInjection/HealthChecksBuilderAddCheckExtensions.cs

dougbu

Let's consistently use attributes for the RS00XY suppressions

* Antiforgery * Azure * Configuration.KeyPerFile * DataProtection

* DefaultBuilder * Features/JsonPatch * FileProviders * HealthChecks * Hosting

Pilchie · 2020-08-31T23:45:44Z

Hmm. I'm going to temporarily remove the PublicAPI files from the AzureAD.UI and AzureADB2C.UI projects. Seems like there are a couple of problems with using them in razor projects:

The analyzer gets passed to both compilation phases, so if the razor generated members are present, it complains during the second compile, but if they are present it complains during the first compile 😦 .
The fix in VS doesn't actually generate anything for the razor generated members.

@mavasani any suggestions on how do support this?

@chsienki - this is similar to the source generator issue with razor compilation. Any ideas?

@pranavkm / @jaredpar - FYI.

The analyzer doesn't play well with the razor compiler :(

dougbu · 2020-09-01T00:38:24Z

src/HealthChecks/HealthChecks/src/HealthCheckService.cs

@@ -38,6 +39,7 @@ public abstract class HealthCheckService
        /// A <see cref="Task{T}"/> which will complete when all the health checks have been run,
        /// yielding a <see cref="HealthReport"/> containing the results.
        /// </returns>
+        [SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "Required to maintain compatibility")]
        public Task<HealthReport> CheckHealthAsync(CancellationToken cancellationToken = default)


nit: I wish the analyzer would suppress itself when the only optional parameter is a CancellationToken. The "overload for int, float, … cases e.g. WriteAsync(...) methods and similar are causing a lot of suppression noise.

@sharwell @mavasani @jcouv❔

ghost · 2020-09-01T01:59:23Z

Hello @Pilchie!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Pilchie · 2020-09-01T01:59:32Z

I'm going to go ahead and approve my own PR.

ghost · 2020-09-01T01:59:49Z

Apologies, while this PR appears ready to be merged, it looks like release/5.0 is a protected branch and I have not been granted permission to perform the merge.

dougbu · 2020-09-01T02:27:32Z

Didn't I approve it❔ And, ugh, you're hitting the overlapping PDB problem here too☹️

Pilchie · 2020-09-01T02:32:12Z

I mean ask-mode approval ;)

ghost · 2020-09-01T04:24:29Z

Apologies, while this PR appears ready to be merged, it looks like release/5.0 is a protected branch and I have not been granted permission to perform the merge.

Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 28, 2020

Pilchie added this to the 5.0.0-rc1 milestone Aug 28, 2020

Pilchie requested review from dougbu and javiercn August 28, 2020 23:57

Pilchie requested a review from Tratcher as a code owner August 28, 2020 23:57

Pilchie commented Aug 29, 2020

View reviewed changes

dougbu reviewed Aug 29, 2020

View reviewed changes

AspNetCore.sln Show resolved Hide resolved

dougbu closed this Aug 29, 2020

dougbu reopened this Aug 29, 2020

Pilchie force-pushed the api-baselines branch from f3e91bc to dd9a073 Compare August 29, 2020 00:40

dougbu reviewed Aug 29, 2020

View reviewed changes

src/HealthChecks/HealthChecks/src/DependencyInjection/HealthChecksBuilderAddCheckExtensions.cs Outdated Show resolved Hide resolved

dougbu reviewed Aug 29, 2020

View reviewed changes

src/HealthChecks/HealthChecks/src/DependencyInjection/HealthChecksBuilderAddCheckExtensions.cs Outdated Show resolved Hide resolved

dougbu requested changes Aug 29, 2020

View reviewed changes

Pilchie added 5 commits August 31, 2020 16:07

Enable Public API analyzer for some projects

3668c22

* Antiforgery * Azure * Configuration.KeyPerFile * DataProtection

Enable Public API Analyzer for more projects

28bc63f

* DefaultBuilder * Features/JsonPatch * FileProviders * HealthChecks * Hosting

Enable Public API analyzer for analzyers

0e804eb

Fix for rebase

956b843

Fix razor PublicAPI files

51d419a

Pilchie force-pushed the api-baselines branch from dd9a073 to 51d419a Compare August 31, 2020 23:43

Pilchie added 2 commits August 31, 2020 16:47

Remove API baselines from AzureAD.UI and AzureADB2C.UI

f5b83b9

The analyzer doesn't play well with the razor compiler :(

Convert from pragma to [SuppressMesage]

83a2dee

dougbu approved these changes Sep 1, 2020

View reviewed changes

Pilchie added Servicing-approved Shiproom has approved the issue auto-merge labels Sep 1, 2020

Pilchie merged commit 95c526f into dotnet:release/5.0 Sep 1, 2020

Pilchie deleted the api-baselines branch September 1, 2020 14:46

Pilchie mentioned this pull request Sep 1, 2020

Analyzers should be invoked only in a single phase when compiling Razor projects dotnet/roslyn#47350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api baselines #25386

Api baselines #25386

Pilchie commented Aug 28, 2020

Pilchie Aug 28, 2020

dougbu Aug 29, 2020

dougbu Aug 29, 2020

Pilchie Aug 29, 2020

dougbu Aug 29, 2020

Pilchie Aug 29, 2020

mavasani Aug 29, 2020

dougbu Aug 29, 2020

Pilchie commented Aug 29, 2020

dougbu commented Aug 29, 2020

dougbu commented Aug 29, 2020

dougbu left a comment

Pilchie commented Aug 31, 2020

dougbu Sep 1, 2020

ghost commented Sep 1, 2020

Pilchie commented Sep 1, 2020

ghost commented Sep 1, 2020

dougbu commented Sep 1, 2020

Pilchie commented Sep 1, 2020

ghost commented Sep 1, 2020

Api baselines #25386

Api baselines #25386

Conversation

Pilchie commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pilchie commented Aug 29, 2020

dougbu commented Aug 29, 2020

dougbu commented Aug 29, 2020

dougbu left a comment

Choose a reason for hiding this comment

Pilchie commented Aug 31, 2020

Choose a reason for hiding this comment

ghost commented Sep 1, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Pilchie commented Sep 1, 2020

ghost commented Sep 1, 2020

dougbu commented Sep 1, 2020

Pilchie commented Sep 1, 2020

ghost commented Sep 1, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.