Clean up feature flags API

This also cleans up and removes unnecessary usage of FFI-oriented `feature_metadata_t`,
which is only used from Rust code after `builtins/status` was ported.

fish-rust/src/builtins/status.rs

@@ -10,7 +10,7 @@ use crate::ffi::{
     get_job_control_mode, get_login, is_interactive_session, job_control_t, parser_t,
     set_job_control_mode, Repin,
 };
-use crate::future_feature_flags::{feature_metadata, feature_test};
+use crate::future_feature_flags::{self as features, feature_test};
 use crate::wchar::{wstr, L};
 use crate::wchar_ffi::{AsWstr, WCharFromFFI};
 use crate::wgetopt::{
@@ -180,10 +180,10 @@ const LONG_OPTIONS: &[woption] = &[
 fn print_features(streams: &mut io_streams_t) {
     // TODO: move this to features.rs
     let mut max_len = i32::MIN;
-    for md in feature_metadata() {
+    for md in &features::METADATA {
         max_len = max_len.max(md.name.len() as i32);
     }
-    for md in feature_metadata() {
+    for md in &features::METADATA {
         let set = if feature_test(md.flag) {
             L!("on")
         } else {
@@ -192,10 +192,10 @@ fn print_features(streams: &mut io_streams_t) {
         streams.out.append(sprintf!(
             "%-*ls%-3s %ls %ls\n",
             max_len + 1,
-            md.name.as_wstr(),
+            md.name,
             set,
-            md.groups.as_wstr(),
+            md.groups,
-            md.description.as_wstr(),
+            md.description,
         ));
     }
 }
@@ -433,8 +433,8 @@ pub fn status(
             }
             use TestFeatureRetVal::*;
             let mut retval = Some(TEST_FEATURE_NOT_RECOGNIZED as c_int);
-            for md in &feature_metadata() {
+            for md in &features::METADATA {
-                if md.name.as_wstr() == args[0] {
+                if md.name == args[0] {
                     retval = match feature_test(md.flag) {
                         true => Some(TEST_FEATURE_ON as c_int),
                         false => Some(TEST_FEATURE_OFF as c_int),

fish-rust/src/future_feature_flags.rs

@@ -2,8 +2,6 @@
 
 use crate::ffi::wcharz_t;
 use crate::wchar::wstr;
-use crate::wchar_ffi::WCharToFFI;
-use std::array;
 use std::sync::atomic::AtomicBool;
 use std::sync::atomic::Ordering;
 use widestring_suffix::widestrs;
@@ -31,74 +29,49 @@ mod future_feature_flags_ffi {
         ampersand_nobg_in_token,
     }
 
-    /// Metadata about feature flags.
-    struct feature_metadata_t {
-        flag: FeatureFlag,
-        name: UniquePtr<CxxWString>,
-        groups: UniquePtr<CxxWString>,
-        description: UniquePtr<CxxWString>,
-        default_value: bool,
-        read_only: bool,
-    }
-
     extern "Rust" {
-        type Features;
+        #[cxx_name = "feature_test"]
-        fn test(self: &Features, flag: FeatureFlag) -> bool;
+        fn test(flag: FeatureFlag) -> bool;
-        fn set(self: &mut Features, flag: FeatureFlag, value: bool);
+        #[cxx_name = "feature_set"]
-        fn set_from_string(self: &mut Features, str: wcharz_t);
+        fn set(flag: FeatureFlag, value: bool);
-        fn fish_features() -> *const Features;
+        #[cxx_name = "feature_set_from_string"]
-        fn feature_test(flag: FeatureFlag) -> bool;
+        fn set_from_string(str: wcharz_t);
-        fn mutable_fish_features() -> *mut Features;
-        fn feature_metadata() -> [feature_metadata_t; 4];
     }
 }
 
-pub use future_feature_flags_ffi::{feature_metadata_t, FeatureFlag};
+pub use future_feature_flags_ffi::FeatureFlag;
 
-pub struct Features {
+struct Features {
     // Values for the flags.
     // These are atomic to "fix" a race reported by tsan where tests of feature flags and other
     // tests which use them conceptually race.
-    values: [AtomicBool; metadata.len()],
+    values: [AtomicBool; METADATA.len()],
 }
 
 /// Metadata about feature flags.
-struct FeatureMetadata {
+pub struct FeatureMetadata {
     /// The flag itself.
-    flag: FeatureFlag,
+    pub flag: FeatureFlag,
 
     /// User-presentable short name of the feature flag.
-    name: &'static wstr,
+    pub name: &'static wstr,
 
     /// Comma-separated list of feature groups.
-    groups: &'static wstr,
+    pub groups: &'static wstr,
 
     /// User-presentable description of the feature flag.
-    description: &'static wstr,
+    pub description: &'static wstr,
 
     /// Default flag value.
-    default_value: bool,
+    pub default_value: bool,
 
     /// Whether the value can still be changed or not.
-    read_only: bool,
+    pub read_only: bool,
-}
-
-impl From<&FeatureMetadata> for feature_metadata_t {
-    fn from(md: &FeatureMetadata) -> feature_metadata_t {
-        feature_metadata_t {
-            flag: md.flag,
-            name: md.name.to_ffi(),
-            groups: md.groups.to_ffi(),
-            description: md.description.to_ffi(),
-            default_value: md.default_value,
-            read_only: md.read_only,
-        }
-    }
 }
 
 /// The metadata, indexed by flag.
 #[widestrs]
-const metadata: [FeatureMetadata; 4] = [
+pub const METADATA: [FeatureMetadata; 4] = [
     FeatureMetadata {
         flag: FeatureFlag::stderr_nocaret,
         name: "stderr-nocaret"L,
@@ -134,36 +107,52 @@ const metadata: [FeatureMetadata; 4] = [
 ];
 
 /// The singleton shared feature set.
-static mut global_features: Features = Features::new();
+static FEATURES: Features = Features::new();
+
+/// Perform a feature test on the global set of features.
+pub fn test(flag: FeatureFlag) -> bool {
+    FEATURES.test(flag)
+}
+
+pub fn feature_test(flag: FeatureFlag) -> bool {
+    test(flag)
+}
+
+/// Set a flag.
+pub fn set(flag: FeatureFlag, value: bool) {
+    FEATURES.set(flag, value);
+}
+
+/// Parses a comma-separated feature-flag string, updating ourselves with the values.
+/// Feature names or group names may be prefixed with "no-" to disable them.
+/// The special group name "all" may be used for those who like to live on the edge.
+/// Unknown features are silently ignored.
+pub fn set_from_string<'a>(str: impl Into<&'a wstr>) {
+    FEATURES.set_from_string(str);
+}
 
 impl Features {
     const fn new() -> Self {
         Features {
             values: [
-                AtomicBool::new(metadata[0].default_value),
+                AtomicBool::new(METADATA[0].default_value),
-                AtomicBool::new(metadata[1].default_value),
+                AtomicBool::new(METADATA[1].default_value),
-                AtomicBool::new(metadata[2].default_value),
+                AtomicBool::new(METADATA[2].default_value),
-                AtomicBool::new(metadata[3].default_value),
+                AtomicBool::new(METADATA[3].default_value),
             ],
         }
     }
 
-    /// Return whether a flag is set.
+    fn test(&self, flag: FeatureFlag) -> bool {
-    pub fn test(&self, flag: FeatureFlag) -> bool {
         self.values[flag.repr as usize].load(Ordering::SeqCst)
     }
 
-    /// Set a flag.
+    fn set(&self, flag: FeatureFlag, value: bool) {
-    pub fn set(&mut self, flag: FeatureFlag, value: bool) {
         self.values[flag.repr as usize].store(value, Ordering::SeqCst)
     }
 
-    /// Parses a comma-separated feature-flag string, updating ourselves with the values.
-    /// Feature names or group names may be prefixed with "no-" to disable them.
-    /// The special group name "all" may be used for those who like to live on the edge.
-    /// Unknown features are silently ignored.
     #[widestrs]
-    pub fn set_from_string<'a>(&mut self, str: impl Into<&'a wstr>) {
+    fn set_from_string<'a>(&self, str: impl Into<&'a wstr>) {
         let str: &wstr = str.into();
         let whitespace = "\t\n\0x0B\0x0C\r "L.as_char_slice();
         for entry in str.as_char_slice().split(|c| *c == ',') {
@@ -186,14 +175,14 @@ impl Features {
             // is to allow uniform invocations of fish (e.g. disable a feature that is only present in
             // future versions).
             // The special name 'all' may be used for those who like to live on the edge.
-            if let Some(md) = metadata.iter().find(|md| md.name == name) {
+            if let Some(md) = METADATA.iter().find(|md| md.name == name) {
                 // Only change it if it's not read-only.
                 // Don't complain if it is, this is typically set from a variable.
                 if !md.read_only {
                     self.set(md.flag, value);
                 }
             } else {
-                for md in &metadata {
+                for md in &METADATA {
                     if md.groups == name || name == "all"L {
                         if !md.read_only {
                             self.set(md.flag, value);
@@ -205,41 +194,18 @@ impl Features {
     }
 }
 
-/// Return the global set of features for fish.
-pub fn fish_features() -> &'static Features {
-    // Safety: this will become const with atomics after Rust conversion.
-    unsafe { &global_features }
-}
-
-/// Perform a feature test on the global set of features.
-pub fn feature_test(flag: FeatureFlag) -> bool {
-    fish_features().test(flag)
-}
-
-/// Return the global set of features for fish, but mutable. In general fish features should be set
-/// at startup only.
-pub fn mutable_fish_features() -> *mut Features {
-    // Safety: this will be ported to use atomics after Rust conversion.
-    unsafe { &mut global_features as *mut Features }
-}
-
-// The metadata, indexed by flag.
-pub fn feature_metadata() -> [feature_metadata_t; metadata.len()] {
-    array::from_fn(|i| (&metadata[i]).into())
-}
-
 #[test]
 #[widestrs]
 fn test_feature_flags() {
-    let mut f = Features::new();
+    let f = Features::new();
     f.set_from_string("stderr-nocaret,nonsense"L);
     assert!(f.test(FeatureFlag::stderr_nocaret));
     f.set_from_string("stderr-nocaret,no-stderr-nocaret,nonsense"L);
     assert!(f.test(FeatureFlag::stderr_nocaret));
 
     // Ensure every metadata is represented once.
-    let mut counts: [usize; metadata.len()] = [0; metadata.len()];
+    let mut counts: [usize; METADATA.len()] = [0; METADATA.len()];
-    for md in &metadata {
+    for md in &METADATA {
         counts[md.flag.repr as usize] += 1;
     }
     for count in counts {
@@ -247,7 +213,7 @@ fn test_feature_flags() {
     }
 
     assert_eq!(
-        metadata[FeatureFlag::stderr_nocaret.repr as usize].name,
+        METADATA[FeatureFlag::stderr_nocaret.repr as usize].name,
         "stderr-nocaret"L
     );
 }

src/fish.cpp

@@ -500,10 +500,10 @@ int main(int argc, char **argv) {
     // command line takes precedence).
     if (auto features_var = env_stack_t::globals().get(L"fish_features")) {
         for (const wcstring &s : features_var->as_list()) {
-            mutable_fish_features()->set_from_string(s.c_str());
+            feature_set_from_string(s.c_str());
         }
     }
-    mutable_fish_features()->set_from_string(opts.features.c_str());
+    feature_set_from_string(opts.features.c_str());
     proc_init();
     misc_init();
     reader_init();

src/fish_indent.cpp

@@ -300,7 +300,7 @@ int main(int argc, char *argv[]) {
 
     if (auto features_var = env_stack_t::globals().get(L"fish_features")) {
         for (const wcstring &s : features_var->as_list()) {
-            mutable_fish_features()->set_from_string(s.c_str());
+            feature_set_from_string(s.c_str());
         }
     }
 

src/fish_tests.cpp

@@ -2473,15 +2473,14 @@ static void test_wildcards() {
     unescape_string_in_place(&wc, UNESCAPE_SPECIAL);
     do_test(!wildcard_has(wc) && wildcard_has_internal(wc));
 
-    auto feat = mutable_fish_features();
+    auto saved = feature_test(feature_flag_t::qmark_noglob);
-    auto saved = feat->test(feature_flag_t::qmark_noglob);
+    feature_set(feature_flag_t::qmark_noglob, false);
-    feat->set(feature_flag_t::qmark_noglob, false);
     do_test(wildcard_has(L"?"));
     do_test(!wildcard_has(L"\\?"));
-    feat->set(feature_flag_t::qmark_noglob, true);
+    feature_set(feature_flag_t::qmark_noglob, true);
     do_test(!wildcard_has(L"?"));
     do_test(!wildcard_has(L"\\?"));
-    feat->set(feature_flag_t::qmark_noglob, saved);
+    feature_set(feature_flag_t::qmark_noglob, saved);
 }
 
 static void test_complete() {
@@ -4904,7 +4903,7 @@ static void test_highlighting() {
 #endif
 
     bool saved_flag = feature_test(feature_flag_t::ampersand_nobg_in_token);
-    mutable_fish_features()->set(feature_flag_t::ampersand_nobg_in_token, true);
+    feature_set(feature_flag_t::ampersand_nobg_in_token, true);
     for (const highlight_component_list_t &components : highlight_tests) {
         // Generate the text.
         wcstring text;
@@ -4949,7 +4948,7 @@ static void test_highlighting() {
             }
         }
     }
-    mutable_fish_features()->set(feature_flag_t::ampersand_nobg_in_token, saved_flag);
+    feature_set(feature_flag_t::ampersand_nobg_in_token, saved_flag);
     vars.remove(L"VARIABLE_IN_COMMAND", ENV_DEFAULT);
     vars.remove(L"VARIABLE_IN_COMMAND2", ENV_DEFAULT);
 }
@@ -5359,7 +5358,7 @@ static void test_string() {
         {{L"string", L"match", L"?*", L"a", nullptr}, STATUS_CMD_ERROR, L""},
         {{L"string", L"match", L"?*", L"ab", nullptr}, STATUS_CMD_ERROR, L""},
         {{L"string", L"match", L"a*\\?", L"abc?", nullptr}, STATUS_CMD_ERROR, L""}};
-    mutable_fish_features()->set(feature_flag_t::qmark_noglob, true);
+    feature_set(feature_flag_t::qmark_noglob, true);
     for (const auto &t : qmark_noglob_tests) {
         run_one_string_test(t.argv, t.expected_rc, t.expected_out);
     }
@@ -5371,11 +5370,11 @@ static void test_string() {
         {{L"string", L"match", L"?*", L"a", nullptr}, STATUS_CMD_OK, L"a\n"},
         {{L"string", L"match", L"?*", L"ab", nullptr}, STATUS_CMD_OK, L"ab\n"},
         {{L"string", L"match", L"a*\\?", L"abc?", nullptr}, STATUS_CMD_OK, L"abc?\n"}};
-    mutable_fish_features()->set(feature_flag_t::qmark_noglob, false);
+    feature_set(feature_flag_t::qmark_noglob, false);
     for (const auto &t : qmark_glob_tests) {
         run_one_string_test(t.argv, t.expected_rc, t.expected_out);
     }
-    mutable_fish_features()->set(feature_flag_t::qmark_noglob, saved_flag);
+    feature_set(feature_flag_t::qmark_noglob, saved_flag);
 }
 
 /// Helper for test_timezone_env_vars().

src/future_feature_flags.h

@@ -3,6 +3,5 @@
 
 #include "future_feature_flags.rs.h"
 using feature_flag_t = FeatureFlag;
-using features_t = Features;
 
 #endif