Fix doc comments for custom commands (#815)

crates/nu-parser/src/lite_parse.rs

@@ -85,23 +85,29 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
     let mut curr_pipeline = LiteStatement::new();
     let mut curr_command = LiteCommand::new();
 
-    let mut last_token_was_pipe = false;
+    let mut last_token = TokenContents::Eol;
+
+    let mut curr_comment: Option<Vec<Span>> = None;
 
     for token in tokens.iter() {
         match &token.contents {
             TokenContents::Item => {
+                // If we have a comment, go ahead and attach it
+                if let Some(curr_comment) = curr_comment.take() {
+                    curr_command.comments = curr_comment;
+                }
                 curr_command.push(token.span);
-                last_token_was_pipe = false;
+                last_token = TokenContents::Item;
             }
             TokenContents::Pipe => {
                 if !curr_command.is_empty() {
                     curr_pipeline.push(curr_command);
                     curr_command = LiteCommand::new();
                 }
-                last_token_was_pipe = true;
+                last_token = TokenContents::Pipe;
             }
             TokenContents::Eol => {
-                if !last_token_was_pipe {
+                if last_token != TokenContents::Pipe {
                     if !curr_command.is_empty() {
                         curr_pipeline.push(curr_command);
 
@@ -114,6 +120,13 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
                         curr_pipeline = LiteStatement::new();
                     }
                 }
+
+                if last_token == TokenContents::Eol {
+                    // Clear out the comment as we're entering a new comment
+                    curr_comment = None;
+                }
+
+                last_token = TokenContents::Eol;
             }
             TokenContents::Semicolon => {
                 if !curr_command.is_empty() {
@@ -128,10 +141,23 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
                     curr_pipeline = LiteStatement::new();
                 }
 
-                last_token_was_pipe = false;
+                last_token = TokenContents::Semicolon;
             }
             TokenContents::Comment => {
-                curr_command.comments.push(token.span);
+                // Comment is beside something
+                if last_token != TokenContents::Eol {
+                    curr_command.comments.push(token.span);
+                    curr_comment = None;
+                } else {
+                    // Comment precedes something
+                    if let Some(curr_comment) = &mut curr_comment {
+                        curr_comment.push(token.span);
+                    } else {
+                        curr_comment = Some(vec![token.span]);
+                    }
+                }
+
+                last_token = TokenContents::Comment;
             }
         }
     }
@@ -144,7 +170,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
         block.push(curr_pipeline);
     }
 
-    if last_token_was_pipe {
+    if last_token == TokenContents::Pipe {
         (
             block,
             Some(ParseError::UnexpectedEof(

crates/nu-parser/src/parse_keywords.rs

@@ -11,6 +11,7 @@ use std::collections::HashSet;
 
 use crate::{
     lex, lite_parse,
+    lite_parse::LiteCommand,
     parser::{
         check_call, check_name, find_captures_in_block, garbage, garbage_statement, parse,
         parse_block_expression, parse_internal_call, parse_multispan_value, parse_signature,
@@ -173,10 +174,65 @@ pub fn parse_for(
     )
 }
 
+fn build_usage(working_set: &StateWorkingSet, spans: &[Span]) -> String {
+    let mut usage = String::new();
+
+    let mut num_spaces = 0;
+    let mut first = true;
+
+    // Use the comments to build the usage
+    for comment_part in spans {
+        let contents = working_set.get_span_contents(*comment_part);
+
+        let comment_line = if first {
+            // Count the number of spaces still at the front, skipping the '#'
+            let mut pos = 1;
+            while pos < contents.len() {
+                if let Some(b' ') = contents.get(pos) {
+                    // continue
+                } else {
+                    break;
+                }
+                pos += 1;
+            }
+
+            num_spaces = pos;
+
+            first = false;
+
+            String::from_utf8_lossy(&contents[pos..]).to_string()
+        } else {
+            let mut pos = 1;
+
+            while pos < contents.len() && pos < num_spaces {
+                if let Some(b' ') = contents.get(pos) {
+                    // continue
+                } else {
+                    break;
+                }
+                pos += 1;
+            }
+
+            String::from_utf8_lossy(&contents[pos..]).to_string()
+        };
+
+        if !usage.is_empty() {
+            usage.push('\n');
+        }
+        usage.push_str(&comment_line);
+    }
+
+    usage
+}
+
 pub fn parse_def(
     working_set: &mut StateWorkingSet,
-    spans: &[Span],
+    lite_command: &LiteCommand,
 ) -> (Statement, Option<ParseError>) {
+    let spans = &lite_command.parts[..];
+
+    let usage = build_usage(working_set, &lite_command.comments);
+
     // Checking that the function is used with the correct name
     // Maybe this is not necessary but it is a sanity check
     if working_set.get_span_contents(spans[0]) != b"def" {
@@ -260,6 +316,7 @@ pub fn parse_def(
             let declaration = working_set.get_decl_mut(decl_id);
 
             signature.name = name.clone();
+            signature.usage = usage;
 
             *declaration = signature.clone().into_block_command(block_id);
 
@@ -368,8 +425,9 @@ pub fn parse_alias(
 
 pub fn parse_export(
     working_set: &mut StateWorkingSet,
-    spans: &[Span],
+    lite_command: &LiteCommand,
 ) -> (Statement, Option<Exportable>, Option<ParseError>) {
+    let spans = &lite_command.parts[..];
     let mut error = None;
 
     let export_span = if let Some(sp) = spans.get(0) {
@@ -420,7 +478,11 @@ pub fn parse_export(
         let kw_name = working_set.get_span_contents(*kw_span);
         match kw_name {
             b"def" => {
-                let (stmt, err) = parse_def(working_set, &spans[1..]);
+                let lite_command = LiteCommand {
+                    comments: lite_command.comments.clone(),
+                    parts: spans[1..].to_vec(),
+                };
+                let (stmt, err) = parse_def(working_set, &lite_command);
                 error = error.or(err);
 
                 let export_def_decl_id = if let Some(id) = working_set.find_decl(b"export def") {
@@ -615,7 +677,7 @@ pub fn parse_module_block(
 
     let source = working_set.get_span_contents(span);
 
-    let (output, err) = lex(source, span.start, &[], &[], true);
+    let (output, err) = lex(source, span.start, &[], &[], false);
     error = error.or(err);
 
     let (output, err) = lite_parse(&output);
@@ -639,7 +701,7 @@ pub fn parse_module_block(
 
                 let (stmt, err) = match name {
                     b"def" => {
-                        let (stmt, err) = parse_def(working_set, &pipeline.commands[0].parts);
+                        let (stmt, err) = parse_def(working_set, &pipeline.commands[0]);
 
                         (stmt, err)
                     }
@@ -653,7 +715,7 @@ pub fn parse_module_block(
                     // since in the second case, the name of the env var would be $env."foo a".
                     b"export" => {
                         let (stmt, exportable, err) =
-                            parse_export(working_set, &pipeline.commands[0].parts);
+                            parse_export(working_set, &pipeline.commands[0]);
 
                         if err.is_none() {
                             let name_span = pipeline.commands[0].parts[2];

crates/nu-parser/src/parser.rs

@@ -1,5 +1,6 @@
 use crate::{
     lex, lite_parse,
+    lite_parse::LiteCommand,
     parse_keywords::{parse_for, parse_source},
     type_check::{math_result_type, type_compatible},
     LiteBlock, ParseError, Token, TokenContents,
@@ -2823,7 +2824,7 @@ pub fn parse_block_expression(
 
     let source = working_set.get_span_contents(inner_span);
 
-    let (output, err) = lex(source, start, &[], &[], true);
+    let (output, err) = lex(source, start, &[], &[], false);
     error = error.or(err);
 
     working_set.enter_scope();
@@ -3474,30 +3475,33 @@ pub fn parse_variable(
 
 pub fn parse_statement(
     working_set: &mut StateWorkingSet,
-    spans: &[Span],
+    lite_command: &LiteCommand,
 ) -> (Statement, Option<ParseError>) {
-    let name = working_set.get_span_contents(spans[0]);
+    let name = working_set.get_span_contents(lite_command.parts[0]);
 
     match name {
-        b"def" => parse_def(working_set, spans),
-        b"let" => parse_let(working_set, spans),
+        b"def" => parse_def(working_set, lite_command),
+        b"let" => parse_let(working_set, &lite_command.parts),
         b"for" => {
-            let (expr, err) = parse_for(working_set, spans);
+            let (expr, err) = parse_for(working_set, &lite_command.parts);
             (Statement::Pipeline(Pipeline::from_vec(vec![expr])), err)
         }
-        b"alias" => parse_alias(working_set, spans),
-        b"module" => parse_module(working_set, spans),
-        b"use" => parse_use(working_set, spans),
-        b"source" => parse_source(working_set, spans),
+        b"alias" => parse_alias(working_set, &lite_command.parts),
+        b"module" => parse_module(working_set, &lite_command.parts),
+        b"use" => parse_use(working_set, &lite_command.parts),
+        b"source" => parse_source(working_set, &lite_command.parts),
         b"export" => (
-            garbage_statement(spans),
-            Some(ParseError::UnexpectedKeyword("export".into(), spans[0])),
+            garbage_statement(&lite_command.parts),
+            Some(ParseError::UnexpectedKeyword(
+                "export".into(),
+                lite_command.parts[0],
+            )),
         ),
-        b"hide" => parse_hide(working_set, spans),
+        b"hide" => parse_hide(working_set, &lite_command.parts),
         #[cfg(feature = "plugin")]
-        b"register" => parse_register(working_set, spans),
+        b"register" => parse_register(working_set, &lite_command.parts),
         _ => {
-            let (expr, err) = parse_expression(working_set, spans, true);
+            let (expr, err) = parse_expression(working_set, &lite_command.parts, true);
             (Statement::Pipeline(Pipeline::from_vec(vec![expr])), err)
         }
     }
@@ -3632,7 +3636,7 @@ pub fn parse_block(
                     expressions: output,
                 })
             } else {
-                let (stmt, err) = parse_statement(working_set, &pipeline.commands[0].parts);
+                let (stmt, err) = parse_statement(working_set, &pipeline.commands[0]);
 
                 if error.is_none() {
                     error = err;
@@ -3964,7 +3968,7 @@ pub fn parse(
 
     working_set.add_file(name, contents);
 
-    let (output, err) = lex(contents, span_offset, &[], &[], true);
+    let (output, err) = lex(contents, span_offset, &[], &[], false);
     error = error.or(err);
 
     let (output, err) = lite_parse(&output);

crates/nu-parser/tests/test_lite_parser.rs

@@ -97,16 +97,11 @@ fn separated_comments_dont_stack() -> Result<(), ParseError> {
     let lite_block = lite_parse_helper(input)?;
 
     assert_eq!(lite_block.block.len(), 1);
-    assert_eq!(lite_block.block[0].commands[0].comments.len(), 2);
+    assert_eq!(lite_block.block[0].commands[0].comments.len(), 1);
     assert_eq!(lite_block.block[0].commands[0].parts.len(), 3);
 
     assert_eq!(
         lite_block.block[0].commands[0].comments[0],
-        Span { start: 0, end: 19 }
-    );
-
-    assert_eq!(
-        lite_block.block[0].commands[0].comments[1],
         Span { start: 21, end: 38 }
     );
 

src/tests.rs

@@ -46,6 +46,31 @@ pub fn run_test(input: &str, expected: &str) -> TestResult {
     Ok(())
 }
 
+#[cfg(test)]
+pub fn run_test_contains(input: &str, expected: &str) -> TestResult {
+    let mut file = NamedTempFile::new()?;
+    let name = file.path();
+
+    let mut cmd = Command::cargo_bin("engine-q")?;
+    cmd.arg(name);
+
+    writeln!(file, "{}", input)?;
+
+    let output = cmd.output()?;
+
+    let stderr = String::from_utf8_lossy(&output.stderr).to_string();
+    let stdout = String::from_utf8_lossy(&output.stdout).to_string();
+
+    println!("stdout: {}", stdout);
+    println!("stderr: {}", stderr);
+
+    assert!(output.status.success());
+
+    assert!(stdout.contains(expected));
+
+    Ok(())
+}
+
 #[cfg(test)]
 pub fn fail_test(input: &str, expected: &str) -> TestResult {
     let mut file = NamedTempFile::new()?;

src/tests/test_parser.rs

@@ -1,5 +1,7 @@
 use crate::tests::{fail_test, run_test, TestResult};
 
+use super::run_test_contains;
+
 #[test]
 fn env_shorthand() -> TestResult {
     run_test("FOO=BAR if $false { 3 } else { 4 }", "4")
@@ -169,3 +171,16 @@ fn string_interp_with_equals() -> TestResult {
 fn recursive_parse() -> TestResult {
     run_test(r#"def c [] { c }; echo done"#, "done")
 }
+
+#[test]
+fn commands_have_usage() -> TestResult {
+    run_test_contains(
+        r#"
+    # This is a test
+    #
+    # To see if I have cool usage
+    def foo [] {}
+    help foo"#,
+        "cool usage",
+    )
+}