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

Error in autocompletion when trying to access a component object via record access by accident #5241

Closed
zickgraf opened this issue Dec 5, 2022 · 11 comments · Fixed by #5250
Labels
kind: bug Issues describing general bugs, and PRs fixing them

Comments

@zickgraf
Copy link
Contributor

zickgraf commented Dec 5, 2022

Steps to reproduce:

gap> G := SymmetricGroup( 3 );;
gap> G.test.

and now press Tab to autocomplete.

Observed behaviour

Error, illegal access to record component `IsBound(obj.test)'
of the object <obj>. (Objects by default do not have record components.
The error might be a relic from translated GAP3 code.) at /usr/share/gap/lib/record.gi:171 called from
if not hasbang[j - 1] and IsBound( r.(cmps[j]) ) then
    r := r.(cmps[j]);
elif hasbang[j - 1] and IsBound( r!.(cmps[j]) ) then
    r := r!.(cmps[j]);
else
    r := fail;
    break;
fi; at /usr/share/gap/lib/cmdledit.g:979 called from
GAPInfo.CommandLineEditFunctions.Functions.(l[2])( l ) at /usr/share/gap/lib/cmdledit.g:291 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:2
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>

Expected behaviour

Of course the real problem is the user typing a wrong expression, but I think it would be nice to fail in a more graceful way here. If you don't think so, or if fixing this is not easy (there might actually be a method for \. installed and I'm not sure if one can easily detect this), feel free to close this issue.

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

 ┌───────┐   GAP 4.12.1 of 2022-10-20
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.6.6.dev, IO 4.7.0dev, PrimGrp 3.4.2, SmallGrp 1.5, TransGrp 3.6.3
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Dec 5, 2022
@fingolfin
Copy link
Member

Thank you for reporting this. That's indeed a regression from the "nicer" record tab completion introduced in 4.12, and should be fixed.

@fingolfin
Copy link
Member

Actually even in older versions something weird happens, but unrelated to tab completion: If I press "return" instead of tab, I see this:

gap> G.test.
Error, illegal access to record component `obj.test'
of the object <obj>. (Objects by default do not have record components.
The error might be a relic from translated GAP3 code.)       called from
<function "unknown">( <arguments> )
 called from read-eval loop at line 3 of *stdin*
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>

If I now enter quit; I am put into a continuation prompt (as the semicolon was missing). That's really weird: the first time around I exited the break loop via Ctrl-d, and thought GAP was locked up, because nothing happened. I had to press a second time to exit for real. This is an artefact of GAP trying to parse beyond errors. Not sure I like it here, though...

@zickgraf
Copy link
Contributor Author

zickgraf commented Dec 7, 2022

Out of curiosity: Why is there a distinction between . and !.? I guess due to the part of the documentation which says "One must be very careful when using the !. operator"?

@fingolfin
Copy link
Member

IsBound(foo.b) is only supported by records. IsBound(foo!.b) is supported by records and component objects.

@zickgraf
Copy link
Contributor Author

zickgraf commented Dec 7, 2022

IsBound(foo.b) is only supported by records. IsBound(foo!.b) is supported by records and component objects.

The question was not meant in the concrete context of this issue, sorry. New try: Why does GAP not allow to access components of component objects via . in the first place?

@ChrisJefferson
Copy link
Contributor

My belief is this is a safety / organisation issue. In general the names of components of component objects aren't fixed between versions of GAP, and also you can create some horrible bugs (and possibly even crashes) if you started editing components of component objects in ways you are not supposed to.

There isn't (I believe) any reason we couldn't just use "." for accessing all "record-like" objects, and get rid of "!.", except as you point out the documentation discourages uses "!." as you are digging inside objects.

@zickgraf
Copy link
Contributor Author

zickgraf commented Dec 7, 2022

I see, this matches what I expected, thanks for the thorough explanation! :-)

@fingolfin
Copy link
Member

@ChrisJefferson no, that's not right! Objects can overload ., and frequently do; e.g. if G is a permutation group then G.1 is its first generator. Ultimately, this can be used to implement custom record types.

In particular, it is possible for G.FOO and G!.FOO to behave differently, e.g. both may be defined and return different objects.

In contrast, G!.FOO is an access for internal data of a component type object. And for various reasons, we also allow accessing record elements via r!.FOO -- this is actually the oddball, and I'd be happy to remove it, except I bet there is some code out there which shouldn't be using it but is using it and would be broken by a removal...

@ChrisJefferson
Copy link
Contributor

Oh, I didn't realise you overload them both separately.

I've also been surprised that g!.foo works for records, but I also agree that I bet some code is using it -- it doesn't feel worth breaking that code.

@fingolfin
Copy link
Member

Actually, you can only overload G.foo -- while G!.foo always is kernel access to the components of a record or component object. And if you don't overload G.foo, the default method raises an error, which we saw here. To wit:

InstallMethod(\.,"group generators",true,
  [IsGroup and HasGeneratorsOfGroup,IsPosInt],
function(g,n)
  g:=GeneratorsOfGroup(g);
  n:=NameRNam(n);
  n:=Int(n);
  if n=fail or Length(g)<n then
    TryNextMethod();
  fi;
  return g[n];
end);

and

# methods to catch error cases

InstallMethod( \.,
               "catch error",
               true,
               [IsObject,IsObject],
               0,
function(obj,nr)
    local msg;
    msg:=Concatenation("illegal access to record component `obj.",
                        NameRNam(nr),"'\n",
                        "of the object <obj>. (Objects by default do not have record components.\n",
                        "The error might be a relic from translated GAP3 code.)");
    Error(msg);
end);

Note that this can only be overloaded for "external" objects (so positional, component or data objects, and types provided by packages), but not for internal ones like integers, strings, booleans, plist, ...

@zickgraf
Copy link
Contributor Author

zickgraf commented Dec 8, 2022

Thanks for the detailed answers! I have recently started a prototype of a system automatically converting GAP code to Julia code, so all this information is valuable :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them
Projects
None yet
3 participants