fix(parser): pitfall of redefining def#16591
Conversation
| fn detect_params_in_name( | ||
| working_set: &StateWorkingSet, | ||
| name_span: Span, | ||
| decl_name: &str, | ||
| decl_id: DeclId, | ||
| ) -> Option<ParseError> { | ||
| let name = working_set.get_span_contents(name_span); | ||
|
|
||
| let extract_span = |delim: u8| { | ||
| // it is okay to unwrap because we know the slice contains the byte | ||
| let (idx, _) = name | ||
| .iter() | ||
| .find_position(|c| **c == delim) | ||
| .unwrap_or((name.len(), &b' ')); | ||
| let param_span = Span::new(name_span.start + idx - 1, name_span.start + idx - 1); | ||
| let error = ParseError::LabeledErrorWithHelp { | ||
| error: "no space between name and parameters".into(), | ||
| label: "expected space".into(), | ||
| help: format!( | ||
| "consider adding a space between the `{decl_name}` command's name and its parameters" | ||
| ), | ||
| span: param_span, | ||
| }; | ||
| Some(error) | ||
| }; | ||
|
|
||
| if name.contains(&b'[') { | ||
| extract_span(b'[') | ||
| } else if name.contains(&b'(') { | ||
| extract_span(b'(') | ||
| } else { | ||
| None | ||
| for (offset, char) in name.iter().enumerate() { | ||
| if *char == b'[' || *char == b'(' { | ||
| return Some(ParseError::LabeledErrorWithHelp { | ||
| error: "no space between name and parameters".into(), | ||
| label: "expected space".into(), | ||
| help: format!( | ||
| "consider adding a space between the `{}` command's name and its parameters", | ||
| working_set.get_decl(decl_id).name() | ||
| ), | ||
| span: Span::new(offset + name_span.start - 1, offset + name_span.start - 1), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| None |
There was a problem hiding this comment.
This change is irrelevant to the fix.
There was a problem hiding this comment.
Can you nevertheless explain to me what you are doing here. Else may be worth splitting off into its own commit.
There was a problem hiding this comment.
Can you nevertheless explain to me what you are doing here. Else may be worth splitting off into its own commit.
Nothing important, just a simple refactoring into a more readable version.
There was a problem hiding this comment.
There is something regarding this (and its predecessor) code we might need to fix but that is beside the scope of this PR.
Details
When trying out cursed command names I stumbled across this issue: ``` > def "test[]" [] {} Error: × no space between name and parameters ╭─[entry #1:1:9] 1 │ def "test[]" [] {} · ▲ · ╰── expected space ╰──── help: consider adding a space between the `def` command's name and its parameters ```
sholderbach
left a comment
There was a problem hiding this comment.
Makes sense to me to lock that down, we probably want to lock more of our keywords but we can look into generalizing in a second step.
| #[test] | ||
| pub fn test_deprecated_attribute() { | ||
| let engine_state = EngineState::new(); | ||
| let engine_state = nu_cmd_lang::create_default_context(); |
There was a problem hiding this comment.
Certainly more compact code. What was the reason to have that change?
There was a problem hiding this comment.
Originally, command def was temporarily added to working set, so parse_def will complain about not finding the decl_id of it. Merging it to the engine state is another way of fixing it, but this one is simpler.
Fixes #16586
Release notes summary - What our users need to know
None
Tasks after submitting
None