Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completion of filenames containing # does not quote them #6741

Closed
lheckemann opened this issue Oct 15, 2022 · 3 comments · Fixed by #7398
Closed

Completion of filenames containing # does not quote them #6741

lheckemann opened this issue Oct 15, 2022 · 3 comments · Fixed by #7398
Labels
🐛 bug Something isn't working completions Issues related to tab completion

Comments

@lheckemann
Copy link

Describe the bug

Completing filenames containing a # will result in a command containing an unescaped #, which ends earlier than intended/expected.

How to reproduce

  1. touch "file#foo"
  2. Type rm file and press tab
  3. Run the completed command

Expected behavior

Complete file to something that correctly references the file, such as "file#foo".

Screenshots

2022-10-15-111245_screenshot

Configuration

key value
version 0.68.1
branch
commit_hash
build_os linux-x86_64
build_target x86_64-unknown-linux-gnu
rust_version rustc 1.64.0
cargo_version cargo 1.64.0
pkg_version 0.68.1
build_time 1980-01-01 00:00:00 +00:00
build_rust_channel release
features database, dataframe, default, trash, which, zip
installed_plugins

Additional context

No response

@sholderbach sholderbach added completions Issues related to tab completion 🐛 bug Something isn't working labels Oct 15, 2022
@jake-albert
Copy link
Contributor

Hi all,

I suppose there are a few solutions here. In this context, the parsing code in lex.rs unconditionally handles '#' as the start of a line comment, meaning that the "file#foo" part of "rm file#foo" is tokenized as an Item ("file") followed by a Comment ("#foo"), and nu ends up attempting to remove a nonexistent file called "file":

} else if c == b'#' {
// If the next character is `#`, we're at the beginning of a line
// comment. The comment continues until the next newline.
let mut start = curr_offset;
while let Some(input) = input.get(curr_offset) {
if *input == b'\n' {
if !skip_comment {
output.push(Token::new(
TokenContents::Comment,
Span::new(span_offset + start, span_offset + curr_offset),
));
}
start = curr_offset;
break;
} else {
curr_offset += 1;
}
}
if start != curr_offset && !skip_comment {
output.push(Token::new(
TokenContents::Comment,
Span::new(span_offset + start, span_offset + curr_offset),
));
}

As @lheckemann suggests, ensuring that filenames containing '#' are surrounded by quotes upon a completion should solve the problem — I would be happy to work on a PR for that.

Another option to make the parser and file completion consistent here would be to ensure that '#' is processed as the beginning of a line comment only when '#' directly follows whitespace1. Is there any situation where this behavior would be undesirable?

On my own machine with zsh, it seems that preceding whitespace is required for commenting:

> ls#maybe_comment
zsh: command not found: ls#maybe_comment 

which obviously differs from current nu behavior:

> ls#maybe_comment
{runs the `ls` command}

I think I like @lheckemann's approach more, but am curious if people think there's a need to stay consistent with how other shells do things. Played around a bit with the second approach (more than anything just to practice Rust), so I could also clean that up and submit a PR as well. Thanks for any thoughts!


1By "whitespace" I mean at least b' ', b'\t', characters passed to the lex function in additional_whitespace, and b'\n'. Not sure yet how to treat b'\r' or if other characters should be included.

} else if c == b' ' || c == b'\t' || additional_whitespace.contains(&c) {
// If the next character is non-newline whitespace, skip it.
curr_offset += 1;

@sholderbach
Copy link
Member

I think to make this unambiguous in this case we should be adding the quotes when selecting the particular suggestion from the completion as @lheckemann suggests.

But your point @jake-albert about maybe being a bit more strict about tokenization for comments might be worth thinking about with the parser going forward (cc @jntrnr). As comments are load-bearing in nushell for integrated documentation they should behave properly in most contexts.

@lheckemann
Copy link
Author

If changing the language to make the hash symbol less commenty is on the table, I'm all for it! I use Nix a lot, and its new CLI uses # a fair bit (e.g. nix run nixpkgs#nushell), so this change would make Nix a lot more convenient to use from Nushell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working completions Issues related to tab completion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants