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

Puzzling infinite loop with oddly accepted program #6327

Closed
vicuna opened this issue Feb 18, 2014 · 9 comments
Closed

Puzzling infinite loop with oddly accepted program #6327

vicuna opened this issue Feb 18, 2014 · 9 comments

Comments

@vicuna
Copy link

vicuna commented Feb 18, 2014

Original bug ID: 6327
Reporter: @dbuenzli
Status: acknowledged (set by @mshinwell on 2014-05-30T11:46:11Z)
Resolution: open
Priority: low
Severity: tweak
Version: 4.01.0
Target version: later
Category: lexing and parsing
Tags: patch
Related to: #4627 #6961

Bug description

AFAIR my OCaml syntax this program should be rejected. It is accepted and running it (osx) results in an infinite loop.

cat t.ml

let () =
let rec loop n = match n with
| 0 -> ()
| n -> (); ; loop (n - 1)
in
loop 3

ocamlbuild t.native
Finished, 4 targets (0 cached) in 00:00:00.
time ./t.native
^C

real 0m15.264s
user 0m15.260s
sys 0m0.003s

Something must be wrong in the parsing of match branches as for example the following program is rightly rejected

cat t2.ml

let () =
let rec loop n =
if n = 0 then () else
((); ; loop (n - 1))
in
loop 3

ocamlbuild t2.native

  • /Users/dbuenzli/.opam/4.01.0/bin/ocamldep.opt -modules t2.ml > t2.ml.depends
    File "t2.ml", line 5, characters 9-10:
    Error: Syntax error: ')' expected
    File "t2.ml", line 5, characters 4-5:
    Error: This '(' might be unmatched
    Command exited with code 2.
    Compilation unsuccessful after building 1 target (0 cached) in 00:00:00.

Thanks,

Daniel

File attachments

@vicuna
Copy link
Author

vicuna commented Feb 18, 2014

Comment author: @chambart

The problem is that a match case can be terminated with a semicolon, hence this is parsed as:

let () =
let rec loop n = (match n with
| 0 -> ()
| n -> ();) ; loop (n - 1)
in
loop 3

If you added parenthesis around ((); ; loop (n - 1)) you would have the expected parse error. A possible fix (like the ugly patch) is to forbid match (and anything that terminates with a match_case) as the first element of a sequence. I don't know if it breaks any existing code (at least not the compiler).

@vicuna
Copy link
Author

vicuna commented Feb 18, 2014

Comment author: @dbuenzli

Huhuhu, okay, makes sense. I guess this behaviour is to match what is done for ifs.

if n = 0 then () else ();
...

match n with 0 -> () | n -> ();
...

I always thought you had to put parens around any match to make sequencing work. I don't think anything good can be done. I'll only mention that I almost got crazy during a whole hour staring a small loop where I had commented a single instruction but without its semi-colon which made it like this:

let rec loop n = match n with
| 0 -> Printf.printf "stop%!";
| n ->
instr;
instr;
(* instr *);
loop n - 1

And suddendly I had a very weird loop that whas printf'ing "stop" but would still continue...

Oh well.

@vicuna
Copy link
Author

vicuna commented Feb 18, 2014

Comment author: @dbuenzli

Regarding your patch, maybe it should rather be turned into a warning ?

@vicuna
Copy link
Author

vicuna commented Feb 18, 2014

Comment author: @protz

Actually, that's an amazing feature! I'm always been writing:

stmt1;
begin match ... with
  | ...
  | ...
end;
stmt2

now I can do

stmt1;
match ... with
  | ...
  | ...
; ;
stmt2

and I'm saving seven keystrokes!

We should document this.

@vicuna
Copy link
Author

vicuna commented Feb 19, 2014

Comment author: @damiendoligez

This is a variant of #4627, which we decided not to fix. Shall we reverse that decision?

@vicuna
Copy link
Author

vicuna commented Feb 19, 2014

Comment author: @dbuenzli

I don't know. It happened to me once in 12 years of OCaml programing and I it took me time to realize because 1) I was very tired 2) the offending semi-colon was near the 80th column; I think in the loop example I give above it's more evident that this semi-colon may be suspicious when you get weird behaviour.

It could still be an interesting experiment to try to make the change and test it with a switch against the opam repository for failures.

@vicuna
Copy link
Author

vicuna commented Jul 16, 2014

Comment author: @damiendoligez

Another possibility is to make the lexer emit a warning when it sees two single-semicolons in a row (not to be confused with a double-semicolon). Ad hoc, but simple, and it avoids touching the grammar (here be dragons).

@vicuna
Copy link
Author

vicuna commented Nov 28, 2015

Comment author: @xavierleroy

I'm not a fan of optional semicolons here and there, but since we've had them for nearly 20 years, we have to live with them and clench our teeth. I'm afraid no amounts of warnings will fix the problem. At any rate, I'm pushing this and #6961 to "later" than 4.03.

@dbuenzli
Copy link
Contributor

I think this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants