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

Clarify Span<T> rules #1593

Merged
merged 2 commits into from
Jun 4, 2018
Merged

Clarify Span<T> rules #1593

merged 2 commits into from
Jun 4, 2018

Conversation

jaredpar
Copy link
Member

@jaredpar jaredpar commented Jun 1, 2018

While discussing Span<T> safety rules with the F# team I realized we
hadn't fully documented our dependency on the shape of the Span<T>
API. Added that plus some of the future considerations we discussed for
Span<T>.

dotnet/fsharp#4888

While discussing `Span<T>` safety rules with the F# team I realized we
hadn't fully documented our dependency on the shape of the `Span<T>`
API. Added that plus some of the future considerations we discussed for
`Span<T>`.

dotnet/fsharp#4888
@jaredpar
Copy link
Member Author

jaredpar commented Jun 1, 2018

CC @gafter, @dsyme, @TIHan, @VSadov

@jaredpar
Copy link
Member Author

jaredpar commented Jun 1, 2018

CC @OmarTawfik

This feature gets more compelling if we lift the restrictions on (fixed sized buffers)[https://github.com/dotnet/csharplang/blob/master/proposals/fixed-sized-buffers.md] as it would
allow for `Span<T>` instances of even greater length.

If there is ever a need to go down this path then the language could accommodate this by ensuring such `Span<T>` instances
Copy link
Member

@gafter gafter Jun 2, 2018

Choose a reason for hiding this comment

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

Not sure this would work. By the time we would want to make that change, there might already be a user-declared type

internal ref struct MySpan<T>
{
    public MySpan(ref T data) ...;
}

This API is perfectly safe today because it doesn't (has no safe way to) capture the data.

MySpan<int> M(int x)
{
    return new MySpan<int>(ref x);
}

Although this is safe today, but would become illegal under the rule changes we would need to permit a Span that can capture a reference. And so would be a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

That constructor though couldn't be implemented in a way that captured ref T data. If it attempted to do so via Span<T> then it would be an error as the value wouldn't be escapable beyond the constructor body and hence couldn't be assigned to a field.

Copy link
Member

@VSadov VSadov Jun 2, 2018

Choose a reason for hiding this comment

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

Yes, that would work. Because:

  • we do not have such constructors on Span/ReadOnlySpan right now.
  • with suggested restrictions the resulting instances will never escape the scope that contains the constructor invocation, so they will not introduce any new escaping scenarios for the outer scopes.

Copy link
Member

Choose a reason for hiding this comment

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

I would also add that in a case when the ref argument is "definitely referring to a heap object" - like a ref to an array element, for example, the resulting instance could be allowed to escape. It feels like it might be another common case.

# Future Considerations

## Length one Span<T> over ref values
Though legal today there are cases where creating a length one `Span<T>` instance over a value would be beneficial:
Copy link
Member

Choose a reason for hiding this comment

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

typo? not legal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops :(

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@paulomorgado paulomorgado left a comment

Choose a reason for hiding this comment

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

@jaredpar
Copy link
Member Author

jaredpar commented Jun 4, 2018

Updated based on feedback.

@jaredpar jaredpar merged commit c05b6b3 into dotnet:master Jun 4, 2018
Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

5 participants