Raise error if eager float literal conversion fails (fixes #1105)

[fingolfin]

No description provided.

[fingolfin]

The tests hang because parsing .n; or .0n; somehow wrecks GAP's internal state (in the following example, on the two "empty" continuation prompts, I pressed ctrl-D):

gap> .n;
Syntax error: Badly formed number: need a digit before or after the decimal point
.n;
^
Error, failed to convert float literal
not in any function at *stdin*:
> quit;
>
Syntax error: fi expected
^
>
$ # <-- GAP has exited

I am utterly confused as to why that happens, seeing that e.g. IntrTildeExpr also calls ErrorQuit in seemingly a very similar situation, yet there I could not provoke similar problems. Presumably this is because a syntax error also kicks in, but I still don't see how that could break stuff.

Perhaps @ChrisJefferson or @stevelinton can enlighten me?

[codecov]

Codecov Report

Merging #2040 into master will increase coverage by <.01%.
The diff coverage is 69.44%.

@@            Coverage Diff             @@
##           master    #2040      +/-   ##
==========================================
+ Coverage   73.61%   73.61%   +<.01%     
==========================================
  Files         484      484              
  Lines      245651   245655       +4     
==========================================
+ Hits       180832   180838       +6     
+ Misses      64819    64817       -2

Impacted Files	Coverage Δ
src/read.c	89.64% <61.9%> (ø)	⬆️
src/intrprtr.c	94.19% <80%> (+0.14%)	⬆️
lib/float.gi	31.3% <0%> (-0.56%)	⬇️
src/objset.c	81.91% <0%> (+0.22%)	⬆️
src/scanner.c	89.88% <0%> (+0.44%)	⬆️

[ChrisJefferson]

So, the problem is that when we ErrorQuit, we register the problem to the last executed piece of GAP code, but in this case that's some GAP code we called while trying to convert the float, which we've since left, which seems to be what we try to "blame" for the error.

I'm not entirely sure what goes wrong, but there is already some dodgyness in this case -- I notice that 1.n errors fine, and the .n case hits a case in read.c:1645 labelled "HACK".

However, if you change to a SyntaxError it works fine, so I suggest we use a syntaxerror :) Should possibly SyntaxError in Tilde as well, although it doesn't seem to hurt there.

[fingolfin]

I think I figured out the problem: any calls the reader makes into the interpreter must be wrapped inside a TRY_READ { ... } -- even if they don't generate errors themselves. But code in ReadLongNumber wasn't doing that, and this code is called for float inputs starting with dot, like .n and .0n. I fixed that as well as a few other similar places.

At least that fixed entering

[stevelinton]

This looks correct although I hadn't understood this before.

The description of TRY_READ in read,h should be adjusted.

[fingolfin]

@stevelinton I tried to improve the TRY_READ comment in read.h, please have a look, and feel free to suggest how to improve it further. I propably should have done that when I added TRY_READ, but (a) I wasn't actually fully aware of the implications of this when I added it, and (b) don't find it easy to write this. In my defense, I think the READ_ERROR() macro it replaced was even harder to understand (and the documentation was worse, too), besides relying on non-portable behavior of setjmp/longjmp...

In fact, I think I still have not fully grasped all facets and implications of TRY_READ (or its predecessor), and looking at some code, I now have a strong feeling that (almost?!?) nobod really has, looking at some other places that use TRY_READ (which of course I added, but only by mechanically replaceing older uses of READ_ERROR()), resp. that don't use it.

And I have not actually checked which aspect(s) of TRY_READ where responsible for the problem. As I wrote, my thought was that we failed to skipped some call into the interpreter, leading to the problem. But now I realise another potential problem: Since TRY_READ does not save and restore the jump buffer STATE(ReadJmpError), failing to call us can cause us to jump back to far (namely to the last previous TRY_READ). In fact, I suspect this could even lead to crashes (if the stack frame reference in the jump buffer became invalid in the meantime). I find this quite concerning, and we should audit this at some point. Alas, not in this PR, please!