Runtime: Setting static readonly properties fails

[Fabi]

Hey after updating to the latest .net core 3.0 version today I noticed that setting fields like public static LogTypes LogLevel { get; } through the backing field with FieldInfo.SetValue isn't longer working.

It's throwing $exception {System.FieldAccessException: Cannot set initonly static field '<LogLevel>k__BackingField' after type 'Arctium.Manager.Misc.ManagerConfig' is initialized. now.

The base class got a static Initialize method that is setting all values of the config class at runtime.

The class I'm walking about can be found here: https://github.com/Arctium/WildStar-Server/blob/master/src/Arctium.Manager/src/Misc/ManagerConfig.cs

I saw that this was updated here: https://github.com/dotnet/coreclr/commit/c2abe892b227740c4b8baa51f5433dfc1101a52f#diff-a07ab78d2f4a1c72865756edae89399c

[stephentoub]

The backing field of such a property is readonly and shouldn't have been settable but was via reflection; reflection has been fixed now to properly respect the readonly, which is important for various optimizations to be feasible. If you need to set it, add a setter or be explicit about the backing field.

[juliusfriedman]

So then this basically says "use unsafe code and OffsetOf" or don't do anything at all with reguards to compatibility in such a fundamentally used Api yet in other scenarios we contemplate the concerns of breaking changes... Tsk tsk

[juliusfriedman]

And I may have been a little harsh there but the point is valid Irl, Imho

[jkotas]

Mutating readonly statics was never reliable, even in .NET Framework, due to JIT optimizations. If your code was mutating readonly statics, it has been broken in unpredictable ways for years. Your code could have been using old value of the readonly static depending on many factors.

Thanks for the feedback. If we hear a lot that this change is causing issues, we will consider introducing compatibility quirk that you can set to revert to previous behavior. This quirk would result in worse performance since it would also disable the JIT optimizations that are taking advantage of the fact that the readonly statics are readonly.

[jkotas]

For example:

using System;
using System.Runtime.CompilerServices;

public class Test
{
    public static readonly int x; 

    static Test()
    {
        x = 42;
    }

    static void Main()
    {
        Console.WriteLine(x);
        typeof(Test).GetField("x").SetValue(null, 100);
        Console.WriteLine(x);
    }
}

This used to print either 42 42 or 42 100 depending on many factors - try that on .NET Framework vs. .NET Core, with different optimization settings, and on different .NET Core versions.

[jkotas]

Also, the results are going to change if you move the code from Main to a separate method, like:

using System;
using System.Runtime.CompilerServices;

public class Test
{
    public static readonly int x; 

    static Test()
    {
        x = 42;
    }

    static void Work()
    {
        Console.WriteLine(x);
        typeof(Test).GetField("x").SetValue(null, 100);
        Console.WriteLine(x);
    }

    static void Main()
    {
        Work();
    }
}

[jkotas]

I noticed that setting fields like public static LogTypes LogLevel { get; } through the backing field with FieldInfo.SetValue isn't longer working.

@Fabi Could you please update these properties to public static LogTypes LogLevel { get; private set; } to address this issue?

[mikedn]

Could you please update these properties to public static LogTypes LogLevel { get; private set; } to address this issue?

It would be even better to stop using automatic properties and create explicit static fields. Or call the setter through reflection instead of accessing the backing field directly. The C# spec doesn't say anywhere what names autogenerated backing fields have and, while they'll probably never change these names, "guessing" these names is still a hack.

[Fabi]

I noticed that setting fields like public static LogTypes LogLevel { get; } through the backing field with FieldInfo.SetValue isn't longer working.

@Fabi Could you please update these properties to public static LogTypes LogLevel { get; private set; } to address this issue?

That is what I already did yea.

Could you please update these properties to public static LogTypes LogLevel { get; private set; } to address this issue?

It would be even better to stop using automatic properties and create explicit static fields. Or call the setter through reflection instead of accessing the backing field directly. The C# spec doesn't say anywhere what names autogenerated backing fields have and, while they'll probably never change these names, "guessing" these names is still a hack.

Auto properties is always the nicer and more readable way to go for me.
Since I had to add a private setter now I can call that one directly now yes, but that wasn't possible before since it never had a setter.

Also in different tests I never faced the issue stated above. I'll just accept these changes now

[jkotas]

Thanks

[jkotas]

cc @AndyAyersMS

[AndyAyersMS]

@Fabi as Jan says the ability to change readonly statics after class initialization was never reliable. What has changed recently -- and the reason we have added the reflection block -- is that the jit now looks at readonly static values in more cases and uses the information in ways that could compromise type safety, if the value ever changed.

Linking this back to the PR dotnet/coreclr#20886 so we can keep track of the number of times this trips somebody up.

[IanKemp]

So then this basically says "use unsafe code and OffsetOf" or don't do anything at all with reguards to compatibility in such a fundamentally used Api yet in other scenarios we contemplate the concerns of breaking changes... Tsk tsk

More like "the thing that you were doing is undefined and was never officially supported and using it could easily have caused you to shoot yourself in the foot and now the .NET team is preventing yourself from shooting yourself in the foot". tl;dr it was a bug that has now been fixed, and if you were relying on this behaviour you were Doing It Wrong™.

[juliusfriedman]

@IanKemp, love how you put that... right; wrong; indifferent.... all [other] things aside.

It was a bug?

So then just changing the exception to not occur or return type of a function to change to a different type which definitely is no longer compatible with thar of the underlying signature even perhaps an intentional change is doing it wrong?

How then long and behold does one now work around this BUT only for . Net core to do it right?

Wait... don't tell me I'll infer.

I should have a design which doesn't require a compile time change to accomplish this...

An alternate design perhaps?

Great.

Now, next time this changes will MS vow to provide a compatibility layer which lives under the hood of the call overloads and provides a later layer of its own co possibility that I also then check an code against... however as things would go... something changes again...

Should I rely on the MS apparent switches or should I just "Do It The Wrong Way"? I'll do it the wrong way and when it bites me you can have my job.

[nmain]

Adding a little here to aid people in their Google searches: This issue affects package NHibernateProfiler.Appender when updating to .NET Core 3. Updating the package to 5.0.5038 appears to fix the issue.

[replaysMike]

This was a supported scenario in AnySerializer as well, which is now broken but probably shouldn't be supported beyond .Net core 2.