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

Expanding ref support #7555

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Expanding ref support #7555

merged 4 commits into from
Oct 30, 2023

Conversation

jaredpar
Copy link
Member

This proposal expands the ref support in the compiler to include:

  1. ref struct as ref fields
  2. ref scoped parameters and fields
  3. Fully sunsetting restricted types

At the moment I'm not proposing that LDM move forward with the design. I'm still deciding if the complexity / cost is worth the gain it provides. Yet recent discussions crystalized in my mind how this would all work hence I wanted to get it written down and out for review.

The intent here is to socialize these ideas, get feedback on where this would or would not help and then make a decision on whether to push this through LDM / future .NET release.

This proposal expands the `ref` support in the compiler to include:

1. `ref struct` as `ref` fields
2. `ref scoped` parameters and fields
3. Fully sunsetting restricted types

At the moment I'm not proposing that LDM move forward with the design.
I'm still deciding if the complexity / cost is worth the gain it
provides. Yet recent discussions crystalized in my mind how this would
all work hence I wanted to get it written down and out for review.

The intent here is to socialize these ideas, get feedback on where this
would or would not help and then make a decision on whether to push this
through LDM / future .NET release.
@jaredpar jaredpar requested a review from a team as a code owner September 26, 2023 17:26
@jaredpar
Copy link
Member Author

@agocke
Copy link
Member

agocke commented Sep 26, 2023

Confirmed that I see this, but currently a bit underwater and it may take me some time to sit down and digest it

@jaredpar
Copy link
Member Author

@agocke

Confirmed that I see this, but currently a bit underwater and it may take me some time to sit down and digest it

No rush on this. Was just paged in from other discussions so I wanted to get it written out.

proposals/expand-ref.md Outdated Show resolved Hide resolved
proposals/expand-ref.md Outdated Show resolved Hide resolved
proposals/expand-ref.md Outdated Show resolved Hide resolved
proposals/expand-ref.md Outdated Show resolved Hide resolved

Once `ref` fields are available and extended to support `ref struct` these types can be fully rationalized within those rules. As such the compiler will no longer have the notion of restricted types when using a language version that supports `ref` fields of `ref struct`.

To support this our `ref` safety rules will be updated as follows:
Copy link
Member

Choose a reason for hiding this comment

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

Can we assume updates to the following don't indicate anything other than clarity on their semantics rather than changing anything about their "officially supported" state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. This is just about getting clarity on the semantics. Particularly I know the runtime team has wanted to leverage TypedReference more and this would provide clarity on what can and cannot be done safely.

@RikkiGibson RikkiGibson self-assigned this Sep 26, 2023
proposals/csharp-11.0/low-level-struct-improvements.md Outdated Show resolved Hide resolved
proposals/csharp-11.0/low-level-struct-improvements.md Outdated Show resolved Hide resolved
proposals/expand-ref.md Outdated Show resolved Hide resolved
proposals/expand-ref.md Outdated Show resolved Hide resolved
proposals/expand-ref.md Outdated Show resolved Hide resolved
proposals/csharp-11.0/low-level-struct-improvements.md Outdated Show resolved Hide resolved
- The `ref` of a variable has a lifetime of the
- For a ref local, parameter, field or return of type `ref<$a> T` the lifetime is `$a`
- For a `ref` field defined as `ref<$l0> T<$l1, $l2, ... $ln>`:
- All lifetimes `$l1` through `$ln` must be invariant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true even for a ref readonly field?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible that readonly could allow us to relax the restriction a bit here. At the same time it's something we'd have to really map out to make sure we weren't missing anything.

## Detailed Design
The rules for `ref struct` safety are defined in the following documents:

- [ref safety proposal](https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.2/span-safety.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's hard to get use out of this document now because you have to have the other document at hand to cross-reference everything that has changed in later versions of C#.

Copy link
Member Author

Choose a reason for hiding this comment

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

At some point I do need to put together a unified doc that we can reference.


This is accomplished by giving every `ref scoped` parameter a new escape scope named _current parameter N_ where _N_ is the numeric order of the parameter. For example the first parameter has a _safe-to-escape_ of _current parameter 1_. An escape scope of _current parameter N_ can be converted to _current method_ but has no other defined relationship. That serves to restrict their usage to the current method.

It's important to note each parameter has a different _current parameter N_ scope. That means they cannot be assigned to each other. This is necessary to prevent `ref scoped` parameters from returning each others data.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a critical concept. The implementation currently, by representing lifetimes as uint, requires every lifetime to be comparable to every other lifetime. It's equivalent to it being impossible to declare void M<T, U>() {} without also saying where T : U or where U : T.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to do something similar for value-scoped parameters, once we permit RTRS fields? Since it feels like it will be possible to declare a similar signature, just using void Swap(RefSpanWrapper<int> p1, RefSpanWrapper<int> p2).

Copy link
Member Author

@jaredpar jaredpar Sep 29, 2023

Choose a reason for hiding this comment

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

I agree our current implementation does not lend itself to this type of validation. This was one of the motivations I had in wanting us to refactor the implementation significantly in C# 12.0 to better match the existing spec. That would've made the transition to this much easier. As is though we effectively need to do that work before we can do this feature.

}
```

This is accomplished by giving every `ref scoped` field a new two new escape scopes named _current field N_ and _current ref field N_ where _N_ is the numeric order of the field. For example the first field has a _safe-to-escape_ of _current field 1_ and a _ref-safe-to-escape_ of _current ref field N_. Both escape scopes can be converted to _current method_, and _current field N_ can be converted to _current ref field N_, but no other defined relationships exist. That serves to restrict their usage to the current method where the containing value is used. This escape scope applies to both
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that all lifetimes $current_field_N and $current_ref_field_N must also be convertible to lifetime $this. In the absence of this constraint, it would be possible for the this variable to hold references to fields with narrower lifetimes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, depending on how you look at it, it simply wouldn't be possible to declare any 'ref scoped' fields in ref structs, because the field's lifetime would not be convertible to the lifetime of the containing 'this' variable.

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 disagree. Logically yes I agree that the lifetimes must be convertible to $this. The moment you allow that though in all contexts, suddenly it's not scoped anymore, it's returnable

struct S<$this, $refField1, $field1>
  where $refField : $this
  where $field1: $this, $refField1 {
  ref<$refField1> Span<$field1, byte> span;
}

Span<$cm> M<$cm, $l1, $l2>(S<$cm, $l1, $l2> s) => 
  // works because $l2 is convertible to $cm
  s.span;
}

The way this works is that at construction time the rules ensure that the lifetimes of both the ref and the value are convertible to $this. That just falls out from our existing method args rules. Given that the fields lifetimes are invariant there is no need to keep that relationship alive for correctness after that. The only thing we have to worry about is how they are interpreted in the context of a method.

Note: if these were ref unscoped fields then you are 100% correct we'd put that relationship in place at the decl of the type.

Co-authored-by: Jan Jones <[email protected]>
Co-authored-by: Aaron Robinson <[email protected]>
proposals/expand-ref.md Outdated Show resolved Hide resolved
proposals/expand-ref.md Outdated Show resolved Hide resolved
@jaredpar jaredpar merged commit 98d06ae into dotnet:main Oct 30, 2023
1 check passed
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.

6 participants