AddSet, UniteSet, etc. could give more helpful error message if the list argument is not mutable

[ignatsoroko]

Observed behaviour

gap> L:=AsSet([]);
[  ]
gap> AddSet(L,1);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `AddSet' on 2 arguments at /proc/cygdrive/C/gap-4.10.0-x86_64/lib/methsel2.g:250 called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at *stdin*:24
type 'quit;' to quit to outer loop
brk> quit;
gap> L=[];
true

Copy and paste GAP banner (to tell us about your setup)

 ┌───────┐   GAP 4.10.0 of 01-Nov-2018
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-unknown-cygwin-default64
 Configuration:  gmp 6.1.0, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.1, Alnuth 3.1.0, AtlasRep 1.5.1, AutoDoc 2018.09.20, AutPGrp 1.10, Browse 1.8.8, CRISP 1.4.4,
             Cryst 4.1.18, CrystCat 1.1.8, CTblLib 1.2.2, FactInt 1.6.2, FGA 1.4.0, GAPDoc 1.6.2, IO 4.5.4,
             IRREDSOL 1.4, LAGUNA 3.9.0, Polenta 1.3.8, Polycyclic 2.14, PrimGrp 3.3.2, RadiRoot 2.8,
             ResClasses 4.7.1, SmallGrp 1.3, Sophus 1.24, SpinSym 1.5, TomLib 1.2.7, TransGrp 2.0.4, utils 0.59
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'

[stevelinton]

The error message could be better, but this is correct behaviour. AsSet is documented as returning an Immutable result. Since AddSet mutates its first argument, that argument must be mutable and there is no method for adding something to an immutable set.

[fingolfin]

I think AddSet, RemoveSet, UniteSet etc. etc. should explicitly check for the first argument being immutable, and then print an appropriate error message.

[ChrisJefferson]

Would it be reasonable to extend method selection to say things like "The first argument must not be immutable", if the first argument always requires immutable, and the object isn't immutable? Then we wouldn't need to special-case certain methods. I haven't thought carefully about how reasonable this would be.

[stevelinton]

@ChrisJefferson We could attempt to give better error messages for all no method found calls based on the declaration(s) of the Operations. So if every declaration requires argument n to have IsFoo and somewhat calls it with an argument n without IsFoo then the error message could report it. This would catch this case. InstallOtherMethod is a bit of a problem, and we would have to be confident that Operation declarations were actually appropriate.

As an alternative, we could do the same thing based on installed methods -- if every method requires IsFoo for argument n (except perhaps specially flagged ones) then we can use that as the basis for a better error message.

[fingolfin]

The problem with such a general approach is that it only really works if the involved filters are really "simple", like e.g. just IsMutable. For more complex filters, it quickly becomes unreadable for human beings.

Perhaps a general solution would be useful, but in this case, I think we should perhaps start with a much simpler solution -- e.g. installing methods for those operations that apply to arbitrary lists, and which check for mutability, and for immutable inputs print the desired error (and otherwise do TryNextMethod())

That should be fairly easy to implement, too.

[fingolfin]

Thought about it some more; one way to make this generic along the lines @stevelinton suggests would be to introduce a function which can "describe" arbitrary filters. Here is some sample code which shows a way this could be implemented.

filterDesc:=[];

# format of entries:  if a filter with flags [f1, f2, f3] gets a description,
# then we add an entry to  filterDesc[i]  where i ranges over f1,f2,f3  of the form  [ [f1,f2, f3], desc ]
# Then 

AddFilterDesc := function(filter, desc)
    local tf, n, i;
    tf := TRUES_FLAGS(FLAGS_FILTER(filter));
    Assert(0, IsSet(tf));
    for n in tf do
        if IsBound(filterDesc[n]) then
            i := PositionProperty(filterDesc[n], x -> x[1] = tf);
            if i <> fail then
                Error("a description for filter ", filter, " has already been set");
            fi;
        else
            filterDesc[n] := [];
        fi;
        AddSet(filterDesc[n], [tf, desc]);
    od;
end;

AddPropertyDesc := function(prop, desc)
    Assert(0, IsProperty(prop));
    AddFilterDesc(prop, desc);
    AddFilterDesc(Tester(prop), Concatenation("know if it is ", desc));
end;

# TODO: extend the following; possibly also extend DeclareCategory, DeclareFilter,
# DeclareProperty, ... to take this kind of description as an extra argument
AddFilterDesc(IsList, "a list");
AddFilterDesc(IsMutable, "mutable");
AddPropertyDesc(IsFinite, "finite");


# turn a filter into a list of strings...
DescribeFilter:=function(filter)
    local tf, desc, n, candidates;
    tf := Set(TRUES_FLAGS(FLAGS_FILTER(filter)));
    desc := [];
    while Length(tf) > 0 do
        n := Last(tf);
        if IsBound(filterDesc[n]) then
            candidates := Filtered(filterDesc[n], x -> IsSubset(tf, x[1]));
            if Length(candidates) > 0 then
                # TODO: if there is more than one candidate, pick one, but how???
                # prefer those with more flags over those with fewers?
                # how to deal with a tie?
                # ideally these issues should be prevented by having most descriptions
                # apply only to filters with very few flags (1 or 2)
                SortBy(candidates, x -> -Length(x[1]));
                if Length(candidates) > 1 then
                    #Error("TODO: more than one candidate\n");
                fi;
                Add(desc, candidates[1][2]);
                SubtractSet(tf, candidates[1][1]);
                continue;
            fi;
        fi;
        # last resort: use filter name
        Add(desc, String(FILTERS[n]));
        RemoveSet(tf, n);
    od;
    return desc;
end;


# example:
filter:=IsList and IsMutable and IsFinite;
desc:=DescribeFilter(filter);

# we can now combine this in various ways, e.g.
msg := Concatenation("First argument must be ", JoinStringsWithSeparator(desc, " and "));