Skip to content

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Oct 23, 2025

Note: this is still very in progress. Will aim to have it ready for review before 10/29.

@333fred 333fred marked this pull request as ready for review October 28, 2025 22:21
@333fred 333fred requested a review from a team as a code owner October 28, 2025 22:21
@CyrusNajmabadi
Copy link
Member

Note: I see my questions came up later.

Given the extensive rewrite of both the `unsafe` code section and other parts C# specification inherent in this change, it would be unwieldy and likely not useful to provide a line-by-line diff
of the existing rules of the specification. Instead, we will provide an overview of the change to make in a given section, as well as specific new rules for what is allowed in `unsafe` contexts.

#### Expression memory safety state
Copy link
Member

Choose a reason for hiding this comment

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

I didn't follow why this concept is necessary. It feels like this is a different situation than, say, flow analysis, or ref safety analysis. We just need to enforce that if something unsafe is being used, it is occurring in an unsafe context.

Can't we just say that it is an error if:

  • one of the listed expression forms (*p, p->M, and so on) is used outside of an unsafe context, or,
  • when an unsafe member is used outside of an unsafe context.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to reconstruct the mind-palace I was in when I came up with this. I think it was because I wanted to avoid defining rules for every single expression in the language. But I don't think this does it, and likely most expressions and statements will need explicit updates anyway.

// Elsewhere in safe code:
void M(SafeWrapper w)
{
w._p = stackalloc byte[10];
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I see the benefit of marking the field itself unsafe. If it's dereferenced, that would be unsafe anyway. If it's manipulated via other APIs of SafeWrapper, they should be marked as unsafe, otherwise they declare they can handle anything in the field safely.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, the field exposed through the program. So anything that manipulates it needs to make sure that the pointer obeys the expectations of SafeWrapper.

is, is covered in [unsafe contexts](#unsafe-contexts).

In order for this information to flow through the system, we therefore need to have a way to mark methods themselves as `unsafe`. Today, `unsafe` as a method modifier has no external impact,
it only allows pointers to be used in the signature and body of the member. Going forward, `unsafe` as a modifier will actually publicly change the meaning of the member; it will indicate that
Copy link
Member

Choose a reason for hiding this comment

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

I assume there is going to be some attribute synthesized on the new unsafe methods? (So the compiler can round-trip the fact that it's the "new unsafe" semantics through metadata.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I assume we will need this. I haven't delved too much into the actual mechanism by which things will be marked unsafe yet, I plan to do that for next week's meeting.

@333fred 333fred merged commit 8a6bf92 into dotnet:main Nov 6, 2025
1 check passed
@333fred 333fred deleted the unsafe branch November 6, 2025 23:28
@333fred 333fred restored the unsafe branch November 6, 2025 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants