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

Setting static readonly properties fails #11571

Closed
Fabi opened this issue Nov 29, 2018 · 16 comments
Closed

Setting static readonly properties fails #11571

Fabi opened this issue Nov 29, 2018 · 16 comments
Labels
breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.

Comments

@Fabi
Copy link

Fabi commented Nov 29, 2018

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: dotnet/coreclr@c2abe89#diff-a07ab78d2f4a1c72865756edae89399c

@stephentoub
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented Nov 29, 2018

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
Copy link
Member

jkotas commented Nov 29, 2018

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
Copy link
Member

jkotas commented Nov 29, 2018

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
Copy link
Member

jkotas commented Nov 29, 2018

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
Copy link
Contributor

mikedn commented Nov 29, 2018

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
Copy link
Author

Fabi commented Nov 29, 2018

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
Copy link
Member

jkotas commented Nov 29, 2018

Thanks

@jkotas jkotas closed this as completed Nov 29, 2018
@jkotas
Copy link
Member

jkotas commented Nov 29, 2018

cc @AndyAyersMS

@AndyAyersMS
Copy link
Member

@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
Copy link

IanKemp commented Nov 30, 2018

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
Copy link
Contributor

@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.

Turnerj referenced this issue in TurnerSoftware/MongoFramework May 18, 2019
@nmain
Copy link

nmain commented Oct 14, 2019

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
Copy link

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

@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

No branches or pull requests

9 participants