Unexpected Space Added After First Unquoted '$' in CLI Commands

[kevin61416]

Bug Description

When inputting command line commands in lf, the application appends a space after the first '$' without quoting the dollar sign. This results in unexpected behavior when handling arguments.

For example, when I input:

:echo $A $B $C $D

lf output:

$ A $B $C $D

(NOTE: space before 'A')

When I input:

:echo "$A" $B $C $D

lf output:

$A $ B $C $D

(NOTE: space before 'B')

Steps to Reproduce

1. Open lf.
2. Enter the command :echo $A $B $C $D.
3. Check the output of the command.

Expected Behavior

The output of the command :echo $A $B $C $D should display $A $B $C $D without any additional spaces.

Environment

Operating System: macOS Monterey 12.6.1
lf version: r29

[joelim-work]

The prefix : runs a builtin lf command and not a shell command, so the parsing is subject to lf's internal parsing rules and furthermore variables are not expanded in the way you would expect from a shell.

If you want the builtin echo command to literally print $A $B $C $D then just quote the whole string ($ is a special character in the internal parsing rules):

:echo '$A $B $C $D'

If you want to expand variables then you need to run a shell command. For example the following prints the absolute path of the current file:

%echo $f

[kevin61416]

I want to use the builtin echo command to literally print $A $B $C $D without quoting them.

May you fix this inconsistent behavior where only the first unquoted $ sign has whitespace after it?

It will useful for arg-handling and auto-complete, for instance, file "a$b" can be edited by command :edit a<Tab> ==> :edit a$b; however, file "$ab" cannot be edited by :edit $ab, it becomes to edit "$" and "ab" in my Vim due to a unexpected space.

[joelim-work]

The parsing rules is implemented by scanning the input for tokens and then applying some kind of logic depending on the token found. The following explanation is a simplified version of what happens:

• If the first character is a letter, then the scanner will read until the next space and that becomes a token. For example :echo AA BB CC DD results in the tokens echo, AA, BB, CC, DD, with the last four being arguments to the echo command.
• If the first character is $, then the scanner will treat this as a token for running a shell command, and for the next token it will read until the end of the line as a single token. The reason for this is that the entire text following the $ is sent as a single string to be evaluated by the shell. This however also means that :echo $A $B $C $D results in the tokens echo, $, A $B $C $D, with the last two being arguments. The echo command prints all the arguments separated by a space, so the end result is $ A $B $C $D.

I don't know if it's a good idea to change the parsing logic to treat $ as a regular character in certain cases. It will add more complexity to the existing logic, and furthermore the same parsing logic is used for reading config files, so in general making changes to the parsing logic can break the experience for other users.

I'm not sure what your actual use case is, but if your issue is just tab autocompletion for files with special characters in them, then I think a better solution would be to just have the tab autocompletion to escape the special characters for you.

[joelim-work]

BTW the lf autocompletion already does escape special characters, but only whitespace, \, ; and # are treated as special characters. This can be changed though:

diff --git a/complete.go b/complete.go
index 8900518..8bf6fcc 100644
--- a/complete.go
+++ b/complete.go
@@ -325,7 +325,7 @@ func matchFile(s string) (matches []string, longest []rune) {
 
 		name = strings.ToLower(escape(f.Name()))
 		_, last := filepath.Split(s)
-		if !strings.HasPrefix(name, strings.ToLower(last)) {
+		if !strings.HasPrefix(name, strings.ToLower(escape(last))) {
 			continue
 		}
 
diff --git a/misc.go b/misc.go
index 451b760..b1d1e2c 100644
--- a/misc.go
+++ b/misc.go
@@ -55,7 +55,7 @@ func runeSliceWidthRange(rs []rune, beg, end int) []rune {
 func escape(s string) string {
 	buf := make([]rune, 0, len(s))
 	for _, r := range s {
-		if unicode.IsSpace(r) || r == '\\' || r == ';' || r == '#' {
+		if unicode.IsSpace(r) || strings.ContainsRune(`\;#$`, r) {
 			buf = append(buf, '\\')
 		}
 		buf = append(buf, r)
@@ -70,7 +70,7 @@ func unescape(s string) string {
 	buf := make([]rune, 0, len(s))
 	for _, r := range s {
 		if esc {
-			if !unicode.IsSpace(r) && r != '\\' && r != ';' && r != '#' {
+			if !unicode.IsSpace(r) && !strings.ContainsRune(`\;#$`, r) {
 				buf = append(buf, '\\')
 			}
 			buf = append(buf, r)

Now typing :edit $<Tab> will autocomplete to :edit \$ab, which should successfully open your $ab file.

This scenario is somewhat contrived though, I think it might be better to consult with @gokcehan first before proposing such a change. Also we would need to identify the full set of special characters and update the unit tests too.

[kevin61416]

Hi @joelim-work, @gokcehan

Thank you for your detailed explanation of the parsing logic and for the suggestion to use tab autocompletion to escape special characters. But I consider that using \ to escape $ is unintuitive.

Before proposing any changes, I wanted to check with you if this is something that you would be willing to consider. I understand that this may require identifying the full set of special characters and updating the unit tests. If this is not possible or feasible, I will of course continue to use the suggested workaround.

Thank you again for your help and advice.

[joelim-work]

It turns out that it's relatively easy to change the parsing logic so that $ is not treated as a special character specifically when parsing builtin commands like :echo:

diff --git a/parse.go b/parse.go
index e6abeb0..5cde41c 100644
--- a/parse.go
+++ b/parse.go
@@ -217,10 +217,11 @@ func (p *parser) parseExpr() expr {
 				s.scan()
 			}
 
 			result = &cmdExpr{name, expr}
 		default:
+			s.cal = true
 			name := s.tok
 
 			var args []string
 			for s.scan() && s.typ != tokenSemicolon {
 				args = append(args, s.tok)
diff --git a/scan.go b/scan.go
index beb2b5c..da4840f 100644
--- a/scan.go
+++ b/scan.go
@@ -29,10 +29,11 @@ type scanner struct {
 	nln bool      // insert newline
 	eof bool      // buffer ended
 	key bool      // scanning keys
 	blk bool      // scanning block
 	cmd bool      // scanning command
+	cal bool      // scanning call expression
 	typ tokenType // scanned token type
 	tok string    // scanned token value
 	// TODO: pos
 }
 
@@ -285,11 +286,11 @@ scan:
 		s.next()
 		s.next()
 		s.typ = tokenRBraces
 		s.tok = "}}"
 		s.sem = true
-	case isPrefix(s.chr):
+	case isPrefix(s.chr) && !s.cal:
 		s.typ = tokenPrefix
 		s.tok = string(s.chr)
 		s.cmd = true
 		s.next()
 	default:

Now it would be possible to just type :echo $A $B $C $D and :edit $ab without having to escape or quote the $.

The question is more about whether it makes sense to make such a change. $ does technically have a special meaning in the parsing rules, so ignoring it and treating it as a normal character in certain circumstances can become confusing. I should also add that the concept of special characters and escaping/quoting them is fairly common - for example the shell equivalents of the scenarios you provided would be echo $A $B $C $D and vim $ab, and they don't work because the shell treats $ as a special character.

Do you have an actual use case scenario where you find it unreasonable to escape/quote $? Without a practical example, this becomes a theoretical discussion about how special characters should be treated, and it makes it harder to decide what solution is approrpriate.

[kevin61416]

Thank you for your response and explanation. I understand that treating '$' as a normal character in certain cases could become confusing due to its special meaning in the parsing rules.

Regarding your question about my actual use case scenario, I want to clarify that my desire to escape '$' is simply for the convenience of omitting the need for escaping or quoting it. This would make it easier for me to use commands such as the one I provided in my original issue:

cmd CD %{{
    set -F
    IFS=
    dir="$(envsubst <<< "$@")"
    # Other Jobs (e.g., statistics)
    # ...
    lf -remote "send $id cd \"$dir\""
}}

With this change, I would be able to use :CD $HOME/My\ Projects (like SHELL usage) instead of :CD "$HOME/My Projects" or :CD \$HOME/My\ Projects if $HOME doesn't contain any characters in IFS.

As far as I know, '$' does not have any special meaning in lf commands other than as the first character for switching to command mode. This is consistent with the description in the lf documentation:

When the cursor is at the first character in ':' mode, pressing one of the keys '!', '$', '%', or '&' takes you to the corresponding mode. You can go back with 'cmd-delete-back' ('' by default).

I hope this clarifies my use case and reasoning for requesting this change.

[joelim-work]

The statement about $ not having any special meaning unless it's the first character is mostly true. There are a few cases like :cmd d $date and :map d $date where $ is still treated specially though.

Your use case of specifying environment variables to pass to the shell and then manually expanding them using envsubst is not something I have seen before. If you're only need to use $HOME, then probably it's better for you to type :CD ~/<Tab>, since lf can actually expand ~ for completion purposes (you would still need to manually expand ~ afterwards).

Otherwise if this isn't sufficient for you, then you can try the patch in #1219 (comment), but I would leave it up to @gokcehan to say whether it really is a good idea or not.

[gokcehan]

@joelim-work I haven't tried the patch but shouldn't there be a place where you set s.cal back to false?

My original aim when implementing the parser was to keep scanning and parsing separate, which I still think is a good idea from a language design point of view. Over time, we had to add an exception for push in the scanner but I think even that exception did not leak into parse.go.

@kevin61416 I'm not too happy with our argument parsing semantic either but I'm still not sure what we should do about it. I think generally I'm in favor of @joelim-work 's suggestion to add $ escape to autocompletion. We should probably come up with a solution that could also work for #1175.

But I consider that using \ to escape $ is unintuitive.

@kevin61416 Isn't this how it works in shell as well?

[joelim-work]

@gokcehan Sorry I should have clarified that the diff I posted above was more of a quick and dirty change for demonstration purposes rather than a proper fix, and I did not test it extensively. I also agree that the logic should be kept separately, that is to say that scan.go should only be concerned with scanning for tokens, and parse.go should join tokens into a more meaningful syntax tree but not affect the token parsing itself.

The problem with treating $ as a regular character only in the context of a regular builtin command is that now the scanning logic has to understand what a regular builtin command looks like, when that is normally done by the parser. While I think it's still technically possible to implement this, it does make the scanning logic more complex because you have to consider special cases, similar to the handling the push command. I'm not really sure if there is enough motivation to do this, if it is just for the convenience of not having to quote or escape $ in certain contexts.

On the topic of adding $ to autocompletion, I'm actually not sure if there is much motivation for this either. While lf has support for ~ during autocompletion, there is no support for expanding environment variables. Maybe something to consider separately, but I think users will generally prefer to type :edit ~/foo.txt as opposed to :edit $HOME/foo.txt so it won't really add much benefit. Also. the example :e $ab mentioned earlier in this thread is rather contrived - I don't think it's normal for users to work with files with $ in their name.

[gokcehan]

@joelim-work I think the only advantage of adding $ escape to the completion would be to make users realize $ is a character that needs to be escaped. Now that I think about it again, I guess it will still be confusing, so I agree maybe we should keep things as it is. I also agree examples in this thread are rather contrived.

[kevin61416]

Yes, using \ to escape $ is also the way it works in Shells, but the difference is that Shells expand arguments when not using \ for escape, whereas LF does not. I think the example above is not particularly artificial, $HOME is just a simple example.
In reality, our team always deals with $parameter when using Shell and LF, such as: build $FLAGS $PROJECT or find $CODEBASE. Therefore, we believe that omitting escape would be convenient and not a break-change.

[gokcehan]

@kevin61416 This proposal is not about expanding variables, so I don't see how any such change will make things more intuitive as it will still be different than shells. I think the examples are contrived, not because they make use of environment variables, but because they manually expand the variables in the commands. Up until this issue, I wasn't even aware envsubst exists. If anything, I think most people would expect variables to be automatically expanded even after the change proposed here. If you intend to make use of envsubst in your commands, currently the semantics are already quite similar in lf and shells (escape $ with either \ or single quotes).

[kevin61416]

Thanks for the explanation regarding the proposal and how it relates to expanding variables. The clarification about examples and the mention of envsubst helps shed light on the current functionality of lf. And I think your idea is better!

If anything, I think most people would expect variables to be automatically expanded even after the change proposed here.

@joelim-work I'm curious, what do contributors think?

[joelim-work]

lf doesn't expand tildes when parsing commands (there are a few exceptions such as source and cd for convenience, but in general tildes are treated as a regular character), so I would find it confusing if variables were expanded and tildes weren't.

I actually tried to expand tildes in #1246, but this idea was rejected and instead I ended up changing the code to have the autocompletion expand tildes before the command is parsed. I wonder if it's possible to apply this to environment variables but it could become a problem - suppose if you have two environment variables $ABC and $ABCDEF, does typing $ABC<tab> expand to the value of $ABC or the text $ABCDEF?

One other thing I'm uncertain about is how users normally use lf. I feel that it's supposed to be an interactive tool (you can create commands but usually these are mapped to keybindings). Rarely would you need to manually type commands into the command line, and if you really do need a powerful interpreter that can understand environment variables, why not just start a shell (and perhaps use shell scripts over lf commands)? I'm still leaning towards the opinion that the command line in lf is supposed to be a convenience for entering simple commands, as opposed to being a fully-fledged shell.

Of course, this opinion is biased towards my own usage of lf, and other users might think differently. I'm still not entirely what should be done here, perhaps we can leave this issue open and see if any other users are interested in this discussion.