-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Champion "Readonly ref" (C# 7.2) #38
Comments
I'm not quite clear why this has to be a separate feature from readonly locals and paramerters? |
@alrz they are very different features.
These features do have some overlap with |
So potentially, we could have |
Possibly but that's a bit of a redundant notation. All |
I'm under the impression that you are distinguishing something like |
We might support reassignment of ref variables in the future. In order for that to be possible, we will need to distinguish between |
Should just use for |
Can interface ISetter<in T>
{
void SetValue(in T value);
} |
No. The CLI only truly recognizes |
And here I thought that C++-style of consts will never be considered for C#... Are ~~~const~~~ readonly functions also something being considered? |
Could this feature enable |
ITNOA I think #188 that describe about |
What does this mean? |
I happened to find this champion as I was searching the repo for proposals of const/fixed refs. In proposals/readonly-ref.md it is mentioned this example of XNA. Please due note that the proposed solution will not match perf of, in the example used, the ADD operation of XNA, since it's returning a copy of the resulting/new vector. |
In terms of C# 7, it should be something like, instead: static void Add (in Vector3 v1, in Vector3 v2, out var result)
{
// OK
result = new Vector3(v1.X +v2.X, v1.Y + v2.Y, v1.Z + v2.Z);
} |
Would there be a way to elide the |
What about |
That's so tempting. Within the realm of C# it would be fine. We would never actually write to the location hence it should be fine to avoid the type check. But it's still possible for us to call out to other DLLS that ignore Hmm, then again, maybe the mod req could be enough to save us here. At that point the code is willfully ignoring the IL. |
@JosephTremoulet @jaredpar - yes, in the world of C# it would be safe to emit However if the calee misbehaves, we can end up not just with something that is readonly unexpectedly mutated (we are kind of ok with that), but with a type-system hole. That would seem a bigger problem. Here is the scenario: string[] sArr = ... get an array of strings
object[] oArr = sArr; // allowed!!
// bravely suppress type check here via .readonly
Evil(oArr[2]);
void Evil([IsReadOnly] ref object arg)
{
// muhahaha
arg = new Exception();
} |
For interop scenarios, Is the compiler going to emit a warning for That is, when the user attempts to explicitly tell the marshaller that I did log an issue (https://github.com/dotnet/coreclr/issues/11830) requesting that the marshaller be updated to recognize |
No. The compiler doesn't impart any semantic meaning to |
@jaredpar Can the compiler emit [In] by default with ref readonly? This way runtime don't have to make any special assumptions about in semantics. This also aligns with out keyword being [Out] by default. |
I would expect we also have I would also expect that the compiler output @VSadov, I believe you indicated the attributes are compiler features, rather than language features. Is this something we can ensure the compiler emits? |
Also @jaredpar ^ |
There was also #497 and I was mentioned in #497 (comment) |
How does the ability to represent an undefined value with a ref become the ability to state an out variable is a readonly reference? There is some overlap in this particular use case, but there could be other motivating examples, for example in some interop (this is not a real api, but is inspired by one available in shell32.dll):
|
Hey @Thaina I see a similar syntax mentioned in "#497 (comment)" but there it is shortly discussed and then the conversation seems to switch focus to that null problem which we don't have, because we can always make the ref point to something "valid". But you are right, I should have used "out ref readonly" rather than "out readonly" in my example, since I want to initialize the reference and not the value it is pointing to as out parameter: One thing said there seems wrong:
Is not exactly true, because with out the caller provides a reference to a piece of memory on the caller side, where then a value from the callee (Collection) is copied into. |
@Voidzer0 In this particular proposal, a Since this proposal doesn't deal with output parameters, I think you should enter a new issue for |
This is basically a reference to a reference, that's not something that the runtime supports. Besides, it's not clear how that would address your problem - |
@bbarry
We assign a reference to a default initialized value or array element. |
You can use fenced code blocks: https://help.github.com/articles/creating-and-highlighting-code-blocks/ The suffix for C# formatting is |
Not quite. C++ does not allow references to references, pointers to references, null references. It does allow references to pointers which is kind of equivalent to something like
This would work but it has the disadvantage that your class is larger due to that extra It also seems rather risky - it's a form of "null" that's hard to detect. If someone forgets to check the return of You really should open a separate issue if you want to follow up, it's a very different situation than |
While I get that, I wonder if this can not be treated like a normal reference assignment, since an out parameter must be assigned to a valid value anyway.
I wouldn't do a lookup/dictionary if there wouldn't be many structs, so the one struct extra memory is neglectable.
I would never do that, if I wouldn't hand out a readonly ref of an immutable struct. Everything else would be borderline insane I guess xD
You are probably right, but given that there is a (less nice looking) work around (as @bbarry pointed out) and that reference semantic is only valid for declaring the out parameter in the call where it is assigned, |
That's how it looks in C# but in IL this is still a reference (aka byref aka managed pointer) to a reference. I suppose it may be possible to cheat to an extent - byrefs can only live on the stack so a byref to a byref can actually be passed as an unmanaged pointer. But that's probably a can of worms as you end up with a method signature that's basically lying.
For completeness it may be worth detailing how C++ containers handle this:
I'm not aware of a case where standard C++ classes return references to some sort of hidden, null/default-like objects. |
I also support reference of reference. It also solve the return of multiple |
I use the new tools now on a daily basis and I got now stuck on a compiler behaviour I do not understand. class CompileError
{
bool _someInternalValidCheck = false;
int _resultIfValid = 7;
int _resultIfInvalid = 0;
ref readonly int TryGetResult(out bool valid)
{
if (_someInternalValidCheck)
{
valid = true;
return ref _resultIfValid;
}
else
{
valid = false;
return ref _resultIfInvalid;
}
}
int _error = -1;
public ref readonly int Test()
{
ref readonly int result = ref TryGetResult(out bool valid);
if (valid)
return ref result;
return ref _error;
}
} It says "result" was initialized in a way it can not be returned by reference, but it is a reference already, so why not? Just another thing: (ref AnyType name1, ref AnotherType name2, YetAnotherType name3) = FunctionReturningTwoRefsAndAnotherValue();
// That would make things simpler, there is nothing unsafe if the result tuple is not stored anywhere, but I know that with current ValueTuple implementation it cannot be supported. |
@Voidzer0 Your issue is not really about
In your case, the |
Oh this is interesting, I was totally aware that locals are not safe to return, but this is quite around the corner.
So I still don't buy that the compiler has to be this conservative (defensive) in such cases. There must be a way to relax at least some of these limitations to support more valid code. My actual problem looks like this: public ref readonly T GetCorrect(int id) // where T : struct
{
// Compiling fine, but wasteful:
return ref _hastable1.ContainsKey(id) ? ref _hastable1.GetRef(id) : ref _hastable2.GetRef(id);
// ^ lookup ^ lookup ^ lookup
// vs
// Doesn't compile, but correct and more efficient:
ref readonly var current = ref _hastable1.TryGetRef(id, out bool exists);
// ^ lookup
return ref exists ? ref current : ref _hastable2.GetRef(id);
// ^ NO LOOKUP ^ lookup
// => If key exists in _hastable1, only one lookup is needed, but two are required due to compiler limitations
} I really hoped I could safe this additional lookup in C#, but it seems like I can't. |
Maybe, but I think it's too late for that. You can return an
As I understand it, it doesn't have to. It was a design choice made at that point in time and it's one that could be relaxed now. If this is something you want to see, I think you should open a new issue on this repo specifically about that. And it would probably help if you also had a set of rules for what exactly should be allowed. (Consider e.g. that a if a method takes a
I think you can, assuming you control the hashtable type. Though it would require more complex design: |
From the perspective of correctness
Not sure how to convince you of this. But I can assure you that this is how we view |
It was possible to pass ref int M1(out int arg)
{
arg = 42;
return ref ReturnsItsArg(ref arg); // <- which part of this can be made illegal and not break things?
} |
Thanks a lot for these answers guys. Don't get me wrong, I am utterly happy to have these features in C# and I am very positive about the direction that has been taken with C# 7+ and what is coming in version 8. @jaredpar @VSadov
I was just thinking from C# perspective, which could have considered "out" as a higher level construct which is bound to some rules, rules which would prevent it from being referenced in any other way than as "out", which in turn would allow the compiler to make my example code and generally more code a valid and safe option. However I still see potential, despite this limitation: Solution 1)- If a reference in question to be not safe to return has a different type than (and is not a nested type of) the ref parameter passed to a Callee, then the reference returned is still safe to return (unless other reasons are found).
Even if someone could have bad luck with types matching, in my case I would be sorted by this improvment. Solution 2)Furthermore especially now that with C# 7 out variables can be declared right in the place they are initialized (Thank you so much for this devs!), even my original and more elegant suggestion: Solution 3)Well there is yet another idea which seems valid to me and would maybe be the most flexible of all Now refs can neither be returned in tuples nor passed by out parameters. Any method returning a ref is thus limited to one ref only and given the limitations for out parameters with respect to return refs, it is basically limited to only return the ref and nothing else and I think this is a rough limitation in quite some cases which would be worth to relax if anyhow possible. No matter which of the three ideas would be considered, they all should be safe (correct me if I'm wrong) and would fix my current issue. Which brings me to
I didn't think about this possibility and that would work for me.
If you still think this is the case after reading the other answers and if you think it is likely that things could change in this way, I would be clearly up for that. Cheers everyone for patiently reading and answering all the questions. |
A very long time ago, in C++98, I wrote a template class LazyPtr<T> (a.k.a a smart copy-on-write pointer), that acted somewhat as a shared pointer, but would automatically clone (T* T::clone() had to exist) the pointed dynamically allocated data of type T if a non-const access was done, whereas a const access wouldn't duplicate data. The clone only happens when the counter was strictly greater than 1. The duplication effectively split the read-shared data and thus decremented the original counter, while setting the duplicated data counter to one. For example, if three instances of the pointer existed pointed to a single shared data: LazyPtr (count = 3) -> write through my instance of the smart pointer => leaves other smart pointers like LazyPtr (count = 2) and my cloned-then-written copy held by LazyPtr (count = 1) L3 -> write => L2 and L1 (that was written to) This is very useful where you've got lots of business data in a cache, but you need times to times to tweak one data (let's say bump a rate curve if you're doing finance) while leaving the original data in place. This usually leads to the dilemma, should I clone or not? How do I manage my data to keep the original safe while not consuming to much process memory. Shared pointer doesn't really help, etc. Here the LazyPtr solves this elegantly in a single move. But, Here comes C# that doesn't provide this-constness at method level (or So for now, C# 7.2 supports A summary of my test (I didn't check that actual code, but it does the same).
|
C# makes a defensive copy when you invoke |
@HaloFour, hello, Interesting though, it means But But I suspect already that an Hmm, maybe, just maybe... |
For now it looks like ref readonly return act not only like implicit copy on write, but also like implicit copy on access in case of access not field. Suggest to add compiler warning or even better compiler switch to generate error instead of creating implicit copy, so programmer needs to create explicit copy to write to. Another suggestion is to add readonly methods that will have this parameter passed as in parameter so suth methods (and properties) will be save to call on readonly ref returned structs. |
@Jes28 you can do this by writing an analyzer. |
I dont think that writing analyzer myself to work around language or compiler bugs is good way to go. I am not alone and just realized that C# 8 already have all this :) |
It's not a compiler bug afaict. It's working as spec'ed. If you want to place additional restrictions on the language, the right way to do that is per analyzers. |
The text was updated successfully, but these errors were encountered: