Runtime: Proposal for adding System.Runtime.CompilerServices.IsReadOnlyAttribue

Created on 31 Mar 2017 · 32Comments · Source: dotnet/runtime

Rationale and Usage

For upcoming C# features captured in document here, the C# compiler needs a way to annotate metadata as read-only. The proposal is to add a new attribute to System.Runtime.CompilerServices namespace similar toInAttribuate and OutAttribute, named IsReadOnlyAttribue. Example usages:

1) For read-only reference parameters.
2) For return types that are read-only references.
3) On read-only structs.

Proposed API

// .NET Core:
//      System.Runtime (reference assembly)
//      System.Private.CoreLib (implementation)
// .NET Framework 4.7++
//      mscorlib
// .NET Standard 2.0++
//      netstandard

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.All, Inherited = false)]
    public sealed class IsReadOnlyAttribue: Attribute
    {
        public IsReadOnlyAttribue();
    }
}

api-approved area-System.Runtime.CompilerServices

Source

OmarTawfik

All 32 comments

cc @dotnet/roslyn-compiler @terrajobst @karelz @weshaggard

OmarTawfik on 31 Mar 2017

FYI @marek-safar

jcouv on 31 Mar 2017

```c#
[AttributeUsage(AttributeTargets.All, Inherited = false)]

What's the point on allowing `[ReadOnly]` on assebmlies or enums or generic parameters? Shouldn't it be:

```c#
[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.ReturnValue | AttributeTargets.Struct, Inherited = false)]

svick on 31 Mar 2017

In general we don't want to take proactively new APIs to 2.0 at this point, but we can make an exception, esp. if it helps significantly the ecosystem (new Roslyn features) and if someone signs up to do the work ;-)

Is there anything beyond adding the type?
Is Roslyn team 100% sure this is the right way to do the feature end-to-end? We don't want to rush new APIs if we know there is a chance they will not be useful and we may regret them. If there's doubt, let's not rush it. If it is "obviously this way, how else", we can make it happen.

karelz on 31 Mar 2017

@svick

What's the point on allowing [ReadOnly] on assebmlies or enums or generic parameters? Shouldn't it be:

Part of the motivation for All is future proofing. This is a type we're going to be pushing deeper into the stack, say into mscorlib desktop eventually. Once a type gets that deep into the system it's basically impossible to change. Or at least changes take a considerable amount of time to propagate. Hence better to be more flexible and allow redundant usages than be too strict and handcuff ourself later.

A few of the ones you mention have known potential use cases. For instance generic parameters would be necessary if we ever introduced a readonly constraint on generic type parameters. That's probably fairly unlikely today but I'd rather have the potential here than have to add another attribute later when we find some amazing use case for it.

As for assembies, enums, etc ... We don't have a particular use case for them but at the same time there is 0 downside into allowing it.

jaredpar on 31 Mar 2017

@karelz I'll be doing the work for coreclr, corert, and desktop :) LMK if there is something else I should consider.

OmarTawfik on 31 Mar 2017

We need to run it by API review on Tuesday. Please ping @terrajobst to schedule the second hour block for this topic. Roslyn team already used that slot for last 2 weeks for various Roslyn proposals ...
I'd like to understand what you mean by Desktop work. It can wait until the meeting.

karelz on 31 Mar 2017

👍1

There is System.ComponentModel.ReadOnlyAttribute.

Do we really want same one, just in a different namespace?

jkotas on 1 Apr 2017

New attribute is needed.
Ideally we would just use InAttribute since it is a pseudo-attribute, but it may be used for other purposes already and we cannot change that for compat reasons.

VSadov on 1 Apr 2017

@VSadov can you explain your assertion "new attribute is needed"?

karelz on 1 Apr 2017

👍1

What's unclear to me is why you want an attribute over, say, a modopt or modreq. Do you need the ability to mark the definition of types and methods or only usages? Usages seems much more logical to me.

Assuming it's an attribute, I wouldn't use System.ComponentModel.ReadOnlyAttribute. The reason being that this type is designed to influence how properties are handled in the designer. That's a fairly high-level concept and besides the name has little to do with compiler enforcing read-only-ness of refs. We shouldn't mix these so that we can evolve them independently. Also, I buy @jaredpar's argument that we should apply attribute usage constraints generously. We don't want to have the discussion where we need to mark something as read-only and can't because we constrained the usage. If the type lives in System.Runtime.CompilerServices (which it should), then it shouldn't be a concern as customers are unlikely to use that type anyway.

terrajobst on 4 Apr 2017

Talked with @OmarTawfik and @jaredpar afterwards.

We need an attribute because they want to be able to mark structs as read-only:

```C#
// Expected syntax
readonly struct MyStruct
{
}

// Expected metadata representation
[ReadOnly]
struct MyStruct
{
}

2. The compiler will also use a `modreq<IsConst>` to mark types representing `readonly ref SomeType` in parameters, return types, and field declarations.

The `IsConst` type modifier already exists, which is why the ask is only for `ReadOnlyAttribute`.

As outlined above, we don't want to use the existing read-only types for the following reasons:

* `System.ComponentModel.ReadOnlyAttribute` represents a different concept in a higher-level API (the designers). Mixing them seems like a bad idea because each might require unique concepts.
* We should generally not use attribute applications of existing types to encode new language semantics as this could mean that existing code is now interpreted differently by the compiler.

Proposal:

```C#
// .NET Core:
//      System.Runtime (reference assembly)
//      System.Private.CoreLib (implementation)
// .NET Framework 4.7++
//      mscorlib
// .NET Standard 2.0++
//      netstandard

namespace System.Runtime.CompilerServices
{
    [AttributeUsage(AttributeTargets.All, Inherited = false)]
    public sealed class ReadOnlyAttribute : Attribute
    {
        public ReadOnlyAttribute();
    }
}

Please note that we don't plan to make this type available for earlier platforms. If the compiler can't resolve the ReadOnlyAttribute, it will emit an internal type into the user's assembly. When importing structs for other assemblies, the compiler will honor the attribute by name (System.Runtime.CompilerServices.ReadOnlyAttribute). That's in line with how ExtensionAttribute has worked, except that the compiler will automatically generate the type if necessary.

terrajobst on 4 Apr 2017

CoreCLR changes in https://github.com/dotnet/coreclr/pull/10777
CoreRT changes in https://github.com/dotnet/corert/pull/3264

OmarTawfik on 8 Apr 2017

cc @marek-safar the above PRs went in.

OmarTawfik on 11 Apr 2017

@jkotas my understanding is that after this type already went into coreclr and corert, the last piece of work needed is to add a type forward to corefx to forward the type to both of them. This means that I need to wait for official builds to be produced and synced into the corefx repo to do that.
Can you please confirm if my understanding is correct? and if so, what is the timeline for the sync?

OmarTawfik on 11 Apr 2017

@OmarTawfik thanks for head up, updating our part

marek-safar on 11 Apr 2017

Can you please confirm if my understanding is correct?

Yes.

and if so, what is the timeline for the sync?

There are build infrastructure issues currently so the bits are not propagating as fast as they should - give it a few days. In the meantime, you can make the corefx change; verify it against your private CoreCLR build (https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md) and submit the corefx PR that folks can review; but the CI may be failing on it until the CoreCLR gets updated.

jkotas on 11 Apr 2017

Are we really sure this breaking change is necessary? It basically breaks any existing code which has both System.ComponentModel' andSystem.Runtime.CompilerServices' usings

marek-safar on 12 Apr 2017

👍1

The compiler needs some method of marking readonly in metadata. For compat purposes we can't use any existing types.

It basically breaks any existing code which has both System.ComponentModel' andSystem.Runtime.CompilerServices' usings

The code would need both usings and an explicit use of that attribute. That combination seems fairly rare.

jaredpar on 12 Apr 2017

I am not sure how you define rare but I hit that on first compile test I tried in Mono System.dll which has https://github.com/mono/mono/blob/master/mcs/class/System/System.Diagnostics/PerformanceCounter.cs#L34 and https://github.com/mono/mono/blob/master/mcs/class/System/System.Diagnostics/PerformanceCounter.cs#L38 then uses both MethodImplAttribute and ReadOnlyAttribute

As the newly introduced attribute is not intended for users it can be as long as we want and I don't see reason why we cannot rename it to ReadOnlyMetadataAttribute or ImmutableMetadataAttribute or something else.

marek-safar on 12 Apr 2017

why we cannot rename it to ReadOnlyMetadataAttribute or ImmutableMetadataAttribute or something else.

I don't have any objections to these names.

jaredpar on 12 Apr 2017

@terrajobst can you please approve/comment on the latest comment here? to change the type from ReadOnlyAttribute to ReadOnlyMetadataAttribute?
I'll have to send another PR afterwards to change it.

OmarTawfik on 13 Apr 2017

@dotnet/fxdc: there are concerns us breaking code that has usings for both System.Runtime.CompilerServices and System.ComponentModel while using the existing ReadOnlyAttribute as this API shape would now result in two types with the name ReadOnlyAttribute in scope. We don't really care about the name for the new attribute as we don't expect developers to use them directly but by the compiler.

Any objections renaming it to:

C# namespace System.Runtime.CompilerServices { [AttributeUsage(AttributeTargets.All, Inherited = false)] public sealed class IsReadOnlyAttribute : Attribute { public IsReadOnlyAttribute(); } }

I think prefixing with Is might be the most sensible choice as it's really quite similar the other marker types we have. The suffix of Attribute differentiates it enough from modopt/modreqs, IMHO.

terrajobst on 15 Apr 2017

IsReadOnlyAttribue works for me. I definitely prefer it to ReadOnlyMetadataAttribute.