D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 5314 - Wrong error message: struct within immutable member and postblit function
Summary: Wrong error message: struct within immutable member and postblit function
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: Other All
: P2 critical
Assignee: No Owner
URL:
Keywords: diagnostic
: 5917 (view as issue list)
Depends on:
Blocks:
 
Reported: 2010-12-03 09:19 UTC by Haruki Shigemori
Modified: 2015-01-13 10:46 UTC (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Haruki Shigemori 2010-12-03 09:19:34 UTC
// a.d
struct ID
{
        immutable int value;
        this(this){}
}
void main() {}

$ dmd a.d
Error: this is not mutable
Error: this is not mutable
Error: this is not mutable

- No line number.
- Strange *three* error message.
Comment 1 Jonathan M Davis 2010-12-03 11:37:52 UTC
I've noticed that a lot of errors get printed three times. I have no idea why. Regardless, the error message is close to correct. The problem is, of course, that value is immutable, not the this pointer, but the object as a whole is not mutable, because value is immutable, so saying that this is not mutable is essentially correct, if somewhat misleading. It needs a better error message, but the one that's there isn't totally off.
Comment 2 Haruki Shigemori 2010-12-03 18:44:54 UTC
(In reply to comment #1)
> I've noticed that a lot of errors get printed three times. I have no idea why.
> Regardless, the error message is close to correct. The problem is, of course,
> that value is immutable, not the this pointer, but the object as a whole is not
> mutable, because value is immutable, so saying that this is not mutable is
> essentially correct, if somewhat misleading. It needs a better error message,
> but the one that's there isn't totally off.

Of course, I know that.
I said that the error message has two problems.
I didn't say that it is unkindness.
However, it makes me happy if the message is more better, because the current message has not class and variable name.
Comment 3 nfxjfg 2010-12-03 19:48:37 UTC
Are you saying this is not a rejects-valid?

I fail to see why structs with immutable members shouldn't be allowed to have a postblit function.
Comment 4 Haruki Shigemori 2010-12-04 00:16:09 UTC
(In reply to comment #3)
> Are you saying this is not a rejects-valid?

I think that this is maybe a rejects-invalid correctly.

Error: this is not mutable
Error: this is not mutable
Error: this is not mutable

However, this error message is bad clearly, because it hasn't line numbers and type/variable names.
Of course, this message is strange that it is written by three times.

For example, I think that a following sentence is more better.

filename.d(4): struct ID has not both postblit and immutable member value
 
> I fail to see why structs with immutable members shouldn't be allowed to have a
> postblit function.

Uhmmm... I don't discuss the matter because it is other problem.
But, we need probably discussion of the problem that you said.
Comment 5 Haruki Shigemori 2010-12-04 00:18:22 UTC
- filename.d(4): struct ID has not both postblit and immutable member value
+ filename.d(4): struct ID cannot have both postblit and immutable member value
Comment 6 Jonathan M Davis 2010-12-06 00:21:15 UTC
@nfxjfg

This program shows when init, a constructor, the postblit constructor, and opAssign are used:

import std.stdio;

struct Foo
{
    int a = 7;

    this(int a)
    {
        this.a = a;
        writefln("constructor: %s", this.a);
    }

    this(this)
    {
        writefln("postblit: %s", this.a);
    }

/+
    Foo opAssign(const ref Foo other)
    {
        this.a = other.a;
        writefln("opAssign: %s", this.a);

        return this;
    }
+/
}

void main()
{
    writefln("%s(%s): -------", __FILE__, __LINE__);
    Foo a;
    writefln("%s(%s): -------", __FILE__, __LINE__);
    auto b = Foo(6);
    writefln("%s(%s): -------", __FILE__, __LINE__);
    auto c = a;
    writefln("%s(%s): -------", __FILE__, __LINE__);
    b = c;
    writefln("%s(%s): -------", __FILE__, __LINE__);
}


The output is:

test.d(29): -------
test.d(31): -------
constructor: 6
test.d(33): -------
postblit: 7
test.d(35): -------
postblit: 7
test.d(37): -------


Notice that assignment uses the postblit constructor. If you uncomment opAssign(), then you get this instead:

test.d(29): -------
test.d(31): -------
constructor: 6
test.d(33): -------
postblit: 7
test.d(35): -------
opAssign: 7
test.d(37): -------


Now, assignment uses opAssign(). The problem with having an immutable member variable and a postblit constructor is that you cannot reassign that member variable. That portion of the struct cannot be altered.

If I changed the code to

import std.stdio;

struct Foo
{
    immutable int a;

    this(int a) immutable
    {
        this.a = a;
        writefln("constructor: %s", this.a);
    }

}

void main()
{
    writefln("%s(%s): -------", __FILE__, __LINE__);
    Foo a;
    writefln("%s(%s): -------", __FILE__, __LINE__);
    auto b = Foo(6);
    writefln("%s(%s): -------", __FILE__, __LINE__);
    auto c = a;
    writefln("%s(%s): -------", __FILE__, __LINE__);
    b = c;
    writefln("%s(%s): -------", __FILE__, __LINE__);
}


then it fails to compile, giving the message

test.d(24): Error: variable test.main.b cannot modify immutable


The line b = c is illegal. Now, the line auto c = a is still legal, and you could arguably allow a postblit constructor which didn't alter the immutable member variables, but it does get a bit odd. You could arguably even allow for assignment via opAssign() as long as the immutable member variable isn't altered, but then it's probably not really copying, which would not be good.

So, I suppose that a postblit could be allowed in a struct with an immutable member variable, but you couldn't assign to it or do a deep copy or do anything with it that you'd normally do in a postblit constructor (though whether or not that's a problem is debatable). But the compiler would have to be smart enough to realize that having a postblit constructor didn't allow for assignment for structs with immutable member variables, which may or may not be straightforward, depending on how the compiler decides that it cannot do an assignment to a struct with an immutable member variable.

All in all, copying structs with immutable member variables does tend to get a bit funny. I suppose that a postblit constructor _could_ be allowed, but it is a bit odd. Assignment certainly can't be allowed, and the postblit constructor is assignment of a sort, but it could still work as long as it's only used when constructing a variable. So, I don't know what the correct behavior is.
Comment 7 kennytm 2011-05-01 23:32:49 UTC
*** Issue 5917 has been marked as a duplicate of this issue. ***
Comment 8 kennytm 2011-05-01 23:35:22 UTC
Raising severity to "Critical" as per bug 5745.
Comment 9 SomeDude 2012-05-04 22:48:45 UTC
Reducing criticity to normal, as diagnostic bugs are not critical.
Comment 10 Don 2012-05-09 13:44:40 UTC
(In reply to comment #9)
> Reducing criticity to normal, as diagnostic bugs are not critical.

They're equivalent to compiler segfaults. Worse than any rejects-valid bug. They're nearly always very easy to fix, too. There's no excuse for them.
Comment 11 Denis Shelomovskii 2012-11-03 05:16:46 UTC
Original example compiles fine now but is the issue fixed?

Can somebody familiar with dmd internals check if "%s is not mutable" error message gives line now:
https://github.com/D-Programming-Language/dmd/blob/master/src/expression.c



Also example from comment #6 with uncommented `opAssign` new writes this:

main.d(29): -------
main.d(31): -------
constructor: 6
main.d(33): -------
postblit: 7
main.d(35): -------
opAssign: 7
postblit: 7              <- added line
main.d(37): -------

The line is added correctly and indicates a fixed compiler bug because

    Foo opAssign(const ref Foo other)

returns by value so a temporary is created.
Comment 12 Adrien Pensart 2014-04-15 13:07:53 UTC
If you think this bug is fixed (and i think it is), can we consider closing it ?

- every mutable error is now indicated with its line number
- each error message appears only once
- immutable member construction and assignment semantics are correctly handled

if someone think it's not fixed, is there a new relevant test case ?