Thread: Reassigning readonly fields

I kind of don't appreciate the fact I can actually reassigned a readonly field during construction time. Both static and instance versions.

Code:

class foo {
    public static readonly bool sfield = false;
    public readonly bool rfield = false;

    static foo() { sfield = true; }
    public foo() { rfield = true; }
}

static void Main() {
    foo bar = new foo();
    Console.WriteLine(bar.rfield && foo.sfield); // returns 'true'
    Console.ReadLine();
}

I only recent discovered this feature. It was by accident, as I was trying to track an unrelated bug.

I don't know, if the field assignment must include business logic, I could use this feature as a means to support a default value, for instance. But frankly that just doesn't make much sense, since implementing default state is easy as pie and I would want anyways to contain the whole of the business logic within the ctor and not spread it also to the field declaration.

But more than that, this feature doesn't make much sense. Purely from the point of view of language semantics, I'm effectively changing a readonly field previous assignment. This can be permissible due to the semantics of the readonly keyword specifically. But doesn't make much sense on a more general perspective of how the language should illustrate intent.

What do you think? Makes sense to file in a bug report suggesting the issuing of a compile-time warning when explicitly reasigning readonly fields in ctors (no warning would be issued if the explicit assignment happened on just the ctor or at the declaration point)?

I didn't say it was a bug. Hence why suggesting a Warning and not a compile-time Error or, more severely, an actual CLR bug. And frankly the documentation (which I naturally read, as you may guess) doesn't make much sense. Why would I ever want to explicitly instantiate a readonly field at the moment of declaration if not to make it a const? But that's besides the point. What's more pertinent is to assert or not the value of a Warning message.

Let me illustrate the problem in another way (by telling you about my little incident):

This bug I was trying to correct involved exactly a readonly field that contained a percentage value (double) used in some calculations. The bug was in that the current value needed to see its precision increased by 2. I noticed the former programmer initialized the readonly field at the moment of declaration. I made a mental note to possible change this to a const, changed the value and debugged. For all my sins, the calculations were producing the same results! For quite a while there (I even changed the value to ridiculous figures) my results were always the same. Granted, it didn't take me that long to figure it out; The readonly field was being reassigned in the master ctor and thus overwriting my changes. But still it was a frustrating experience. And one that for all purposes didn't make any sense and should have not happened. An error of the programmer, but also a lack of ability of the language to isolate this type of issue.

It is confusing at first. It does make sense though: a readonly field is an instance "variable". It's stored per instance for values that may change at compile time and are immutable from that time on. For example a class saves the timestamp of instance creation in a readonly field. It's surely not a constant, it varies per instance and depends on execution time and sequence. However, the timestamp should not be modified after it has been set. You can achieve this by using a base class with this one value, making it private, setting it in the base class' constructor and providing a getter. But that's a lot of work and it takes away the possibility to derive your class from another class (no multiple base classes in .NET). So there's "readonly"

You were perfectly right by the way. The fact that it was NOT declared a const is a sure sign that either the programmer failed OR it is set at another part of the code. You can set a default value at declaration time and only change this later in a special constructor or something, so giving a value can be useful, but declaring it const or readonly is what makes the difference. In your case, it seems that the original programmer was either right to set the value later, or totally confused so he set the same value again later. That would indeed be bad and he should have made it const.

If you had followed your thoughts right away and made it const, the compiler would have complained about setting the variable and you would have found the culprit a lot faster. Use the force next time

Yeah, I should have indeed.

I'm going to be filling said bug report sometime today. Basically the issue isn't where the readonly field gets instantiated. I haven't got an issue there. But the fact it is being assigned twice that should possibly warrant a compiler warning, since it almost always means a mistake.

Bug reported, in case anyone wishes to take the debate there.

I'm not even sure that the compiler can determine "assigned twice". After all, it's not C/C++ where something can be unassigned. Everything always has a value.

readonly bool somevalue;
readonly bool somevalue = false;

Those are equivalent statements. somevalue will initially be false. Setting it to true in the constructor would now emit a warning for line two and no warning for line one.

I agree. But explicit assignment is easy to catch, me thinks. Because it falls down a different path on the parsing tree than that from unassigned or automatically assigned variables... well, assuming the CLR follows traditional BNF or any of its extensions. A path that can raise a flag. Hence why a compiler can assign default values.

Maybe the warning would be better placed in tools like FxCop, where all the other style-warnings reside. I would hate to get a compiler warning for perfectly good code. It is a source for errors, but that's a human problem, not a technical problem. FxCop already issues a warning for line two of the example (inefficient setting it to the same value twice) so it should be easy to catch your case, too.

Hehe, make no mistake I will probably just insert the warning myself

However I sort of agree and disagree. The task of Warnings aren't to isolate bad code from good code. But to identify possible problematic code. Take a look at, say CS0642 for instance. A clear case of possibly unintended behavior being detected by the compiler. Clearly a human problem as you put it, or instead just a rather terse and ugly coding style. In any case there's nothing in there that isn't perfectly good code. gcc, it too, is ripe with these and many other kind of warnings.

But I do agree partially since I can see the contention. No one likes to have to manually disable warnings if they are bothering them. Still, there's a strong case to be made here that there's very few reasons to believe assigning a readonly field twice is intentional, while at the same time being bad form. I think you can agree on that. And because it results in an actual change of code behavior, this is why I personally support the idea of a warning.

But I won't be raising that flag too high. It's not that important.

Have you guys ever read Eric Lippert's blog? I discovered it a couple of months ago and love it. He's one of the C# compiler developers and talks regularly about reasons for certain decisions his team made with the compiler (among other things, of course).

Clearly a human problem as you put it

From my point of view, this is a technical problem or at least a problem that can be (or is) found by a piece of code (aka compiler): The programmer wrote lines of code, that the compiler will simply dump. The loop and the block (without it's contents) are null statements, they can be safely ignored. So I gave the compiler a pile of bs, that's a technical problem. In case of readonly, your code would not display the same results if the compiler dumped some code. Every line is absolutely neccessary for it to run the way it was intended. If it was intended that way, is up to you. If it were, then it does make sense, contrary to the CS0642 example, which makes no sense even if intended, because the intention is not transformed into code but rather silently dumped.

If I have a class with 10 constructors, all of them setting my readonly field to 5 and only one setting it to 7, then I think it's ok to have 5 as the default value in the declaration assignment and only one constructor setting it to 7. With your solution, I would need to set it to 5 about 9 times in my code: once for every constructor I have that isn't setting it to 7.

What I do think could be implemented as a warning is that when I actually set the field in every constructor, then the assignment at declatation level should give a warning. That would be a technical problem, because this assignment could be optimised away.

Originally Posted by nvoigt

What I do think could be implemented as a warning is that when I actually set the field in every constructor, then the assignment at declatation level should give a warning. That would be a technical problem, because this assignment could be optimised away.

Well, maybe I didn't clarify enough. But that's pretty much what I had in mind.

As for your reasoning behind CS0642 and the usage of "technical problem" to describe these and other similar code issues present in Warnings, I couldn't possibly disagree more I don't know how willing you are to discuss that. I don't mind. But for now to make a point that you didn't give the compiler any bs. There's clearly state altering code there, even if you choose to relegate that important bit of information to inside a parenthesis in your post above.

The compiler simply realizes there's a coding pattern that may not be what the programmer intended. What really motivates the creation of CS0642 and may influence positively or negatively the decision to include a new warning to inform users of readonly fields reassignments is how common such an error pattern that is.

EDIT: Meanwhile, I included that clarification about multiple constructors on the Microsoft Connect. Thanks for bringing that to my attention.