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

Suppress warnings for taking address of TypedReference #80484

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jan 11, 2023

Relates to compiler change: dotnet/roslyn#66328
Tagging @AaronRobinsonMSFT to confirm those suppressions are okay.

@jcouv jcouv self-assigned this Jan 11, 2023
@dotnet-issue-labeler
Copy link

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.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

At some point soon we may want to just suppress 8500 for the whole repo, but for now doing it locally is good.

@jcouv jcouv marked this pull request as ready for review January 11, 2023 17:56
@AaronRobinsonMSFT
Copy link
Member

At some point soon we may want to just suppress 8500 for the whole repo, but for now doing it locally is good.

Agreed. Do we think this is one of those warnings we want to permit when performed in an unsafe context? I can't recall all those details but we were discussing the idea of if you are in an unsafe context Roslyn is more permissive? Or was that around converting an error to warning - I don't recall all the details.

@stephentoub
Copy link
Member

Or was that around converting an error to warning

Right. Previously this was an unsuppressable error. Now in an unsafe context it becomes a suppressable warning CS8500.

@ghost
Copy link

ghost commented Jan 11, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

Relates to compiler change: dotnet/roslyn#66328
Tagging @AaronRobinsonMSFT to confirm those suppressions are okay.

Author: jcouv
Assignees: jcouv
Labels:

area-System.Runtime.CompilerServices

Milestone: -

@danmoseley
Copy link
Member

danmoseley commented Jan 11, 2023

S.R.CS seems a resaonable label even though it's not?

@jcouv
Copy link
Member Author

jcouv commented Jan 12, 2023

@stephentoub My understanding is that this is generally unsafe, but there are a few situations where it is okay. When it comes to compiler warnings, this is one that is stern :-P
In that light, having a local suppression seems beneficial to call reviewers' attention to something subtle. So I'm not sure that a repo-wide suppression is a good idea.

@stephentoub
Copy link
Member

My understanding is that this is generally unsafe, but there are a few situations where it is okay.

What are some examples where the warning legitimately protects someone from a bad mistake? (That's a real question; I'm trying to understand how the problems this averts are significantly worse than many other things I can do with pointers in an unsafe context with zero warning.)

@jcouv jcouv marked this pull request as draft January 18, 2023 08:35
@jcouv jcouv marked this pull request as ready for review January 19, 2023 07:11
@jcouv
Copy link
Member Author

jcouv commented Jan 19, 2023

how the problems this averts are significantly worse than many other things I can do with pointers in an unsafe context with zero warning

I see your point. This is not worse than other things you can do with pointers in unsafe context. A broader suppression would make sense.

@jcouv jcouv merged commit 8598658 into dotnet:main Jan 19, 2023
@jcouv jcouv deleted the typed-reference branch January 19, 2023 20:07
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
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.

4 participants