Forbid installing new implications to representations using InstallTrueMethod

[ThomasBreuer]

(This pull request addresses a comment in issue #2818.)

Filters of type representation are intended to describe
the internal data of an object.
Thus a logical implication to such a filter makes sense only
if the implying filters are also representations.

As had be noticed by @fingolfin,
there was a now forbidden hidden implication in the library,
from IsHandledByNiceBasis to IsAttributeStoringRep.
Logically, one wants this rather the other way round,
that is, the mechanism relies on stored data and thus should be restricted
to situations where IsAttributeStoringRep is present.
I have now changed this setup accordingly, that is,
IsAttributeStoringRep is no longer implied by IsHandledByNiceBasis.
As a consequence, one Objectify call in the library had to be changed
such that IsAttributeStoringRep gets explicitly set.

(The same might happen also in packages.)

[fingolfin]

Very nice, thank you!

This fails in testpackages -- not a surprise, we already know of several packages affected by this:

• Incorrect implications from IsMapping to IsNearRingElement, resp. IsGeneralMapping to IsNearAdditiveElementWithInverse gap-packages/sonata#12 actually, the implication problem there does not involve representations, so it does not concern this PR here
• There should be no implications to IsEnumerableSemigroupRep semigroups/Semigroups#556 -- fix pending
• Bad implication to IsAttributeStoringRep gap-packages/numericalsgps#25 -- merged and fix released in v1.1.11 (2019-03-28)
• Remove bad implication IsLinearCodeRep => IsCodeDefaultRep gap-packages/guava#43
• Change definitions of various categories gap-packages/hecke#6 -- merged and fix released in v1.5.2
• Bad implication to a representation semigroups/Semigroups#862

As such, we'll have to wait till all packages are updated before merging this.

[olexandr-konovalov]

I have added this PR to the list of PR blocked by packages (#2984) and added a "Do not merge label" until we will have package updates in place. At the moment, with this PR it is not even possible to load all packages because of an errors (a link to the Jenkins test available only from St Andrews is https://gap-ci.cs.st-andrews.ac.uk/view/Pull%20requests/job/GAP-pull-request-quicktest/48/).

[ThomasBreuer]

@zickgraf Sorry.

I am just not sure how we should proceed.

If we regard your idea as useful that attribute storing for certain objects can be forced via an implication of filters that define these objects then the current pull request makes no sense in the current form.
If we still think that concluding IsAttributeStoringRep or other representations from some combination of filters (as mentioned in the initial comment) is a bad idea then some check as proposed in the pull request should be made.

At the moment it is not clear to me what exactly we want to achieve.

[zickgraf]

Some thoughts:

Maybe it makes sense to distinguish between "definitional implications" (i.e. what we currently do in CAP) and "acquired implications" (i.e. additional implications triggered via InstallTrueMethod). I think the former kind makes sense whereas the latter does not. Implementing the relaxation proposed above would respect this, i.e. allow the former but prevent the latter. And the problem which triggered this PR (#2818) falls into the later case, i.e. trigger an error, if I understand things correctly. So the PR would still achieve what it was supposed to.

[fingolfin]

@ThomasBreuer and me discussed this on Gather. I support the idea of using the logic that an implication "A => B" where B implies a rep is OK when the same rep is already part of "A". He'll work on implementing something in this vein.

[ThomasBreuer]

@zickgraf I have updated the pull request according to our discussion. Could you check whether CAP does no longer have problems with it?

[zickgraf]

@ThomasBreuer The tests of CAP, LinearAlgebraForCAP etc. now pass with the latest commit, thanks!

One question out of curiosity: This PR (both the original and the updated version) allows installing implications of formerly unrelated representations:

gap> DeclareRepresentation( "test1", IsComponentObjectRep );
gap> DeclareRepresentation( "test2", IsComponentObjectRep );
gap> InstallTrueMethod( test2, test1 );

The documentation says

The representations to which an object belongs should form a chain and either two representations are disjoint or one is contained in the other.

Thus, I cannot see a setting where InstallTrueMethod could be used for installing new implications to representations at all, except some weird cases where one wants to declare a representation without knowing its parent yet. So maybe this PR can even be strengthened to disallow any new implications to representations? I tried this by commenting the if condition right before the error

            if not ( INFO_FILTERS[j] = FNUM_REP_KERN or
                     INFO_FILTERS[j] = FNUM_REP ) then

and the only immediate issue I found was the call to InstallTrueMethod in DeclareRepresentationKernel, which can easily be avoided by using InstallTrueMethodNewFilter as in DeclareRepresentation (I'm not sure if this has other implications).

[ThomasBreuer]

@zickgraf You are right. Implications between representations should be handled in the declarations of the representations, not via InstallTrueMethod. I am going to update the pull request accordingly (although this new aspect is not covered by the title of the pull request).

[zickgraf]

I am going to update the pull request accordingly

Just checked it out and it seems to work perfectly :-)

although this new aspect is not covered by the title of the pull request

I think the title is misleading anyway after all the changes made. I suggest something like the following:

Forbid installing new implications to representations using InstallTrueMethod

[ThomasBreuer]

@zickgraf Thanks for the confirmation. I have changed the title. Now the only problem is one CI test failure concerning a problem with testexpect, see issue #5309 for details.
This problem seems to be independent of the changes proposed here, thus I think that this pull request could be merged if there would be an approving review.

[fingolfin]

Thanks, looks good to me!