Fix APICompat errors for atttributes on parameters #49408
Conversation
safern
requested review from
eiriktsarpalis,
jozkee,
layomia and
steveharter
as code owners
ghost
added
the
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| public bool Expired { get { throw null; } set { } } | ||
| public System.DateTime Expires { get { throw null; } set { } } | ||
| public bool HttpOnly { get { throw null; } set { } } | ||
| [System.Diagnostics.CodeAnalysis.AllowNullAttribute] |
There was a problem hiding this comment.
This is a breaking change against .NET 5.0, however the implementation indeed doesn't allow null in this case:
runtime/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs
Lines 230 to 233 in 632a56b
There was a problem hiding this comment.
IIRC we are still permitting corrections to nullable annotations?
There was a problem hiding this comment.
I think we are. cc: @stephentoub @jeffhandley
There was a problem hiding this comment.
Yup. We are tracking all of them for a breaking change issue.
dotnet/docs#21202
| @@ -0,0 +1,9 @@ | |||
| CannotRemoveAttribute : Attribute 'System.Diagnostics.CodeAnalysis.AllowNullAttribute' exists on parameter 'left' on member 'System.Runtime.CompilerServices.Unsafe.AreSame<T>(T, T)' in the contract but not the implementation. | |||
There was a problem hiding this comment.
I added this to avoid having to add the attributes to the IL as it was verbose because we need to if def depending on the tfm to reference System.Private.CoreLib.
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Thanks for fixing this all up.
|
I'll follow up on the browser failures tomorrow. |
5 participants
This reacts to: dotnet/arcade#7058
The build will fail as we need to update API Compat, but before merging the change in arcade I wanted to do a run on dotnet/runtime and see if there was something weird with the rule going on.