Permalink
Browse files

Properly implement break/continue/return.

The existing implementation was buggy, as exposed by the tests I added
in loop.test.sh and func.test.sh.

I removed break/continue/return as builtins, and I now parse them
**statically**.  This will break some overly clever programs like:

    b=break;
    for i in a b c; done
      echo $i
      $b
    done

But I'm being consistent with statically-parsed assignment, and it may
make it easier to convert to Oil and do other program analysis.

Representation (i.e. the thin waist of the interpreter):

- core/id_kind.py: Add Id for break/return/continue, under new Kind
  ControlFlow
- osh.asdl: add a ControlFlow variant to the command type

Algorithm:

- osh/lex.py: the lexer will return the new Ids
- osh/word_parse.py: ControlFlow tokens are treated as literals here, like
  Kind.KW, Kind.Assign, Kind.BoolUnary, etc.
- osh/cmd_parse.py: parse words consisting of only ControlFlow tokens
  into a ControlFlow node.
  - Consistently catch errors like redirects and env prefixes on
    non-commands like ControlFlow and Assign.  Tested in
    command-parsing.test.sh.
- cmd/cmd_exec.py:
  - Introduce a new _ControlFlow exception.  Get rid of the cflow return
    value.
  - Loops catch break and continue; functions catch return
  - Check some errors dynamically.  Print an error when
    break/continue/return bubble up to the top level.  The error message
    should be improved.

Tests:

- Rename func -> func-parsing, and func test
- Misc tweaks, e.g. fix a test that wasn't checking the error status.
- Bump down --osh-allowed-failures because the fix to print to stderr in
  ui.PrintError() made several of them pass
- Fix bug where we were capped at 40 tests.  Now 528 total cases, OSH
  passing 305, OSH failing 165.

Other cleanup:

- Get rid of ExecuteTop function.
  • Loading branch information...
Andy Chu
Andy Chu committed Mar 14, 2017
1 parent 700c223 commit 4da19bf6e83a6fb59c2195f7e442b2f26481d9b1
Showing with 451 additions and 243 deletions.
  1. +5 −5 bin/oil.py
  2. +6 −11 core/builtin.py
  3. +121 −83 core/cmd_exec.py
  4. +4 −0 core/id_kind.py
  5. +1 −1 core/shell_test.py
  6. +10 −10 core/ui.py
  7. +12 −10 core/word.py
  8. +49 −20 osh/cmd_parse.py
  9. +4 −0 osh/lex.py
  10. +1 −0 osh/osh.asdl
  11. +4 −3 osh/word_parse.py
  12. +1 −1 spec-runner.sh
  13. +11 −3 spec.sh
  14. +53 −0 tests/command-parsing.test.sh
  15. +97 −0 tests/func-parsing.test.sh
  16. +44 −92 tests/func.test.sh
  17. +28 −4 tests/loop.test.sh
View
@@ -97,10 +97,10 @@ def InteractiveLoop(opts, ex, c_parser, w_parser, line_reader):
if ast_f:
ast.PrettyPrint(node)
status, cflow = ex.ExecuteTop(node)
status = ex.Execute(node)
if opts.print_status:
print('STATUS', repr(status), cflow)
print('STATUS', repr(status))
# Reset prompt and clear memory. TODO: If there are any function
# definitions ANYWHERE in the node, you should not clear the underlying
@@ -231,9 +231,9 @@ def OshMain(argv):
ui.PrintError(err, arena, sys.stderr)
return 2 # parse error is code 2
status, cflow = ex.Execute(rc_node)
status = ex.Execute(rc_node)
#print('oilrc:', status, cflow, file=sys.stderr)
# Ignore bad status? What about cflow?
# Ignore bad status?
except IOError as e:
if e.errno != errno.ENOENT:
raise
@@ -324,7 +324,7 @@ def OshMain(argv):
ast_f.write('\n')
if opts.do_exec:
status, cflow = ex.Execute(node)
status = ex.Execute(node)
else:
util.log('Execution skipped because --no-exec was passed')
status = 0
View
@@ -110,8 +110,7 @@
# - So you can just add "complete" and have it work.
EBuiltin = util.Enum('EBuiltin', """
NONE BREAK CONTINUE RETURN READ ECHO EXIT SOURCE DOT TRAP EVAL EXEC SET
COMPLETE COMPGEN DEBUG_LINE
NONE READ ECHO EXIT SOURCE DOT TRAP EVAL EXEC SET COMPLETE COMPGEN DEBUG_LINE
""".split())
@@ -120,10 +119,13 @@
# On the other hand, 'cd' CAN be redefined.
# TODO:
# - use these
# - local and declare should be here, since export and readonly are.
SPECIAL_BUILTINS = [
'break', ':', 'continue', '.', 'eval', 'exec', 'exit', 'export',
'readonly', 'return', 'set', 'shift', 'times', 'trap', 'unset',
# local and declare are not POSIX, but should be here since export and
# readonly are.
'local', 'declare',
]
@@ -309,14 +311,7 @@ def Resolve(self, argv0):
# For completion, this is a flat list of names. Although coloring them
# would be nice.
if argv0 == "break":
return EBuiltin.BREAK
elif argv0 == "continue":
return EBuiltin.CONTINUE
elif argv0 == "return":
return EBuiltin.RETURN
elif argv0 == "read":
if argv0 == "read":
return EBuiltin.READ
elif argv0 == "echo":
return EBuiltin.ECHO
Oops, something went wrong.

0 comments on commit 4da19bf

Please sign in to comment.