Skip to content

Commit

Permalink
Span ID Refactor (Step 2): Make Call SpanId-friendly (nushell#13268)
Browse files Browse the repository at this point in the history
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

Part of nushell#12963, step 2.

This PR refactors Call and related argument structures to remove their
dependency on `Expression::span` which will be removed in the future.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

Should be none. If you see some error messages that look broken, please
report.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
  • Loading branch information
kubouch authored Jul 3, 2024
1 parent 9b63e17 commit 0cfd5fb
Show file tree
Hide file tree
Showing 31 changed files with 468 additions and 296 deletions.
2 changes: 1 addition & 1 deletion crates/nu-cli/src/commands/keybindings_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl Command for KeybindingsList {
.collect()
} else {
call.named_iter()
.flat_map(|(argument, _, _)| get_records(argument.item.as_str(), call.head))
.flat_map(|(argument, _, _, _)| get_records(argument.item.as_str(), call.head))
.collect()
};

Expand Down
29 changes: 16 additions & 13 deletions crates/nu-cli/src/completions/custom_completions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use nu_protocol::{
ast::{Argument, Call, Expr, Expression},
debugger::WithoutDebug,
engine::{Stack, StateWorkingSet},
PipelineData, Span, Type, Value,
PipelineData, Span, Spanned, Type, Value,
};
use nu_utils::IgnoreCaseExt;
use std::collections::HashMap;
Expand Down Expand Up @@ -51,18 +51,21 @@ impl Completer for CustomCompletion {
&Call {
decl_id: self.decl_id,
head: span,
arguments: vec![
Argument::Positional(Expression::new_unknown(
Expr::String(self.line.clone()),
Span::unknown(),
Type::String,
)),
Argument::Positional(Expression::new_unknown(
Expr::Int(line_pos as i64),
Span::unknown(),
Type::Int,
)),
],
arguments: Spanned {
item: vec![
Argument::Positional(Expression::new_unknown(
Expr::String(self.line.clone()),
Span::unknown(),
Type::String,
)),
Argument::Positional(Expression::new_unknown(
Expr::Int(line_pos as i64),
Span::unknown(),
Type::Int,
)),
],
span: Span::unknown(),
},
parser_info: HashMap::new(),
},
PipelineData::empty(),
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cli/src/syntax_highlight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ fn find_matching_block_end_in_expr(
}
}

Expr::Call(call) => call.arguments.iter().find_map(|arg| {
Expr::Call(call) => call.arguments.item.iter().find_map(|arg| {
arg.expr().and_then(|expr| {
find_matching_block_end_in_expr(
line,
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/bytes/build_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Command for BytesBuild {
_input: PipelineData,
) -> Result<PipelineData, ShellError> {
let mut output = vec![];
for val in call.rest_iter_flattened(0, |expr| {
for val in call.rest_iter_flattened(&engine_state, 0, |expr| {
let eval_expression = get_eval_expression(engine_state);
eval_expression(engine_state, stack, expr)
})? {
Expand Down
4 changes: 2 additions & 2 deletions crates/nu-command/src/debug/explain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ fn get_arguments(
) -> Vec<Value> {
let mut arg_value = vec![];
let span = Span::test_data();
for arg in &call.arguments {
for arg in &call.arguments.item {
match arg {
// I think the second argument to Argument::Named is the short name, but I'm not really sure.
// Please fix it if it's wrong. :)
Argument::Named((name, short, opt_expr)) => {
Argument::Named((name, short, opt_expr, _)) => {
let arg_type = "named";
let arg_value_name = name.item.clone();
let arg_value_name_span_start = name.span.start as i64;
Expand Down
6 changes: 3 additions & 3 deletions crates/nu-command/src/debug/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati
collect_values,
collect_exprs,
collect_lines,
call.span(),
call.span(&engine_state),
);

let lock_err = |_| ShellError::GenericError {
Expand All @@ -125,12 +125,12 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati
let pipeline_data = result?;

// Collect the output
let _ = pipeline_data.into_value(call.span());
let _ = pipeline_data.into_value(call.span(&engine_state));

Ok(engine_state
.deactivate_debugger()
.map_err(lock_err)?
.report(engine_state, call.span())?
.report(engine_state, call.span(&engine_state))?
.into_pipeline_data())
}

Expand Down
7 changes: 6 additions & 1 deletion crates/nu-command/src/filesystem/open.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,12 @@ impl Command for Open {
let block = engine_state.get_block(block_id);
eval_block(engine_state, stack, block, stream)
} else {
decl.run(engine_state, stack, &Call::new(call_span), stream)
decl.run(
engine_state,
stack,
&Call::new(call_span, call.arguments_span()),
stream,
)
};
output.push(command_output.map_err(|inner| {
ShellError::GenericError{
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/filesystem/save.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ fn convert_to_extension(
let eval_block = get_eval_block(engine_state);
eval_block(engine_state, stack, block, input)
} else {
decl.run(engine_state, stack, &Call::new(span), input)
decl.run(engine_state, stack, &Call::new(span, span.past()), input)
}
} else {
Ok(input)
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/filesystem/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ pub fn get_rest_for_glob_pattern(
let mut output = vec![];
let eval_expression = get_eval_expression(engine_state);

for result in call.rest_iter_flattened(starting_pos, |expr| {
for result in call.rest_iter_flattened(&engine_state, starting_pos, |expr| {
let result = eval_expression(engine_state, stack, expr);
match result {
Err(e) => Err(e),
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/formats/from/csv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ fn from_csv(
} else {
return Err(ShellError::NonUtf8Custom {
msg: "separator should be a single char or a 4-byte unicode".into(),
span: call.span(),
span: call.span(&engine_state),
});
}
}
Expand Down
4 changes: 3 additions & 1 deletion crates/nu-command/src/generators/cal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use nu_color_config::StyleComputer;
use nu_engine::command_prelude::*;
use nu_protocol::ast::{Expr, Expression};

use nu_protocol::engine::UNKNOWN_SPAN_ID;
use std::collections::VecDeque;

#[derive(Clone)]
Expand Down Expand Up @@ -143,7 +144,7 @@ pub fn cal(
style_computer,
)?;

let mut table_no_index = Call::new(Span::unknown());
let mut table_no_index = Call::new(Span::unknown(), Span::unknown());
table_no_index.add_named((
Spanned {
item: "index".to_string(),
Expand All @@ -155,6 +156,7 @@ pub fn cal(
Span::unknown(),
Type::Bool,
)),
UNKNOWN_SPAN_ID,
));

let cal_table_output =
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/misc/tutor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ fn display(help: &str, engine_state: &EngineState, stack: &mut Stack, span: Span
let result = decl.run(
engine_state,
stack,
&Call::new(span),
&Call::new(span, span.past()),
Value::string(item, Span::unknown()).into_pipeline_data(),
);

Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/network/http/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ fn transform_response_using_content_type(
Some(converter_id) => engine_state.get_decl(converter_id).run(
engine_state,
stack,
&Call::new(span),
&Call::new(span, span.past()),
output,
),
None => Ok(output),
Expand Down
7 changes: 6 additions & 1 deletion crates/nu-command/src/platform/is_terminal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,12 @@ impl Command for IsTerminal {
_ => {
return Err(ShellError::IncompatibleParametersSingle {
msg: "Only one stream may be checked".into(),
span: Span::merge_many(call.arguments.iter().map(|arg| arg.span())),
span: Span::merge_many(
call.arguments
.item
.iter()
.map(|arg| arg.span(&engine_state)),
),
});
}
};
Expand Down
8 changes: 6 additions & 2 deletions crates/nu-command/src/system/run_external.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,12 @@ fn write_pipeline_data(
Arc::make_mut(&mut engine_state.config).use_ansi_coloring = false;

// Invoke the `table` command.
let output =
crate::Table.run(&engine_state, &mut stack, &Call::new(Span::unknown()), data)?;
let output = crate::Table.run(
&engine_state,
&mut stack,
&Call::new(Span::unknown(), Span::unknown()),
data,
)?;

// Write the output.
for value in output {
Expand Down
17 changes: 10 additions & 7 deletions crates/nu-command/src/viewers/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl Command for Table {
return Ok(val.into_pipeline_data());
}

let cfg = parse_table_config(call, engine_state, stack)?;
let cfg = parse_table_config(engine_state, call, engine_state, stack)?;
let input = CmdInput::new(engine_state, stack, call, input);

// reset vt processing, aka ansi because illbehaved externals can break it
Expand Down Expand Up @@ -247,6 +247,7 @@ impl TableConfig {
}

fn parse_table_config(
engine_state: &EngineState,
call: &Call,
state: &EngineState,
stack: &mut Stack,
Expand All @@ -269,9 +270,9 @@ fn parse_table_config(
flatten_separator,
},
};
let theme =
get_theme_flag(call, state, stack)?.unwrap_or_else(|| get_config(state, stack).table_mode);
let index = get_index_flag(call, state, stack)?;
let theme = get_theme_flag(engine_state, call, state, stack)?
.unwrap_or_else(|| get_config(state, stack).table_mode);
let index = get_index_flag(engine_state, call, state, stack)?;

let term_width = get_width_param(width_param);

Expand All @@ -285,6 +286,7 @@ fn parse_table_config(
}

fn get_index_flag(
engine_state: &EngineState,
call: &Call,
state: &EngineState,
stack: &mut Stack,
Expand All @@ -308,7 +310,7 @@ fn get_index_flag(
Err(ShellError::UnsupportedInput {
msg: String::from("got a negative integer"),
input: val.to_string(),
msg_span: call.span(),
msg_span: call.span(&engine_state),
input_span: internal_span,
})
} else {
Expand All @@ -319,13 +321,14 @@ fn get_index_flag(
_ => Err(ShellError::CantConvert {
to_type: String::from("index"),
from_type: String::new(),
span: call.span(),
span: call.span(&engine_state),
help: Some(String::from("supported values: [bool, int, nothing]")),
}),
}
}

fn get_theme_flag(
engine_state: &EngineState,
call: &Call,
state: &EngineState,
stack: &mut Stack,
Expand All @@ -335,7 +338,7 @@ fn get_theme_flag(
TableMode::from_str(&theme).map_err(|err| ShellError::CantConvert {
to_type: String::from("theme"),
from_type: String::from("string"),
span: call.span(),
span: call.span(&engine_state),
help: Some(format!("{}, but found '{}'.", err, theme)),
})
})
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-engine/src/call_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl CallExt for Call {
let stack = &mut stack.use_call_arg_out_dest();
let mut output = vec![];

for result in self.rest_iter_flattened(starting_pos, |expr| {
for result in self.rest_iter_flattened(&engine_state, starting_pos, |expr| {
eval_expression::<WithoutDebug>(engine_state, stack, expr)
})? {
output.push(FromValue::from_value(result)?);
Expand Down
19 changes: 14 additions & 5 deletions crates/nu-engine/src/documentation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ fn nu_highlight_string(code_string: &str, engine_state: &EngineState, stack: &mu
if let Ok(output) = decl.run(
engine_state,
stack,
&Call::new(Span::unknown()),
&Call::new(Span::unknown(), Span::unknown()),
Value::string(code_string, Span::unknown()).into_pipeline_data(),
) {
let result = output.into_value(Span::unknown());
Expand Down Expand Up @@ -241,7 +241,10 @@ fn get_documentation(
&Call {
decl_id,
head: span,
arguments: vec![],
arguments: Spanned {
item: vec![],
span: span.past(),
},
parser_info: HashMap::new(),
},
PipelineData::Value(Value::list(vals, span), None),
Expand Down Expand Up @@ -273,7 +276,7 @@ fn get_documentation(
match decl.run(
engine_state,
stack,
&Call::new(Span::unknown()),
&Call::new(Span::unknown(), Span::unknown()),
Value::string(example.example, Span::unknown()).into_pipeline_data(),
) {
Ok(output) => {
Expand All @@ -296,7 +299,7 @@ fn get_documentation(
}

if let Some(result) = &example.result {
let mut table_call = Call::new(Span::unknown());
let mut table_call = Call::new(Span::unknown(), Span::unknown());
if example.example.ends_with("--collapse") {
// collapse the result
table_call.add_named((
Expand All @@ -306,6 +309,7 @@ fn get_documentation(
},
None,
None,
UNKNOWN_SPAN_ID,
))
} else {
// expand the result
Expand All @@ -316,6 +320,7 @@ fn get_documentation(
},
None,
None,
UNKNOWN_SPAN_ID,
))
}
let table = engine_state
Expand Down Expand Up @@ -368,13 +373,17 @@ fn get_ansi_color_for_component_or_default(
// Call ansi command using argument
if let Some(argument) = argument_opt {
if let Some(decl_id) = engine_state.find_decl(b"ansi", &[]) {
let arg_span = argument.span(&engine_state);
if let Ok(result) = eval_call::<WithoutDebug>(
engine_state,
caller_stack,
&Call {
decl_id,
head: span,
arguments: vec![argument],
arguments: Spanned {
item: vec![argument],
span: arg_span,
},
parser_info: HashMap::new(),
},
PipelineData::Empty,
Expand Down
Loading

0 comments on commit 0cfd5fb

Please sign in to comment.