Support $(cmd) command substitution as alternative to (cmd)

For consistency with "$(cmd)" and with other shells.

src/common.cpp

@@ -1390,9 +1390,13 @@ static bool unescape_string_internal(const wchar_t *const input, const size_t in
                 }
                 case L'$': {
                     if (unescape_special) {
+                        bool is_cmdsub =
+                            input_position + 1 < input_len && input[input_position + 1] == L'(';
+                        if (!is_cmdsub) {
                             to_append_or_none = VARIABLE_EXPAND;
                             vars_or_seps.push_back(input_position);
                         }
+                    }
                     break;
                 }
                 case L'{': {

src/expand.cpp

@@ -761,7 +761,7 @@ static expand_result_t expand_cmdsubst(wcstring input, const operation_context_t
             wcstring whole_item;
             whole_item.reserve(paren_begin + 1 + sub_item2.size() + 1 +
                                tail_item.completion.size());
-            whole_item.append(input, 0, paren_begin);
+            whole_item.append(input, 0, paren_begin - have_dollar);
             whole_item.push_back(INTERNAL_SEPARATOR);
             whole_item.append(sub_item2);
             whole_item.push_back(INTERNAL_SEPARATOR);

src/fish_tests.cpp

@@ -4654,6 +4654,8 @@ void history_tests_t::test_history_formats() {
         // We don't expect whitespace to be elided (#4908: except for leading/trailing whitespace)
         const wchar_t *expected[] = {L"EOF",
                                      L"sleep 123",
+                                     L"posix_cmd_sub $(is supported but only splits on newlines)",
+                                     L"posix_cmd_sub \"$(is supported)\"",
                                      L"a && echo valid construct",
                                      L"final line",
                                      L"echo supsup",
@@ -5102,8 +5104,7 @@ static void test_error_messages() {
                        {L"echo $", ERROR_NO_VAR_NAME},
                        {L"echo foo\"$\"bar", ERROR_NO_VAR_NAME},
                        {L"echo \"foo\"$\"bar\"", ERROR_NO_VAR_NAME},
-                       {L"echo foo $ bar", ERROR_NO_VAR_NAME},
+                       {L"echo foo $ bar", ERROR_NO_VAR_NAME}};
-                       {L"echo foo$(foo)bar", ERROR_BAD_VAR_SUBCOMMAND1}};
 
     parse_error_list_t errors;
     for (const auto &test : error_tests) {
@@ -5193,6 +5194,12 @@ static void test_highlighting() {
         {L"|", highlight_role_t::statement_terminator},
         {L"cat", highlight_role_t::command},
     });
+    highlight_tests.push_back({
+        {L"true", highlight_role_t::command},
+        {L"$(", highlight_role_t::operat},
+        {L"true", highlight_role_t::command},
+        {L")", highlight_role_t::operat},
+    });
     highlight_tests.push_back({
         {L"true", highlight_role_t::command},
         {L"\"before", highlight_role_t::quote},

src/parse_constants.h

@@ -262,9 +262,6 @@ enum class pipeline_position_t {
 /// Error issued on $@.
 #define ERROR_NOT_ARGV_AT _(L"$@ is not supported. In fish, please use $argv.")
 
-/// Error issued on $(...).
-#define ERROR_BAD_VAR_SUBCOMMAND1 _(L"$(...) is not supported. In fish, please use '(%ls)'.")
-
 /// Error issued on $*.
 #define ERROR_NOT_ARGV_STAR _(L"$* is not supported. In fish, please use $argv.")
 

src/parse_util.cpp

@@ -915,26 +915,6 @@ void parse_util_expand_variable_error(const wcstring &token, size_t global_token
             append_syntax_error(errors, global_dollar_pos, ERROR_NO_VAR_NAME);
             break;
         }
-        case '(': {
-            // e.g.: 'echo "foo$(bar)baz"
-            // Try to determine what's in the parens.
-            wcstring token_after_parens;
-            wcstring paren_text;
-            size_t open_parens = dollar_pos + 1, cmdsub_start = 0, cmdsub_end = 0;
-            if (parse_util_locate_cmdsubst_range(token, &open_parens, &paren_text, &cmdsub_start,
-                                                 &cmdsub_end, true) > 0) {
-                token_after_parens = tok_first(paren_text);
-            }
-
-            // Make sure we always show something.
-            if (token_after_parens.empty()) {
-                token_after_parens = get_ellipsis_str();
-            }
-
-            append_syntax_error(errors, global_dollar_pos, ERROR_BAD_VAR_SUBCOMMAND1,
-                                truncate(token_after_parens, var_err_len).c_str());
-            break;
-        }
         case L'\0': {
             append_syntax_error(errors, global_dollar_pos, ERROR_NO_VAR_NAME);
             break;
@@ -960,39 +940,6 @@ void parse_util_expand_variable_error(const wcstring &token, size_t global_token
     assert(errors->size() == start_error_count + 1);
 }
 
-/// Detect cases like $(abc). Given an arg like foo(bar), let arg_src be foo and cmdsubst_src be
-/// bar. If arg ends with VARIABLE_EXPAND, then report an error.
-static parser_test_error_bits_t detect_dollar_cmdsub_errors(size_t arg_src_offset,
-                                                            const wcstring &arg_src,
-                                                            const wcstring &cmdsubst_src,
-                                                            parse_error_list_t *out_errors) {
-    parser_test_error_bits_t result_bits = 0;
-    wcstring unescaped_arg_src;
-
-    if (!unescape_string(arg_src, &unescaped_arg_src, UNESCAPE_SPECIAL) ||
-        unescaped_arg_src.empty()) {
-        return result_bits;
-    }
-
-    wchar_t last = unescaped_arg_src.at(unescaped_arg_src.size() - 1);
-    if (last == VARIABLE_EXPAND) {
-        result_bits |= PARSER_TEST_ERROR;
-        if (out_errors != nullptr) {
-            wcstring subcommand_first_token = tok_first(cmdsubst_src);
-            if (subcommand_first_token.empty()) {
-                // e.g. $(). Report somthing.
-                subcommand_first_token = get_ellipsis_str();
-            }
-            append_syntax_error(
-                out_errors,
-                arg_src_offset + arg_src.size() - 1,  // global position of the dollar
-                ERROR_BAD_VAR_SUBCOMMAND1, truncate(subcommand_first_token, var_err_len).c_str());
-        }
-    }
-
-    return result_bits;
-}
-
 /// Test if this argument contains any errors. Detected errors include syntax errors in command
 /// substitutions, improperly escaped characters and improper use of the variable expansion
 /// operator.
@@ -1038,15 +985,6 @@ parser_test_error_bits_t parse_util_detect_errors_in_argument(const ast::argumen
 
                 if (out_errors != nullptr) {
                     out_errors->insert(out_errors->end(), subst_errors.begin(), subst_errors.end());
-
-                    // Hackish. Take this opportunity to report $(...) errors. We do this because
-                    // after we've replaced with internal separators, we can't distinguish between
-                    // "" and (), and also we no longer have the source of the command substitution.
-                    // As an optimization, this is only necessary if the last character is a $.
-                    if (paren_begin > 0 && arg_src.at(paren_begin - 1) == L'$') {
-                        err |= detect_dollar_cmdsub_errors(
-                            source_start, arg_src.substr(0, paren_begin), subst, out_errors);
-                    }
                 }
                 break;
             }

tests/checks/cmdsub.fish

@@ -1,5 +1,8 @@
 #RUN: %fish %s
 
+echo $(echo 1\n2)
+# CHECK: 1 2
+
 # Command substitution inside double quotes strips trailing newline.
 echo "a$(echo b)c"
 # CHECK: abc

tests/history_sample_bash

@@ -15,7 +15,8 @@ backticks `are not allowed`
 a && echo valid construct
 [[ x = y ]] && echo double brackets not allowed
 (( 1 = 2 )) && echo double parens not allowed
-posix_cmd_sub $(is not supported)
+posix_cmd_sub "$(is supported)"
+posix_cmd_sub $(is supported but only splits on newlines)
 sleep 123
 /** # see issue 7407
 jq <<"EOF"