Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: catch some invalid uses of '~'; add tests #2750

Merged
merged 2 commits into from
Aug 30, 2018

Conversation

fingolfin
Copy link
Member

Also, print a (more) helpful error message.

Before:

gap> ~;
Error, '~' does not have a value here
not in any function at *stdin*:1
gap> x -> ~;
function( x ) ... end
gap> x -> (1,~);
function( x ) ... end
gap> (1,~);
Error, '~' does not have a value here
not in any function at *stdin*:4

After:

gap> ~;
Syntax error: '~' not allowed here
~;
^
gap> x -> ~;
Syntax error: '~' not allowed here
x -> ~;
     ^
gap> x -> (1,~);
Syntax error: '~' not allowed here
x -> (1,~);
        ^
gap> (1,~);
Syntax error: '~' not allowed here
(1,~);
   ^

Also, print a (more) helpful error message.

Before:

    gap> ~;
    Error, '~' does not have a value here
    not in any function at *stdin*:1
    gap> x -> ~;
    function( x ) ... end
    gap> x -> (1,~);
    function( x ) ... end
    gap> (1,~);
    Error, '~' does not have a value here
    not in any function at *stdin*:4

After:

    gap> ~;
    Syntax error: '~' not allowed here
    ~;
    ^
    gap> x -> ~;
    Syntax error: '~' not allowed here
    x -> ~;
         ^
    gap> x -> (1,~);
    Syntax error: '~' not allowed here
    x -> (1,~);
            ^
    gap> (1,~);
    Syntax error: '~' not allowed here
    (1,~);
       ^
@fingolfin
Copy link
Member Author

Just to clarify: this is far from perfect, e.g. this is still accepted: x->[(1,~)];. But then, it's probably impossible (read: require effort similar to solving the halting problem) to catch all misuses of ~ during coding time. But I figured that's no reason to at least catch a few more of misuses which are trivial to catch.

@fingolfin
Copy link
Member Author

fingolfin commented Aug 30, 2018

The more I think about it, the less sure am I whether we should do it... This is related to the somewhat unclear definition of what ~ is supposed to be, as opposed to what the implementation right now does.

Perhaps allowing f := x -> ~; always was the intent, so that one can use it in l := [ f(1) ];. Also, what's the point in forbidding it, if one still create the same function via f:=[x->~][1];...

The tilde operator is really useful in some cases, but also under documented (actually... is there any documentation for it? where?); and it very tightly interlaces the reader, interpreter, coder/executor and compiler. I tried to isolate STATE(Tilde) but it's hopeless with the current state of thing. The only way around that I can think of would be to forbid ~ inside a function from referencing a list/record outside the function. I created a quick patch to implement that, and this actually raised no errors, except in the Browse package, in app/sudoku.g around line 500:

# global record to hold most data and functions of this section
BindGlobal("Sudoku",  rec(
# we count fields of board rowwise from 1..81; first collect relevant
# indices in row, column and subsquare for each entry
rows := List([0..8], c-> c*9+[1..9]),
cols := List([1..9], i-> i+[0,9..72]),
squs := List([1,4,7,28,31,34,55,58,61], i-> i-1+[1,2,3,10,11,12,19,20,21]),
rcsind := List([1..81], i-> [QuoInt(i-1,9)+1, (i-1) mod 9 + 1,
          First([1..9], j-> i in ~.squs[j])]),
inds := List(~.rcsind, ind -> Set(Concatenation(~.rows[ind[1]],
              ~.cols[ind[2]], ~.squs[ind[3]])))
              ));

But this can be rewritten quite easily to avoid that. E.g.:

BindGlobal("Sudoku",  rec(
# we count fields of board rowwise from 1..81; first collect relevant
# indices in row, column and subsquare for each entry
rows := List([0..8], c-> c*9+[1..9]),
cols := List([1..9], i-> i+[0,9..72]),
squs := List([1,4,7,28,31,34,55,58,61], i-> i-1+[1,2,3,10,11,12,19,20,21]),
));
Sudoku.rcsind := List([1..81], i-> [QuoInt(i-1,9)+1, (i-1) mod 9 + 1,
          First([1..9], j-> i in Sudoku.squs[j])]);
Sudoku.inds := List(Sudoku.rcsind, ind -> Set(Concatenation(Sudoku.rows[ind[1]],
              Sudoku.cols[ind[2]], Sudoku.squs[ind[3]])));

or in many other ways. At worst, one can convert any code using ~ to code which doesn't via a trick: change CODE to (tilde->CODE)(~) and then replace ~ inside of CODE by tilde. This yields:

BindGlobal("Sudoku",  rec(
# we count fields of board rowwise from 1..81; first collect relevant
# indices in row, column and subsquare for each entry
rows := List([0..8], c-> c*9+[1..9]),
cols := List([1..9], i-> i+[0,9..72]),
squs := List([1,4,7,28,31,34,55,58,61], i-> i-1+[1,2,3,10,11,12,19,20,21]),
rcsind := (r->List([1..81], i-> [QuoInt(i-1,9)+1, (i-1) mod 9 + 1,
          First([1..9], j-> i in r.squs[j])]))(~),
inds := (r->List(r.rcsind, ind -> Set(Concatenation(r.rows[ind[1]],
              r.cols[ind[2]], r.squs[ind[3]]))))(~)
              ));

The only other example of code using this "feature" is in the lpres package:

    return rec(group := TrivialSubgroup(Q),
               quo := GroupHomomorphismByFunction(G,~.group,x->One(~.group)),
               set := X,
               top := Q,
               wreath := WreathProduct(~.group,Q),
               epi := GroupHomomorphismByImages(~.wreath,~.group,GeneratorsOfGroup(~.wreath),List(GeneratorsOfGroup(~.wreath),x->One(Q))));

It's clear how to change this code away from using ~, too.

@ChrisJefferson
Copy link
Contributor

I'm happy with these fixes.

My feeling on tilde in general is that it is (as you say) horribly underspecified and underdocumented. However, I don't think it's worth the work to either (a) specify it better and handle every horrible corner case, or (b) remove it. As you showed, both in packages (and I've seen in code in the wild) some weird uses, which I'd prefer not to break unless we have to.

If at some point it arises some new feature / optimisation we want to add is blocked by Tilde, then I'm happy to re-evaluate the pain to make substansal changes, but for now I'd just leave it as a minor feature, applying (like this PR) minor fixes and improves where necessary, and trying not to break it.

@fingolfin fingolfin merged commit 35e3ffb into gap-system:master Aug 30, 2018
@fingolfin fingolfin deleted the mh/Tilde branch August 30, 2018 20:41
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants