Add Info statement triggered when re-assigning already assigned attributes and the info level InfoAttributes is at least 3

[ChrisJefferson]

An occasional source of subtle GAP bugs is that assignments to attributes are silently ignored, after they are assigned:

gap> g := Group(());
gap> SetTransitivity(g, 4);
gap> Transitivity(g);
4
gap> SetTransitivity(g, 3);
gap> Transitivity(g);
4

Now, for properties this is checked always, and for specifically Size this is checked when the assertionlevel is high. However, it isn't checked in general.

This PR adds checking when the AssertionLevel is 2. I don't add it in general because it could be the case someone assigns an attribute to a value which is the same, but where = is not implemented, or extremely slow.

If anyone is interested, I has a look at when attributes are assigned the same value (as this could be considered inefficient). It turns out it happens quite a bit for what seem to me to be reasonable reasons (sometimes the same simple property, like IsTrivial, can be assigned for a number of different reasons. It would be more expensive to check if it was already set).

[ChrisJefferson]

I'd be interested in seeing this run over the packages, to see if anything fails.

[codecov]

Codecov Report

Merging #3628 into master will decrease coverage by 22.99%.
The diff coverage is 45.71%.

@@             Coverage Diff             @@
##           master    #3628       +/-   ##
===========================================
- Coverage   84.59%   61.59%      -23%     
===========================================
  Files         698      637       -61     
  Lines      345602   288240    -57362     
===========================================
- Hits       292362   177553   -114809     
- Misses      53240   110687    +57447

Impacted Files	Coverage Δ
lib/coll.gi	61.49% <0%> (-35.17%)	⬇️
src/opers.c	84.01% <100%> (-11.58%)	⬇️
src/c_oper1.c	77.56% <43.93%> (-8.97%)	⬇️
src/modules.h	0% <0%> (-100%)	⬇️
src/fibhash.h	0% <0%> (-100%)	⬇️
src/syntaxtree.c	5.44% <0%> (-90.72%)	⬇️
lib/ctblmoli.gi	6.34% <0%> (-90.21%)	⬇️
src/trans.cc	10.37% <0%> (-89.43%)	⬇️
lib/meatauto.gi	5.99% <0%> (-89.32%)	⬇️
lib/nilpquot.gi	11.11% <0%> (-88.89%)	⬇️
... and 466 more

[ChrisJefferson]

I'm looking at the instances where this is failing (there are a bunch of such cases, I am looking into special casing, or removing, them.

[fingolfin]

I like the idea, but having it on assertion level 2 might be too aggressive, as test files using START_TEST are on that assertion level, and so it'd automatically affect all packages. Of course if we can find and fix all problematic tests in all packages, and get those packages to release new versions with those changes, we could do it...

But on level 2, this definitely shouldn't be merged before GAP 4.11 is branched

[fingolfin]

I wouldn't print the old and new values: doing so can lead to errors itself; and it can also simply flood the screen, or cause all kinds of other problems. Inspecting these values if desired is what we have the break loop for. I'd instead give the relevant values suggestive names and indicate them in the error message, e.g. like so:

Suggested change

	ErrorNoReturn(name, " of ", obj, " already set to ", obj!.(name), ", cannot be changed to ", val);
	ErrorNoReturn("Attribute ", name, " of <obj> already set to <oldval>, cannot be changed to <val>);

Then of course a local variable oldval should be added and set to obj!.(name).

(I'd also add the "Attribute ", lead to the message, to match the other message)

[ChrisJefferson]

This certainly shouldn't go in 4.11.

I'm currently further investigating when this is being hit -- in some cases (like ParentAttr), we explictly say in the documentation it can be set multiple times, and late calls will be ignored, and that then happens in several places.

I'm thinking this might have to be weakened to an Info level, at least initially.

[coveralls]

Coverage decreased (-0.01%) to 84.424% when pulling 504c55b on ChrisJefferson:attribute-check into 470098f on gap-system:master.

[fingolfin]

Note that checking for equality can potentially be very expensive or even run into an infinite loop. So we should be careful to only check this if we are going to print a message anyway!

I.e., this function should probably at the very start check if the info level is right, and if not, return immediately.

[ChrisJefferson]

Yes, this is a serious problem, which I will fix.

[fingolfin]

Of course we could also just check for identity, using IsIdenticalObj, but then there might be more false warnings... hm

[fingolfin]

I am somewhat wary about printing obj as well as the old/new value here. This can lead to pages of output, or trigger errors of its own, or even recursively run into this very same info message.

Which in turn makes me wonder if this should be an info message at all; perhaps it should be a call to Error instead (only called if the assertion level is higher than some threshold, and/or controlled by info levels as it is right now). Because thinking about how I'd deal with my code triggering these warning, the first thing I'd do is to edit this function to replace Info by Error, so that I get a backtrace to find out where the issue actually happened; in contrast, in my experience the printed object or value usually won't be helpful for me.

I guess in the end, it depends a bit on "where do we want to go with this"? Is our goal to stop people from setting non-mutable attributes to new, different values? Then wouldn't an Error (only at high enough level) be the way to go?
This reminds me of my idea of adding a "strict" mode in which all kinds of checks are stricter (see #1191)...

Anyway: please don't get me wrong, I don't want to hold back this PR over some bike shedding, and in the end, I am fine with this PR as it is as well (well, with the performance concern above addressed); I just wonder what the rationale for Info messages vs. Errors is, as naively, I'd expect the Error to be more useful -- but unlike you, I have not yet actually tried this PR, so my naive thoughts here might be totally offbase!

[ChrisJefferson]

I originally used Error in this PR, but there are lots of places where we do try to change attributes to different values. There are places (Like ?SetParent) where we document that trying to assign a different value will be ignored, and it is done in a bunch of places, so we can't just forbid it. Therefore, it can't really be put at any (reasonable) assertion level, as it is valid and (reasonably) common behaviour to do these reassignments.

However, there are cases where looking at this uncovers bugs (I already reported one, which was fixed), and we could consider moving towards forbidding / reducing how many occur eventually.

I'd also prefer an error, but I'd have to make up a new kind of thing (like a Strict mode as you mentioned), so I thought an Info was better than nothing, and at least means we know in future where we have to "hook" to get more useful information about.

[fingolfin]

BTW, I actually thought that we do document that calling an attribute setter again always has no effect (which of course is contradicted by your leading example). I can't find this in the manual now, though, but I thought that e.g. HPC-GAP relies on this principle in some parts -- but then perhaps this is simply different in HPC-GAP?

Hmmm.. but actually, I cannot reproduce your example at all. I get this:

gap> g := Group(());
Group(())
gap> SetTransitivity(g, 4);
gap> Transitivity(g);
4
gap> SetTransitivity(g, 3);
gap> Transitivity(g);
4

which is in line with what I thought was happening.

Now I wonder why you see something different?

[fingolfin]

@ChrisJefferson can you explain the discrepancy between the leading example in this PR versus what I observe instead on master?

[ChrisJefferson]

The example at the top was a terrible typo, which went against the whole point :)

There are some setters which document you can try reassigning and it will be ignored (SetParent). Some setters have an explicit check already to see if the user tries to set the value to something different and error if that happens (SetSize). Also properties never let you set them the other way (but you can assign the same value multiple times):

gap> g := Group(());                                                  
Group(())
gap> SetIsAbelian(g, true);
gap> SetIsAbelian(g, true);
gap> SetIsAbelian(g, false);
Error, property is already set the other way
not in any function at *stdin*:8
type 'quit;' to quit to outer loop

[fingolfin]

Note that Parent is not an attribute, so SetParent is not a good example.

[fingolfin]

Oh, and one more thing: I believe that in HPC-GAP, we kind of rely on being allowed to set an attribute twice, in case two threads race for computing the value of an attribute. Of course that should mean that the threads end up setting the attribute to "equal" values; but that also means that for HPC-GAP we'd have to be careful about turning these new checks on by default, as we'd then add those (possibly expensive) equality tests. Well, unless we can come up with a way to distinguish these "N threads raced for a computation" cases from others?

[ChrisJefferson]

Just to say (here at the bottom), my personal prefered long-term goal for attributes is:

1. Users can always assign an attribute the same value multiple times
2. When the AssertionLevel is < 2, assigning an attribute a different value is silently ignored (as present)
3. When the AssertionLevel is >= 2, then assigning an attribute a different value causes an Error (this would be new).

But, it turns out we are further from (3) than I thought, and furthermore even document the current behaviour in some places.

[fingolfin]

I think investigating these is worthwhile. I believe I spotted already at least one actual bug using this PR. I'll report tomorrow, after some sleep :)

[fingolfin]

Could be merged, though perhaps also adjust SetDimension? See comments on the PR.

[fingolfin]

Looks good to me!