[oil-language] Disallow typed args as usage error in most builtins

Addresses #1106.

Builtins that take blocks:

- cd, shopt
- shvar, try, fork, forkwait
- command, builtin, runproc (pass through)

Also fix pushd and popd usage.  They don't accept flags, but they need
flag parsers to accept

    pushd -- /tmp
    popd --

as bash does.  This also makes the typed args issue simpler.

cpp/frontend_flag_spec.cc

@@ -290,6 +290,13 @@ Tuple2<args::_Attributes*, args::Reader*> ParseCmdVal(
 #endif
 }
 
+// With optional arg
+Tuple2<args::_Attributes*, args::Reader*> ParseCmdVal(
+    Str* spec_name, runtime_asdl::cmd_value__Argv* cmd_val, bool accept_typed_args) {
+  // TODO: disallow typed args!
+  ParseCmdVal(spec_name, cmd_val);
+}
+
 Tuple2<args::_Attributes*, args::Reader*> ParseLikeEcho(
     Str* spec_name, runtime_asdl::cmd_value__Argv* cmd_val) {
 #ifdef CPP_UNIT_TEST

cpp/frontend_flag_spec.h

@@ -120,6 +120,10 @@ args::_Attributes* Parse(Str* spec_name, args::Reader* arg_r);
 Tuple2<args::_Attributes*, args::Reader*> ParseCmdVal(
     Str* spec_name, runtime_asdl::cmd_value__Argv* cmd_val);
 
+// With optional arg
+Tuple2<args::_Attributes*, args::Reader*> ParseCmdVal(
+    Str* spec_name, runtime_asdl::cmd_value__Argv* cmd_val, bool accept_typed_args);
+
 Tuple2<args::_Attributes*, args::Reader*> ParseLikeEcho(
     Str* spec_name, runtime_asdl::cmd_value__Argv* cmd_val);
 

frontend/flag_def.py

@@ -137,6 +137,9 @@
 CD_SPEC.ShortFlag('-L')
 CD_SPEC.ShortFlag('-P')
 
+PUSHD_SPEC = FlagSpec('pushd')
+
+POPD_SPEC = FlagSpec('popd')
 
 DIRS_SPEC = FlagSpec('dirs')
 DIRS_SPEC.ShortFlag('-c')

frontend/flag_spec.py

@@ -47,8 +47,14 @@ def Parse(spec_name, arg_r):
   return args.Parse(spec, arg_r)
 
 
-def ParseCmdVal(spec_name, cmd_val):
-  # type: (str, cmd_value__Argv) -> Tuple[args._Attributes, args.Reader]
+def ParseCmdVal(spec_name, cmd_val, accept_typed_args=False):
+  # type: (str, cmd_value__Argv, bool) -> Tuple[args._Attributes, args.Reader]
+
+  from frontend import typed_args  # break circular dependency
+
+  if not accept_typed_args:
+    typed_args.DoesNotAccept(cmd_val.typed_args)
+
   arg_r = args.Reader(cmd_val.argv, spids=cmd_val.arg_spids)
   arg_r.Next()  # move past the builtin name
 
@@ -58,6 +64,11 @@ def ParseCmdVal(spec_name, cmd_val):
 
 def ParseLikeEcho(spec_name, cmd_val):
   # type: (str, cmd_value__Argv) -> Tuple[args._Attributes, args.Reader]
+
+  from frontend import typed_args  # break circular dependency
+
+  typed_args.DoesNotAccept(cmd_val.typed_args)
+
   arg_r = args.Reader(cmd_val.argv, spids=cmd_val.arg_spids)
   arg_r.Next()  # move past the builtin name
 

frontend/typed_args.py

@@ -13,6 +13,13 @@
 from typing import Optional, cast, TYPE_CHECKING
 
 
+def DoesNotAccept(arg_list):
+  # type: (Optional[ArgList]) -> None
+  if arg_list is not None:
+    span_id = arg_list.spids[0]
+    e_usage('got unexpected typed args', span_id=span_id)
+
+
 def RequiredExpr(arg_list):
   # type: (Optional[ArgList]) -> Optional[expr_t]
   if arg_list is None:

osh/builtin_meta.py

@@ -145,7 +145,10 @@ def __init__(self, shell_ex, funcs, aliases, search_path):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    attrs, arg_r = flag_spec.ParseCmdVal('command', cmd_val)
+
+    # accept_typed_args=True because we invoke other builtins
+    attrs, arg_r = flag_spec.ParseCmdVal('command', cmd_val,
+                                         accept_typed_args=True)
     arg = arg_types.command(attrs.attrs)
     if arg.v:
       status = 0
@@ -215,10 +218,8 @@ def __init__(self, shell_ex, procs, errfmt):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-
-    attrs, arg_r = flag_spec.ParseCmdVal('runproc', cmd_val)
-    arg = arg_types.runproc(attrs.attrs)
-
+    _, arg_r = flag_spec.ParseCmdVal('runproc', cmd_val,
+                                     accept_typed_args=True)
     argv, spids = arg_r.Rest2()
 
     if len(argv) == 0:
@@ -265,9 +266,7 @@ def __init__(self, mutable_opts, mem, cmd_ev, shell_ex, errfmt):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-
-    attrs, arg_r = flag_spec.ParseCmdVal('try_', cmd_val)
-    arg = arg_types.try_(attrs.attrs)
+    _, arg_r = flag_spec.ParseCmdVal('try_', cmd_val, accept_typed_args=True)
 
     block = typed_args.GetOneBlock(cmd_val.typed_args)
     if block:

osh/builtin_misc.py

@@ -538,7 +538,7 @@ def __init__(self, mem, dir_stack, cmd_ev, errfmt):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    attrs, arg_r = flag_spec.ParseCmdVal('cd', cmd_val)
+    attrs, arg_r = flag_spec.ParseCmdVal('cd', cmd_val, accept_typed_args=True)
     arg = arg_types.cd(attrs.attrs)
 
     dest_dir, arg_spid = arg_r.Peek2()
@@ -636,20 +636,26 @@ def __init__(self, mem, dir_stack, errfmt):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    num_args = len(cmd_val.argv) - 1
-    if num_args == 0:
+    _, arg_r = flag_spec.ParseCmdVal('pushd', cmd_val)
+
+    dir_arg, dir_arg_spid = arg_r.Peek2()
+    if dir_arg is None:
       # TODO: It's suppose to try another dir before doing this?
       self.errfmt.Print_('pushd: no other directory')
+      # bash oddly returns 1, not 2
       return 1
-    elif num_args > 1:
-      e_usage('got too many arguments')
+
+    arg_r.Next()
+    extra, extra_spid = arg_r.Peek2()
+    if extra is not None:
+      e_usage('got too many arguments', span_id=extra_spid)
 
     # TODO: 'cd' uses normpath?  Is that inconsistent?
-    dest_dir = os_path.abspath(cmd_val.argv[1])
+    dest_dir = os_path.abspath(dir_arg)
     err_num = pyos.Chdir(dest_dir)
     if err_num != 0:
       self.errfmt.Print_("pushd: %r: %s" % (dest_dir, posix.strerror(err_num)),
-                         span_id=cmd_val.arg_spids[1])
+                         span_id=dir_arg_spid)
       return 1
 
     self.dir_stack.Push(dest_dir)
@@ -687,8 +693,11 @@ def __init__(self, mem, dir_stack, errfmt):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    if len(cmd_val.arg_spids) > 1:
-      e_usage('got extra argument', span_id=cmd_val.arg_spids[1])
+    _, arg_r = flag_spec.ParseCmdVal('pushd', cmd_val)
+
+    extra, extra_spid = arg_r.Peek2()
+    if extra is not None:
+      e_usage('got extra argument', span_id=extra_spid)
 
     if not _PopDirStack(self.mem, self.dir_stack, self.errfmt):
       return 1  # error

osh/builtin_process.py

@@ -522,7 +522,8 @@ def __init__(self, shell_ex):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    attrs, arg_r = flag_spec.ParseCmdVal('fork', cmd_val)
+    _, arg_r = flag_spec.ParseCmdVal('fork', cmd_val, accept_typed_args=True)
+
     arg, span_id = arg_r.Peek2()
     if arg is not None:
       e_usage('got unexpected argument %r' % arg, span_id=span_id)
@@ -542,7 +543,7 @@ def __init__(self, shell_ex):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    attrs, arg_r = flag_spec.ParseCmdVal('forkwait', cmd_val)
+    _, arg_r = flag_spec.ParseCmdVal('forkwait', cmd_val, accept_typed_args=True)
     arg, span_id = arg_r.Peek2()
     if arg is not None:
       e_usage('got unexpected argument %r' % arg, span_id=span_id)

osh/builtin_pure.py

@@ -53,6 +53,9 @@ def __init__(self, status):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
+
+    # These ignore regular args, but shouldn't accept typed args.
+    typed_args.DoesNotAccept(cmd_val.typed_args)
     return self.status
 
 
@@ -182,7 +185,8 @@ def __init__(self, mutable_opts, cmd_ev):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    attrs, arg_r = flag_spec.ParseCmdVal('shopt', cmd_val)
+    attrs, arg_r = flag_spec.ParseCmdVal('shopt', cmd_val,
+                                         accept_typed_args=True)
 
     arg = arg_types.shopt(attrs.attrs)
     opt_names = arg_r.Rest()
@@ -622,8 +626,7 @@ def __init__(self, mem, cmd_ev):
 
   def Run(self, cmd_val):
     # type: (cmd_value__Argv) -> int
-    attrs, arg_r = flag_spec.ParseCmdVal('shvar', cmd_val)
-    #arg = arg_types.shvar(attrs.attrs)
+    _, arg_r = flag_spec.ParseCmdVal('shvar', cmd_val, accept_typed_args=True)
 
     block = typed_args.GetOneBlock(cmd_val.typed_args)
     if not block:

spec/builtin-dirs.test.sh

@@ -20,15 +20,43 @@ pwd=/
 ## N-I dash/mksh status: 127
 ## N-I dash/mksh stdout-json: ""
 
+#### pushd usage
+pushd -z
+echo status=$?
+pushd /tmp >/dev/null
+echo status=$?
+pushd -- /tmp >/dev/null
+echo status=$?
+## STDOUT:
+status=2
+status=0
+status=0
+## END
+## OK zsh STDOUT:
+status=1
+status=0
+status=0
+## END
+
 #### popd usage error
 pushd / >/dev/null
 popd zzz
 echo status=$?
+
+popd -- >/dev/null
+echo status=$?
+
+popd -z
+echo status=$?
 ## STDOUT:
 status=2
+status=0
+status=2
 ## END
 ## BUG zsh STDOUT:
 status=0
+status=0
+status=0
 ## END
 
 #### popd returns error on empty directory stack

spec/oil-blocks.test.sh

@@ -262,3 +262,36 @@ shvar=0
 try=0
 ## END
 
+#### Consistency: Unwanted Blocks Are Errors
+shopt --set parse_brace
+
+true { echo BAD }
+echo true $?
+
+false ( 42, 43 )
+echo false $?
+
+echo { echo BAD }
+echo echo block $?
+
+echo ( 42, 43 )
+echo echo args $?
+
+command echo 'command block' { echo BAD }
+echo command echo $?
+
+builtin echo 'builtin block' { echo BAD }
+echo builtin echo $?
+
+pushd $TMP { echo BAD }
+echo pushd $?
+
+## STDOUT:
+true 2
+false 2
+echo block 2
+echo args 2
+command echo 2
+builtin echo 2
+pushd 2
+## END

spec/oil-builtins.test.sh

@@ -552,3 +552,18 @@ status=0
 status=1
 status=2
 ## END
+
+#### runproc typed args
+shopt --set parse_brace
+
+proc p {
+  echo hi
+}
+
+# The block is ignored for now
+runproc p { 
+  echo myblock 
+}
+## STDOUT:
+hi
+## END