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

Reference types can be passed in #23327

Closed
benaadams opened this issue Nov 21, 2017 · 11 comments
Closed

Reference types can be passed in #23327

benaadams opened this issue Nov 21, 2017 · 11 comments

Comments

@benaadams
Copy link
Member

benaadams commented Nov 21, 2017

Version Used:

7.2

Steps to Reproduce:

public class MyClass
{
    public int Value { set; get; }
}

public void Thing(in MyClass c)
{
    c.Value = 5;
}

Expected Behavior:

  1. Either doesn't compile (is a de-optimization as its a double dereference; for no benefit)

  2. Or prevents .Value being assigned (as per struct)

If 2. then it should stay as reference passed by-value but just be a compiler check; rather than reference passed by-reference.

Actual Behavior:

Compiles fine; passes by reference and allows the assignment.

@mikedn
Copy link

mikedn commented Nov 21, 2017

Either doesn't compile (is a de-optimization as its a double dereference; for no benefit)

In theory it's not without benefit. Perhaps Thing can actually take advantage of the fact that the object reference stored in c can somehow change and it can read the updated reference. Though in practice such a use case it's likely to be extremely rare, not to mention rather weird...

Also, this applies to primitive types as well. There's little reason to pass an int using in.

Or prevents .Value being assigned (as per struct)

That would make the whole thing rather useless. You won't be able to invoke any method on c because you have no way of knowing if it mutates the object or not. And unlike in the case of struct you can't make a defensive copy of the object.

@benaadams
Copy link
Member Author

In theory it's not without benefit. Perhaps Thing can actually take advantage of the fact that the object reference stored in c can somehow change and it can read the updated reference.

So you can do this?

public class C 
{
    static MyClass v = new MyClass();
    
    public class MyClass
    {
        public int Value { set; get; }
    }

    static void Thing(in MyClass c)
    {
        Console.WriteLine(c.Value);
        c.Value = 5;
        Console.WriteLine(c.Value);
        Thing();
        Console.WriteLine(c.Value);
    }
    
    static void Thing()
    {
        v = new MyClass();
    }
    
    static void Main()
    {
        Thing(in v);
    }
}

And output

0
5
0

That's terrifyingly leaky...

@benaadams
Copy link
Member Author

I suppose it is doing what it says...

Maybe it just needs a "wtf are you doing" warning squiggle

@mikedn
Copy link

mikedn commented Nov 21, 2017

That's terrifyingly leaky...

It certainly is and I can't think of a real world example that would justify using in this way. I'm just not sure the lack of good use cases warrants disallowing use of in with reference types. ref is a rather exotic feature of the language. ref readonly even more so. So let these features be usable in exotic ways.

Anyway, the more I look at this the more convinced I am that the issue is the keyword choice - in.

@benaadams
Copy link
Member Author

I can only see it being useful in a multi-threaded scenario; except it doesn't have the guarantees needed for that...

@jnm2
Copy link
Contributor

jnm2 commented Nov 21, 2017

Sure it's not useful but it's there for completeness with out. Is it worth breaking the symmetry by adding a special case to warn for it?

@mikedn
Copy link

mikedn commented Nov 21, 2017

Is it worth breaking the symmetry by adding a special case to warn for it?

Judging by the confusion it seems to cause I'd say that it's worth it.

@jnm2
Copy link
Contributor

jnm2 commented Nov 21, 2017

Who's confused? in, ref, out... it seems so straightforward to me.

@mikedn
Copy link

mikedn commented Nov 21, 2017

Who's confused? in, ref, out... it seems so straightforward to me.

Some examples:
dotnet/csharplang#1126
https://github.com/dotnet/corefx/issues/25355

@jnm2
Copy link
Contributor

jnm2 commented Nov 21, 2017

Hmm... I'd say that's just people forgetting that in is byref, which is just as likely to be confusing to them when talking about value types. So a warning on passing reference types wouldn't address the actual problem. I think this is more an education thing/David just seeing what he was hoping to see. I doubt people will actually forget in is byref in the long run. Especially if we end up with readonly locals and parameters.

@mikedn
Copy link

mikedn commented Nov 21, 2017

I'd say that's just people forgetting that in is byref, which is just as likely to be confusing to them when talking about value types

That's true but the current implementation still looks like it's asking for trouble without providing significant benefits. There is a combination of factors that I think make in rather special when compared to the existing ref and out:

  • Not requiring in at the call site like ref/out do
  • Allowing passing literal arguments to in parameters by creating temporaries
  • The rather ambiguous meaning of the word in. OK, the code inside the method can't modify the value but can code outside the method do that? Well, it can. And readonly seems a better word to describe this fact.
  • It's a useful feature to have but I doubt that it's useful enough to warrant such a terse syntax. ref readonly should be good enough. That is - simple things should be simple and complex things, well, complex.

Especially if we end up with readonly locals and parameters.

To me that's an additional argument for keeping only ref readonly. You'd have:

  • ref int
  • ref readonly int
  • readonly int

If you look at it like this then out ends up looking as an outlier. It's its fault 😄

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

No branches or pull requests

3 participants