`do` command: Make closure support default parameters and type checking by WindSoilder · Pull Request #12056 · nushell/nushell

WindSoilder · 2024-03-03T15:09:34Z

Description

It's implemented by porting the similar logic in eval_call, I've tried to reduce duplicate code, but it seems that it's hard without using macros.

nushell/crates/nu-engine/src/eval.rs

Lines 60 to 130 in 3ee2fc6

    
           for (param_idx, (param, required)) in decl 
        
               .signature() 
        
               .required_positional 
        
               .iter() 
        
               .map(|p| (p, true)) 
        
               .chain( 
        
                   decl.signature() 
        
                       .optional_positional 
        
                       .iter() 
        
                       .map(|p| (p, false)), 
        
               ) 
        
               .enumerate() 
        
           { 
        
               let var_id = param 
        
                   .var_id 
        
                   .expect("internal error: all custom parameters must have var_ids"); 
        
               if let Some(arg) = call.positional_nth(param_idx) { 
        
                   let result = eval_expression(engine_state, caller_stack, arg)?; 
        
                   let param_type = param.shape.to_type(); 
        
                   if required && !result.get_type().is_subtype(&param_type) { 
        
                       // need to check if result is an empty list, and param_type is table or list 
        
                       // nushell needs to pass type checking for the case. 
        
                       let empty_list_matches = result 
        
                           .as_list() 
        
                           .map(|l| { 
        
                               l.is_empty() && matches!(param_type, Type::List(_) | Type::Table(_)) 
        
                           }) 
        
                           .unwrap_or(false); 
        
                       if !empty_list_matches { 
        
                           return Err(ShellError::CantConvert { 
        
                               to_type: param.shape.to_type().to_string(), 
        
                               from_type: result.get_type().to_string(), 
        
                               span: result.span(), 
        
                               help: None, 
        
                           }); 
        
                       } 
        
                   } 
        
                   callee_stack.add_var(var_id, result); 
        
               } else if let Some(value) = &param.default_value { 
        
                   callee_stack.add_var(var_id, value.to_owned()); 
        
               } else { 
        
                   callee_stack.add_var(var_id, Value::nothing(call.head)); 
        
               } 
        
           } 
        
           if let Some(rest_positional) = decl.signature().rest_positional { 
        
               let mut rest_items = vec![]; 
        
               for result in call.rest_iter_flattened( 
        
                   decl.signature().required_positional.len() 
        
                       + decl.signature().optional_positional.len(), 
        
                   |expr| eval_expression(engine_state, caller_stack, expr), 
        
               )? { 
        
                   rest_items.push(result); 
        
               } 
        
               let span = if let Some(rest_item) = rest_items.first() { 
        
                   rest_item.span() 
        
               } else { 
        
                   call.head 
        
               }; 
        
               callee_stack.add_var( 
        
                   rest_positional 
        
                       .var_id 
        
                       .expect("Internal error: rest positional parameter lacks var_id"), 
        
                   Value::list(rest_items, span), 
        
               ) 
        
           }

It only works for do command.

User-Facing Changes

Closure supports optional parameter

let code = {|x?| print ($x | default "i'm the default")}
do $code

Previously it raises an error, after this change, it prints i'm the default.

Closure supports type checking

let code = {|x: int| echo $x}
do $code "aa"

After this change, it will raise an error with a message: can't convert string to int

Tests + Formatting

Done

After Submitting

NaN

sholderbach · 2024-03-10T17:11:43Z

While I can kind of follow what you are doing for do, how would all this play out for our other commands that accept a closure?

Can we perform type checking?

And do we have any remaining commands that sniff for the number of parameters of a closure to decide what they do? Here default values may complicate the semantics.

WindSoilder · 2024-03-11T09:49:28Z

how would all this play out for our other commands that accept a closure?

I don't think we need to handle for them, because many other commands like each, skip while auto-accept a parameter from previous command. Currently user can only use do to pass argument to closure manually.

Can we perform type checking?

I'm not really sure if it's worth to add type checking for something like each and skip while closure. And I guess it can be a big change if we want ot support it, we might need to restructure the codebase around closure.

sholderbach · 2024-03-11T10:00:17Z

I am OK with going step by step and landing this improvement for do. Then I would just suggest adding to the PR title that this is for do and go ahead with landing.

WindSoilder · 2024-03-11T10:09:50Z

Sure! I have changed the title and description

NotTheDr01ds · 2024-04-21T13:40:36Z

Currently user can only use do to pass argument to closure manually

and generate - See #12084

NotTheDr01ds · 2024-04-21T13:44:27Z

Also, FWIW, reduce, but in that case the default argument is, of course, passed in with --fold.

However, it's probably worth considering allowing reduce to also accept a default parameter for acc. (Useless) Simple example:

[ 1 2 3 4 5 ] | reduce  {|it,acc=10| $acc + $it }

This would be expected to return 25, but since the default acc=10 is ignored, the result is 15.

As far as I can tell, reduce --fold/-f seems like it could be replaced with default arguments.

# Description Fixes: #12690 The issue is happened after #12056 is merged. It will raise error if user doesn't supply required parameter when run closure with do. And parser adds a `$it` parameter when parsing closure or block expression. I believe the previous behavior is because we allow such syntax on previous version(0.44): ```nushell let x = { print $it } ``` But it's no longer allowed after 0.60. So I think they can be removed. # User-Facing Changes ```nushell let tmp = { let it = 42 print $it } do -c $tmp ``` should be possible again. # Tests + Formatting Added 1 test

# Description Fixes: nushell#12690 The issue is happened after nushell#12056 is merged. It will raise error if user doesn't supply required parameter when run closure with do. And parser adds a `$it` parameter when parsing closure or block expression. I believe the previous behavior is because we allow such syntax on previous version(0.44): ```nushell let x = { print $it } ``` But it's no longer allowed after 0.60. So I think they can be removed. # User-Facing Changes ```nushell let tmp = { let it = 42 print $it } do -c $tmp ``` should be possible again. # Tests + Formatting Added 1 test

# Description #12056 added support for default and type-checked arguments in `do` closures. This PR adds examples for those features. It also: * Fixes the TODO (a closure parameter that wasn't being used) that was preventing a result from being added * Removes extraneous commas from the descriptions * Adds an example demonstrating multiple positional closure arguments # User-Facing Changes Help examples only # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting

WindSoilder added 5 commits March 3, 2024 09:50

better closure behavior

581e7fa

try refactor: but it failed

8f792d3

refactor back

a7c5ef1

fix typo

f967950

rename func

0e9ffa8

WindSoilder mentioned this pull request Mar 4, 2024

Add support for engine calls from plugins #12029

Merged

This was referenced Mar 5, 2024

Swap the argument order for generate to be consistent with do #12083

Closed

Allow generate to accept just a closure with a default value #12084

Closed

WindSoilder added notes:fixes Include the release notes summary in the "Bug fixes" section deprecated:pr-language (deprecated: this label is too vague) This PR makes some language changes labels Mar 6, 2024

merge from main

540b136

WindSoilder changed the title ~~Make closure support default parameters and type checking~~ do command: Make closure support default parameters and type checking Mar 11, 2024

WindSoilder merged commit 5596190 into nushell:main Mar 11, 2024

hustcer added this to the v0.92.0 milestone Mar 11, 2024

WindSoilder mentioned this pull request May 16, 2024

allow define it as a variable inside closure #12888

Merged

WindSoilder deleted the better_closure_with_do branch May 23, 2024 02:57

NotTheDr01ds mentioned this pull request Jun 20, 2024

Do example #13190

Merged

sholderbach mentioned this pull request Aug 14, 2024

Blocks allow specifying argument types but do not enforce them #5286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`do` command: Make closure support default parameters and type checking#12056

`do` command: Make closure support default parameters and type checking#12056
WindSoilder merged 6 commits intonushell:mainfrom
WindSoilder:better_closure_with_do

WindSoilder commented Mar 3, 2024 •

edited

Loading

Uh oh!

sholderbach commented Mar 10, 2024

Uh oh!

WindSoilder commented Mar 11, 2024

Uh oh!

sholderbach commented Mar 11, 2024

Uh oh!

WindSoilder commented Mar 11, 2024

Uh oh!

NotTheDr01ds commented Apr 21, 2024

Uh oh!

NotTheDr01ds commented Apr 21, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	for (param_idx, (param, required)) in decl
	.signature()
	.required_positional
	.iter()
	.map(\|p\| (p, true))
	.chain(
	decl.signature()
	.optional_positional
	.iter()
	.map(\|p\| (p, false)),
	)
	.enumerate()
	{
	let var_id = param
	.var_id
	.expect("internal error: all custom parameters must have var_ids");

	if let Some(arg) = call.positional_nth(param_idx) {
	let result = eval_expression(engine_state, caller_stack, arg)?;
	let param_type = param.shape.to_type();
	if required && !result.get_type().is_subtype(&param_type) {
	// need to check if result is an empty list, and param_type is table or list
	// nushell needs to pass type checking for the case.
	let empty_list_matches = result
	.as_list()
	.map(\|l\| {
	l.is_empty() && matches!(param_type, Type::List(_) \| Type::Table(_))
	})
	.unwrap_or(false);

	if !empty_list_matches {
	return Err(ShellError::CantConvert {
	to_type: param.shape.to_type().to_string(),
	from_type: result.get_type().to_string(),
	span: result.span(),
	help: None,
	});
	}
	}
	callee_stack.add_var(var_id, result);
	} else if let Some(value) = &param.default_value {
	callee_stack.add_var(var_id, value.to_owned());
	} else {
	callee_stack.add_var(var_id, Value::nothing(call.head));
	}
	}

	if let Some(rest_positional) = decl.signature().rest_positional {
	let mut rest_items = vec![];

	for result in call.rest_iter_flattened(
	decl.signature().required_positional.len()
	+ decl.signature().optional_positional.len(),
	\|expr\| eval_expression(engine_state, caller_stack, expr),
	)? {
	rest_items.push(result);
	}

	let span = if let Some(rest_item) = rest_items.first() {
	rest_item.span()
	} else {
	call.head
	};

	callee_stack.add_var(
	rest_positional
	.var_id
	.expect("Internal error: rest positional parameter lacks var_id"),
	Value::list(rest_items, span),
	)
	}

Conversation

WindSoilder commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

Closure supports optional parameter

Closure supports type checking

Tests + Formatting

After Submitting

Uh oh!

sholderbach commented Mar 10, 2024

Uh oh!

WindSoilder commented Mar 11, 2024

Uh oh!

sholderbach commented Mar 11, 2024

Uh oh!

WindSoilder commented Mar 11, 2024

Uh oh!

NotTheDr01ds commented Apr 21, 2024

Uh oh!

NotTheDr01ds commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

WindSoilder commented Mar 3, 2024 •

edited

Loading

NotTheDr01ds commented Apr 21, 2024 •

edited

Loading