Skip to content

Commit

Permalink
fix custom command's default value (nushell#11043)
Browse files Browse the repository at this point in the history 
# Description
Fixes: nushell#11033

Sorry for the issue, it's a regression which introduce by this pr:
nushell#10456.
And this pr is going to fix it.

About the change: create a new field named `type_annotated` for
`Arg::Flag` and `Arg::Signature` instead of `arg_explicit_type`
variable.
When we meet a type in `TypeMode`, we set `type_annotated` field of the
argument to be true, then we know that if the arg have a annotated type
easily
  • Loading branch information
@WindSoilder @dmatos2012
WindSoilder authored and dmatos2012 committed Feb 20, 2024
1 parent 47128c4 commit 54f8508
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 65 deletions.
166 changes: 101 additions & 65 deletions crates/nu-parser/src/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3085,9 +3085,16 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->

#[derive(Debug)]
enum Arg {
Positional(PositionalArg, bool), // bool - required
Positional {
arg: PositionalArg,
required: bool,
type_annotated: bool,
},
RestPositional(PositionalArg),
Flag(Flag),
Flag {
flag: Flag,
type_annotated: bool,
},
}

let source = working_set.get_span_contents(span);
Expand All @@ -3105,7 +3112,6 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->

let mut args: Vec<Arg> = vec![];
let mut parse_mode = ParseMode::ArgMode;
let mut arg_explicit_type = false;

for token in &output {
match token {
Expand Down Expand Up @@ -3134,7 +3140,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
// The = symbol separates a variable from its default value
else if contents == b"=" {
match parse_mode {
ParseMode::ArgMode | ParseMode::TypeMode => {
ParseMode::TypeMode | ParseMode::ArgMode => {
parse_mode = ParseMode::DefaultValueMode;
}
ParseMode::AfterCommaArgMode => {
Expand All @@ -3160,7 +3166,6 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
working_set.error(ParseError::Expected("default value", span));
}
}
arg_explicit_type = false;
} else {
match parse_mode {
ParseMode::ArgMode | ParseMode::AfterCommaArgMode => {
Expand Down Expand Up @@ -3192,15 +3197,18 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->

// If there's no short flag, exit now. Otherwise, parse it.
if flags.len() == 1 {
args.push(Arg::Flag(Flag {
arg: None,
desc: String::new(),
long,
short: None,
required: false,
var_id: Some(var_id),
default_value: None,
}));
args.push(Arg::Flag {
flag: Flag {
arg: None,
desc: String::new(),
long,
short: None,
required: false,
var_id: Some(var_id),
default_value: None,
},
type_annotated: false,
});
} else if flags.len() >= 3 {
working_set.error(ParseError::Expected(
"only one short flag alternative",
Expand Down Expand Up @@ -3250,15 +3258,18 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
);

if chars.len() == 1 {
args.push(Arg::Flag(Flag {
arg: None,
desc: String::new(),
long,
short: Some(chars[0]),
required: false,
var_id: Some(var_id),
default_value: None,
}));
args.push(Arg::Flag {
flag: Flag {
arg: None,
desc: String::new(),
long,
short: Some(chars[0]),
required: false,
var_id: Some(var_id),
default_value: None,
},
type_annotated: false,
});
} else {
working_set.error(ParseError::Expected("short flag", span));
}
Expand Down Expand Up @@ -3289,15 +3300,18 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
let var_id =
working_set.add_variable(variable_name, span, Type::Any, false);

args.push(Arg::Flag(Flag {
arg: None,
desc: String::new(),
long: String::new(),
short: Some(chars[0]),
required: false,
var_id: Some(var_id),
default_value: None,
}));
args.push(Arg::Flag {
flag: Flag {
arg: None,
desc: String::new(),
long: String::new(),
short: Some(chars[0]),
required: false,
var_id: Some(var_id),
default_value: None,
},
type_annotated: false,
});
parse_mode = ParseMode::ArgMode;
}
// Short flag alias for long flag, e.g. --b (-a)
Expand All @@ -3321,7 +3335,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->

if chars.len() == 1 {
match args.last_mut() {
Some(Arg::Flag(flag)) => {
Some(Arg::Flag { flag, .. }) => {
if flag.short.is_some() {
working_set.error(ParseError::Expected(
"one short flag",
Expand Down Expand Up @@ -3355,16 +3369,17 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
let var_id =
working_set.add_variable(contents, span, Type::Any, false);

args.push(Arg::Positional(
PositionalArg {
args.push(Arg::Positional {
arg: PositionalArg {
desc: String::new(),
name,
shape: SyntaxShape::Any,
var_id: Some(var_id),
default_value: None,
},
false,
));
required: false,
type_annotated: false,
});
parse_mode = ParseMode::ArgMode;
}
// Rest param
Expand Down Expand Up @@ -3407,16 +3422,17 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
working_set.add_variable(contents_vec, span, Type::Any, false);

// Positional arg, required
args.push(Arg::Positional(
PositionalArg {
args.push(Arg::Positional {
arg: PositionalArg {
desc: String::new(),
name,
shape: SyntaxShape::Any,
var_id: Some(var_id),
default_value: None,
},
true,
));
required: true,
type_annotated: false,
});
parse_mode = ParseMode::ArgMode;
}
}
Expand All @@ -3430,22 +3446,30 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
);
//TODO check if we're replacing a custom parameter already
match last {
Arg::Positional(PositionalArg { shape, var_id, .. }, ..) => {
Arg::Positional {
arg: PositionalArg { shape, var_id, .. },
required: _,
type_annotated,
} => {
working_set.set_variable_type(var_id.expect("internal error: all custom parameters must have var_ids"), syntax_shape.to_type());
*shape = syntax_shape;
*type_annotated = true;
}
Arg::RestPositional(PositionalArg {
shape, var_id, ..
}) => {
working_set.set_variable_type(var_id.expect("internal error: all custom parameters must have var_ids"), Type::List(Box::new(syntax_shape.to_type())));
*shape = syntax_shape;
}
Arg::Flag(Flag { arg, var_id, .. }) => {
Arg::Flag {
flag: Flag { arg, var_id, .. },
type_annotated,
} => {
working_set.set_variable_type(var_id.expect("internal error: all custom parameters must have var_ids"), syntax_shape.to_type());
*arg = Some(syntax_shape);
*type_annotated = true;
}
}
arg_explicit_type = true;
}
parse_mode = ParseMode::ArgMode;
}
Expand All @@ -3455,20 +3479,22 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->

//TODO check if we're replacing a custom parameter already
match last {
Arg::Positional(
PositionalArg {
shape,
var_id,
default_value,
..
},
Arg::Positional {
arg:
PositionalArg {
shape,
var_id,
default_value,
..
},
required,
) => {
type_annotated,
} => {
let var_id = var_id.expect("internal error: all custom parameters must have var_ids");
let var_type = &working_set.get_variable(var_id).ty;
match var_type {
Type::Any => {
if !arg_explicit_type {
if !*type_annotated {
working_set.set_variable_type(
var_id,
expression.ty.clone(),
Expand Down Expand Up @@ -3501,7 +3527,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
None
};

if !arg_explicit_type {
if !*type_annotated {
*shape = expression.ty.to_shape();
}
*required = false;
Expand All @@ -3513,12 +3539,16 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
expression.span,
))
}
Arg::Flag(Flag {
arg,
var_id,
default_value,
..
}) => {
Arg::Flag {
flag:
Flag {
arg,
var_id,
default_value,
..
},
type_annotated,
} => {
let expression_span = expression.span;

*default_value = if let Ok(value) =
Expand All @@ -3540,7 +3570,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
// in the case, `var_type` is any.
match var_type {
Type::Any => {
if !arg_explicit_type {
if !*type_annotated {
*arg = Some(expression_ty.to_shape());
working_set
.set_variable_type(var_id, expression_ty);
Expand Down Expand Up @@ -3580,13 +3610,15 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->

if let Some(last) = args.last_mut() {
match last {
Arg::Flag(flag) => {
Arg::Flag { flag, .. } => {
if !flag.desc.is_empty() {
flag.desc.push('\n');
}
flag.desc.push_str(&contents);
}
Arg::Positional(positional, ..) => {
Arg::Positional {
arg: positional, ..
} => {
if !positional.desc.is_empty() {
positional.desc.push('\n');
}
Expand All @@ -3609,7 +3641,11 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->

for arg in args {
match arg {
Arg::Positional(positional, required) => {
Arg::Positional {
arg: positional,
required,
..
} => {
if required {
if !sig.optional_positional.is_empty() {
working_set.error(ParseError::RequiredAfterOptional(
Expand All @@ -3622,7 +3658,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
sig.optional_positional.push(positional)
}
}
Arg::Flag(flag) => sig.named.push(flag),
Arg::Flag { flag, .. } => sig.named.push(flag),
Arg::RestPositional(positional) => {
if positional.name.is_empty() {
working_set.error(ParseError::RestNeedsName(span))
Expand Down
28 changes: 28 additions & 0 deletions src/tests/test_custom_commands.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,34 @@ fn custom_switch2() -> TestResult {
)
}

#[test]
fn custom_switch3() -> TestResult {
run_test(
r#"def florb [
--age: int = 0
--name = "foobar"
] {
($age | into string) + $name
}
florb"#,
"0foobar",
)
}

#[test]
fn custom_switch4() -> TestResult {
run_test(
r#"def florb [
--age: int
--name = "foobar"
] {
($age | into string) + $name
}
florb --age 3"#,
"3foobar",
)
}

#[test]
fn simple_var_closing() -> TestResult {
run_test("let $x = 10; def foo [] { $x }; foo", "10")
Expand Down

0 comments on commit 54f8508

Please sign in to comment.