make missing_var a singleton

Make the `env_var_t::missing_var()` object a singleton rather than a
dynamically constructed object. This requires some discipline in its use
since C++ doesn't directly support immutable objects. But it is slightly
more efficient and helps identify code that incorrectly mutates `env_var_t`
objects that should not be modified.

src/common.cpp

@@ -1581,6 +1581,7 @@ static void validate_new_termsize(struct winsize *new_termsize) {
 /// Export the new terminal size as env vars and to the kernel if possible.
 static void export_new_termsize(struct winsize *new_termsize) {
     wchar_t buf[64];
+
     env_var_t cols = env_get(L"COLUMNS", ENV_EXPORT);
     swprintf(buf, 64, L"%d", (int)new_termsize->ws_col);
     env_set(L"COLUMNS", ENV_GLOBAL | (cols.missing_or_empty() ? 0 : ENV_EXPORT), buf);

src/env.cpp

@@ -107,6 +107,15 @@ static void init_locale();
 static void init_curses();
 static void tokenize_variable_array(const wcstring &val, wcstring_list_t &out);
 
+/// Construct a missing var object
+env_var_t create_missing_var() {
+    env_var_t var;
+    var.set_missing();
+    return var;
+}
+
+env_var_t missing_var = create_missing_var();
+
 // Struct representing one level in the function variable stack.
 // Only our variable stack should create and destroy these
 class env_node_t {
@@ -306,7 +315,7 @@ static bool is_electric(const wcstring &key) {
 const env_var_t env_node_t::find_entry(const wcstring &key) {
     var_table_t::const_iterator entry = env.find(key);
     if (entry != env.end()) return entry->second;
-    return env_var_t::missing_var();
+    return missing_var;
 }
 
 /// Return the current umask value.
@@ -1209,7 +1218,7 @@ int env_remove(const wcstring &key, int var_mode) {
 }
 
 wcstring env_var_t::as_string(void) const {
-    //assert(!is_missing);
+    assert(!is_missing);
     return val;
 }
 
@@ -1235,12 +1244,12 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) {
     // Make the assumption that electric keys can't be shadowed elsewhere, since we currently block
     // that in env_set().
     if (is_electric(key)) {
-        if (!search_global) return env_var_t::missing_var();
+        if (!search_global) return missing_var;
         if (key == L"history") {
             // Big hack. We only allow getting the history on the main thread. Note that history_t
             // may ask for an environment variable, so don't take the lock here (we don't need it).
             if (!is_main_thread()) {
-                return env_var_t::missing_var();
+                return missing_var;
             }
 
             history_t *history = reader_get_history();
@@ -1280,7 +1289,7 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) {
         }
     }
 
-    if (!search_universal) return env_var_t::missing_var();
+    if (!search_universal) return missing_var;
 
     // Another hack. Only do a universal barrier on the main thread (since it can change variable
     // values). Make sure we do this outside the env_lock because it may itself call `env_get()`.
@@ -1289,15 +1298,17 @@ env_var_t env_get(const wcstring &key, env_mode_flags_t mode) {
         env_universal_barrier();
     }
 
+    // Okay, we couldn't find a local or global var given the requirements. If there is a matching
+    // universal var return that.
     if (uvars()) {
         env_var_t env_var = uvars()->get(key);
-        if (env_var == ENV_NULL ||
-            !(uvars()->get_export(key) ? search_exported : search_unexported)) {
-            env_var = env_var_t::missing_var();
+        if (!env_var.missing() &&
+            (uvars()->get_export(key) ? search_exported : search_unexported)) {
+            return env_var;
         }
-        return env_var;
     }
-    return env_var_t::missing_var();
+
+    return missing_var;
 }
 
 bool env_exist(const wchar_t *key, env_mode_flags_t mode) {
@@ -1561,7 +1572,7 @@ env_var_t env_vars_snapshot_t::get(const wcstring &key) const {
         return env_get(key);
     }
     std::map<wcstring, wcstring>::const_iterator iter = vars.find(key);
-    return iter == vars.end() ? env_var_t::missing_var() : env_var_t(iter->second);
+    return iter == vars.end() ? missing_var : env_var_t(iter->second);
 }
 
 const wchar_t *const env_vars_snapshot_t::highlighting_keys[] = {L"PATH", L"CDPATH",

src/env.h

@@ -77,19 +77,13 @@ class env_var_t {
    public:
     bool exportv;  // whether the variable should be exported
 
-    static env_var_t missing_var() {
-        env_var_t result((wcstring()));
-        result.is_missing = true;
-        result.exportv = false;
-        return result;
-    }
-
-    env_var_t(const env_var_t &x) : val(x.val), is_missing(x.is_missing), exportv(x.exportv) {}
     env_var_t(const wcstring &x) : val(x), is_missing(false), exportv(false) {}
     env_var_t(const wchar_t *x) : val(x), is_missing(false), exportv(false) {}
     env_var_t() : val(L""), is_missing(false), exportv(false) {}
 
-    bool empty(void) const { return val.empty() || val == ENV_NULL; };
+    void set_missing(void) { is_missing = true; }
+
+    bool empty(void) const { return val.empty() || val == ENV_NULL; }
     bool missing(void) const { return is_missing; }
     bool missing_or_empty(void) const { return missing() || empty(); }
 
@@ -97,29 +91,33 @@ class env_var_t {
     void to_list(wcstring_list_t &out) const;
     wcstring as_string() const;
 
-    env_var_t &operator=(const env_var_t &v) {
-        is_missing = v.is_missing;
-        exportv = v.exportv;
-        val = v.val;
-        return *this;
+    // You cannot convert a missing var to a non-missing var. You can only change the value of a var
+    // that is not missing.
+    void set_val(const wcstring &s) {
+        assert(!is_missing);
+        val = s;
+    }
+    void set_val(const wchar_t *s) {
+        assert(!is_missing);
+        val = s;
     }
-
-    void set_val(const wcstring &s) { val = s; is_missing = false; }
-    void set_val(const wchar_t *s) { val = s; is_missing = false; }
 
     bool operator==(const env_var_t &s) const { return is_missing == s.is_missing && val == s.val; }
 
     bool operator==(const wcstring &s) const { return !is_missing && val == s; }
 
+    bool operator==(const wchar_t *s) const { return !is_missing && val == s; }
+
     bool operator!=(const env_var_t &v) const { return val != v.val; }
 
     bool operator!=(const wcstring &s) const { return val != s; }
 
     bool operator!=(const wchar_t *s) const { return val != s; }
-
-    bool operator==(const wchar_t *s) const { return !is_missing && val == s; }
 };
 
+env_var_t create_missing_var();
+extern env_var_t missing_var;
+
 /// Gets the variable with the specified name, or env_var_t::missing_var if it does not exist or is
 /// an empty array.
 ///

src/env_universal_common.cpp

@@ -270,12 +270,9 @@ env_universal_t::env_universal_t(const wcstring &path)
 env_universal_t::~env_universal_t() { pthread_mutex_destroy(&lock); }
 
 env_var_t env_universal_t::get(const wcstring &name) const {
-    env_var_t result = env_var_t::missing_var();
     var_table_t::const_iterator where = vars.find(name);
-    if (where != vars.end()) {
-        result = where->second;
-    }
-    return result;
+    if (where != vars.end()) return where->second;
+    return missing_var;
 }
 
 bool env_universal_t::get_export(const wcstring &name) const {
@@ -465,7 +462,8 @@ bool env_universal_t::write_to_fd(int fd, const wcstring &path) {
         // variable; soldier on.
         const wcstring &key = iter->first;
         const env_var_t &var = iter->second;
-        append_file_entry(var.exportv ? SET_EXPORT : SET, key, var.as_string(), &contents, &storage);
+        append_file_entry(var.exportv ? SET_EXPORT : SET, key, var.as_string(), &contents,
+                          &storage);
 
         // Go to next.
         ++iter;

src/expand.cpp

@@ -144,7 +144,7 @@ static void append_cmdsub_error(parse_error_list_t *errors, size_t source_start,
 
 /// Return the environment variable value for the string starting at \c in.
 static env_var_t expand_var(const wchar_t *in) {
-    if (!in) return env_var_t::missing_var();
+    if (!in) return missing_var;
     return env_get(in);
 }
 
@@ -777,7 +777,7 @@ static int expand_variables(const wcstring &instr, std::vector<completion_t> *ou
         var_tmp.append(instr, start_pos, var_len);
         env_var_t var;
         if (var_len == 1 && var_tmp[0] == VARIABLE_EXPAND_EMPTY) {
-            var = env_var_t::missing_var();
+            var = missing_var;
         } else {
             var = expand_var(var_tmp.c_str());
         }

src/fish_tests.cpp

@@ -2658,7 +2658,7 @@ static void test_universal() {
             const wcstring key = format_string(L"key_%d_%d", i, j);
             env_var_t expected_val;
             if (j == 0) {
-                expected_val = env_var_t::missing_var();
+                expected_val = missing_var;
             } else {
                 expected_val = format_string(L"val_%d_%d", i, j);
             }

src/highlight.cpp

@@ -222,7 +222,7 @@ static bool is_potential_cd_path(const wcstring &path, const wcstring &working_d
     } else {
         // Get the CDPATH.
         env_var_t cdpath = env_get(L"CDPATH");
-        if (cdpath.missing_or_empty()) cdpath = L".";
+        if (cdpath.missing_or_empty()) cdpath = env_var_t(L".");
 
         // Tokenize it into directories.
         std::vector<wcstring> pathsv;