http://d.puremagic.com/issues/show_bug.cgi?id=5058

           Summary: invariant() should not be called before opAssign()
           Product: D
           Version: unspecified
          Platform: Other
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: jmdavisProg@gmx.com


--- Comment #0 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-10-15 10:03:11 PDT ---
As it stands, the invariant is called before every public function is called and after every public function is called. On the whole, this is correct behavior. However, there is at least one case where this is undesirable: opAssign().

Because it is quite possible to have struct which violates its invariants (thanks to the whole init vs default constructor fiasco), it's quite easy to have a struct which was default-initialized which you're trying to assign to and which violates the invariant. So, the invariant fails and an AssertError is thrown. Since, opAssign() is theoretically supposed to completely replace the state of the object, there's no need for the object to be in a correct state prior to opAssign() being called. It obviously needs to be in a correct state afterwards, and if the invariant is run afterwards, it will catch if opAssign() did not correctly replace the state of the object such that it no longer violates its invariant. However, there's no need to call the invariant before opAssign() is called. The state prior to opAssign() is irrelevant, and calling the invariant just makes it really hard to have invariants in a struct where you can't make it so that the init value for that struct doesn't violate its invariant.

So, _please_ make it so that the invariant is _not_ called prior to opAssign()
being called.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058


Don <clugdbug@yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |clugdbug@yahoo.com.au


--- Comment #1 from Don <clugdbug@yahoo.com.au> 2010-10-27 02:58:32 PDT ---
I don't think this is a bug.
I think the real bug is described in this paragraph:

"Because it is quite possible to have struct which violates its invariants (thanks to the whole init vs default constructor fiasco), it's quite easy to have a struct which was default-initialized which you're trying to assign to and which violates the invariant."

Which is closely related to bug 519. It should NOT be possible to have a struct which violates its invariant.

For all instantiated structs which have an invariant, the compiler should insert a check that .init satisfies the invariant. This only has to be done once per struct (it doesn't need to be checked for each instance of the struct). If the invariant is CTFEable, it could even be checked at compile time.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058



--- Comment #2 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-10-27 10:05:54 PDT ---
The problem is that that then seriously reduces the useability of struct invariants. At least if init violates the invariant, then the invariant will fail if you used init rather than properly initializing a variable of that struct type.

Regardless, I don't see why it would matter what the state of the object is prior to opAssign() being called. That's like caring whether the invariant is true prior to the constructor call.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058


Peter Alexander <peter.alexander.au@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |peter.alexander.au@gmail.co
                   |                            |m


--- Comment #3 from Peter Alexander <peter.alexander.au@gmail.com> 2010-10-27 12:02:23 PDT ---
(In reply to comment #2)
> Regardless, I don't see why it would matter what the state of the object is prior to opAssign() being called. That's like caring whether the invariant is true prior to the constructor call.

It matters if the object being assigned to have resources that it needs to free (with the invariant possibly being that a pointer to the resource is non-null).

I agree 100% with Don here: .init should satisfy the invariant, which makes this bug into a non-bug (unless you can think of other valid situations where the invariant is broken prior to an opAssign call?)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058



--- Comment #4 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-10-27 15:50:46 PDT ---
All it takes is assigning to a public member variable or a reference to private member data, and you can invalidate an invariant. Granted, having an invariant with a type where you can do that, isn't necessarily the best idea, but it's quite possible to have a type where you have an invariant and it's invalid before opAssign() (or any public function) is called, and, unlike most public functions, there's no need for the invariant to be true before opAssign() is called.

Of course, what would really help here would be proper default constructors for structs, but no one has been able to figure that one out yet it seems.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058


Steven Schveighoffer <schveiguy@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schveiguy@yahoo.com


--- Comment #5 from Steven Schveighoffer <schveiguy@yahoo.com> 2010-10-27 19:37:33 PDT ---
While I agree technically, the invariant can be required to pass T.init, this does not serve the purpose -- it makes invariants more annoying to write.

Would it be feasible to have the invariant check function first do a pre-check to see if the value is T.init, and if so, pass?  This way, the user is not bothered with the init case, and it should solve the opAssign issue as well.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058



--- Comment #6 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-10-27 22:13:01 PDT ---
The problem with that is what if you don't _want_ T.init to pass the invariant? It seems to me that if T.init doesn't pass the invariant, then really, T.init shouldn't be used or should at least be assigned to before it's used. For instance, SysTime in the datetime module that I've been working on has a TimeZone class as a member. It's not possible to properly initialize it at compile time, so you're stuck with SysTime.init with a null TimeZone - which should _never_ happen. Any code which uses SysTime.init _should_ fail the invariant that the TimeZone is non-null. The alternative is segmentation faults when the null TimeZone is used. Granted, making T.init pass T's invariant would solve the opAssign() problem, but I think that it would be bad in the general case. I also think that it would be bad if T.init had to pass the invariant, since then struct invariants become border-line useless in many cases. Now, making it so that the invariant for opAssign() is run normally but not in the case where the value is T.init, then that could work, but I don't like the idea of making T.init pass the invariant in the general case.

If default struct constructors were possible, then the problem with SysTime.init and inits for other structs like it could be solved, but we're stuck for now it seems.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058



--- Comment #7 from Peter Alexander <peter.alexander.au@gmail.com> 2010-10-27 23:52:41 PDT ---
(In reply to comment #4)
> All it takes is assigning to a public member variable or a reference to private member data, and you can invalidate an invariant. Granted, having an invariant with a type where you can do that, isn't necessarily the best idea, but it's quite possible to have a type where you have an invariant and it's invalid before opAssign() (or any public function) is called, and, unlike most public functions, there's no need for the invariant to be true before opAssign() is called.

1. The whole point of invariant is to ensure that invariants are always true. If your class allows people to invalidate the invariant by assigning to public member variables then the class is outright broken and you need to fix it. In this case the invariant will fire (before opAssign or any other function), which is correct and desired behaviour.

2. As I explained above, there is a need for the invariant to be true before opAssign in the case where objects have resources to release prior to building up the new value.


> Of course, what would really help here would be proper default constructors for structs, but no one has been able to figure that one out yet it seems.

I agree with this.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058



--- Comment #8 from Steven Schveighoffer <schveiguy@yahoo.com> 2010-11-01 12:47:25 PDT ---
I look at invariants differently than you do I guess.  To me, an invariant is a self-checking mechanism that says "Every public function is going to leave this item in a sane state, and therefore, every public function should expect this to be in a sane state".  It should not be possible for a user who is using a struct to break an invariant unless they violate the type system.

To me, T.init should always pass the invariant because the user is allowed to declare:

T t;

And this is guaranteed by the language.  Therefore, it's always part of the public interface.

In other words, invariants are more to protect the user against the struct misbehaving, not to protect the struct against the user misbehaving.

For example, a poorly constructed invariant:

struct S
{
  public int i;
  invariant()
  { assert(i == 0); }
}

This looks to me like what you are doing -- ensuring the user has not mucked with your struct data.  What an invariant really should do is ensure that the struct has not mucked with the struct data.  If the user does it, they do so at their own risk.

It would be like expecting the invariant of a class to assert the reference is not null first.

And really, what is the difference between a segfault and an assert error? Both should halt execution, and both should print out a valid stack trace.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5058



--- Comment #9 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-11-01 13:10:01 PDT ---
Generally, I would agree with you. However, it's quite easy to have a struct which has things which _should_ be checked in the invariant but which aren't true with T.init - either because it's stuff (like class objects) which cannot be created with CTFE or it requires that stuff be done at runtime to set up properly. The real problem there is that T.init doesn't go through any constructor. Personally, I hate the fact that T.init exists as it does for structs, but apparently no one has come up with a good enough solution to allow proper default constructors or to disallow default construction entirely for a struct (both of which would ideally be possible). So, for the most part, I'm not trying to check that the user is using the struct properly, but I _would_ like to be able to make the invariant fail if they use T.init for a struct which should never have T.init used.

As for segfaults resulting in a stack trace, I've often seen them not result in stack traces, but I don't know if that's because of the segfault being in C code rather than D code or because it was an older version of dmd. Regardless, I'd prefer to be able to have the invariant complain as soon as they try to use the struct rather than later on when they happen to use one of its functions which would result in a segfault.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------