Search Unity

  1. Unity Asset Manager is now available in public beta. Try it out now and join the conversation here in the forums.
    Dismiss Notice

Issue with readonly members of struct type?

Discussion in 'Experimental Scripting Previews' started by stefan_furcht, Apr 5, 2017.

  1. stefan_furcht

    stefan_furcht

    Joined:
    Feb 10, 2017
    Posts:
    10
    I am not sure if this is an issue related to the new compiler or a general C# issue, but I encountered an interesting oddity with readonly struct members in the beta which took me a while to track down and it seems to be something a C# compiler should warn about.

    Here some example code:
    Code (CSharp):
    1. struct MyTest
    2. {
    3.     public bool Test;
    4.     public void Set(bool test) { Test = test; }
    5. }
    6.  
    7. class ReadOnlyTest
    8. {      
    9.     private readonly MyTest _struct;
    10.     public ReadOnlyTest()
    11.     {
    12.         _struct = new MyTest();
    13.         // below code would set MyTest.Test correctly to True
    14.         //_struct.Set(true);
    15.         // Debug.Log("Test1 is: " + _struct.Test);
    16.         // also permitted is
    17.         // _struct.Test = true;
    18.     }
    19.  
    20.     public void Run()
    21.     {
    22.         // The following code would pretend to set MyTest.Test to true
    23.         // but wont change it and the compiler does not warn about.
    24.         _struct.Set(true);
    25.         Debug.Log("Test2 is: " + _struct.Test);
    26.         // NOT permitted is on the other hand
    27.         // _struct.Test = true;  
    28.         // Is this consistent?
    29.     }
    30. }
    What do you think?
    Shouldn't the compiler warn about line 24 or is it an limitation because the compiler can not know if the method does modify the underlying struct?
     
  2. Dave-Carlile

    Dave-Carlile

    Joined:
    Sep 16, 2012
    Posts:
    967
    The struct itself isn't read only, so the struct can modify itself. Only the property in ReadOnlyTest is readonly, so the property can't be modified directly. If you take this to the extreme, what if a struct property referenced a class that had a property that referenced another class. All of those internal classes would have to be prevented from changing themselves, and that seems like an intractable problem that isn't worth the effort. Nor do I think it would be the best way to do things.

    From my point of view, readonly should apply to just the immediate value the property is referencing - the value stored in the property field - which is the behavior you're seeing.

    I'm fairly certain that this is a C# feature.

    Edit: Joe Duffy, who ran the languages and tools group at Microsoft, has a blog post discussing this.
     
    Last edited: Apr 5, 2017
  3. stefan_furcht

    stefan_furcht

    Joined:
    Feb 10, 2017
    Posts:
    10
    I agree and this is all correct.
    I am not talking about protecting anything referenced but the immediate value referred directly by the readonly variable.
    (However it would be possible to do more if C# had const methods like C++ does)
    And the entire content of this one struct instance stored in above readonly field is part of what we would call "the immediate value directly referred by the variable" that is what a C# value type is meant to be.

    So, if this particular struct instance would be able to change itself, it would violate the readonly constraint.
    Thus I guess the "_struct.Set(true);" ends up being executed on a throw away copy of the struct instance rather than the original and thus the change happens on a copy but does not get visible to the variable?

    But this is exactly what I mean and what I see as an issue.
    Why is it doing such weird thing under the hood rather than warning me that my code does something that can not help anything?
    There seems to be not any case where this behaviour could be intented or make sense especially since MyStruct.Set does not even have a return value, so it can only modify or do nothing.
    And because this code can not do anything good, at least for me it still would make sense, if the compiler would at least warn here.

    Nevertheless if this is just C# standard and not an issue with the beta,
    then this is likely the wrong place to come up with the suggestion to add a compiler warning for this case
    and maybe its just a language limittation that isn't worth the effort.

    Thanks for your detailed answer though.
     
  4. M_R

    M_R

    Joined:
    Apr 15, 2015
    Posts:
    559
    "_struct.Set(true);" could have other side effects, e.g. logging or writing to a global.

    btw mutable structs are evil according to c# designers (e.g. an IDisposable struct does not get disposed when using() it, an hidden copy of it does)
     
  5. stefan_furcht

    stefan_furcht

    Joined:
    Feb 10, 2017
    Posts:
    10
    For me just another reason to warn about that code :D

    But honestly if the compiler knew it was an attempted modification on the original readonly struct instance it should not be permitted by the compiler.
     
  6. Dave-Carlile

    Dave-Carlile

    Joined:
    Sep 16, 2012
    Posts:
    967
    Oh, I was thinking it allowed the change, but it just executes but you don't see the field value change? If so then that's kind of similar to what happens when you set values on a struct property since you're working with a copy. It does seem like something should be called out with a warning since it's unexpected behavior. But there is at least logical reasoning behind the behavior itself.

    This. And it makes all of these wonky edge cases cease to be an issue.
     
  7. stefan_furcht

    stefan_furcht

    Joined:
    Feb 10, 2017
    Posts:
    10
    Yes now you got me, it's actually executing but not changing the value and there is very limitted means of doing something useful with this behaviour but chances are that it is unintended and misleading.