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

Champion: Readonly members on structs #1710

Open
tannergooding opened this Issue Jul 12, 2018 · 90 comments

Comments

@tannergooding
Copy link
Member

tannergooding commented Jul 12, 2018

Readonly Instance Methods

Summary

Provide a way to specify individual instance members on a struct do not modify state, in the same way that readonly struct specifies no instance members modify state.

It is worth noting that readonly instance member != pure instance member. A pure instance member guarantees no state will be modified. A readonly instance member only guarantees that instance state will not be modified.

All instance members on a readonly struct could be considered implicitly readonly instance members. Explicit readonly instance members declared on non-readonly structs would behave in the same manner. For example, they would still create hidden copies if you called an instance member (on the current instance or on a field of the instance) which was itself not-readonly.

Motivation

Today, users have the ability to create readonly struct types which the compiler enforces that all fields are readonly (and by extension, that no instance members modify the state). However, there are some scenarios where you have an existing API that exposes accessible fields or that has a mix of mutating and non-mutating members. Under these circumstances, you cannot mark the type as readonly (it would be a breaking change).

This normally doesn't have much impact, except in the case of in parameters. With in parameters for non-readonly structs, the compiler will make a copy of the parameter for each instance member invocation, since it cannot guarantee that the invocation does not modify internal state. This can lead to a multitude of copies and worse overall performance than if you had just passed the struct directly by value. For an example, see this code on sharplab

Some other scenarios where hidden copies can occur include static readonly fields and literals. If they are supported in the future, blittable constants would end up in the same boat; that is they all currently necessitate a full copy (on instance member invocation) if the struct is not marked readonly.

Design

Allow a user to specify that an instance member is, itself, readonly and does not modify the state of the instance (with all the appropriate verification done by the compiler, of course). For example:

public struct Vector2
{
    public float x;
    public float y;

    public readonly float GetLengthReadonly()
    {
        return MathF.Sqrt(LengthSquared);
    }

    public float GetLength()
    {
        return MathF.Sqrt(LengthSquared);
    }

    public readonly float GetLengthIllegal()
    {
        var tmp = MathF.Sqrt(LengthSquared);

        x = tmp;    // Compiler error, cannot write x
        y = tmp;    // Compiler error, cannot write y

        return tmp;
    }

    public float LengthSquared
    {
        readonly get
        {
            return (x * x) +
                   (y * y);
        }
    }
}

public static class MyClass
{
    public static float ExistingBehavior(in Vector2 vector)
    {
        // This code causes a hidden copy, the compiler effectively emits:
        //    var tmpVector = vector;
        //    return tmpVector.GetLength();
        //
        // This is done because the compiler doesn't know that `GetLength()`
        // won't mutate `vector`.

        return vector.GetLength();
    }

    public static float ReadonlyBehavior(in Vector2 vector)
    {
        // This code is emitted exactly as listed. There are no hidden
        // copies as the `readonly` modifier indicates that the method
        // won't mutate `vector`.

        return vector.GetLengthReadonly();
    }
}

Readonly can be applied to property accessors to indicate that this will not be mutated in the accessor.

public int Prop1
{
    readonly get
    {
        return this._store["Prop1"];
    }
    readonly set
    {
        this._store["Prop1"] = value;
    }
}

When readonly is applied to the property syntax, it means that all accessors are readonly.

public readonly int Prop2
{
    get
    {
        return this._store["Prop2"];
    }
    set
    {
        this._store["Prop2"] = value;
    }
}

Readonly can only be applied to accessors which do not mutate the containing type.

public int Prop3
{
    readonly get
    {
        return this._prop3;
    }
    set
    {
        this._prop3 = value;
    }
}

Readonly can be applied to some auto-implemented properties, but it won't have a meaningful effect. The compiler will treat all auto-implemented getters as readonly whether or not the readonly keyword is present.

// Allowed
public readonly int Prop4 { get; }
public int Prop5 { readonly get; }
public int Prop6 { readonly get; set; }

// Not allowed
public readonly int Prop7 { get; set; }
public int Prop8 { get; readonly set; }

Readonly can be applied to manually-implemented events, but not field-like events. Readonly cannot be applied to individual event accessors (add/remove).

// Allowed
public readonly event Action<EventArgs> Event1
{
    add { }
    remove { }
}

// Not allowed
public readonly event Action<EventArgs> Event2;
public event Action<EventArgs> Event3
{
    readonly add { }
    readonly remove { }
}
public static readonly event Event4
{
    add { }
    remove { }
}

Some other syntax examples:

  • Expression bodied members: public readonly float ExpressionBodiedMember => (x * x) + (y * y);
  • Generic constraints: public static readonly void GenericMethod<T>(T value) where T : struct { }

The compiler would emit the instance member, as usual, and would additionally emit a compiler recognized attribute indicating that the instance member does not modify state. This effectively causes the hidden this parameter to become in T instead of ref T.

This would allow the user to safely call said instance method without the compiler needing to make a copy.

The restrictions would include:

  • The readonly modifier cannot be applied to static methods, constructors or destructors.
  • The readonly modifier cannot be applied to delegates.
  • The readonly modifier cannot be applied to members of class or interface.

Drawbacks

Same drawbacks as exist with readonly struct methods today. Certain code may still cause hidden copies.

Notes

Using an attribute or another keyword may also be possible.

This proposal is somewhat related to (but is more a subset of) functional purity and/or constant expressions, both of which have had some existing proposals.

LDM history:

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 12, 2018

FYI. @jaredpar

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 12, 2018

This would allow better use of in with things like System.Numerics.Matrix4x4, which is big enough that it is sometimes better to pass around using in (due to size -- 64 bytes), but which can't be marked readonly due to a design-decision to make the fields public and writeable (even if the majority of methods exposed don't modify state).

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 12, 2018

It isn't mentioned above, but it may be worth discussing if a readonly method should only be able to call other readonly methods...

Today, with readonly fields of non-readonly structs, C# will make a copy of the struct. For example, the following prints 0 twice, rather than 0, 1: https://sharplab.io/#v2:C4LglgNgNAJiDUAfAAgZgAQCcCmBDGA9gHYQCe6AzsJgK4DGw6AGgLABQA3u+j+mlnkIlyATXSkA3O269+yACzoAytmAiAFAEoZPLm14HxAOhVr1ARiMAGAGaap+3gF92Ltu35VaDdCPZ6DfhsIAlxGADdcCBpsBwMdPgwFZVUNYNCIqJjtR10Eg2AACzAKI0jo7ABedHKYuOdXaTY5cwA2PgAmPnMAdn8Elvbk5HMADnURqwBtAF10XEwAcwocgwDDHiZ0AA90aqJsAHdmLXqNkaMRgE51baNSMqzse3zeV5470w0X3MML69u90eFR+BjcTiAA=

@HaloFour

This comment has been minimized.

Copy link
Contributor

HaloFour commented Jul 12, 2018

Conceptually I like it, a lot. I do have some issues with it, though.

First, such an attribute already exists, System.Diagnostics.Contracts.PureAttribute, which is supposed to mean the same thing but isn't enforced by the compiler. Adding another attribute to support this feature would be noise.

There's also the chicken&egg problem in that the BCL and ecosystem would have to be updated for this to be particularly useful otherwise these methods are stuck not being able to call out to other methods.

Next there is the issue of what defines "pure". In this case we could keep it to situations where the compiler can safely not make defensive copies, but philosophically it extends beyond that. Perhaps readonly as a keyword would help to divorce this concept from "purity", although they do seem to cover the same bases.

Lastly, readonly is already a valid modifier on the return value of a method. The position is different, but that might still lead to confusion, especially since most of the time modifier keywords can be placed in any order.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 12, 2018

Adding another attribute to support this feature would be noise.

We would have to add another attribute, however. The existing attribute has no enforcement, as you indicated, and as such would be a breaking change to begin enforcing it now.

There's also the chicken&egg problem in that the BCL and ecosystem would have to be updated for this to be particularly useful otherwise these methods are stuck not being able to call out to other methods.

This is only applicable if we restrict the ability to only call other readonly methods (#1710 (comment))

There could be a difference between a readonly and a pure method. A pure method, by definition, cannot modify any state. A readonly method (in order to satisfy the in constraint and elide the copy) must only guarantee that it does not modify instance state (it could be allowed to do things like call Console.WriteLine, which would allow copy elision, but would not be functionally pure).

It can be summed as All pure methods are readonly, but not all readonly methods are pure.

Depending on what LDM thinls, it could go one way or the other. If we did differentiate, then pure functions would be a natural stepping stone from readonly (and constexpr would again be a natural stepping stone from pure).

Lastly, readonly is already a valid modifier on the return value of a method. The position is different, but that might still lead to confusion, especially since most of the time modifier keywords can be placed in any order.

Right, I called this out in the notes section.

@theunrepentantgeek

This comment has been minimized.

Copy link

theunrepentantgeek commented Jul 12, 2018

I'm 85% sure this has all been thrashed out before - but I'm unable to find it with a quick search.

One of the real problems is that different developers will have different expectations. Some will expect this to mean that literally no changes are made, none at all, by the method. Others will expect this to mean that no externally visible changes are made.

The former is rigorous and useful from both theoretical and practical perspectives. The later permits result caching (which can be really useful for performance) and other internal changes.

Many of the original decisions in the design of C# were driven by a desire to avoid versioning issues as code evolves - this is, for example, one of the reasons why checked exceptions were not included.

What restrictions on the evolution of my code do I have if I have a property declared like this:

    public string AccessKey
    {
        readonly get { ... }
    }

My own opinion is that I don't think that adding internal caching should have any effect on the external interface; if I need to remove the readonly modifier to add caching, that could have very large cascading effects - and if it's a published API (e.g. via NuGet) then I might not be able to do it at all without breaking people.

That said, I know smart developers who disagree with me fervently on this topic ... 😁

@theunrepentantgeek

This comment has been minimized.

Copy link

theunrepentantgeek commented Jul 12, 2018

Another scenario to consider ... if you have something with a pure API, and then you modify it to generate audit logs (for legal compliance reasons), should you need to change the declaration of the API?

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 12, 2018

@theunrepentantgeek, right. pure has a number of additional considerations. However, the original post just covers readonly methods, which would not have many of these considerations.

At its root, a readonly method (as proposed) would only guarantee that instance state isn't modified (which would imply calls to System.Console.WriteLine, while not pure, would be allowe).

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 12, 2018

Added a root note, at the top of the post, clarifying that readonly method != pure method

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Jul 12, 2018

@tannergooding

Provide a way to specify that methods do not modify state.

Consider instead

Provide a way to specify individual methods on a struct do not modify state in the same way that readonly struct specifies no instance method modifies state.

@tannergooding tannergooding changed the title Provide a way to specify that methods do not modify state. Provide a way to specify individual methods on a struct do not modify state in the same way that readonly struct specifies no instance method modifies state. Jul 12, 2018

@tannergooding tannergooding changed the title Provide a way to specify individual methods on a struct do not modify state in the same way that readonly struct specifies no instance method modifies state. Provide a way to specify individual methods on a struct do not modify state, in the same way that readonly struct specifies no instance method modifies state. Jul 12, 2018

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Jul 12, 2018

@tannergooding

Using the readonly keyword is useful, since it is an existing keyword. However, it may get confusing to read if you have something like public readonly ref readonly float GetRefToX() (const T* const anyone 😄)

It's not just that it's confusing, it's also ambiguous with other potential features. The compiler specifically chose to use ref readonly instead of readonly ref for locals / returns because we wanted to allow for the future addition of readonly ref readonly local variables once readonly locals are introduced.

The individual variations are the following for locals in that context:

  • ref: a reference which can be assigned to and reassigned to a new location
  • ref readonly: a reference which cannot be assigned to but can be reassigned to a new location
  • readonly ref : a reference which can be assigned to but cannot be reassigned to a new location
  • readonly ref readonly: a reference which cannot be assigned to or reassigned to a new location

I wouldn't want to use readonly as proposed here because it would create a bit of confusion between the two cases: specifying a readonly method and readonly locals.

This feature was discussed in LDM when we discussed the readonly struct feature. At the time the syntax we were considering is the following:

struct S1 { 
  int i; 
  public void M() readonly { ... } 
}

That is putting the readonly modifier after the method signature. This is unambiguous and fairly easy to read. This is how we actually implemented permissions in Midori and it works out fairly well. Admittedly it does take some getting used to though for anyone who doesn't have a C++ background.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 12, 2018

After the method seems reasonable to me. Will update the proposal later tonight.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Jul 13, 2018

@jaredpar, where would you envision the keyword for properties?

Is it before or after the get keyword (changed it to after in the OP):

public float LengthSquared
{
    get readonly
    {
        return (x * x) +
               (y * y);
    }
}

// vs

public float LengthSquared
{
    readonly get
    {
        return (x * x) +
               (y * y);
    }
}

I would imagine that specifying it individually on the get/set is preferred here, since set (in most normal API designs) wouldn't qualify for the attribute 😄

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Jul 13, 2018

@tannergooding indeed it would be on the get and set as they are separate methods that can have separate mutating semantics.

There is no ambiguity here hence readonly can appear before or after the get / set. I'd lean towards after for consistency.

since set (in most normal API designs) wouldn't qualify for the attribute)

It actually applies in more cases than you would expect. There are a non-trivial number of structs which are just wrappers around reference types. Such structs could themselves be easily marked as readonly, or have a readonly set as they don't modify the contents of the struct.

@Starnick

This comment has been minimized.

Copy link

Starnick commented Jul 15, 2018

I really hope this is considered. I was excited about ref-readonly/in...until I ran into the defensive copy problem. An all-or-nothing approach (declaring the struct readonly) makes using the ref-readonly feature difficult in my context (game development / graphics).

I have a math library that is similar as the examples discussed - low level and mutable math types. Historically, a source of bugs have been when a function takes a math type as a ref, but the parameter is supposed to be readonly (basically have always used the honor system...). Also, while the types are mutable, more often than not the context they are used are in readonly collections (e.g. rendering that acts on the data, the data can only change during a simulation step...so its a "readonly view of the data"); having those collections return a writable reference breaks the design. So I was excited about having more options to express design intent and reduce errors with ref-readonly.

To get around the defensive copies, however, has become something of a pain. I'm considering using extension methods to "fake" instance methods, but then there aren't options for instance properties/indexers.

Admittedly it does take some getting used to though for anyone who doesn't have a C++ background.

I would argue most developers who would be in need of this feature are already pretty familiar with const methods in C++.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Jul 16, 2018

@Starnick

An all-or-nothing approach (declaring the struct readonly) makes using the ref-readonly feature difficult in my context (game development / graphics).

Thanks for the examples provided here. This matches up with the other asks we've seen for this.

One of the biggest items that held us from doing this at the same time we did readonly struct was a lack of motivating samples. The language feature itself is fairly straight forward but it wasn't clear the extra complexity was justified. Examples like this help us prioritize this properly.

@jaredpar jaredpar self-assigned this Jul 16, 2018

@4creators

This comment has been minimized.

Copy link

4creators commented Jul 18, 2018

Admittedly it does take some getting used to though for anyone who doesn't have a C++ background

I would argue most developers who would be in need of this feature are already pretty familiar with const methods in C++.

While readonly is used in many places in C# I would prefer to have a bit more clarity with using already existing const keyword after method name declaration i.e.:

struct S1 { 
  int i; 
  public void M() const { ... } 
}

Otherwise proposal seems to be very useful in multiple scenarios and it would be very nice to have it available sooner rather than later.

@HaloFour

This comment has been minimized.

Copy link
Contributor

HaloFour commented Jul 18, 2018

@4creators

IMHO I'd rather save const for possibly writing const-expression methods which are evaluated by the compiler at compile-time.

@4creators

This comment has been minimized.

Copy link

4creators commented Jul 18, 2018

IMHO I'd rather save const for possibly writing const-expression methods which are evaluated by the compiler at compile-time.

@HaloFour IMHO it could be reconciled with const-expression methods but obviously it is syntactic detail which will be discussed last :)

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Jul 18, 2018

@4creators, @HaloFour

I don't like using const here because the behavior isn't in line with what const means today. Specifically const today refers to a value that can be expressed as a literal, manipulated at compile time and round tripped through metadata by the compiler. These don't describe the behavior of the methods here.

The reason for using readonly here is the implementation lines up with existing uses of readonly in the language today. The other potential modifier to use here is in. After all this feature can best be described as saying:

The this parameter of a struct method annotated with readonly has a type of in T instead of ref T.

There is a good case for in and readonly here. The choice in LDM was for readonly as it's what we use as the type modifier toady hence makes sense at a method level. If we had chosen in struct instead of readonly struct I imagine we would have picked in for this as well.

@qrli

This comment has been minimized.

Copy link

qrli commented Jul 19, 2018

I'm all in for this feature. Without it, the in feature does not help the Vector/Matrix in game/graphics at all.

And, in C++, if you call a mutable method on a const object, you get an error. In current C#, you get a silent copy. This contradicts with the goal, if I understand correctly, the feature is all for performance.

It actually applies in more cases than you would expect. There are a non-trivial number of structs which are just wrappers around reference types.

In case the struct just wraps a reference, it does not need in at all. The argument on stack is the same size. And the performance could be worse because of the extra mem lookup.

@acple

This comment has been minimized.

Copy link

acple commented Jul 19, 2018

What about an expression-bodied getter/method ?
public float LengthSquared readonly => ...?

@Starnick

This comment has been minimized.

Copy link

Starnick commented Jul 19, 2018

If we had chosen in struct instead of readonly struct I imagine we would have picked in for this as well.

I'm really glad readonly struct was chosen over in struct, the former reads so much better (at least to a native english speaker).

@qrli

In case the struct just wraps a reference, it does not need in at all. The argument on stack is the same size. And the performance could be worse because of the extra mem lookup.

The simple case where the struct wraps a single reference type, sure. Might be an issue when the struct holds a number of references. But that case is probably exceedingly rare.

@theunrepentantgeek, @jaredpar

My own opinion is that I don't think that adding internal caching should have any effect on the external interface; if I need to remove the readonly modifier to add caching, that could have very large cascading effects - and if it's a published API (e.g. via NuGet) then I might not be able to do it at all without breaking people.

qrli's comment about C++ const correctness made me think about this. Annotating ready-only methods to avoid silent copies is really a struct-only feature, and would basically be useless on a method for a reference type (at least I think so?). So the worry about caching in a reference type would be moot in my opinion, but for structs it certainly has interesting implications.

The case where a struct has to cache some state in a read-only method would probably be rare, but I can see it - maybe a geometry intersection algorithm, or the like where you might need to keep track of some state incrementally as data is processed. And I really like the idea of giving more flexibility to using structs everywhere and in more contexts, e.g. low-allocation apps that manages data using large arrays of structs passed around by ref or ref-readonly. Not common in business apps, but certainly common for games.

C++ of course has a solution to this scenario, the mutable keyword. When a field is marked as mutable, it can be modified in a const method. What are your thoughts in bringing this concept to C#?

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Jul 19, 2018

@Starnick

C++ of course has a solution to this scenario, the mutable keyword. When a field is marked as mutable, it can be modified in a const method. What are your thoughts in bringing this concept to C#?

There is already an API available to do this hence I wouldn't want to add a language concept.

void Example(in T value) { 
  ref T refValue = Unsafe.AsRef(in value); 
  ...
}

The Unsafe Type. There are lots of dangerous / powerful helpers there that let you subvert C# type system rules. Note though that it's the safety equivalent of using unsafe (buyer beware).

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Jul 19, 2018

@acple

What about an expression-bodied getter/method ?

Yes that's the likely syntax.

@yaakov-h

This comment has been minimized.

Copy link
Contributor

yaakov-h commented Jul 19, 2018

@jaredpar isn’t that a Core-only API?

@RikkiGibson

This comment has been minimized.

Copy link
Contributor

RikkiGibson commented Feb 14, 2019

@jaredpar @tannergooding what's the word on applying readonly to auto-properties like the following?

// (1) Could reasonably be allowed-- but the compiler will already
// generate a `readonly` field for a getter-only auto-property.
// It could be misleading because `readonly` has no effect on the semantics here.
public readonly int P { get; }

// (2) Shouldn't be allowed because the underlying field has to be mutable
public readonly int P { get; set; }

// (3) Probably shouldn't be allowed because `static readonly` methods are not allowed
public static readonly int P { get; }
@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Feb 14, 2019

@RikkiGibson yes it should be applied for auto-generated properties

@Joe4evr

This comment has been minimized.

Copy link

Joe4evr commented Feb 14, 2019

What I'd think is, for a getter-only auto-property like public int P { get; }, once this member-level readonly modifier exists, it could reasonably be allowed to be implicitly applied on recompile.

@pentp

This comment has been minimized.

Copy link

pentp commented Feb 14, 2019

It cannot be applied implicitly because it changes the method (property getter) public contract - for example when in the future you want to change the value to be lazy initialized it would need to be changed from readonly back to normal and this would break binary compatibility with existing callers.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Feb 14, 2019

@pentp

The readonly modifier does not impact binary compatibility. It's simply an attribute on a method and nothing more. Yes it would be a subtle behavior change when moved to a non-auto implemented property but I'm not too concerned about that. I imagine for instance we'll change our refactoring tools to add the readonly modifier as part of the refactoring.

@pentp

This comment has been minimized.

Copy link

pentp commented Feb 14, 2019

It's part of the method contract in the same way as readonly struct - it changes the way the compiler emits callsites, so it's a (binary) breaking change to remove this attribute.

Is my understanding of the following scenario correct?

public int A { get; } // readonly backing field, but normal getter method
// this could be later changed to a non-readonly behavior with no contract changes
public int A => _a ?? (_a = ...);
// but it wouldn't work if the first version implicitly got the readonly attribute

public readonly int B { get; } // initial version with readonly attribute
public readonly int B => _b ?? (_b = ...); // invalid
public int B => _b ?? (_b = ...); // breaking change
@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Feb 14, 2019

@pentp

It's part of the method contract in the same way as readonly struct - it changes the way the compiler emits callsites, so it's a (binary) breaking change to remove this attribute.

Sorry but that's not how we define a binary breaking change. A binary breaking change is one where by an existing assembly which compiled against an API would cease to function if the change is made. This is not just my personal nomeclature here, this is an established term within the .NET space.

In this case the readonly modifier for a member is an attribute on a member. Adding or removing it does not impact the binary behavior here. Yes it can result in an observable functional difference than if the same binary was recompiled from source but C# has plenty of such cases: adding new overloads, adding / removing readonly on a struct, etc ...

@HaloFour

This comment has been minimized.

Copy link
Contributor

HaloFour commented Feb 14, 2019

@jaredpar

I thought the presence of this modifier would instruct the compiler to not make defensive copies of the struct before calling the method. Wouldn't removal of that attribute and the observation of those changes be considered a breaking binary change?

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Feb 14, 2019

@HaloFour

No. Binary breaking changes are as I laid out above: will it cause an assembly which is compiled against the old code to fail to load / execute at all in the face of this change. That won't happen here. The code will continue to run just with behavior that is different from what would happen if it were compiled as source.

Examples of binary breaking changes are: m

  • Making a virtual method non-virtual or abstract
  • Adding a sealed modifier to an existing type
  • Adding a ref modifier to an existing struct

All of these would cause assemblies compiled against them to fail to load given reasonable code patterns.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Feb 14, 2019

@pentp, @HaloFour

Wanted to add a bit more detail here as I think this may just be mostly a terminology issue. Yes this absolutely results in a behavior change but it's not a change we'd classify as binary breaking. As I noted that has a very specific meaning for us. Wanted to elaborate a bit on all the different types of breaking changes we care about in the order of severity:

Binary Breaking

This will cause an assembly which is compiled against the old code to fail to load / execute at all in the face of this change. Typically this results in an assembly or type load exception.

Examples of binary breaking changes are:

  • Making a virtual method non-virtual or abstract
  • Adding a sealed modifier to an existing type
  • Adding a ref modifier to an existing struct

All of these would cause assemblies compiled against them to fail to load given reasonable code patterns.

Source Breaking

This is a change which will cause existing C# code which compiles to potentially fail to compile in the face of this change. The change itself is not a binary breaking change though.

Examples of source breaking changes:

  • Adding an overload of a method where it is considered a "better" candidate by C# language rules.
  • Adding a type which has the same name as a contextual keyword in the language: say var or dynamic.
  • Adding readonly to an accessible field.
  • Adding static to a class which has a private ctor and only static methods

Note: in most cases binary breaking changes are also source breaking.

Behavior Breaking

This is a change which is not source or binary breaking but will cause an observable difference, positive or negative, when existing C# code is recompiled against it.

Examples of this:

  • Adding or removing readonly on a struct definition.
  • Changing a method return type from object to dynamic
  • Removing readonly from a field.
  • Adding virtual to an instance member.

This are generally noted but not considered blockers. The reason is that these type of changes are generally specific to the C# type system. The changes emitted here could always be seen / exploited by other 100% compliant .NET languages.

The fact that C# elides copies of a readonly struct is purely a C# convention. Other languages can, and do, have different behaviors here. Hence the change is something another language could already observe in your program.

Note: I don't even think we have an official name for this.

@HaloFour

This comment has been minimized.

Copy link
Contributor

HaloFour commented Feb 15, 2019

@jaredpar

Wanted to add a bit more detail here as I think this may just be mostly a terminology issue. Yes this absolutely results in a behavior change but it's not a change we'd classify as binary breaking.

Understood. It'd be nice to have a glossary that describes these kinds of situations so that we can speak the same language.

"Breaking behavioral change" seems appropriate. I'm much less concerned about other languages interpreting this metadata or acting pathologically than I am about C# following its own conventions and that still accidentally leading to unwanted mutations or copies. Silently doing the wrong thing is so much more insidious than failing to load, especially in non-pathological cases.

@svick

This comment has been minimized.

Copy link
Contributor

svick commented Feb 15, 2019

@HaloFour CoreFX has a glossary on this topic and also a list of what changes they allow and which ones they disallow. It's CoreFX, so it's targeted more at API evolution than language evolution, but I think a lot of it should still be applicable here.

@HaloFour

This comment has been minimized.

Copy link
Contributor

HaloFour commented Feb 15, 2019

@svick Thanks, that's helpful.

@dgrunwald dgrunwald referenced this issue Feb 24, 2019

Open

C# language support status #829

49 of 92 tasks complete
@RikkiGibson

This comment has been minimized.

Copy link
Contributor

RikkiGibson commented Feb 25, 2019

It seems like the design text in this issue is a bit out of date/incomplete compared to what's been discussed in LDM. Once we have confirmation on some of the cases with properties I'm going to update the issue description.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Feb 25, 2019

@RikkiGibson, could you also update the proposal (https://github.com/dotnet/csharplang/blob/master/proposals/readonly-instance-members.md) when you updated the OP.

@Joe4evr

This comment has been minimized.

Copy link

Joe4evr commented Feb 26, 2019

// Allowed
public readonly int Prop4 { get; }

I have some reservations about allowing this specific option: It looks very non-obvious here that readonly is applied to the getter method, not the property, especially considering beginners who may start thinking that declaring a getter-only auto-prop without readonly doesn't make it truly readonly. By contrast, the option of { readonly get; } is much more intuitively understandable where the modifier applies.

I just feel that this one is much more likely to trap beginners rather than help them.

@qrli

This comment has been minimized.

Copy link

qrli commented Feb 26, 2019

@Joe4evr I think that's because our brains are used to the C++ rules, where the position of the modifier matters a lot. Beginners do not have this. It is just readonly property or readonly method, in addition to readonly fields.

@acple

This comment has been minimized.

Copy link

acple commented Feb 26, 2019

Can add readonly modifier for readonly struct's member? That's definitely no effect so should not allowed. Proposal doc's restrictions section does not mention it.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Feb 26, 2019

@acple

Can add readonly modifier for readonly struct's member? That's definitely no effect so should not allowed.

That is allowed. There are cases in the language where redundant modifiers exist and are allowed or even required. The application of readonly on a member of a readonly struct is just another one of those cases.

@RikkiGibson

This comment has been minimized.

Copy link
Contributor

RikkiGibson commented Mar 2, 2019

I'm trying to understand exactly what the language should do when a readonly method's implementation calls a non-readonly method.

The strictest approach would be to disallow calling any non-readonly methods on this in the scope of the readonly method.

Another possibility is to have the language permit such calls by implicitly creating a local copy before invoking the non-readonly member. Optionally a compiler warning could be given.

Now consider a program like the following:

public struct S
{
    public int i;

    public readonly void M1()
    {
        // should create local copy
        M2();
        System.Console.Write(i);
    }

    void M2()
    {
        i = 42;
    }

    static void Main()
    {
        var s = new S { i = 1 };
        s.M1();
    }
}

In this case, the expected output is 1. However, if I remove the readonly modifier, the expected output changes to 42. As a user I might find this pretty confusing.

Wondering if you have any thoughts on this @jaredpar @tannergooding.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Mar 2, 2019

My opinion is that the most correct behavior is to disallow calling any non-readonly methods on this in the scope of a readonly method. It makes the source code blatantly obvious that there is a copy and forces the user to think about the side-effects.

However, I don't think we can necessarily do that give the way the existing readonly struct feature works.

The next best thing would be to give a warning. I expect this is what the readonly struct feature will eventually centralize on when and if we get warning waves. The downside is that you now have a difference in behavior between readonly methods (give a warning) and readonly structs (do not give a warning). But with the potential upside that you could "opt-in" on readonly structs by explicitly marking the methods as readonly (giving users a way to get this validation, in a non breaking-way, without warning waves existing).

I think that ultimately, it will come down to an LDM decision and @jaredpar's weigh in is likely much more important than mine 😄

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented Mar 2, 2019

Hindsight is 20-20. If I'd spent more time thinking about the readonly member feature and unintended copies in general I would've likely tweaked the ref readonly / readonly struct proposal along the following lines:

An invocation of a member on a standard struct field when the containing receiver type is explicitly ref readonly will result in a warning.

That would've caught cases like the following:

struct Point { ... }
readonly struct Container { 
  Point p; 

  void M() {
    string s = p.ToString(); 
  }
}

There is likely an unintended copy of Point here during the invocation of ToString. The ref readonly feature was new here hence this warning wouldn't be a compat break. It would've also caught cases like the following:

struct Point { ... }
struct Container2 { 
  public Point p; 
}
class Example {
  void M(ref readonly Container2 c) { 
    string s = c.p.ToString();
  } 
}

The language of my rule probably needs a bit of tweaking to avoid all the cases where you'd insert a warning for existing code. But hopefully it gets the basic point across. It would've also paved the way to have better warning support in readonly members because the rule would naturally expand out. I still think we'll recommend to LDM that calling a non-readonly member from a readonly member on the same type should be a warning. The rest though will likely end up waiting for warning waves. I'm optimistic that will ship at the same time as C# 8.0 though hence developers who want the additional warnings will just wait until then.

@RikkiGibson

This comment has been minimized.

Copy link
Contributor

RikkiGibson commented Mar 4, 2019

This seems to predate the readonly structs feature.

public struct S1
{
    public readonly S2 s2;
    public void M1()
    {
        s2.M2();
    }
}

public struct S2
{
    public int i;
    public void M2()
    {
        i = 42;
    }

    static void Main()
    {
        var s1 = new S1();
        s1.M1();
        System.Console.Write(s1.s2.i);
    }
}

Just removing the readonly modifier on S1.s2 will change the output of this code from 0 to 42. So the language has been implicitly value copying for a long time.

It seems like the value proposition for readonly members is to help developers reduce value copying in their programs. So I am inclined to warn when calling a non-readonly member from a readonly member.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.