Change option lists to be specified as separate arguments on commands line

Option lists and maps are specified using separate arguments, avoiding
the need for additional escaping of their separator and reusing the
existing command line spliting logic instead.

As discussed on #2087, this should make it much easier to work with
list options, and make the general option system feel cleaner.

src/command_manager.cc

@@ -253,9 +253,31 @@ Token parse_percent_token(Reader& reader, bool throw_on_unterminated)
     }
 }
 
-String expand_token(const Token& token, const Context& context,
+auto expand_option(Option& opt, std::true_type)
-                    const ShellContext& shell_context)
 {
+    return opt.get_as_string();
+}
+
+auto expand_option(Option& opt, std::false_type)
+{
+    return opt.get_as_strings();
+}
+
+String expand_arobase(ConstArrayView<String> params, std::true_type)
+{
+    return join(params, ' ', false);
+}
+
+Vector<String> expand_arobase(ConstArrayView<String> params, std::false_type)
+{
+    return {params.begin(), params.end()};
+}
+
+template<bool single>
+std::conditional_t<single, String, Vector<String>>
+expand_token(const Token& token, const Context& context, const ShellContext& shell_context)
+{
+    using IsSingle = std::integral_constant<bool, single>;
     auto& content = token.content;
     switch (token.type)
     {
@@ -273,35 +295,35 @@ String expand_token(const Token& token, const Context& context,
             ++trailing_eol_count;
         }
         str.resize(str.length() - trailing_eol_count, 0);
-        return str;
+        return {str};
     }
     case Token::Type::RegisterExpand:
-        return context.main_sel_register_value(content).str();
+        return {context.main_sel_register_value(content).str()};
     case Token::Type::OptionExpand:
-        return context.options()[content].get_as_string();
+        return expand_option(context.options()[content], IsSingle{});
     case Token::Type::ValExpand:
     {
         auto it = shell_context.env_vars.find(content);
         if (it != shell_context.env_vars.end())
-            return it->value;
+            return {it->value};
-        return ShellManager::instance().get_val(content, context);
+        return {ShellManager::instance().get_val(content, context)};
     }
     case Token::Type::ArgExpand:
     {
         auto& params = shell_context.params;
         if (content == '@')
-            return join(params, ' ');
+            return expand_arobase(params, IsSingle{});
 
         const int arg = str_to_int(content)-1;
         if (arg < 0)
             throw runtime_error("invalid argument index");
-        return arg < params.size() ? params[arg] : String{};
+        return {arg < params.size() ? params[arg] : String{}};
     }
     case Token::Type::RawEval:
-        return expand(content, context, shell_context);
+        return {expand(content, context, shell_context)};
     case Token::Type::Raw:
     case Token::Type::RawQuoted:
-        return content;
+        return {content};
     default: kak_assert(false);
     }
     return {};
@@ -378,8 +400,7 @@ String expand_impl(StringView str, const Context& context,
             else
             {
                 res += reader.substr_from(beg);
-                res += postprocess(expand_token(parse_percent_token(reader, true),
+                res += postprocess(expand_token<true>(parse_percent_token(reader, true), context, shell_context));
-                                                context, shell_context));
                 beg = reader.pos;
             }
         }
@@ -483,7 +504,12 @@ void CommandManager::execute(StringView command_line,
             params.insert(params.end(), shell_context.params.begin(),
                           shell_context.params.end());
         else
-            params.push_back(expand_token(token, context, shell_context));
+        {
+            auto tokens = expand_token<false>(token, context, shell_context);
+            params.insert(params.end(),
+                          std::make_move_iterator(tokens.begin()),
+                          std::make_move_iterator(tokens.end()));
+        }
     }
     execute_single_command(params, context, shell_context, command_coord);
 }
@@ -640,7 +666,7 @@ Completions CommandManager::complete(const Context& context,
             context, flags, params, tokens.size() - 2,
             cursor_pos_in_token), start);
 
-        if (token.type != Token::Type::RawQuoted)
+        if (not completions.quoted and token.type != Token::Type::RawQuoted)
         {
             StringView to_escape = token.type == Token::Type::Raw ? "% \t;" : "%";
             for (auto& candidate : completions.candidates)

src/commands.cc

@@ -1318,7 +1318,7 @@ const CommandDesc set_option_cmd = {
     "scope the option is set in",
     ParameterDesc{
         { { "add", { false, "add to option rather than replacing it" } } },
-        ParameterDesc::Flags::SwitchesOnlyAtStart, 3, 3
+        ParameterDesc::Flags::SwitchesOnlyAtStart, 2, (size_t)-1
     },
     CommandFlags::None,
     option_doc_helper,
@@ -1337,13 +1337,11 @@ const CommandDesc set_option_cmd = {
         else if (token_to_complete == start + 1)
             return { 0_byte, params[start + 1].length(),
                      GlobalScope::instance().option_registry().complete_option_name(params[start + 1], pos_in_token) };
-        else if (not add and token_to_complete == start + 2  and
+        else if (not add and token_to_complete == start + 2  and params[start + 2].empty() and
                  GlobalScope::instance().option_registry().option_exists(params[start + 1]))
         {
             OptionManager& options = get_scope(params[start], context).options();
-            String val = options[params[start + 1]].get_as_string();
+            return { 0_byte, params[start + 2].length(), { options[params[start + 1]].get_as_string() }, true };
-            if (prefix_match(val, params[start + 2]))
-                return { 0_byte, params[start + 2].length(), { std::move(val) } };
         }
         return Completions{};
     },
@@ -1351,9 +1349,9 @@ const CommandDesc set_option_cmd = {
     {
         Option& opt = get_options(parser[0], context, parser[1]).get_local_option(parser[1]);
         if (parser.get_switch("add"))
-            opt.add_from_string(parser[2]);
+            opt.add_from_strings(parser.positionals_from(2));
         else
-            opt.set_from_string(parser[2]);
+            opt.set_from_strings(parser.positionals_from(2));
     }
 };
 
@@ -1427,7 +1425,7 @@ const CommandDesc declare_option_cmd = {
     ParameterDesc{
         { { "hidden",    { false, "do not display option name when completing" } },
           { "docstring", { true,  "specify option description" } } },
-        ParameterDesc::Flags::SwitchesOnlyAtStart, 2, 3
+        ParameterDesc::Flags::SwitchesOnlyAtStart, 2, (size_t)-1
     },
     CommandFlags::None,
     CommandHelper{},
@@ -1470,8 +1468,8 @@ const CommandDesc declare_option_cmd = {
         else
             throw runtime_error(format("no such option type: '{}'", parser[0]));
 
-        if (parser.positional_count() == 3)
+        if (parser.positional_count() > 2)
-            opt->set_from_string(parser[2]);
+            opt->set_from_strings(parser.positionals_from(2));
     }
 };
 

src/completion.hh

@@ -21,6 +21,7 @@ struct Completions
     CandidateList candidates;
     ByteCount start;
     ByteCount end;
+    bool quoted = false;
 
     Completions()
         : start(0), end(0) {}
@@ -28,8 +29,8 @@ struct Completions
     Completions(ByteCount start, ByteCount end)
         : start(start), end(end) {}
 
-    Completions(ByteCount start, ByteCount end, CandidateList candidates)
+    Completions(ByteCount start, ByteCount end, CandidateList candidates, bool quoted = false)
-        : candidates(std::move(candidates)), start(start), end(end) {}
+        : candidates(std::move(candidates)), start(start), end(end), quoted{quoted} {}
 };
 
 enum class CompletionFlags
@@ -56,7 +57,7 @@ Completions shell_complete(const Context& context, CompletionFlags,
 inline Completions offset_pos(Completions completion, ByteCount offset)
 {
     return { completion.start + offset, completion.end + offset,
-             std::move(completion.candidates) };
+             std::move(completion.candidates), completion.quoted };
 }
 
 template<typename Container>

src/option.hh

@@ -16,6 +16,33 @@ inline String option_to_string(int opt);
 inline String option_to_string(size_t opt);
 inline String option_to_string(bool opt);
 
+// Default fallback to single value functions
+template<typename T>
+decltype(option_from_string(Meta::Type<T>{}, StringView{}))
+option_from_strings(Meta::Type<T>, ConstArrayView<String> strs)
+{
+    if (strs.size() != 1)
+        throw runtime_error("expected a single value for option");
+    return option_from_string(Meta::Type<T>{}, strs[0]);
+}
+
+template<typename T>
+Vector<decltype(option_to_string(std::declval<T>()))>
+option_to_strings(const T& opt)
+{
+    return Vector<String>{option_to_string(opt)};
+}
+
+template<typename T>
+decltype(option_add(std::declval<T>(), std::declval<String>()))
+option_add_from_strings(T& opt, ConstArrayView<String> strs)
+{
+    if (strs.size() != 1)
+        throw runtime_error("expected a single value for option");
+    return option_add(opt, strs[0]);
+}
+
+
 template<typename P, typename T>
 struct PrefixedList
 {

src/option_manager.hh

@@ -54,9 +54,10 @@ public:
     template<typename T> bool is_of_type() const;
 
     virtual String get_as_string() const = 0;
-    virtual void   set_from_string(StringView str) = 0;
+    virtual Vector<String> get_as_strings() const = 0;
-    virtual void   add_from_string(StringView str) = 0;
+    virtual void set_from_strings(ConstArrayView<String> strs) = 0;
-    virtual void   update(const Context& context) = 0;
+    virtual void add_from_strings(ConstArrayView<String> strs) = 0;
+    virtual void update(const Context& context) = 0;
 
     virtual Option* clone(OptionManager& manager) const = 0;
     OptionManager& manager() const { return m_manager; }
@@ -141,19 +142,27 @@ public:
     const T& get() const { return m_value; }
     T& get_mutable() { return m_value; }
 
+    Vector<String> get_as_strings() const override
+    {
+        return option_to_strings(m_value);
+    }
+
     String get_as_string() const override
     {
         return option_to_string(m_value);
     }
-    void set_from_string(StringView str) override
+
+    void set_from_strings(ConstArrayView<String> strs) override
     {
-        set(option_from_string(Meta::Type<T>{}, str));
+        set(option_from_strings(Meta::Type<T>{}, strs));
     }
-    void add_from_string(StringView str) override
+
+    void add_from_strings(ConstArrayView<String> strs) override
     {
-        if (option_add(m_value, str))
+        if (option_add_from_strings(m_value, strs))
             m_manager.on_option_changed(*this);
     }
+
     void update(const Context& context) override
     {
         option_update(m_value, context);

src/option_types.cc

@@ -5,19 +5,20 @@ namespace Kakoune
 {
 
 UnitTest test_option_parsing{[]{
-    auto check = [](auto&& value, StringView str)
+    auto check = [](auto&& value, ConstArrayView<String> strs)
     {
-        auto repr = option_to_string(value);
+        auto repr = option_to_strings(value);
-        kak_assert(repr == str);
+        kak_assert(strs == ConstArrayView<String>{repr});
-        auto parsed = option_from_string(Meta::Type<std::decay_t<decltype(value)>>{}, str);
+        auto parsed = option_from_strings(Meta::Type<std::decay_t<decltype(value)>>{}, strs);
         kak_assert(parsed == value);
     };
 
-    check(123, "123");
+    check(123, {"123"});
-    check(true, "true");
+    check(true, {"true"});
-    check(Vector<String>{"foo", "bar:", "baz"}, "foo:bar\\::baz");
+    check(Vector<String>{"foo", "bar:", "baz"}, {"foo", "bar:", "baz"});
-    check(HashMap<String, int>{{"foo", 10}, {"b=r", 20}, {"b:z", 30}}, "foo=10:b\\=r=20:b\\:z=30");
+    check(Vector<int>{10, 20, 30}, {"10", "20", "30"});
-    check(DebugFlags::Keys | DebugFlags::Hooks, "hooks|keys");
+    check(HashMap<String, int>{{"foo", 10}, {"b=r", 20}, {"b:z", 30}}, {"foo=10", "b\\=r=20", "b:z=30"});
+    check(DebugFlags::Keys | DebugFlags::Hooks, {"hooks|keys"});
 }};
 
 }

src/option_types.hh

@@ -18,6 +18,12 @@
 namespace Kakoune
 {
 
+
+inline String quote(StringView s)
+{
+    return format("'{}'", replace(s, "'", "''"));
+}
+
 template<typename T>
 constexpr decltype(T::option_type_name) option_type_name(Meta::Type<T>)
 {
@@ -67,13 +73,16 @@ inline Codepoint option_from_string(Meta::Type<Codepoint>, StringView str)
 }
 constexpr StringView option_type_name(Meta::Type<Codepoint>) { return "codepoint"; }
 
-constexpr char list_separator = ':';
+template<typename T, MemoryDomain domain>
+Vector<String> option_to_strings(const Vector<T, domain>& opt)
+{
+    return opt | transform([](const T& t) { return option_to_string(t); }) | gather<Vector<String>>();
+}
 
 template<typename T, MemoryDomain domain>
 String option_to_string(const Vector<T, domain>& opt)
 {
-    return join(opt | transform([](const T& t) { return option_to_string(t); }),
+    return join(opt | transform([](const T& t) { return quote(option_to_string(t)); }), ' ', false);
-                list_separator);
 }
 
 template<typename T, MemoryDomain domain>
@@ -81,20 +90,18 @@ void option_list_postprocess(Vector<T, domain>& opt)
 {}
 
 template<typename T, MemoryDomain domain>
-Vector<T, domain> option_from_string(Meta::Type<Vector<T, domain>>, StringView str)
+Vector<T, domain> option_from_strings(Meta::Type<Vector<T, domain>>, ConstArrayView<String> strs)
 {
-    auto res =  str | split<StringView>(list_separator, '\\')
+    auto res =  strs | transform([](auto&& s) { return option_from_string(Meta::Type<T>{}, s); })
-                    | transform(unescape<list_separator, '\\'>)
+                     | gather<Vector<T, domain>>();
-                    | transform([](auto&& s) { return option_from_string(Meta::Type<T>{}, s); })
-                    | gather<Vector<T, domain>>();
     option_list_postprocess(res);
     return res;
 }
 
 template<typename T, MemoryDomain domain>
-bool option_add(Vector<T, domain>& opt, StringView str)
+bool option_add_from_strings(Vector<T, domain>& opt, ConstArrayView<String> strs)
 {
-    auto vec = option_from_string(Meta::Type<Vector<T, domain>>{}, str);
+    auto vec = option_from_strings(Meta::Type<Vector<T, domain>>{}, strs);
     opt.insert(opt.end(),
                std::make_move_iterator(vec.begin()),
                std::make_move_iterator(vec.end()));
@@ -109,31 +116,35 @@ String option_type_name(Meta::Type<Vector<T, D>>)
 }
 
 template<typename Key, typename Value, MemoryDomain domain>
-String option_to_string(const HashMap<Key, Value, domain>& opt)
+Vector<String> option_to_strings(const HashMap<Key, Value, domain>& opt)
 {
-    String res;
+    return opt | transform([](auto&& item) {
-    for (auto it = opt.begin(); it != opt.end(); ++it)
+        return format("{}={}",
-    {
+                      escape(option_to_string(item.key), '=', '\\'),
-        if (it != opt.begin())
+                      escape(option_to_string(item.value), '=', '\\'));
-            res += list_separator;
+    }) | gather<Vector<String>>();
-        String elem = escape(option_to_string(it->key), '=', '\\') + "=" +
-                      escape(option_to_string(it->value), '=', '\\');
-        res += escape(elem, list_separator, '\\');
-    }
-    return res;
 }
 
 template<typename Key, typename Value, MemoryDomain domain>
-bool option_add(HashMap<Key, Value, domain>& opt, StringView str)
+String option_to_string(const HashMap<Key, Value, domain>& opt)
+{
+    return join(opt | transform([](auto&& item) {
+        return quote(format("{}={}",
+                      escape(option_to_string(item.key), '=', '\\'),
+                      escape(option_to_string(item.value), '=', '\\')));
+    }), ' ', false);
+}
+
+template<typename Key, typename Value, MemoryDomain domain>
+bool option_add_from_strings(HashMap<Key, Value, domain>& opt, ConstArrayView<String> strs)
 {
     bool changed = false;
-    for (auto&& elem : str | split<StringView>(list_separator, '\\')
+    for (auto&& str : strs)
-                           | transform(unescape<list_separator, '\\'>))
     {
         struct error : runtime_error { error(size_t) : runtime_error{"map option expects key=value"} {} };
-        auto key_value = elem | split<StringView>('=', '\\')
+        auto key_value = str | split<StringView>('=', '\\')
-                              | transform(unescape<'=', '\\'>)
+                             | transform(unescape<'=', '\\'>)
-                              | static_gather<error, 2>();
+                             | static_gather<error, 2>();
 
         opt[option_from_string(Meta::Type<Key>{}, key_value[0])] = option_from_string(Meta::Type<Value>{}, key_value[1]);
         changed = true;
@@ -142,10 +153,10 @@ bool option_add(HashMap<Key, Value, domain>& opt, StringView str)
 }
 
 template<typename Key, typename Value, MemoryDomain domain>
-HashMap<Key, Value, domain> option_from_string(Meta::Type<HashMap<Key, Value, domain>>, StringView str)
+HashMap<Key, Value, domain> option_from_strings(Meta::Type<HashMap<Key, Value, domain>>, ConstArrayView<String> str)
 {
     HashMap<Key, Value, domain> res;
-    option_add(res, str);
+    option_add_from_strings(res, str);
     return res;
 }
 
@@ -304,29 +315,32 @@ EnableIfWithBitOps<Flags, bool> option_add(Flags& opt, StringView str)
     return res != (Flags)0;
 }
 
+template<typename P, typename T>
+inline Vector<String> option_to_strings(const PrefixedList<P, T>& opt)
+{
+    Vector<String> res{option_to_string(opt.prefix)};
+    auto list = option_to_strings(opt.list);
+    res.insert(res.end(), std::make_move_iterator(list.begin()), std::make_move_iterator(list.end()));
+    return res;
+}
+
 template<typename P, typename T>
 inline String option_to_string(const PrefixedList<P, T>& opt)
 {
-    if (opt.list.empty())
+    return option_to_string(opt.prefix) + " " + option_to_string(opt.list);
-        return format("{}", escape(option_to_string(opt.prefix), list_separator, '\\'));
-    else
-        return format("{}{}{}", escape(option_to_string(opt.prefix), list_separator, '\\'),
-                      list_separator, option_to_string(opt.list));
 }
 
 template<typename P, typename T>
-inline PrefixedList<P, T> option_from_string(Meta::Type<PrefixedList<P, T>>, StringView str)
+inline PrefixedList<P, T> option_from_strings(Meta::Type<PrefixedList<P, T>>, ConstArrayView<String> strs)
 {
-    using VecType = Vector<T, MemoryDomain::Options>;
+    return {option_from_string(Meta::Type<P>{}, strs[0]), 
-    auto it = find(str, list_separator);
+            option_from_strings(Meta::Type<Vector<T, MemoryDomain::Options>>{}, strs.subrange(1))};
-    return {option_from_string(Meta::Type<P>{}, StringView{str.begin(), it}), 
-            it != str.end() ? option_from_string(Meta::Type<VecType>{}, {it+1, str.end()}) : VecType{}};
 }
 
 template<typename P, typename T>
-inline bool option_add(PrefixedList<P, T>& opt, StringView str)
+inline bool option_add_from_strings(PrefixedList<P, T>& opt, ConstArrayView<String> str)
 {
-    return option_add(opt.list, str);
+    return option_add_from_strings(opt.list, str);
 }
 
 }

src/parameters_parser.hh

@@ -6,6 +6,7 @@
 #include "meta.hh"
 #include "array_view.hh"
 #include "optional.hh"
+#include "flags.hh"
 #include "string.hh"
 #include "string_utils.hh"
 
@@ -115,6 +116,12 @@ struct ParametersParser
         return m_params[m_positional_indices[index]];
     }
 
+    ConstArrayView<String> positionals_from(size_t first) const
+    {
+        kak_assert(m_desc.flags & (ParameterDesc::Flags::SwitchesOnlyAtStart | ParameterDesc::Flags::SwitchesAsPositional));
+        return m_params.subrange(m_positional_indices[first]);
+    }
+
     iterator begin() const { return iterator(*this, 0); }
     iterator end() const { return iterator(*this, m_positional_indices.size()); }