diff --git a/src/builtins/function.rs b/src/builtins/function.rs index 439269f1a..737c1a193 100644 --- a/src/builtins/function.rs +++ b/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, diff --git a/tests/checks/function.fish b/tests/checks/function.fish index 39397b46a..57e997f0f 100644 --- a/tests/checks/function.fish +++ b/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