builtin function: error when trying to inherit read-only variable

Also, deduplicate error checks and do them as early as possible since
we always return on error.

src/builtins/function.rs

@@ -82,6 +82,34 @@ fn parse_cmd_opts(
     let print_hints = false;
     let mut handling_named_arguments = false;
     let mut w = WGetopter::new(SHORT_OPTIONS, LONG_OPTIONS, argv);
+
+    let mut validate_variable_name =
+        |streams: &mut IoStreams, varname: &wstr, read_only_ok: bool| {
+            if !valid_var_name(varname) {
+                streams.err.append(&varname_error(cmd, varname));
+                return Err(STATUS_INVALID_ARGS);
+            }
+            if !read_only_ok && is_read_only(varname) {
+                streams.err.append(&wgettext_fmt!(
+                    "%s: variable '%s' is read-only\n",
+                    cmd,
+                    varname
+                ));
+                return Err(STATUS_INVALID_ARGS);
+            }
+            Ok(())
+        };
+    fn add_named_argument(
+        validate_variable_name: &mut impl FnMut(&mut IoStreams, &wstr, bool) -> Result<(), i32>,
+        streams: &mut IoStreams,
+        opts: &mut FunctionCmdOpts,
+        varname: &wstr,
+    ) -> Result<(), i32> {
+        validate_variable_name(streams, varname, /*read_only_ok=*/ false)?;
+        opts.named_arguments.push(varname.to_owned());
+        Ok::<(), ErrorCode>(())
+    }
+
     while let Some(opt) = w.next_opt() {
         // NON_OPTION_CHAR is returned when we reach a non-permuted non-option.
         if opt != 'a' && opt != NON_OPTION_CHAR {
@@ -90,17 +118,9 @@ fn parse_cmd_opts(
         match opt {
             NON_OPTION_CHAR => {
                 // A positional argument we got because we use RETURN_IN_ORDER.
-                let woptarg = w.woptarg.unwrap().to_owned();
+                let woptarg = w.woptarg.unwrap();
                 if handling_named_arguments {
-                    if is_read_only(&woptarg) {
-                        streams.err.append(&wgettext_fmt!(
-                            "%s: variable '%s' is read-only\n",
-                            cmd,
-                            woptarg
-                        ));
-                        return Err(STATUS_INVALID_ARGS);
-                    }
-                    opts.named_arguments.push(woptarg);
+                    add_named_argument(&mut validate_variable_name, streams, opts, woptarg)?;
                 } else {
                     streams.err.append(&wgettext_fmt!(
                         "%s: %s: unexpected positional argument",
@@ -126,10 +146,7 @@ fn parse_cmd_opts(
             }
             'v' => {
                 let name = w.woptarg.unwrap().to_owned();
-                if !valid_var_name(&name) {
-                    streams.err.append(&varname_error(cmd, &name));
-                    return Err(STATUS_INVALID_ARGS);
-                }
+                validate_variable_name(streams, &name, /*read_only_ok=*/ true)?;
                 opts.events.push(EventDescription::Variable { name });
             }
             'e' => {
@@ -174,17 +191,13 @@ fn parse_cmd_opts(
                 opts.events.push(e);
             }
             'a' => {
-                let name = w.woptarg.unwrap().to_owned();
-                if is_read_only(&name) {
-                    streams.err.append(&wgettext_fmt!(
-                        "%s: variable '%s' is read-only\n",
-                        cmd,
-                        name
-                    ));
-                    return Err(STATUS_INVALID_ARGS);
-                }
                 handling_named_arguments = true;
-                opts.named_arguments.push(name);
+                add_named_argument(
+                    &mut validate_variable_name,
+                    streams,
+                    opts,
+                    w.woptarg.unwrap(),
+                )?;
             }
             'S' => {
                 opts.shadow_scope = false;
@@ -194,10 +207,7 @@ fn parse_cmd_opts(
             }
             'V' => {
                 let woptarg = w.woptarg.unwrap();
-                if !valid_var_name(woptarg) {
-                    streams.err.append(&varname_error(cmd, woptarg));
-                    return Err(STATUS_INVALID_ARGS);
-                }
+                validate_variable_name(streams, woptarg, /*read_only_ok=*/ false)?;
                 opts.inherit_vars.push(woptarg.to_owned());
             }
             'h' => {
@@ -232,11 +242,7 @@ fn parse_cmd_opts(
         if !opts.named_arguments.is_empty() {
             // Remaining arguments are named arguments.
             for &arg in argv[optind..].iter() {
-                if !valid_var_name(arg) {
-                    streams.err.append(&varname_error(cmd, arg));
-                    return Err(STATUS_INVALID_ARGS);
-                }
-                opts.named_arguments.push(arg.to_owned());
+                add_named_argument(&mut validate_variable_name, streams, opts, arg)?;
             }
         } else {
             streams.err.append(&wgettext_fmt!(
@@ -329,13 +335,6 @@ pub fn function(
         })
         .collect();
 
-    for named in &opts.named_arguments {
-        if !valid_var_name(named) {
-            streams.err.append(&varname_error(cmd, named));
-            return Err(STATUS_INVALID_ARGS);
-        }
-    }
-
     // We have what we need to actually define the function.
     let props = function::FunctionProperties {
         func_node,

tests/checks/function.fish

@@ -162,6 +162,11 @@ function foo --argument-names status; end
 # CHECKERR: function foo --argument-names status; end
 # CHECKERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
 
+function foo --inherit-variable status; end
+# CHECKERR: {{.*}}function.fish (line {{\d+}}): function: variable 'status' is read-only
+# CHECKERR: function foo --inherit-variable status; end
+# CHECKERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
+
 echo status $status
 # CHECK: status 2