Quit evaluator if first symbol had syntax error #1015

markuspf · 2016-12-13T22:34:34Z

Please make sure that this pull request:

is submitted to the correct branch (the stable branch is only for bugfixes)
contains an accurate description of changes for the release notes below
provides new tests or relies on existing ones
correctly refers to other issues and related pull requests

Tick all what applies to this pull request

Adds new features
Improves and extends functionality
Fixes bugs that could lead to crashes
Fixes bugs that could lead to incorrect results
Fixes bugs that could lead to break loops

Write below the description of changes (for the release notes)

This PR addresses #995.

If the parser encountered a syntax error in the first symbol it read, it did not abandon the expression it was trying to parse early enough. This lead to inconsistent state with the prompt gap> being printed, even though the interpreter is still waiting for the expression to finish parsing.

codecov-io · 2016-12-13T23:33:40Z

Current coverage is 54.54% (diff: 100%)

Merging #1015 into master will increase coverage by <.01%

@@             master      #1015   diff @@
==========================================
  Files           432        432          
  Lines        224765     224767     +2   
  Methods        3431       3431          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits         122572     122595    +23   
+ Misses       102193     102172    -21   
  Partials          0          0

Diff Coverage	File Path
•••••••••• 100%	src/read.c
•••••••••• 100%	src/scanner.c

Powered by Codecov. Last update 75245e7...f99a514

markuspf · 2016-12-13T23:36:36Z

Not sure I want to know why the test coverage changed so dramatically, but I'll investigate...

markuspf · 2016-12-14T11:33:18Z

This seems to be breaking the break loop in a bad way.

fingolfin · 2016-12-14T12:30:46Z

src/scanner.c

-    SyntaxError("Missing single quote in character constant");
+  if ( *TLS(In) == '\n' ) {
+    SyntaxError("Character literal must not include <newline>");
+  } else {


This commit has the description "Refactor GetChar()", but for me "refactor" suggests that there is no functional difference to the previous code. But that does not seem to be the case for this refactoring. For example, old and new code differ on how they handle a newline (both give a syntax error, but a different one). So at the very least, I find the commit message misleading.

Since this doesn't work as is, I'll have to re-do this PR anyway.

Changing the error message was really an afterthought, to make it line up with the error message for strings.

fingolfin · 2016-12-14T12:33:58Z

src/scanner.c

+      /* put normal chars into 'TLS(Value)' */
+      TLS(Value)[0] = *TLS(In);
+    }
+    TLS(Symbol) = S_CHAR;


The new code sets Symbol to S_CHAR before invoking GET_CHAR. And in particular, before checking for the "missing single quote character" error. Does that have any significance? (I am not familiar with this code, not saying it is wrong, just wondering).

GET_CHAR (essentially...) advances the pointer into the input stream buffer; here we are looking to get the closing '.

This refactor is an attempt at making the function less obscure, but maybe I am not very successful at it yet.

The TLS(Symbol) = S_CHAR should probably happen at the end of the function, because only then can we be sure that we (successfully) parsed a character literal.

markuspf · 2016-12-15T23:49:28Z

I am not quite sure yet that this is the correct fix, but it fixes the crashes and other problems.

fingolfin · 2016-12-22T08:24:55Z

So is this still "Do not merge"? Or is it open for review?

markuspf · 2016-12-22T09:24:23Z

Have a look at it. I am not sure this is the correct solution to the problem: the evaluator actually keeps running in an inconsistent state, hence prints the gap> prompt.

In the string case one gets:

gap> "
Syntax error: String must not include <newline>
"
^
>

i.e. the parse is not abandoned, but all you can do is leave the parse there anyway I think.

This change just abandons the parse if the first symbol cannot be correctly parsed, I don't think there's a point in a continuation prompt, but maybe there are arguments to the contrary.

fingolfin · 2016-12-22T17:39:47Z

Hmm, yeah, the continuation prompt doesn't really make sense here. But I am afraid I don't know much about that code, so can't help. Perhaps @stevelinton or @ChrisJefferson have an idea?

markuspf · 2016-12-22T20:28:00Z

Actually one thing one could do is to ignore the \n and contine accepting input. That is broken with the string case in master too though.

ChrisJefferson · 2016-12-23T00:07:25Z

I'll look into the continuation code, and see what's going on.

ChrisJefferson · 2016-12-24T10:59:42Z

I think the real thing that wants fixing here is ReadEvalCommand, lines 2685ish:

    /* get the first symbol from the input                                 */
    Match( TLS(Symbol), "", 0UL );

    /* if we have hit <end-of-file>, then give up                          */
    if ( TLS(Symbol) == S_EOF )  { return STATUS_EOF; }

    /* print only a partial prompt from now on                             */
    if ( !SyQuiet )
      TLS(Prompt) = "> ";
    else
      TLS(Prompt) = "";

The problem is we only switch on the partial prompt after reading the first symbol. This prompt changing code should perhaps be somewhere else -- perhaps ReadLine should switch after reading it's first line. Going to think about it.

markuspf · 2016-12-24T20:10:06Z

Well, that's exactly what 9b4fe40 does: It aborts ReadEvalCommand if there's been a syntax error in the first symbol.
The alternative is to mimic what happens with the string case right now, i.e. spew a syntax error, print the continuation prompt, and have the user do the one thing they can do: leave that prompt.

I'll also have a vaguely related poke at this again because I want to be able to interpret exactly one statement from an input stream, but keep that stream intact; at the moment the stream is munched using all kinds of Read and GetLine, and then the stream is closed and everything disappears....

ChrisJefferson · 2016-12-24T21:31:02Z

I wonder if a better option, for both string an character, would be to treat the newline as end of string / character, and just bail.

fingolfin

Here's my take on this thing (without having tried to run the code in here myself, though I trust @markuspf and Travis that it does not break things like crazy): This PR fixes an existing problem. Perhaps there is a hypothetcal "better" fix out there, and perhaps (or rather: most likely...) the whole way we deal with input should be revised... but all these "ifs" should not stop this existing PR from being merged right away. After all, it is quite minimal, and so I don't think it'll make any of those hypothetical "better" solutions any harder to implemnt :-).

fingolfin · 2016-12-29T22:18:10Z

I have one minor complaint: This PR supposedly fixes a issue #995. But looking at the three commit messages in this PR, it does not become clear which of them (or rather, that any of them) fixes this bug. For people reading through the commit log in the future, this bit of information would be quite helpful. (I suspect it it the first commit which contains the fix; but it only talks about continuaton prompts, while #995 does not involve a continuation prompt)

ChrisJefferson · 2017-01-05T17:11:42Z

Having looked a this, I've decided that it's a good fix. I had another idea, but this is fine. I'll let @markuspf comment in @fingolfin 's comment about the changelog.

fingolfin · 2017-01-22T00:18:02Z

Still waiting for a reply by @markuspf

markuspf · 2017-01-22T01:06:25Z

Mhm. The actual fix is in 9b4fe40, it addresses a wider range of behaviour than just the one mentioned in #995. I could amend the commit message to say so,

* If the user pressed enter after entering ' to start a character literal, GetChar would try getting more characters from the input, which lead to printing of a (continuation) prompt. * This is also trying to make GetChar slightly easier to follow

If the first Match in ReadEvalCommand leads to an error just return. This fixes issue gap-system#995.

Fixes a mild regression introduce by PR gap-system#1015, which in turn attempted to fix parts of issue gap-system#995. With GAP 4.8 and also with this fix: gap> 1.2\3; Syntax error: Badly formed number 1.2\3; ^ In GAP 4.9: gap> 1.2\3; Syntax error: Badly formed number 1.2\3; ^ 3

Fixes a mild regression introduce by PR #1015, which in turn attempted to fix parts of issue #995. With GAP 4.8 and also with this fix: gap> 1.2\3; Syntax error: Badly formed number 1.2\3; ^ In GAP 4.9: gap> 1.2\3; Syntax error: Badly formed number 1.2\3; ^ 3

Fixes a mild regression introduce by PR gap-system#1015, which in turn attempted to fix parts of issue gap-system#995. With GAP 4.8 and also with this fix: gap> 1.2\3; Syntax error: Badly formed number 1.2\3; ^ In GAP 4.9: gap> 1.2\3; Syntax error: Badly formed number 1.2\3; ^ 3

markuspf added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Dec 14, 2016

fingolfin reviewed Dec 14, 2016

View reviewed changes

markuspf force-pushed the fix-read-eval-syntaxerror branch from 97d0d3b to 9b4fe40 Compare December 15, 2016 23:45

markuspf requested review from fingolfin and ChrisJefferson December 22, 2016 09:21

markuspf removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Dec 22, 2016

fingolfin approved these changes Dec 29, 2016

View reviewed changes

markuspf added 3 commits January 22, 2017 01:09

Rewrite GetChar

ac0c72f

* If the user pressed enter after entering ' to start a character literal, GetChar would try getting more characters from the input, which lead to printing of a (continuation) prompt. * This is also trying to make GetChar slightly easier to follow

Adapt and add tests for parser fixes

398805f

Quit interpreter early if first symbol has syntax error

f99a514

If the first Match in ReadEvalCommand leads to an error just return. This fixes issue gap-system#995.

markuspf force-pushed the fix-read-eval-syntaxerror branch from 9b4fe40 to f99a514 Compare January 22, 2017 01:09

fingolfin merged commit 51182b2 into gap-system:master Jan 23, 2017

markuspf deleted the fix-read-eval-syntaxerror branch February 5, 2017 12:31

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018

fingolfin mentioned this pull request May 6, 2018

Flush input line after early syntax error; some minor reader/scanner tweaks #2445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quit evaluator if first symbol had syntax error #1015

Quit evaluator if first symbol had syntax error #1015

markuspf commented Dec 13, 2016

codecov-io commented Dec 13, 2016 •

edited

Loading

markuspf commented Dec 13, 2016

markuspf commented Dec 14, 2016

fingolfin Dec 14, 2016

markuspf Dec 14, 2016

fingolfin Dec 14, 2016

fingolfin Dec 14, 2016

markuspf Dec 14, 2016

markuspf commented Dec 15, 2016

fingolfin commented Dec 22, 2016

markuspf commented Dec 22, 2016

fingolfin commented Dec 22, 2016

markuspf commented Dec 22, 2016

ChrisJefferson commented Dec 23, 2016

ChrisJefferson commented Dec 24, 2016

markuspf commented Dec 24, 2016

ChrisJefferson commented Dec 24, 2016

fingolfin left a comment

fingolfin commented Dec 29, 2016

ChrisJefferson commented Jan 5, 2017

fingolfin commented Jan 22, 2017

markuspf commented Jan 22, 2017

Quit evaluator if first symbol had syntax error #1015

Quit evaluator if first symbol had syntax error #1015

Conversation

markuspf commented Dec 13, 2016

Tick all what applies to this pull request

Write below the description of changes (for the release notes)

codecov-io commented Dec 13, 2016 • edited Loading

Current coverage is 54.54% (diff: 100%)

markuspf commented Dec 13, 2016

markuspf commented Dec 14, 2016

fingolfin Dec 14, 2016

Choose a reason for hiding this comment

markuspf Dec 14, 2016

Choose a reason for hiding this comment

fingolfin Dec 14, 2016

Choose a reason for hiding this comment

fingolfin Dec 14, 2016

Choose a reason for hiding this comment

markuspf Dec 14, 2016

Choose a reason for hiding this comment

markuspf commented Dec 15, 2016

fingolfin commented Dec 22, 2016

markuspf commented Dec 22, 2016

fingolfin commented Dec 22, 2016

markuspf commented Dec 22, 2016

ChrisJefferson commented Dec 23, 2016

ChrisJefferson commented Dec 24, 2016

markuspf commented Dec 24, 2016

ChrisJefferson commented Dec 24, 2016

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin commented Dec 29, 2016

ChrisJefferson commented Jan 5, 2017

fingolfin commented Jan 22, 2017

markuspf commented Jan 22, 2017

codecov-io commented Dec 13, 2016 •

edited

Loading