Objectify: warn if Is{Component,Positional}ObjectRep absent

[fingolfin]

This works towards resolving #1043: now Objectify warns if IsPositionalObjectRep resp. IsComponentObjectRep is not set for an object that should have one of them; and it also gives an error if both are set / "the wrong one" is set (i.e. IsPositionalObjectRep for a T_COMOBJ or IsComponentObjectRep for a T_POSOBJ).

I've eliminated all the warnings triggered by loading GAP and running tst/testinstall.g, and submitted gap-packages/fr#50 to fix one violation in a package (I've already fixed a bunch of others in packages in the past couple years). I am guessing there may still be a couple more, so I am reluctant to turn the warnings into an error just now... at the very least, before we attempt such a thing, the test suites of all packages should be run against a GAP with that error activated.

Also, before merging this PR we should check how this extra test affects performance: Objectify is a bit of a bottleneck, and we don't want to make it worse than it already is...

[fingolfin]

Hmm, thinking about it, I am not even sure why to warn -- instead we could just add that flag? Perhaps for performance reasons: a missing flag that has to be added in requires us to create a new sub-type...

[fingolfin]

Maybe disable this by default, but have it enabled e.g. in most test suites (as long as the use START_TEST)?

Suggested change

	#Assert(0, IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsPositionalObjectRep)));
	Assert(0, not IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsComponentObjectRep)));
	#Assert(1, IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsPositionalObjectRep)));
	Assert(1, not IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsComponentObjectRep)));

[fingolfin]

Suggested change

	#Assert(0, IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsComponentObjectRep)));
	Assert(0, not IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsPositionalObjectRep)));
	#Assert(1, IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsComponentObjectRep)));
	Assert(1, not IS_SUBSET_FLAGS(flags, FLAGS_FILTER(IsPositionalObjectRep)));

[fingolfin]

I'd like to get this out of my system one way or another:

1. we could just change this PR to add the missing filter (any reason against that)
2. we could just give up and close this
3. we could try to get all packages that are distributed and violated this fixed (but of course there are also many not distributed packages)

Perhaps @ThomasBreuer has some insights on this?

[ThomasBreuer]

When Objectify( type, r ) is called with a record r then SET_TYPE_COMOBJ is called, and the result admits component access.
When Objectify( type, l ) is called with a plain list l then SET_TYPE_POSOBJ is called, and the result admits access to positions.
This is independent of the type type, and it fits to the documentation of Objectify.
(In other words, apparently IsComponentObjectRep is not needed to guarantee component access, and IsPositionalObjectRep is not needed to guarantee position access.)

In this sense, it is safe to set automatically IsComponentObjectRep in the first case and IsPositionalObjectRep in the second case, and to signal an error if this yields an object in IsComponentObjectRep and IsPositionalObjectRep.

Perhaps Objectify should check that IsInternalRep or IsDataObjectRep is not set in the given type.

[ThomasBreuer]

As far as I see, this change is a bugfix.
(And NormalizedElementOfMagmaRingModuloRelations is a misleading name.)

[fingolfin]

I wish I had made a note what the error was / why this change is the correct fix sigh

[ThomasBreuer]

NormalizedElementOfMagmaRingModuloRelations is called in the GAP library only in lib/mgmring.gi. In all cases, Objectify is called immediately afterwards with the result. For the old code, this means that the object had already the desired type before the Objectify call, and gets objectified again with the same type.

In this sense, I think the library code does not produce wrong results. Nevertheless, the old code contradicts the definition of NormalizedElementOfMagmaRingModuloRelations. And there may be code outside lib/mgmring.gi which wants to use NormalizedElementOfMagmaRingModuloRelations. If the package tests do not show an example where the proposed change yields a different result then we should just apply the proposed fix and add a test, perhaps even as a manual example.

gap> R:= GroupRing( Rationals, SymmetricGroup( 3 ) );;
gap> Fam:= FamilyObj( Zero( R ) );;
gap> x:= Sum( GeneratorsOfAlgebra( R ) );;
gap> descr:= [ 0, CoefficientsAndMagmaElements( x ) ];;
gap> NormalizedElementOfMagmaRingModuloRelations( Fam, descr ) = descr;
true
gap> R:= FreeLieAlgebra( Rationals, 2 );;
gap> Fam:= FamilyObj( Zero( R ) );;
gap> x:= Sum( GeneratorsOfAlgebra( R ) );;
gap> descr:= [ 0, CoefficientsAndMagmaElements( x ) ];;
gap> NormalizedElementOfMagmaRingModuloRelations( Fam, descr ) = descr;
true

(The first true comes from a correct NormalizedElementOfMagmaRingModuloRelations method, the second true will occur only after the fix.)

[ThomasBreuer]

I'd like to get this out of my system one way or another:

1. we could just change this PR to add the missing filter (any reason against that)
2. we could just give up and close this
3. we could try to get all packages that are distributed and violated this fixed (but of course there are also many not distributed packages)

I suggest variant 1.:
In Objectify, set IsComponentObjectRep if the argument in question is a record, and set IsPositionalObjectRep if the argument in question is a plain list.

And add an example for the NormalizedElementOfMagmaRingModuloRelations fix, for example the one shown above.