Readonly members metadata

[RikkiGibson]

Related to #32911

• commit 1: Add feature doc (largely pulled from Champion: Readonly members on structs (16.3, Core 3) csharplang#1710)
  This makes it easier to track the relationship between the specification evolving and the implementation changing for the purpose of feature development.
• commit 2: Emit IsReadOnlyAttribute in method and property metadata

[RikkiGibson]

Note that MethodSymbol.IsEffectivelyReadOnly now always returns false for a static method. #Resolved

[RikkiGibson]

Could I get a second review please @dotnet/roslyn-compiler #Closed

[jcouv]

!IsStatic; [](start = 149, length = 10)

should we also check that the containing type isn't a value type, as we do in PEMethodSymbol.IsDeclaredReadOnly? #Closed

[RikkiGibson]

PEMethodSymbol.IsDeclaredReadOnly is meant to avoid traversing attributes in cases where readonly can be ruled out, in the vein of PEMethodSymbol.IsExtensionMethod. One thing I'm curious about is to what extent the existing concessions/optimizations around IsExtensionMethod are necessary for IsReadOnly.

It also appears that ContainingType.IsReadOnly returns false on a readonly class declaration due to the design of SourceMemberContainerSymbol.MakeModifiers(). #Resolved

[agocke]

Not sure I understand the IsStatic addition here. Shouldn't all static methods be implicitly readonly since they can't modify this?

[jcouv]

I looked at it the other way: "readonly" is not even a valid concept for static methods, so we can say they are not "readonly".
I could go either way. #Closed

[RikkiGibson]

Seems like it depends on what you want the API to mean. Do you want it to mean "this is not writable" or do you want it to mean "this is a ref readonly parameter"?

In other words, this cannot even be read in the static method, so to say that the static method is "effectively readonly" is maybe wrong.

---

In reply to: 267528694 [](ancestors = 267528694)

[jcouv]

    internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)

something to consider for a follow-up: I see that we're only adding the attribute for source method symbols. I wonder if there are other method symbols we should consider (some synthesized ones in particular). #Closed

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs:1617 in c9bd1b6. [](commit_id = c9bd1b6, deletion_comment = False)

[jcouv]

Done with review pass (iteration 4).

[jaredpar]

🕐

[RikkiGibson]

    internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)

Not certain. There's a few requirements for this to provide a benefit:

1. must be an instance method on a struct type
2. must be called on a ref readonly/in variable

Might be useful to enumerate any cases where (1) is true and then determine if (2) could be true for it.

---

In reply to: 474958652 [](ancestors = 474958652)

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs:1617 in c9bd1b6. [](commit_id = c9bd1b6, deletion_comment = False)

[RikkiGibson]

Finished the requested changes. Please have a look. @jaredpar #Closed

[jaredpar]

Done with review pass (iteration 7). Some minor feedback. #Closed

[jaredpar]

[jcouv]

    internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)

I added a note to the test plan

---

In reply to: 474994280 [](ancestors = 474994280,474958652)

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs:1617 in c9bd1b6. [](commit_id = c9bd1b6, deletion_comment = False)