IsPositionalObjectRep and IsComponentObjectRep are not consistently set

[fingolfin]

(This is taken from issue #897)

Many representation for positional respectively component representations do now have the IsPositionalObjectRep respectively IsComponentObjectRep filters set. This can lead to surprising behavior and bugs, see e.g. issue #897. According to @markuspf this affects at least these representations in the library (and more in packages):

• IsSyllableAssocWordRep
• IsLetterAssocWordRep
• IsBLetterAssocWordRep
• IsWLetterAssocWordRep
• IsPermGroupGeneralMapping
• IsSubgroupOfWholeGroupByQuotientRep
• IsGenericCharacterTableRep
• IsNullMapMatrix
• IsDefaultGeneralMappingRep
• IsInverseGeneralMappingRep
• IsMemberPcSeriesPermGroup
• IsSlicedPerm
• IsSlicedPermInv
• IsFromTheLeftCollectorRep
• IsGroupClassByListRep
• IsNilpotentLieAutomorphismRep

(Found by filtering AllRepresentations() by not having any of the four Reps above in their IMPL_FLAGS, AllRepresentations() being a function @markuspf added to the library).

I was bitten by this yesterday, as polycyclic's IsFromTheLeftCollectorRep suffers from the same problem (and thus a collector was shown to use 300 bytes of RAM when it really are 300,000 bytes...)

So all in all, it seems that IsPositionalObjectRep serves no useful purpose, other than perhaps to increase some ranks a little bit. It is not well-documented either: entering ?IsPositionalObjectRep gives nothing, though it is mentioned in section 79.11 "Positional Objects".

So, what to do? Some random ideas (without consideration how easy and/or efficient it would be to implement them)

1. Change GAP to automatically add set IsInternalRep, IsPositionalObjectRep, IsComponentObjectRep, IsDataObjectRep for objects based on their TNUMs.
2. Change NewRepresentation to enforce that every newly declared representation sets one of the four representation types
3. Phase out those four types as filters, instead use IS_COMOBJ, IS_POSOBJ, IS_DATOBJ in code that really has to act based on the representation kind.
4. A combination of the above.
5. ... other ideas?

Idea 1 might work by hooking into Objectify resp. SetTypeObj, which already has to look at the object in order to invoke SET_TYPE_POSOBJ or SET_TYPE_COMOBJ. So adding in the correct filters would be doable there. Some caveats: This would change some filter ranks and could thus lead to some indirect breakage (arguably caused by unsafe reliance on certain relative filter ranks, but breakage nevertheless).
Moreover, this could require creating a new type object on the fly (?), again and again, which is inefficient.

Approach 2 would help to avoid that inefficiency, by inspecting the (currently unused, see issue #1042) slots argument to NewRepresentation. Alas, that does not always work, and since the argument has been unused so far, it is quite likely that somebody used it incorrectly, and thus this heuristic will fail :-(.

Approach 3 avoids all that mucking around with filters, and is fast and reliable. However, it would not allow for method dispatch. But then, I think few (if any things) rely on that with the exception perhaps of some very special things like MemoryUsage (a notable exception here is IsFFE and IsInternalRep which is used quite a bit in the library).

We could also combine these, say do approach 1, but instead of changing the type, print an InfoWarning to help us track down incorrect representation declarations and fix them. Or print the warning and change the type. Etc.

[frankluebeck]

The representations IsPositionalObjectRep do occur in the documentation, try for example ?Representation or ?NewRepresentation. It would be good to add some <Index> elements to the documentation.

In view of the documentation I would consider representation declarations which do not imply one of the four mentioned representations a bug. I would vote for a check when a representation is declared and to raise an error, if none of the four base representations is implied.

[stevelinton]

I'd support Frank's proposal. All representations should imply exactly one of the four base representations.

[fingolfin]

In my original issue description, I referred to a library function AllRepresentations, but I can't find any trace of that?!? Anyway, here is how one can compute a list of offending representations:

reps:=FILTERS{Filtered(CATS_AND_REPS,i->INFO_FILTERS[i] in FNUM_REPS)};;
badreps:=Filtered(reps,r->not IS_IMPLIED_BY(IsComponentObjectRep,r) and not IS_IMPLIED_BY(IsPositionalObjectRep,r) and not IS_IMPLIED_BY(IsDataObjectRep,r) and not IS_IMPLIED_BY(IsInternalRep,r));
for r in Set(badreps,NAME_FUNC) do Print("- `",r,"`\n"); od;

The result as of right now, after LoadAllPackages() (but with a few packages missing on my system):

• IsBisetElementByElement (from fr package, see Adjust representations to imply and IsComponentObjectRep gap-packages/fr#59)
• IsBisetElementByPair (from fr package, see Adjust representations to imply and IsComponentObjectRep gap-packages/fr#59)
• IsBLetterAssocWordRep (resolved by PR Fix definition of a few representations #4966)
• IsDefaultGeneralMappingRep (maybe shouldn't be a representation?!)
• IsFromTheLeftCollectorRep (from polycyclic package, see IsFromTheLeftCollectorRep implies IsPositionalObjectRep gap-packages/polycyclic#78)
• IsGenericCharacterTableRep (resolved by PR Fix definition of a few representations #4966)
• IsInverseGeneralMappingRep (maybe shouldn't be a representation?!)
• IsLetterAssocWordRep (resolved by PR Fix definition of a few representations #4966)
• IsMemberPcSeriesPermGroup (probably shouldn't even be a representation?!)
• IsNilpotentLieAutomorphismRep (from sophus package, see IsNilpotentLieAutomorphismRep implies IsComponentObjectRep gap-packages/sophus#11)
• IsPermGroupGeneralMapping (probably shouldn't even be a representation?!)
• IsSubgroupOfWholeGroupByQuotientRep (resolved by PR Fix definition of a few representations #4966)
• IsSyllableAssocWordRep (resolved by PR Fix definition of a few representations #4966)
• IsVectorSpaceTransversalRep (from fining package, see IsVectorSpaceTransversalRep implies IsComponentObjectRep gap-packages/FinInG#32)
• IsWLetterAssocWordRep (resolved by PR Fix definition of a few representations #4966)