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

Parser bug on character constants #995

Closed
fingolfin opened this issue Dec 7, 2016 · 10 comments
Closed

Parser bug on character constants #995

fingolfin opened this issue Dec 7, 2016 · 10 comments
Assignees
Labels
kind: bug Issues describing general bugs, and PRs fixing them topic: kernel

Comments

@fingolfin
Copy link
Member

I was looking at our old private tracker (we really should go through all issues there, and migrate any sensible issues to this tracker).

One of them was by @markuspf (see http://tracker.gap-system.org/issues/470) and reported the following buggy behavior: Enter a single quote, then press return twice and you get this:

gap> '
Syntax error: Newline not allowed in character literal
'
^
gap>
Syntax error: Missing single quote in character constant

So it look as if GAP did never finish parsing that character constant, despite already showing a syntax error (that first syntax error was not present in Markus' original report, so it has been added since then).

Apparently Markus even managed to get it to crash.

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Dec 7, 2016
@markuspf
Copy link
Member

I thought we had fixed it back then. I will check this again, maybe we didn't fix it properly or merges between HPC-GAP and GAP re-broke it.

@markuspf
Copy link
Member

A similar problem occurs here:

gap> s := "abc
Syntax error: String must not include <newline>
s := "abc
        ^
> 
> ";

@markuspf
Copy link
Member

There are more instances of when we don't abandon the parse early when a syntax error occurs, like this here:

gap> f := function(abc);
> local x
Syntax error: end expected
local x

I will have a closer look at this this week.

@ChrisJefferson
Copy link
Contributor

@markuspf : There are many, many places where we don't abandon parsing when a syntax error occurs, that is I think "standard behaviour".

If we wanted to fix that, we should introduce some kind of general method of cleaning that up -- some kind of "empty out the input stream" (that needs planning better before we start it)

markuspf added a commit to markuspf/gap that referenced this issue Jan 22, 2017
If the first Match in ReadEvalCommand leads to an error
just return.

This fixes issue gap-system#995.
fingolfin pushed a commit that referenced this issue Jan 23, 2017
If the first Match in ReadEvalCommand leads to an error
just return.

This fixes issue #995.
@fingolfin
Copy link
Member Author

This has been mostly resolved, except for that last instance @markuspf reported in December 2016.

@markuspf whatever happened to this?

@fingolfin
Copy link
Member Author

I think the fact that parsing goes on here is in general meant as a feature: we try to keep parsing through files, even if they contain a syntax error; this is useful if e.g. you change the library, and your change has a typo; then you only break the method you modified, but not everything else in that file.

But for interactive input, perhaps we should abort parsing immediately when there is a syntax error... OTOH, if one copy&pastes a longer function into a terminal, it might end up being pretty confusing if it contains a syntax error...

@olexandr-konovalov
Copy link
Member

@fingolfin said

I was looking at our old private tracker (we really should go through all issues there, and migrate any sensible issues to this tracker).

+10. To prevent flooding the tracker with mass issue migration, would be good if those who had created them would go through them themselves and either close or migrate them...

@fingolfin
Copy link
Member Author

@alex-konovalov note that the comment you are quoting is from Dec 7, 2016 ;-)

@olexandr-konovalov
Copy link
Member

@fingolfin figured that out 5 minutes ago ;-)

fingolfin added a commit to fingolfin/gap that referenced this issue May 6, 2018
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
fingolfin added a commit to fingolfin/gap that referenced this issue May 6, 2018
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
fingolfin added a commit to fingolfin/gap that referenced this issue May 7, 2018
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
fingolfin added a commit that referenced this issue May 11, 2018
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
ChrisJefferson pushed a commit to ChrisJefferson/gap that referenced this issue Jun 14, 2018
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
@fingolfin
Copy link
Member Author

Personally, I consider this fixed, now that PR #2445 is merged. People may still dislike aspects of the current behaviour, but I am not aware of anything actively buggy or misleading. If somebody is, please open a new issue.

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 topic: kernel
Projects
None yet
Development

No branches or pull requests

4 participants