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

[API Proposal]: {ReadOnly}Span(ref T) constructor #67445

Closed
stephentoub opened this issue Apr 1, 2022 · 23 comments · Fixed by #71589
Closed

[API Proposal]: {ReadOnly}Span(ref T) constructor #67445

stephentoub opened this issue Apr 1, 2022 · 23 comments · Fixed by #71589
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Apr 1, 2022

Background and motivation

As part of the C# 11 work to enable ref fields, it will be safe to create a {ReadOnly}Span from a ref (with an implicit length of 1), because the compiler will track the lifetime of the ref and ensure the created span can't outlive it. Creating spans for a single item is quite common, and we should have public constructors for this situation.

This has been proposed by others offline... I agree with it and am just opening the issue for it so we can make forward progress on it.
cc: @jaredpar, @AaronRobinsonMSFT, @tannergooding

API Proposal

namespace System
{
    public ref struct Span<T>
    {
+        public Span(ref T reference);
    }

    public ref struct ReadOnlySpan<T>
    {
+        public ReadOnlySpan(in T reference);
    }
}

API Usage

int i = ...;
CallMethodThatTakesASpan(new ReadOnlySpan<T>(in i));

Alternative Designs

No response

Risks

No response

@stephentoub stephentoub added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Apr 1, 2022
@stephentoub stephentoub added this to the 7.0.0 milestone Apr 1, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Memory untriaged New issue has not been triaged by the area owner labels Apr 1, 2022
@ghost
Copy link

ghost commented Apr 1, 2022

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

Issue Details

Background and motivation

As part of the C# 11 work to enable ref fields, it will be safe to create a {ReadOnly}Span from a ref (with an implicit length of 1), because the compiler will track the lifetime of the ref and ensure the created span can't outlive it. Creating spans for a single item is quite common, and we should have public constructors for this situation.

This has been proposed by others offline... I agree with it and am just opening the issue for it so we can make forward progress on it.
cc: @jaredpar, @AaronRobinsonMSFT, @tannergooding

API Proposal

namespace System
{
    public ref struct Span<T>
    {
+        public Span(ref T reference);
    }

    public ref struct ReadOnlySpan<T>
    {
+        public ReadOnlySpan(ref T reference);
    }
}

API Usage

int i = ...;
CallMethodThatTakesASpan(new ReadOnlySpan<T>(ref i));

Alternative Designs

No response

Risks

No response

Author: stephentoub
Assignees: -
Labels:

area-System.Memory, untriaged, api-ready-for-review

Milestone: 7.0.0

@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Apr 1, 2022
@stephentoub
Copy link
Member Author

Examples for all the places in corelib where we create length-1 spans around a single reference are in #67447.

@jaredpar
Copy link
Member

jaredpar commented Apr 1, 2022

public ReadOnlySpan(ref T reference);

Believe this should be in T reference

@stephentoub
Copy link
Member Author

Fixed

@Sergio0694
Copy link
Contributor

Is this (the ReadOnlySpan<T> constructor) meant to be shipped with an analyzer to disallow rvalue-s as previous discussed for similar cases? Otherwise this would allow eg. new ReadOnlySpan<int>(5), or creating one from a by-value return of some API, with the span actually just pointing to a hidden copy of the value. Unless allowing this is by design, or just not a concern? 🤔

@stephentoub
Copy link
Member Author

stephentoub commented Apr 1, 2022

That's why I'd gone with ref (that, and more generally I really dislike not having to specify that the thing you're passing in is being passed by ref, e.g. I wish you had to write new Span<T>(in value) instead being allowed to write new Span<T>(value)), but I don't care enough to fight about it, and I realize using ref blocks certain valid usage (i.e. where you already have an in you want to wrap). If you pass in an rvalue, you get what you get I guess.

@jaredpar
Copy link
Member

jaredpar commented Apr 1, 2022

Otherwise this would allow eg. new ReadOnlySpan(5), or creating one from a by-value return of some API, with the span actually just pointing to a hidden copy of the value. Unless allowing this is by design, or just not a concern?

This is explicitly supported by the language hence it doesn't pose a safety concern.

and more generally I really dislike not having to specify that the thing you're passing in is being passed by ref, e.g. I wish you had to write new Span(in value) instead being allowed to write new Span(value)

That is something that we can consider adding to the language. One simple idea that came to my mind as I was reading this is allowing two syntaxes on parameters:

  • in: means what it means today
  • ref readonly: provides a warning at the call site when parameter is not an lvalue but otherwise acts like an in parameter

Could also tweak the second idea to be an error but then you're removing your ability to upgrade existing APIs.

@AaronRobinsonMSFT
Copy link
Member

ref readonly: provides a warning at the call site when parameter is not an lvalue but otherwise acts like an in parameter

+1

@tfenise
Copy link
Contributor

tfenise commented Apr 1, 2022

Why is public Span(in T reference); also in?

@jaredpar
Copy link
Member

jaredpar commented Apr 1, 2022

Why is public Span(in T reference); also in?

That should be ref otherwise it violates readonly rules

@stephentoub
Copy link
Member Author

Why is public Span(in T reference); also in?

Editing in haste.

@tannergooding
Copy link
Member

That is something that we can consider adding to the language. One simple idea that came to my mind as I was reading this is allowing two syntaxes on parameters:

Yes please. I would love this and it would solve one of the biggest issues I have with in (the other is changing a signature from ref to in is a source breaking change; if we allowed that for ref to ref readonly then its even better as we could update already shipped APIs).

@Sergio0694
Copy link
Contributor

I've linked this issue in the proposal as well, but just to notify folks in here too - we now have an official proposal on csharplang for ref readonly parameters, which would help in scenarios such as this (as well as enabling us to "fix" the signature of lots of APIs we've shipped already, like Unsafe.Read, Vector128.LoadUnsafe, Volatile.Read, etc.). See dotnet/csharplang#6010.

@terrajobst
Copy link
Member

terrajobst commented Apr 14, 2022

namespace System;

public partial ref struct Span<T>
{
    public Span(ref T reference);
}
public partial ref struct ReadOnlySpan<T>
{
    public ReadOnlySpan(in T reference);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 14, 2022
@stephentoub
Copy link
Member Author

(Just noting we should not expose these until the C# safety work is completed.)

@halter73
Copy link
Member

Are there really safety concerns with passing an rvalue to the ReadOnlySpan constructor or is just inefficient? Does it basically just box the value?

@tannergooding
Copy link
Member

tannergooding commented Apr 14, 2022

Its not, IMO, a safety concern, but rather one of intent. Basically there are two considerations around readonly byrefs.

In one hand you have cases where it being a readonly byref is really an "implementation detail", it largely only exists as a perf optimization. Most of the places we use it in the BCL (or that you see const T& in C/C++) are similar.

On the other hand, you have cases where it being a readonly byref is a very important detail, generally because the API is directly dealing with the concept of memory. Consider for example Unsafe.IsNullRef(ref T). While there is nothing "conceptually" wrong with passing an rvalue here and Unsafe.IsNullRef(5) will do "the right thing". However, it's likely an error/mistake on the end of the consumer and is effectively a "pointless" API call to do because the answer is constant.

In the same way, Span and ReadOnlySpan are intentionally capturing and tracking the passed in byref. There is nothing conceptually wrong with new ReadOnlySpan(5), but it represents a potential error/problem case due to the API dealing explicitly with the concept of memory. The cases where a user might want an rvalue are cases like params ReadOnlySpan<T> and in that case other syntax should handle the creation of said span.

@stephentoub
Copy link
Member Author

stephentoub commented Apr 14, 2022

The safety work I was referring to above wasn't about rvalues; it was about the C# compiler tracking ref lifetime. If with the C# compiler today we were to expose these constructors, you could do:

public Span<int> DoNotDoThis()
{
    int i = 0;
    return new Span<int>(ref i);
}

the compiler would happily compile that, and it would be really bad.

@terrajobst
Copy link
Member

the compiler would happily compile that, and it would be really bad.

I don't always peek into the stack, but when I do, I make sure it's into a stack frame that was unwound

@deeprobin
Copy link
Contributor

Does this issue include changes in the JIT or VM or just exposing the private/internal constructors + appropriate tests?

@tannergooding
Copy link
Member

This can't be implemented until the relevant language changes are in.

@jaredpar
Copy link
Member

jaredpar commented May 6, 2022

@deeprobin this issue just covers the API changes around Span / ReadOnlySpan. There are many other changes that need to happen to support ref fields. The overarching issue tracking the work is #63768

@deeprobin
Copy link
Contributor

Thank you 👍

@stephentoub stephentoub self-assigned this May 17, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 3, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants