Updated Microsoft.CodeAnalysis.PublicApiAnalyzers to latest stable #41115

BrennanConroy · 2022-04-08T21:55:58Z

No description provided.

dougbu

I'm not sure how to review this thoroughly since the changes mix the different cases. Might be worthwhile splitting out (for example) the removal of true duplicate lines from the rest. The next stage might be to handle nullability changes for existing methods (remove a large number of *REMOVED* lines from the Unshipped files while annotating the Shipped files correctly). Finally, could do the Microsoft.CodeAnalysis.PublicApiAnalyzers version bump while adding default constructors.

Another debatable issue is how to handle default constructors we shipped in .NET 6.0.0. They all show up in PublicAPI.Unshipped.txt files in this PR.

src/Middleware/CORS/src/PublicAPI.Shipped.txt

dougbu · 2022-04-09T01:22:35Z

This is actually very close to passing CI validation. Congrats @BrennanConroy❕

Should decide a few things like

whether we instead want fixes for the PublicAPI analyzer regressions instead (see Introduce IBindableFromHttpContext<TSelf> #41100)
where to place default constructors we shipped in .NET 6.0.0, and
how to get a thorough review

DamianEdwards · 2022-04-12T18:36:08Z

Any update on if when we might get this merged?

BrennanConroy · 2022-04-13T16:29:39Z

whether we instead want fixes for the PublicAPI analyzer regressions instead

The only real regression IMO is the forwarded types being messy. I think the issue with REMOVED for non-nullable annotated APIs when we annotate them as nullable is fine, it should just be this one time cost.

2. where to place default constructors we shipped in .NET 6.0.0

I don't feel strongly about figuring out the "correct" solution to this, I think it's completely fine to leave them in the unshipped file and move on with our lives.

3. how to get a thorough review

Good luck 😄 , I'm not going to spend a couple hours trying to have separate commits for "removing duplicates", adding default ctors, etc.

BrennanConroy · 2022-04-23T05:22:39Z

PR updated with split commits.

Added default ctors to unshipped files
Removed duplicate entries (Same APIs but one with ~ and one without ~)
Normalized nullability in APIs

The third one is a bit odd since there are still APIs with ~, basically just kept modifying based on the analyzers errors over and over until the build passed. I think we should remove all ~ from our shipped files at the end of 7.0.

dougbu · 2022-04-25T18:50:01Z

Thanks for the splitting @BrennanConroy❕ I'll try to look at this today but would also like to hear from @DamianEdwards, @dotnet/aspdoi and / or @dotnet/aspnet-build on the other questions.

@jmarolf any chance the package will be or has been fixed enough to avoid these shenanigans❔

jmarolf · 2022-04-25T18:52:22Z

@jmarolf any chance the package will be or has been fixed enough to avoid these shenanigans❔

Which shenanigans? We accept PRs :) Microsoft.CodeAnalysis.PublicApiAnalyzers is just infrastructure that roslyn wrote to meet our own needs.

dougbu · 2022-04-25T19:04:32Z

@jmarolf any chance the package will be or has been fixed enough to avoid these shenanigans❔

Which shenanigans? We accept PRs :) Microsoft.CodeAnalysis.PublicApiAnalyzers is just infrastructure that roslyn wrote to meet our own needs.

That's not great for a public project we and others have taken a hard dependency on.

The tooling behaves rather differently in v3.3.0 and v.3.3.3. We didn't make those changes.

@dotnet/aspnet-build @dotnet/aspdoi any suggestions for alternative public API verifiers❔ We can't go back to ApiCompat because that product has too many bugs and restrictions. And PublicApiAnalyzers v3.3.0 can't handle #41100. I'm thinking this PR is a lot of work without a future.

😦

jmarolf · 2022-04-25T19:10:20Z

Probably also worth discussing why the supported features for this don't meet your needs https://docs.microsoft.com/dotnet/fundamentals/package-validation/overview

wtgodbe · 2022-04-25T19:24:30Z

Doesn't package validation only confirm that a package has the same surface area across the TFMs & RIDs it supports? We also need to verify that we don't change API surface in servicing branches, which I'm not sure the package validation tool does.

wtgodbe · 2022-04-25T19:25:20Z

Added default ctors to unshipped files

Do you mean added to .shipped files (for ones from pre-7.0)? I'd be alright with this PR if that was the case

dougbu · 2022-04-25T19:26:50Z

Probably also worth discussing why the supported features for this don't meet your needs https://docs.microsoft.com/dotnet/fundamentals/package-validation/overview

We have that enabled in this repo, have a few CompatibilitySuppressions.xml files, and enable baseline version validation in servicing builds but…

The baseline-version-validator doesn't track incremental API additions
The baseline-version-validator can't prevent public API additions in servicing (it focuses on breaking changes, not all changes)
Our CompatibilitySuppressions.xml would be huge if they listed all of the nullability changes we've made since the 6.0.0 API shipped

dougbu · 2022-04-25T19:44:52Z

@jmarolf for some history here, this goes back past #24188. @Pilchie let @mavasani know our plans to use Microsoft.CodeAnalysis.PublicApiAnalyzers before we started work on that PR. @sharwell was also involved.

dougbu

Am I correct all of the "duplicate" entries removed in the second commit are shipped signatures lacking all nullable annotations, *REMOVED* signatures for the same, or *REMOVED* signatures containing a leading ~ but also the correct nullable annotations❔ If that's the case, the analyzer must previously have ignored the nullable annotations when eliminating duplicates. I can see that as fixing a bug but the default constructor thing isn't great and I haven't looked at the third commit.

src/Middleware/HeaderPropagation/src/PublicAPI.Unshipped.txt

wtgodbe · 2022-04-25T23:20:11Z

@SteveSandersonMS @mattleibow has dotnet/maui had any issues with the 3.3.3 version of Microsoft.CodeAnalysis.PublicApiAnalyzers?

dougbu · 2022-04-25T23:34:21Z

@SteveSandersonMS @mattleibow see also #41115 (comment)

dougbu · 2022-04-29T23:06:42Z

Sorry, coming back to this after a bit. Other than rebasing on 'main', any changes in the first two commits @BrennanConroy❔

dougbu · 2022-04-29T23:08:30Z

Oh, the "new but old" default constructors remain in PublicApi.Unshipped.txt files. As @wtgodbe suggested, those should all or mostly be n Shipped files.

dougbu · 2022-04-29T23:13:59Z

src/Security/Authentication/WsFederation/src/PublicAPI.Shipped.txt

+~Microsoft.AspNetCore.Authentication.WsFederation.WsFederationOptions.ConfigurationManager.get -> Microsoft.IdentityModel.Protocols.IConfigurationManager<Microsoft.IdentityModel.Protocols.WsFederation.WsFederationConfiguration!>!
+~Microsoft.AspNetCore.Authentication.WsFederation.WsFederationOptions.ConfigurationManager.set -> void


Where did these supposedly non-null-aware additions come from❔

dougbu

I think I understand most of the third commit though

It's not clear to me why there remains so much in the PublicAPI.Unshipped.txt files. Did that much really change since 6.0.0❔
I don't know the code nor what's actually changed aside from nullability touch-ups that should now be handled exclusively in the PublicAPI.Shipped.txt files. Suggest we need area owners to look closely at the PublicAPI.Unshipped.txt files.

dougbu · 2022-04-29T23:19:11Z

src/Http/Http.Features/src/PublicAPI.Shipped.txt

-~Microsoft.AspNetCore.Http.Features.FeatureReference<> (forwarded, contained in Microsoft.Extensions.Features)
-~Microsoft.AspNetCore.Http.Features.FeatureReferences<> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T>.Fetch(Microsoft.AspNetCore.Http.Features.IFeatureCollection! features) -> T? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T>.Update(Microsoft.AspNetCore.Http.Features.IFeatureCollection! features, T feature) -> T (forwarded, contained in Microsoft.Extensions.Features)
+static readonly Microsoft.AspNetCore.Http.Features.FeatureReference<T>.Default -> Microsoft.AspNetCore.Http.Features.FeatureReference<T> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReference<T>.FeatureReference() -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache> (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Cache -> TCache? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Collection.get -> Microsoft.AspNetCore.Http.Features.IFeatureCollection! (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.FeatureReferences(Microsoft.AspNetCore.Http.Features.IFeatureCollection! collection) -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Fetch<TFeature, TState>(ref TFeature? cached, TState state, System.Func<TState, TFeature?>! factory) -> TFeature? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Fetch<TFeature>(ref TFeature? cached, System.Func<Microsoft.AspNetCore.Http.Features.IFeatureCollection!, TFeature?>! factory) -> TFeature? (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Initalize(Microsoft.AspNetCore.Http.Features.IFeatureCollection! collection) -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Initalize(Microsoft.AspNetCore.Http.Features.IFeatureCollection! collection, int revision) -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.FeatureReferences() -> void (forwarded, contained in Microsoft.Extensions.Features)
+Microsoft.AspNetCore.Http.Features.FeatureReferences<TCache>.Revision.get -> int (forwarded, contained in Microsoft.Extensions.Features)


I think you mentioned this earlier but to confirm: Forwarding a type no longer implicitly forwards its members❔ That's very messy if so because it's not possible to forward a subset of the members w/o something weird going on (maybe partial classes but I doubt it). Feels like a regression in the analyzer.

src/Middleware/HeaderPropagation/src/PublicAPI.Unshipped.txt

BrennanConroy · 2022-05-27T20:42:59Z

It's not clear to me why there remains so much in the PublicAPI.Unshipped.txt files. Did that much really change since 6.0.0❔

Yes? Besides the struct ctors, there were a bunch of new APIs.

Forwarding a type no longer implicitly forwards its members❔

I don't know the exact behavior here, but the fix I found was to explicitly add any new API in the forwarded type to the old forward-from assembly as well as the actual location the type lives now.

The third commit contains lots of obvious changes that just remove the leading tilde. Are removals like this due to duplicates between the Shipped and Unshipped files that weren't handled in earlier commits❔

I think most of the changes were due to APIs either being marked as nullable in 7.0 or having nullable changes (such as when Extensions was nullable attributed and affected a lot of our APIs). The new version of the analyzer does not like having the same API only differ by nullability it seems like.

BrennanConroy · 2022-06-02T02:35:29Z

Default ctors moved to the shipped file.

wtgodbe

Changes seem reasonable. Will there be manual changes needed going forward? Might be worth letting people know that any active changes will need to be rebased/regenerated

dougbu

Ugh, sorry this took longer than I expected. Nothing much to comment on 😸

src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt

DamianEdwards · 2022-06-08T20:30:30Z

This is looking close 🎉

BrennanConroy · 2022-06-08T20:31:32Z

You'll have to merge @dougbu to override the Code Check task

dougbu · 2022-06-08T20:34:24Z

I'll check in a few hours (have an appointment I need to leave for soon) and get this in before @JamesNK nullifies something else 😃

…otnet#41115) * Adding default ctors * Remove duplicates * normalize nullable * fixup * fb

BrennanConroy mentioned this pull request Apr 8, 2022

Introduce IBindableFromHttpContext<TSelf> #41100

Merged

BrennanConroy force-pushed the brecon/publicapi branch from 29b9559 to abd697f Compare April 8, 2022 22:06

dougbu reviewed Apr 8, 2022

View reviewed changes

src/Middleware/CORS/src/PublicAPI.Shipped.txt Show resolved Hide resolved

Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Apr 11, 2022

BrennanConroy force-pushed the brecon/publicapi branch from abd697f to 42957bd Compare April 23, 2022 05:19

BrennanConroy marked this pull request as ready for review April 23, 2022 05:19

BrennanConroy requested review from a team, halter73, Tratcher, JamesNK, javiercn, wtgodbe and Pilchie as code owners April 23, 2022 05:19

dougbu reviewed Apr 25, 2022

View reviewed changes

src/Middleware/HeaderPropagation/src/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

BrennanConroy force-pushed the brecon/publicapi branch from 42957bd to d288ceb Compare April 29, 2022 16:56

dougbu reviewed Apr 29, 2022

View reviewed changes

BrennanConroy force-pushed the brecon/publicapi branch from d288ceb to c327fe1 Compare June 2, 2022 02:34

BrennanConroy requested review from captainsafia and brunolins16 as code owners June 2, 2022 02:34

BrennanConroy requested review from wtgodbe and dougbu and removed request for wtgodbe June 6, 2022 15:58

wtgodbe approved these changes Jun 6, 2022

View reviewed changes

dougbu approved these changes Jun 6, 2022

View reviewed changes

src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

BrennanConroy added 5 commits June 8, 2022 13:11

Adding default ctors

b6993b3

Remove duplicates

901bffb

normalize nullable

54aaf49

fixup

915254f

fb

cb817a9

BrennanConroy force-pushed the brecon/publicapi branch from c327fe1 to cb817a9 Compare June 8, 2022 20:22

dougbu approved these changes Jun 8, 2022

View reviewed changes

dougbu merged commit 4094430 into main Jun 9, 2022

dougbu deleted the brecon/publicapi branch June 9, 2022 00:41

ghost added this to the 7.0-preview6 milestone Jun 9, 2022

captainsafia pushed a commit to captainsafia/aspnetcore that referenced this pull request Jun 13, 2022

Updated Microsoft.CodeAnalysis.PublicApiAnalyzers to latest stable (d…

d3d0f4e

…otnet#41115) * Adding default ctors * Remove duplicates * normalize nullable * fixup * fb

wtgodbe mentioned this pull request Dec 13, 2022

Update to latest stable version of Microsoft.CodeAnalysis.PublicApiAnalyzers (3.3.3) to fix Roslyn crashing issue #41099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Microsoft.CodeAnalysis.PublicApiAnalyzers to latest stable #41115

Updated Microsoft.CodeAnalysis.PublicApiAnalyzers to latest stable #41115

BrennanConroy commented Apr 8, 2022

dougbu left a comment

dougbu commented Apr 9, 2022

DamianEdwards commented Apr 12, 2022

BrennanConroy commented Apr 13, 2022

BrennanConroy commented Apr 23, 2022

dougbu commented Apr 25, 2022

jmarolf commented Apr 25, 2022 •

edited

Loading

dougbu commented Apr 25, 2022

jmarolf commented Apr 25, 2022

wtgodbe commented Apr 25, 2022

wtgodbe commented Apr 25, 2022

dougbu commented Apr 25, 2022

dougbu commented Apr 25, 2022

dougbu left a comment

wtgodbe commented Apr 25, 2022

dougbu commented Apr 25, 2022

dougbu commented Apr 29, 2022

dougbu commented Apr 29, 2022

dougbu Apr 29, 2022

dougbu left a comment

dougbu Apr 29, 2022

BrennanConroy commented May 27, 2022

BrennanConroy commented Jun 2, 2022

wtgodbe left a comment

dougbu left a comment

DamianEdwards commented Jun 8, 2022

BrennanConroy commented Jun 8, 2022

dougbu commented Jun 8, 2022

		~Microsoft.AspNetCore.Authentication.WsFederation.WsFederationOptions.ConfigurationManager.get -> Microsoft.IdentityModel.Protocols.IConfigurationManager<Microsoft.IdentityModel.Protocols.WsFederation.WsFederationConfiguration!>!
		~Microsoft.AspNetCore.Authentication.WsFederation.WsFederationOptions.ConfigurationManager.set -> void

Updated Microsoft.CodeAnalysis.PublicApiAnalyzers to latest stable #41115

Updated Microsoft.CodeAnalysis.PublicApiAnalyzers to latest stable #41115

Conversation

BrennanConroy commented Apr 8, 2022

dougbu left a comment

Choose a reason for hiding this comment

dougbu commented Apr 9, 2022

DamianEdwards commented Apr 12, 2022

BrennanConroy commented Apr 13, 2022

BrennanConroy commented Apr 23, 2022

dougbu commented Apr 25, 2022

jmarolf commented Apr 25, 2022 • edited Loading

dougbu commented Apr 25, 2022

jmarolf commented Apr 25, 2022

wtgodbe commented Apr 25, 2022

wtgodbe commented Apr 25, 2022

dougbu commented Apr 25, 2022

dougbu commented Apr 25, 2022

dougbu left a comment

Choose a reason for hiding this comment

wtgodbe commented Apr 25, 2022

dougbu commented Apr 25, 2022

dougbu commented Apr 29, 2022

dougbu commented Apr 29, 2022

dougbu Apr 29, 2022

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

dougbu Apr 29, 2022

Choose a reason for hiding this comment

BrennanConroy commented May 27, 2022

BrennanConroy commented Jun 2, 2022

wtgodbe left a comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

DamianEdwards commented Jun 8, 2022

BrennanConroy commented Jun 8, 2022

dougbu commented Jun 8, 2022

jmarolf commented Apr 25, 2022 •

edited

Loading