[math] Remove more bare variable support

Prior to this fix, a "bare variable" in math like 'x + 1' would be
looked up in the environment, i.e. equivalent to '$x + 1'. This appears
to have been done for performance. However this breaks the orthogonality
of fish; performance is not a sufficient justification to give math this
level of built-in power, especially because the performance of math is
not a bottleneck. The implementation is also ugly.

Remove this feature so that variables must be prefixed with the dollar
sign and undergo normal variable expansion. Reading 'git grep' output
does not show any uses of this in fish functions or completions.

Also added to changelog.

Fixes #4393

CHANGELOG.md

@@ -26,6 +26,7 @@ This section is for changes merged to the `major` branch that are not also merge
 - `bind` has a new `--silent` option to ignore bind requests for named keys not available under the current `$TERMINAL` (#4188, #4431)
 - `read` writes directly to stdout if called without arguments (#4407)
 - Globs are faster (#4579)
+- "Bare variables" in math are no longer accepted. math uses normal variable expansion. (#4393)
 
 ## Other significant changes
 - Command substitution output is now limited to 10 MB by default (#3822).

src/builtin_math.cpp

@@ -115,52 +115,6 @@ static const wchar_t *math_get_arg(int *argidx, wchar_t **argv, wcstring *storag
     return math_get_arg_argv(argidx, argv);
 }
 
-// The MuParser mechanism for dynamic lookup of variables requires that we return a unique address
-// for each variable. The following limit is arbitrary but anyone writing a math expression in fish
-// that references more than one hundred unique variables is abusing fish.
-#define MAX_RESULTS 100
-static double double_results[MAX_RESULTS];
-static int next_result = 0;
-
-/// Return a fish var converted to a double. This allows the user to use a bar var name in the
-/// expression. That is `math a + 1` rather than `math $a + 1`.
-static double *retrieve_var(const wchar_t *var_name, void *user_data) {
-    UNUSED(user_data);
-    static double zero_result = 0.0;
-
-    auto var = env_get(var_name, ENV_DEFAULT);
-    if (!var) {
-        // We could report an error but we normally don't treat missing vars as a fatal error.
-        // throw mu::ParserError(L"Var '%ls' does not exist.");
-        return &zero_result;
-    }
-    if (var->empty()) {
-        return &zero_result;
-    }
-
-    const wchar_t *first_val = var->as_list()[0].c_str();
-    wchar_t *endptr;
-    errno = 0;
-    double result = wcstod(first_val, &endptr);
-    if (*endptr != L'\0' || errno) {
-        wchar_t errmsg[500];
-        swprintf(errmsg, sizeof(errmsg) / sizeof(wchar_t),
-                 _(L"Var '%ls' not a valid floating point number: '%ls'."), var_name, first_val);
-        throw mu::ParserError(errmsg);
-    }
-
-    // We need to return a unique address for the var. If we used a `static double` var and returned
-    // it's address then multiple vars in the expression would all refer to the same value.
-    if (next_result == MAX_RESULTS - 1) {
-        wchar_t errmsg[500];
-        swprintf(errmsg, sizeof(errmsg) / sizeof(wchar_t),
-                 _(L"More than %d var names in math expression."), MAX_RESULTS);
-        throw mu::ParserError(errmsg);
-    }
-    double_results[next_result++] = result;
-    return double_results + next_result - 1;
-}
-
 /// Implement integer modulo math operator.
 static double moduloOperator(double v, double w) { return (int)v % std::max(1, (int)w); };
 
@@ -168,12 +122,9 @@ static double moduloOperator(double v, double w) { return (int)v % std::max(1, (
 static int evaluate_expression(wchar_t *cmd, parser_t &parser, io_streams_t &streams,
                                math_cmd_opts_t &opts, wcstring &expression) {
     UNUSED(parser);
-    next_result = 0;
 
     try {
         mu::Parser p;
-        // Setup callback so variables can be retrieved dynamically.
-        p.SetVarFactory(retrieve_var, nullptr);
         // MuParser doesn't implement the modulo operator so we add it ourselves since there are
         // likely users of our old math wrapper around bc that expect it to be available.
         p.DefineOprtChars(L"%");

tests/math.err

@@ -3,4 +3,4 @@
 # Validate basic expressions
 
 ####################
-# Validate how bare variables in an epxression are handled
+# Validate how variables in an expression are handled

tests/math.in

@@ -14,11 +14,11 @@ math 5 \* -2
 math -- -4 / 2
 math -- '-4 * 2'
 
-logmsg Validate how bare variables in an epxression are handled
-math x + 1
+logmsg Validate how variables in an expression are handled
+math $x + 1
 set x 1
-math x + 1
+math $x + 1
 set x 3
 set y 1.5
-math '-x * y'
-math -s1 '-x * y'
+math "-$x * $y"
+math -s1 "-$x * $y"

tests/math.out

@@ -17,7 +17,7 @@
 -8
 
 ####################
-# Validate how bare variables in an epxression are handled
+# Validate how variables in an expression are handled
 1
 2
 -4