Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Reflection.* missed Equals nullable annotations #52162

Merged
merged 7 commits into from
May 22, 2021

Conversation

hrrrrustic
Copy link
Contributor

No description provided.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

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.

@hrrrrustic
Copy link
Contributor Author

Ugh, bot didn't ping anyone
@buyaa-n @joperezr @krwq @steveharter
Found all of you in area owners for System.Reflection, sry if I ping someone incorrectly
This PR is continuation of #47598

@hrrrrustic
Copy link
Contributor Author

Btw there are some changes for reflection in #52167 under CoreLib and I'm not sure whom should I ping for that PR

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hrrrrustic, could you remove extra castings in the implementation? Other than that LGTM.

@hrrrrustic
Copy link
Contributor Author

Does it make sense to remove nullable marking here?

public bool Equals(RoAssemblyName? other)
{
Debug.Assert(other is not null);
if (Name != other.Name)

@hrrrrustic hrrrrustic requested a review from buyaa-n May 20, 2021 15:46
@buyaa-n
Copy link
Contributor

buyaa-n commented May 20, 2021

Does it make sense to remove nullable marking here?

public bool Equals(RoAssemblyName? other)
{
Debug.Assert(other is not null);
if (Name != other.Name)

That makes sense, but I guess it has ? because the implemented interface signature has ?, if you remove that you will need to suppress the warning, by accounting that this is an internal type probably no need to remove it

@hrrrrustic hrrrrustic requested a review from buyaa-n May 21, 2021 13:54
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @hrrrrustic!

@jeffhandley
Copy link
Member

@buyaa-n Once this is merged, please edit the description on dotnet/docs#21202 to include the affected APIs from this PR.

@buyaa-n buyaa-n merged commit 57c3a85 into dotnet:main May 22, 2021
@buyaa-n
Copy link
Contributor

buyaa-n commented May 22, 2021

@buyaa-n Once this is merged, please edit the description on dotnet/docs#21202 to include the affected APIs from this PR.

@jeffhandley I do not really think that is necessary, first, this is not breaking change and not really changing the annotation, just adding a [NotNullWhen(true)] for Object.Equals(object obj) overload which applies almost all types that overwrites Object.Equals method which is way too many to add into that issue (most of the annotations added previously here)

EDIT: We might want to add just one generic statement that the attribute is added to the most Object.Equals(object obj) overrides, what you think?

@jeffhandley
Copy link
Member

I had noticed that the issue already mentioned #47598, so I thought we'd want to do something similar for other PRs. I don't think we need to call out each specific API. I wasn't sure if any of these could be considered breaking or not.

#47598 [Breaking]
[NotNullWhen(true)] added to hundreds of arguments

@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants