WIP Stop system setters storing attribute values in mutable objects

[stevelinton]

Please use the following template to submit a pull request, filling
in at least the "Text for release notes" and/or "Further details".
Thank You!

Description

The system setters used for objects in IsAttributeStoringRep did not check the objects for mutability.
This leads to unsafe behaviour where attribute values were stored in mutable objects. Mutable objects in IsAttributeStoringRep are thankfully rare, but they are not impossible or unreasonable, especially for objects that may be made immutable by MakeImmutable in the future (whereupon they should start storing attribute values).

This addresses #1371 and questions that arose in #3560

At this stage this PR is just a proof of concept to get comment an wider testing

Text for release notes

The values of computed Attributes will no longer be stored automatically in mutable attribute-storing
objects, since they may become out of date as the object mutates. If you wish to store such a value and know that it is safe to do so, then you can address this by defining a custom setter method for your object.

Further details

Still needed before merge:

• documentation
• tests

One of the diffs is a bit bigger than it could be because I clang-formatted a bit too much code.

Checklist for pull request reviewers

[coveralls]

Coverage remained the same at 84.296% when pulling 9bca969 on stevelinton:mut-attrs into d0933a5 on gap-system:master.

[ThomasBreuer]

@stevelinton just for curiosity:
In the case of properties, currently automatically setting a computed value is avoided for mutable objects by a mutability check in the kernel function DoProperty.
This has the effect that calling IsSomething will return the value but will not set the filters IsSomething and HasIsSomething to the computed value and true, respectively; however, explicitly calling SetIsSomething will still set the two filters, also for mutable objects.

In the case of attributes, the change you propose seems to switch off also the effect of explicitly calling SetSomething. Is this intended?

[stevelinton]

@ThomasBreuer That's a tricky question. What I wanted to do was to allow for specially installed Setter methods, so that if you knew that the allowed mutations of your object could not change the value of a particular attribute, you could install a setter for it. Hence I put the test in the system setter.

We certainly could (and perhaps for consistency with properties should) do the test in DoAttribute and simply not call any setter on mutable objects. This would then ignore custom setters as well as the system one, but allow any setter to be called directly.

I don't have a strong feeling as to which is nicer. I don't see a sensible compromise option although that doesn't mean there isn't one.

[ThomasBreuer]

@stevelinton Thanks for your reply.
My point of view is that properties and attributes should behave as similar as possible.

1. Calling a property (such as IsFinite) yields a value.
2. The place for storing this value is the type of the object.
3. One can set the value (without calling the property) in the type by calling the property setter (SetIsFinite), which marks the value as stored, via the tester flag (HasIsFinite). This holds for both mutable and immutable objects.
4. For immutable objects, the call of the property automatically triggers a call of the property setter, with the effect that only the first call of IsFinite with an immutable object causes a computation, and any subsequent call of IsFinite just fetches the stored value.

This is the current behaviour of properties.
Attributes are logically different because there is in general no natural place for storing their values.
The representation IsAttributeStoringRep is designed to provide such a place.
Thus the behaviour of objects in IsAttributeStoringRep w.r.t. attributes could be (but currently is not) analogous to their behaviour w.r.t. properties:

1. Calling an attribute (such as Size) yields a value.
2. The place for storing this value is known to the handling mechanism.
3. One can set the value (without calling the attribute) in the object by calling the attribute setter (SetSize), which marks the value as stored, via the tester flag (HasSize). This holds for both mutable and immutable objects.
4. For immutable objects, the call of the attribute automatically triggers a call of the attribute setter, with the effect that only the first call of Size with an immutable object causes a computation, and any subsequent call of Size just fetches the stored value.

Currently 4. does not hold in the sense that attribute setters are called automatically also for mutable objects as soon as the attribute call returns its value --this is the bug described in #1371 .
The proposed change would disable 3. for attributes of mutable objects in IsAttributeStoringRep.

However, the above statements describe only the "default behaviour" of attributes.
Attribute setters are operations, and attribute values are in fact set by attribute setter methods.
One can argue that one should be able to store values of certain attributes in certain mutable objects, because one is able to control the situations where the object gets changed.
Part of this can be implemented by individual attribute setter methods.
Then the call of an attribute for an object should always trigger a call of the attribute setter operation, independent of the mutability of the object, just the default setter method for IsAttributeStoringRep should do nothing in the case of a mutable object and in this situation.
One could still get the behaviour 3. for mutable objects if the default setter method could distinguish whether the setter operation was called explicitly or as a side effect of the attribute call (for example via an option?).

However, the situation is different for property setters.
They are also operations, but their methods are not called; thus it has no effect to install individual methods for property setters.
Note that GAP has already a special treatment of the property IsSSortedList for mutable lists in IsPlistRep whose entries are immutable, due to GAP kernel support for this special situation.
I do not see how we can implement such a special treatment for other properties, on the GAP level.
That is, I do not know how to achieve a consistent/analogous behaviour of properties and attributes in this respect.

[ThomasBreuer]

@stevelinton I am not sure where to put my comments, now we have issue #1371 and pull requests #3560, #3588, #3592.

As I wrote in a comment to #3588, I think that a proper handling of attributes for mutable objects in IsAttributeStoringRep is possible, in the sense that

• calling the attribute does by default store the value only for immutable objects,
• calling the attribute setter explicitly stores the value,
• one can install special attribute setter methods which decide whether they store the value.

(One possible implementation would distinguish between unconditional calls of the setter operation (in the case of an explicit setter call) and conditional calls (just as a side effect of calling the attribute).
For example, we could add a third argument for the attribute setter, call the setter with false in the conditional case and with true in the unconditional case, install a two-argument method for the setter that dispatches to the unconditional variant, and expect all individual setter methods to have three arguments --the current system setter would be replaced by a function that does nothing for mutable objects in the conditional situation.
Does this make sense?)

I think the real problem is that the same approach will not work for properties, in the sense that we cannot work with individual property setter methods.

[stevelinton]

@ThomasBreuer My feeling is that we can implement the first, and either one of your second and third bullet points:

calling the attribute setter explicitly stores the value,

one can install special attribute setter methods which decide whether they store the value.

quite easily and with only very local changes. Essentially the two PRs demonstrate the two choices. I agree it's possible to have all three, for instance as you describe, but I don't see a way to do it which is simple and doesn't involve very widespread changes. So the question is then, is it worth it? and if not, which compromise to we take?

[ThomasBreuer]

@stevelinton o.k., if I have to choose between the two proposed solutions, I am in favour of #3592.
It is the one which

• provides a bugfix (do not store attribute values in mutable objects by default),
• yields consistent behaviour for properties and attributes (one can store values by calling the setter explicitly, but there is no possibility to install special setter methods that are called automatically), and
• allows us to turn operations such as IsDiagonalMat(rix) and DiagonalOfMat(rix) safely into properties and attributes, respectively.

[stevelinton]

Superceded by #3592