Allow initonly field during ldflda and stfld import

[am11]

Fixes #55033

An immediate fix for this issue could be:

--- a/src/coreclr/tools/ILVerification/ILImporter.Verify.cs
+++ b/src/coreclr/tools/ILVerification/ILImporter.Verify.cs
@@ -2108,7 +2108,7 @@ void ImportAddressOfField(int token, bool isStatic)
                 instance = actualThis.Type;
 
                 if (field.IsInitOnly)
-                    Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.InitOnly);
+                    Check(_method.IsVirtual || (_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr), VerifierError.InitOnly);
             }
 
             Check(_method.OwningType.CanAccess(field, instance), VerifierError.FieldAccess);

in case of ToString, GetHashCode examples, we are:

1. not in a constructor
2. field's owning type is not the method owning type
3. this is not a thisptr

However, reading the conversation in related thread dotnet/roslyn#22485, this check seems too conservative and does not take modern ECMA rules into account. This PR removes the check and adds +/- test cases where it should or shouldn't be allowed.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #55033

An immediate fix for this issue could be:

--- a/src/coreclr/tools/ILVerification/ILImporter.Verify.cs
+++ b/src/coreclr/tools/ILVerification/ILImporter.Verify.cs
@@ -2108,7 +2108,7 @@ void ImportAddressOfField(int token, bool isStatic)
                 instance = actualThis.Type;
 
                 if (field.IsInitOnly)
-                    Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.InitOnly);
+                    Check(_method.IsVirtual || (_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr), VerifierError.InitOnly);
             }
 
             Check(_method.OwningType.CanAccess(field, instance), VerifierError.FieldAccess);

in case of ToString, GetHashCode examples, we are:

1. not in a constructor
2. field's owning type is not the method owning type
3. this is not a thisptr

However, reading the conversation in related thread dotnet/roslyn#22485, this check seems too conservative and does not take modern ECMA rules into account. This PR removes the check and adds +/- test cases where it should or shouldn't be allowed.

Author:	am11
Assignees:	-
Labels:	area-ILVerification
Milestone:	-

[am11]

@jkotas, PTAL. I was able to reproduce it in tests using special. naming convention as specified in ilverify readme.

[jkotas]

It would be better to add the verification rules for readonly references proposed in dotnet/designs#21 instead of just deleting this check. Have you thought about what it would take?

If it is not something you would like to spend time on right now, we should open a new issue on adding the verification rules for readonly references and comment out this check instead of deleting it, with the link to the new issue.

[jkotas]

The same check for instance fields in ImportStoreField has this problem as well. It should get the same treatment. It will fail on C# program like this:

using System;

class Test
{
    public int MyProperty { get; init; }
    static void Main() { }
}

[am11]

The same check for instance fields in ImportStoreField has this problem as well.

I have updated stfld and stsfld checks such that:

• specialname modreq(IsExternalInit) methods and .ctors of this type are allowed to modify initonly instance fields.
• .cctors of this type are allowed to modify initonly static fields.

[am11]

It would be better to add the verification rules for readonly references proposed in dotnet/designs#21 instead of just deleting this check. Have you thought about what it would take?

Thanks for sharing this link. From the perspective of ldflda and ldsflda, the spec seem to have relaxed the rules for initonly fields. I have kept the checks commented for now.

[jkotas]

TypeAccess.InitOnly

Should this be TypeAccess_InitOnly? I do not see where we are recognizing . as the separator.

[am11]

We have 25 cases using period as a error code separator when there is more than one error:

$ git grep '_Invalid_.*\..*\(.*\)' ':/src/tests/ilverify/ILTests'

It is documented here https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/ILVerify#methods-with-special-names. I agree it is quite complicated; imo, we should use xml or json list for this rather than complex naming convention parsing.

[jkotas]

we should use xml or json list

Then you have a problem with the IL and json being out-of-sync all the time. I actually like the convention based scheme we have here.

[am11]

Then you have a problem with the IL and json being out-of-sync all the time.

Current approach is equally fragile. The test discovery does not give any indication whether or not it has skipped the test during parsing (which is spread across three separate methods of TestDataLoader.cs). We have to remember total count before and after the new additions.

[jkotas]

body is actually not doing what method name suggests.

As far as I can tell, StoreInitonlyFieldOtherInstance described well what the body was doing. Am I missing something?

[jkotas]

LGTM. Thank you!

[lovettchris]

Thanks!