Fix Issue 11714 - Improve error message for wrongly initialized thread-local class instances

[WebDrake]

The existing error message can give the impression that the thread-local class instance itself must be const or immutable, not its initial value. This revised version aims to make clearer what is wrong and how to solve it.

Background to this patch: http://forum.dlang.org/post/mailman.408.1386663198.3242.digitalmars-d-learn@puremagic.com

https://d.puremagic.com/issues/show_bug.cgi?id=11714

[yebblies]

Should have a diagnostic test case, and in the future you should add a link to the bugzilla issue in the description. (I've added it to this one)

[WebDrake]

Understood. I've attached a test case to the bug report, whose code is as follows:

class A
{
    int a;
    int b;
}

A a1 = null;    // works
A a2 = new A;   // fails

Run with rdmd -main this produces the error:

tlclass.d(8): Error: variable tlclass.a2 is mutable. Only const or immutable class thread local variable are allowed, not tlclass.A

[yebblies]

The test case needs to go in the test suite, see tests/fail_compilation/diag*.d for examples

[WebDrake]

Is that really appropriate for a patch that is simply changing an error message? Of course I'll do so if needed, but I don't see the logic in this case. There is not a bug that is being fixed here, just an error message whose meaning is being clarified.

[yebblies]

Yes. Ideally we'd have a test case for every error message, and since you're changing it, it would be a good time to add one.

[WebDrake]

Ah, that makes things clear. Thanks for explanation; I'll get onto it.

[dnadlinger]

The change to the error message is not in fact correct (i.e. the check really disallows initializing static, mutable class variables with an instance, the issue IIRC being that a new one would have to be constructed for each thread on startup). Rewording the error message might still be a good idea, though.

[WebDrake]

So the error message should read, ... cannot be instantiated at compile time. etc.?

[dnadlinger]

@WebDrake: Well, if you make the variable either constant, or shared, then it's legal.

[WebDrake]

... is a mutable thread-local class variable and can only be instantiated at compile time with a const, immutable or shared value (e.g. null), not a mutable value %s.

... works? :-)

[dnadlinger]

@WebDrake: I'm not sure as I haven't looked at the related source, but you can never initialize a mutable reference with const, immutable or shared data. null works because it's not actually a class instance, but zeros the reference, which can just be written to the data section without having to "re-instantiate null" on each new thread.

[dnadlinger]

Maybe @IgorStepanov can comment on this.

[WebDrake]

Can you guys think it over and confirm for me what's allowed and not allowed in terms of initialization, and then I'll follow up with a revised patch and a test case as requested?

[dnadlinger]

Conceptually, I think the error message should be something along the lines of "thread-local mutable class variable cannot be initialized with reference to compile-time constant". How to best word that - no idea...

[dnadlinger]

And maybe suggest a fix: "Use a static constructor instead."

[9rnsr]

My suggestion:

fail_compilation/diag11714.d

/*
TEST_OUTPUT:
---
fail_compilation/diag11714.d(12): Error: thread-local mutable class reference diag11714.c is not allowed. Please use static this() instead.
fail_compilation/diag11714.d(13): Error: thread-local pointer to mutable struct diag11714.p is not allowed. Please use static this() instead.
---
*/

class C {}
struct S {}

C c = new C();
S* p = new S();

[WebDrake]

AFAICS the constraint is that the variable name must come right after Error: as to do otherwise would involve rewriting much more code (and therefore error messages).

How about,

%s is a thread-local class and cannot be initialized with mutable reference %s. Suggestion: initialize with null, or use a static constructor (static this).

%s is a thread-local pointer and cannot be initialized with mutable struct %s. Suggestion: initialize with null, or use a static constructor (static this).

Works for everyone?

[dnadlinger]

I still think "initialized with mutable struct" is a bit misleading, as it's not the initializer that is a problem, but the variable type.

[ghost]

Any consensus on the diagnostic? The current one in the test is way too long IMO.

[WebDrake]

Sorry all for the delay in any follow-up here. Moving country and job in the last month has been a distraction. ;-)

I think from my point of view what still needs to be worked out here is an error message that really sheds light on what the problem is and how to solve it. References to static this() are one such, but not the only solution. You wouldn't work out from that for example that it's perfectly legit to initialize to null and use a singleton-like pattern to new the class. Again, remember that the whole motivation for this patch was because the original error message was completely misleading and confusing to me.

How about,

%s is a mutable thread-local {class, pointer} and can only be initialized at compile time with null or static this().

[dnadlinger]

@WebDrake: To me, that makes it sound as if static this() was a way to initialize the variable at compile time, which it is obviously not.

[WebDrake]

%s is a mutable thread-local {class, pointer}. Initialize with null, or implement a static this(). ... ?

[IgorStepanov]

Maybe @IgorStepanov can comment on this.

I apologize for the very long answer.
Now dmd disables initializing non-const class variables:

Foo foo = new Foo(); //wrong
const Foo foo = new Foo(); //Ok

In other words, l-value can be mutable now.
We may fix it and disallow mutable l-value, if non shared/__gshared variable is initialized.
There are another side of this issue:

class Test
{
    Foo f = new Foo(); //Alert: Foo will be shared between all Test instances.
}

This problem also applies to other reference types:

class Test
{
    int[] a = [1, 2]; //a shared between all Test instances
}

int[] arr = [1, 2]; //arr shared between all threads. You may modify arr[0] in the first thread and check this change in another thread.

I suggest the next rule: mutable literal of reference type (including pointer, array, AA, class) should be disallowed, except they used for initialize an automatic or shared/__gshared varibale.

However, it is considerably more of this PR.

is a thread-local class variable and cannot be initialized with a mutable (not known at compile time) value.

This message is wrong: value is known at compile time. However it shouldn't be used for initializing a variable, because we may get an implicit sharing of mutable data.

[andralex]

Quite a bit of discussion here - any closure?

[ibuclaw]

Moving PR to #7508.