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

Updated draft for the safety of ref-like types such as Span<T> #472

Merged
merged 2 commits into from
Jun 8, 2017

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Apr 21, 2017

No description provided.

- it is ok to return ref-like values from methods/lambdas, including by reference.
- it is ok to pass ref-like values to methods/lambdas, including by reference.
- it is ok for a ref-like type be a field of another struct as long as the struct itself is ref-like.
- it is ok for an intermediate span values to be used in lambdas/async methods.
Copy link
Member

Choose a reason for hiding this comment

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

This needs explaining - you've not explained "intermediate span values" previously.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basicaly M1(M2()) is ok in an async method, even if M2() returns a span. Will add an example.

In particular: _Slice of a stack-referring value is a stack-referring value._

- "No Mixing with refs to ordinary" rule:
Stack-referring spans can be passed as an argument as long as no other argument is an ordinary span variable (not a stack-referring local), and:
Copy link
Member

Choose a reason for hiding this comment

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

What's the rationale for this? It's not clear to me (not being involved in this, why this rule exists)

Copy link
Member Author

@VSadov VSadov Apr 21, 2017

Choose a reason for hiding this comment

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

If we classified one variable as safe to return and another variable as stack-referring, we will need to prevent assignments from second to the first, since that would invalidate the classification (and we do not want to do global data flow analysis - it is hardly tractable).

The goal here is to prevent such assignments while not causing too much inconvenience. The call boundary is an issue though.

If I make a call like M1(p1: ref safeToReturn), I am risking that M1 will do p1 = stackalloc int[10];

In a more complex case I am making a call like M2(p1: ref safeToReturn, p2: unsafeToReturn) and M2 does p1 = p2;.

We need to make both scenarios illegal.
The original proposal simply disallowed passing spans by reference. It was found too restricting so we need something else.

In this proposal assigning stack-referring values to parameters (or to anything other than stack-referring locals) is disallowed. That handles the p1 = stackalloc int[10];case.

The other case is harder. Inside M2 we cannot tell whether assigning the second parameter to the first is unsafe, so we disallow at the call side passing an ordinary span variable by reference and any kind of stack-referring spans in the same call, so that M2 could assign one parameter to another without worrying.

Thanks for commenting. I will add these details to the document.

@jkotas
Copy link
Member

jkotas commented Apr 27, 2017

We are going to need attribute to mark ref-like types - to avoid spooky action at a distance when Span is embedded in other structs.

@VSadov
Copy link
Member Author

VSadov commented Apr 27, 2017

@jkotas - yes, we will use [Obsolete] with a well-known message in metadata. That will also allow us to error on compilers that are not aware of span requirements.

And in the source we will use ref struct S1

We had some follow up discussions on that. I will add that to the document.

@jkotas
Copy link
Member

jkotas commented Apr 27, 2017

[Obsolete] with a well-known message may be fine to poison the type for old compilers; but I do not think that it should be the ultimate way to identify these types. Note that these types need to be identified accross the board - in the runtime, tools, etc.

@VSadov
Copy link
Member Author

VSadov commented Apr 27, 2017

It is possible to add another attribute, but it would always have to be combined with the Obsolete.
It is possible, but seems a bit redundant.

@VSadov
Copy link
Member Author

VSadov commented Apr 27, 2017

Note that the Obsolete message must be precise or we cannot distinguish it from an ordinary obsolete attribute.

It does feel that having two attributes having distinct meanings is more direct way of doing this, but there is also the part that they will have to be always emitted in pairs makes it seem a bit redundant.

We should think about this.
There is nothing inherently wrong with having "IsRefStructAttribute"


```

Designating a struct as ref-like will allow the struct to have ref-like instance fields and will also make all the requirements of ref-like types applicable to the struct.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it somehow be possible to create a ref-like struct that contains ref fields?


```cs
//This usage of stackalloc does not require unsafe
Span<int> sp = stackalloc int[100];
Copy link
Member

@mgravell mgravell May 4, 2017

Choose a reason for hiding this comment

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

Firstly: I love this very much; this would remove the unsafe from much of my stack span code.

Probably simply "no" (and that's fine), but: is there any utility to exposing something akin to array initializer syntax here?

Span<int> sp = stackalloc { 1, 42, 13 };

(Note preserving the stackalloc keyword to retain the intent in the code)

Copy link

Choose a reason for hiding this comment

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

This is interesting. It doesn't require unsafe but the resulting code is unverifiable. It would seem that we're moving towards a system where the managed compiler(s) are responsible for ensuring type safety and not the runtime.

@mikedn
Copy link

mikedn commented May 5, 2017

Can perhaps modreq be used instead of this Obsolete based hack? It seems that the main problem with an old compiler using these types is the possibility of exposing references to stack locations that have been "freed" and this can only happen via method calls.

Other improper uses of ref-like types will be blocked by the runtime. Sure, it would be it would be nice to be blocked at compile time but using Obsolete for that is quite ugly.

@davkean
Copy link
Member

davkean commented May 5, 2017

@mikedn It's an interesting idea, and definitely falls in the spirit of what they were designed for (think of some the C++ modifiers; IsUdtReturn, IsByValue, etc).

@omariom
Copy link

omariom commented Jun 3, 2017

@VSadov

Will such scenario be allowed?

private void M()
{
    var myStruct = new MyStruct(stackalloc int[10]);
    Foo(myStruct);
    Bar(myStruct.ReadOnlyView);
}

private void Foo(MyStruct myStruct)
{
}

private void Bar(ReadOnlySpan<int> roSpan)
{
}

reflike struct MyStruct
{
    private Span<int> span;

    MyStruct(Span<int> span) => this.span = span;
    
    public ReadOnlySpan<int> ReadOnlyView => span.AsReadOnly(); 
}

@VSadov
Copy link
Member Author

VSadov commented Jun 4, 2017

@omariom - not exactly like that, stackalloc can be used only when initializing a local, so you would need one more span-typed variable, but yes, conceptually the above example will work. At least according to the rules in the proposal.

@mgravell
Copy link
Member

mgravell commented Jun 4, 2017 via email

@omariom
Copy link

omariom commented Jun 4, 2017

@VSadov

stackalloc can be used only when initializing a local, so you would need one more span-typed variable

It should be relaxed for safe (Spans) case.

@gafter gafter merged commit 6ec04f5 into dotnet:master Jun 8, 2017
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.

8 participants