From 9e1c66daf72ef7f629ca954d78654c70248c1b20 Mon Sep 17 00:00:00 2001
From: Tim Chevalier <tjc@igalia.com>
Date: Fri, 29 Mar 2024 14:54:26 -0700
Subject: [PATCH] ICU-22261 Refactor MF2 attributes and options parsing code

Previously, there were separate overrides for the options and
attributes parsing methods in the parser that were used in different
context. (Options can appear in Operator and Markup, while attributes
can appear in Expression and Markup.)

This is a refactoring that eliminates this duplicated code.
To enable it, a builder is added for the internal OptionMap type.

Separately, this patch also explicitly deletes copy constructors
and copy assignment operators for all Builder classes; a bug in an
earlier version of this patch caused me to notice this hadn't been
done. Also explicitly deletes move constructors/assignment operators
with the exception of OptionMap::Builder (OptionMap is non-public,
so that shouldn't cause confusion).
---
 .../source/i18n/messageformat2_data_model.cpp | 178 +++++++--------
 icu4c/source/i18n/messageformat2_parser.cpp   | 202 ++----------------
 icu4c/source/i18n/messageformat2_parser.h     |  42 +++-
 .../i18n/unicode/messageformat2_data_model.h  |  96 +++++----
 4 files changed, 208 insertions(+), 310 deletions(-)

diff --git a/icu4c/source/i18n/messageformat2_data_model.cpp b/icu4c/source/i18n/messageformat2_data_model.cpp
index b29f29e8134..822d20c3b84 100644
--- a/icu4c/source/i18n/messageformat2_data_model.cpp
+++ b/icu4c/source/i18n/messageformat2_data_model.cpp
@@ -262,7 +262,7 @@ OptionMap::OptionMap(const UVector& opts, UErrorCode& status) {
 
     len = opts.size();
     Option* result = copyVectorToArray<Option>(opts, len);
-    if (result == nullptr) {
+    if (result == nullptr && len != 0) {
         bogus = true;
         status = U_MEMORY_ALLOCATION_ERROR;
         return;
@@ -304,14 +304,64 @@ int32_t OptionMap::size() const {
     return len;
 }
 
-Operator::Builder& Operator::Builder::setOptionMap(OptionMap&& m) {
-    optionMap = std::move(m);
-    delete options;
-    options = nullptr;
+OptionMap::~OptionMap() {}
+
+OptionMap OptionMap::Builder::build(UErrorCode& status) {
+    return OptionMap(*options, status);
+}
+
+OptionMap::Builder::Builder(UErrorCode& status) {
+    options = createStringUVector(status);
+}
+
+OptionMap::Builder::Builder(OptionMap::Builder&& other) {
+    checkDuplicates = other.checkDuplicates;
+    options = other.options;
+    other.options = nullptr;
+}
+
+OptionMap::Builder& OptionMap::Builder::operator=(OptionMap::Builder other) noexcept {
+    swap(*this, other);
     return *this;
 }
 
-OptionMap::~OptionMap() {}
+/* static */ OptionMap::Builder OptionMap::Builder::attributes(UErrorCode& status) {
+    Builder b(status);
+    // The same code is re-used for representing attributes and options.
+    // Duplicate attributes are allowed, while duplicate options are disallowed.
+    b.checkDuplicates = false;
+    return b;
+}
+
+static UBool hasOptionNamed(const UVector& v, const UnicodeString& s) {
+    for (int32_t i = 0; i < v.size(); i++) {
+        const Option* opt = static_cast<Option*>(v[i]);
+        U_ASSERT(opt != nullptr);
+        if (opt->getName() == s) {
+            return true;
+        }
+    }
+    return false;
+}
+
+OptionMap::Builder& OptionMap::Builder::add(Option&& opt, UErrorCode& status) {
+    THIS_ON_ERROR(status);
+
+    // If the option name is already in the map, emit a data model error
+    if (checkDuplicates && hasOptionNamed(*options, opt.getName())) {
+        status = U_MF_DUPLICATE_OPTION_NAME_ERROR;
+    } else {
+        Option* newOption = create<Option>(std::move(opt), status);
+        options->adoptElement(newOption, status);
+    }
+    return *this;
+}
+
+OptionMap::Builder::~Builder() {
+    if (options != nullptr) {
+        delete options;
+    }
+}
 
 const Reserved& Operator::asReserved() const {
     U_ASSERT(isReserved());
@@ -332,13 +382,12 @@ Option& Option::operator=(Option other) noexcept {
 
 Option::~Option() {}
 
-Operator::Builder::Builder(UErrorCode& status) {
-    options = createStringUVector(status);
-}
+Operator::Builder::Builder(UErrorCode& status) : options(OptionMap::Builder(status)) {}
 
 Operator::Builder& Operator::Builder::setReserved(Reserved&& reserved) {
     isReservedSequence = true;
     hasFunctionName = false;
+    hasOptions = false;
     asReserved = std::move(reserved);
     return *this;
 }
@@ -355,31 +404,12 @@ const FunctionName& Operator::getFunctionName() const {
     return std::get_if<Callable>(&contents)->getName();
 }
 
-static UBool hasOptionNamed(const UVector& v, const UnicodeString& s) {
-    for (int32_t i = 0; i < v.size(); i++) {
-        const Option* opt = static_cast<Option*>(v[i]);
-        U_ASSERT(opt != nullptr);
-        if (opt->getName() == s) {
-            return true;
-        }
-    }
-    return false;
-}
-
 Operator::Builder& Operator::Builder::addOption(const UnicodeString &key, Operand&& value, UErrorCode& errorCode) noexcept {
     THIS_ON_ERROR(errorCode);
 
     isReservedSequence = false;
     hasOptions = true;
-    U_ASSERT(options != nullptr);
-    // If the option name is already in the map, emit a data model error
-    if (hasOptionNamed(*options, key)) {
-        errorCode = U_MF_DUPLICATE_OPTION_NAME_ERROR;
-    } else {
-        Option* newOption = create<Option>(Option(key, std::move(value)), errorCode);
-        THIS_ON_ERROR(errorCode);
-        options->adoptElement(newOption, errorCode);
-    }
+    options.add(Option(key, std::move(value)), errorCode);
     return *this;
 }
 
@@ -404,11 +434,7 @@ Operator Operator::Builder::build(UErrorCode& errorCode) {
             errorCode = U_INVALID_STATE_ERROR;
             return result;
         }
-        if (options != nullptr) {
-            // Initialize options from what was set with setOptions()
-            optionMap = OptionMap(*options, errorCode);
-         }
-        result = Operator(functionName, optionMap);
+        result = Operator(functionName, options.build(errorCode));
     }
     return result;
 }
@@ -425,11 +451,7 @@ Operator::Operator(const FunctionName& f, const UVector& optsVector, UErrorCode&
 
 Operator::Operator(const FunctionName& f, const OptionMap& opts) : contents(Callable(f, opts)) {}
 
-Operator::Builder::~Builder() {
-    if (options != nullptr) {
-        delete options;
-    }
-}
+Operator::Builder::~Builder() {}
 
 Operator::~Operator() {}
 
@@ -444,28 +466,26 @@ Callable::~Callable() {}
 
 // ------------ Markup
 
-Markup::Builder::Builder(UErrorCode& status) {
-    options = createUVector(status);
-    attributes = createUVector(status);
-}
+Markup::Builder::Builder(UErrorCode& status)
+    : options(OptionMap::Builder(status)), attributes(OptionMap::Builder::attributes(status)) {}
 
-Markup::Builder& Markup::Builder::setAttributeMap(OptionMap&& m) {
-    attributeMap = std::move(m);
-    delete attributes;
-    attributes = nullptr;
+Markup::Markup(UMarkupType ty, UnicodeString n, OptionMap&& o, OptionMap&& a)
+    : type(ty), name(n), options(std::move(o)), attributes(std::move(a)) {}
+
+Markup::Builder& Markup::Builder::addOption(const UnicodeString &key,
+                                            Operand&& value,
+                                            UErrorCode& errorCode) {
+    options.add(Option(key, std::move(value)), errorCode);
     return *this;
 }
 
-Markup::Builder& Markup::Builder::setOptionMap(OptionMap&& m) {
-    optionMap = std::move(m);
-    delete options;
-    options = nullptr;
+Markup::Builder& Markup::Builder::addAttribute(const UnicodeString &key,
+                                               Operand&& value,
+                                               UErrorCode& errorCode) {
+    attributes.add(Option(key, std::move(value)), errorCode);
     return *this;
 }
 
-Markup::Markup(UMarkupType ty, UnicodeString n, OptionMap&& o, OptionMap&& a) : type(ty), name(n), options(std::move(o)), attributes(std::move(a)) {
-}
-
 Markup Markup::Builder::build(UErrorCode& errorCode) {
     Markup result;
 
@@ -479,34 +499,22 @@ Markup Markup::Builder::build(UErrorCode& errorCode) {
         // setName() must be called before calling build()
         errorCode = U_INVALID_STATE_ERROR;
     } else {
-        if (options != nullptr) {
-            // Initialize options from what was done with setOptions
-            optionMap = OptionMap(*options, errorCode);
-        }
-        if (attributes != nullptr) {
-            attributeMap = OptionMap(*attributes, errorCode);
-        }
-        result = Markup(type, name, std::move(optionMap), std::move(attributeMap));
+        result = Markup(type,
+                        name,
+                        options.build(errorCode),
+                        attributes.build(errorCode));
     }
     return result;
 }
 
-Markup::Builder::~Builder() {
-    if (options != nullptr) {
-        delete options;
-    }
-    if (attributes != nullptr) {
-        delete attributes;
-    }
-}
+Markup::Builder::~Builder() {}
 
 Markup::~Markup() {}
 
 // ------------ Expression
 
-Expression::Builder::Builder(UErrorCode& status) {
-    attributes = createUVector(status);
-}
+Expression::Builder::Builder(UErrorCode& status)
+    : attributes(OptionMap::Builder::attributes(status)) {}
 
 UBool Expression::isStandaloneAnnotation() const {
     return rand.isNull();
@@ -549,10 +557,10 @@ Expression::Builder& Expression::Builder::setOperator(Operator&& rAtor) {
     return *this;
 }
 
-Expression::Builder& Expression::Builder::setAttributeMap(OptionMap&& m) {
-    attributeMap = std::move(m);
-    delete attributes;
-    attributes = nullptr;
+Expression::Builder& Expression::Builder::addAttribute(const UnicodeString& k,
+                                                       Operand&& v,
+                                                       UErrorCode& status) {
+    attributes.add(Option(k, std::move(v)), status);
     return *this;
 }
 
@@ -568,16 +576,14 @@ Expression Expression::Builder::build(UErrorCode& errorCode) {
         return result;
     }
 
-    if (attributes != nullptr) {
-        attributeMap = OptionMap(*attributes, errorCode);
-    }
+    OptionMap attributeMap = attributes.build(errorCode);
     if (hasOperand && hasOperator) {
-        result = Expression(rator, rand, attributeMap);
+        result = Expression(rator, rand, std::move(attributeMap));
     } else if (hasOperand && !hasOperator) {
-        result = Expression(rand, attributeMap);
+        result = Expression(rand, std::move(attributeMap));
     } else {
         // rator is valid, rand is not valid
-        result = Expression(rator, attributeMap);
+        result = Expression(rator, std::move(attributeMap));
     }
     return result;
 }
@@ -591,11 +597,7 @@ Expression& Expression::operator=(Expression other) noexcept {
     return *this;
 }
 
-Expression::Builder::~Builder() {
-    if (attributes != nullptr) {
-        delete attributes;
-    }
-}
+Expression::Builder::~Builder() {}
 
 Expression::~Expression() {}
 
diff --git a/icu4c/source/i18n/messageformat2_parser.cpp b/icu4c/source/i18n/messageformat2_parser.cpp
index 1e3995fea97..15b185762dc 100644
--- a/icu4c/source/i18n/messageformat2_parser.cpp
+++ b/icu4c/source/i18n/messageformat2_parser.cpp
@@ -908,7 +908,8 @@ Literal Parser::parseLiteral(UErrorCode& errorCode) {
 
   Adds the option to `options`
 */
-void Parser::parseAttribute(UVector& options, UErrorCode& errorCode) {
+template<class T>
+void Parser::parseAttribute(AttributeAdder<T>& attrAdder, UErrorCode& errorCode) {
     U_ASSERT(inBounds(source, index));
 
     U_ASSERT(source[index] == AT);
@@ -950,26 +951,7 @@ void Parser::parseAttribute(UVector& options, UErrorCode& errorCode) {
         index = savedIndex;
     }
 
-    CHECK_ERROR(errorCode);
-
-    // No duplicate check -- the spec doesn't require one
-    Option* opt = new Option(lhs, std::move(rand));
-    if (opt == nullptr) {
-        errorCode = U_MEMORY_ALLOCATION_ERROR;
-        return;
-    }
-    options.adoptElement(static_cast<void*>(opt), errorCode);
-}
-
-// `options` is a vector of Option*
-static bool hasOption(const UVector& options, const UnicodeString& optionName) {
-    for (int32_t i = 0; i < options.size(); i++) {
-        const Option* opt = static_cast<Option*>(options.elementAt(i));
-        if (opt->getName() == optionName) {
-            return true;
-        }
-    }
-    return false;
+    attrAdder.addAttribute(lhs, std::move(rand), errorCode);
 }
 
 /*
@@ -977,50 +959,8 @@ static bool hasOption(const UVector& options, const UnicodeString& optionName) {
 
   Adds the option to `optionList`
 */
-void Parser::parseOption(UVector& options, UErrorCode& errorCode) {
-    U_ASSERT(inBounds(source, index));
-
-    // Parse LHS
-    UnicodeString lhs = parseIdentifier(errorCode);
-
-    // Parse '='
-    parseTokenWithWhitespace(EQUALS, errorCode);
-
-    UnicodeString rhsStr;
-    Operand rand;
-    // Parse RHS, which is either a literal or variable
-    switch (source[index]) {
-    case DOLLAR: {
-        rand = Operand(parseVariableName(errorCode));
-        break;
-    }
-    default: {
-        // Must be a literal
-        rand = Operand(parseLiteral(errorCode));
-        break;
-    }
-    }
-    U_ASSERT(!rand.isNull());
-
-    // Finally, add the key=value mapping
-    if (hasOption(options, lhs)) {
-        errors.setDuplicateOptionName(errorCode);
-    } else {
-        Option* opt = new Option(lhs, std::move(rand));
-        if (opt == nullptr) {
-            errorCode = U_MEMORY_ALLOCATION_ERROR;
-            return;
-        }
-        options.adoptElement(static_cast<void*>(opt), errorCode);
-    }
-}
-
-/*
-  Consume a name-value pair, matching the `option` nonterminal in the grammar.
-
-  Adds the option to `optionList`
-*/
-void Parser::parseOption(Operator::Builder &builder, UErrorCode& errorCode) {
+template<class T>
+void Parser::parseOption(OptionAdder<T>& addOption, UErrorCode& errorCode) {
     U_ASSERT(inBounds(source, index));
 
     // Parse LHS
@@ -1049,7 +989,7 @@ void Parser::parseOption(Operator::Builder &builder, UErrorCode& errorCode) {
     // Use a local error code, check for duplicate option error and
     // record it as with other errors
     UErrorCode status = U_ZERO_ERROR;
-    builder.addOption(lhs, std::move(rand), status);
+    addOption.addOption(lhs, std::move(rand), status);
     if (U_FAILURE(status)) {
       U_ASSERT(status == U_MF_DUPLICATE_OPTION_NAME_ERROR);
       errors.setDuplicateOptionName(errorCode);
@@ -1066,7 +1006,8 @@ void Parser::parseOption(Operator::Builder &builder, UErrorCode& errorCode) {
   Consume optional whitespace followed by a sequence of options
   (possibly empty), separated by whitespace
 */
-void Parser::parseOptions(Operator::Builder& builder, UErrorCode& errorCode) {
+template <class T>
+void Parser::parseOptions(OptionAdder<T>& addOption, UErrorCode& errorCode) {
     // Early exit if out of bounds -- no more work is possible
     CHECK_BOUNDS(source, index, parseError, errorCode);
 
@@ -1144,121 +1085,21 @@ an option or an attribute.
             index = firstWhitespace;
             break;
         }
-        parseOption(builder, errorCode);
+        parseOption(addOption, errorCode);
     }
 }
 
-/*
-  Consume optional whitespace followed by a sequence of options
-  (possibly empty), separated by whitespace
-*/
-OptionMap Parser::parseOptions(UErrorCode& errorCode) {
-    OptionMap options;
-
-    // Early exit if out of bounds -- no more work is possible
-    if (!inBounds(source, index)) {
-        ERROR(parseError, errorCode, index);
-        return options;
-    }
-
-    // Initialize options vector, which supports equality on elements
-    LocalPointer<UVector> optionVec(createStringUVector(errorCode));
-    if (U_FAILURE(errorCode)) {
-        return options;
-    }
-/*
-Arbitrary lookahead is required to parse option lists. To see why, consider
-these rules from the grammar:
-
-expression = "{" [s] (((literal / variable) [s annotation]) / annotation) [s] "}"
-annotation = (function *(s option)) / reserved
-
-And this example:
-{:foo  }
-
-Derivation:
-expression -> "{" [s] (((literal / variable) [s annotation]) / annotation) [s] "}"
-           -> "{" [s] annotation [s] "}"
-           -> "{" [s] ((function *(s option)) / reserved) [s] "}"
-           -> "{" [s] function *(s option) [s] "}"
-
-In this example, knowing whether to expect a '}' or the start of another option
-after the whitespace would require arbitrary lookahead -- in other words, which
-rule should we apply?
-    *(s option) -> s option *(s option)
-  or
-    *(s option) ->
-
-The same would apply to the example {:foo k=v } (note the trailing space after "v").
-
-This is addressed using a form of backtracking and (to make the backtracking easier
-to apply) a slight refactoring to the grammar.
-
-This code is written as if the grammar is:
-  expression = "{" [s] (((literal / variable) ([s] / [s annotation])) / annotation) "}"
-  annotation = (function *(s option) [s]) / (reserved [s])
-
-Parsing the `*(s option) [s]` sequence can be done within `parseOptions()`, meaning
-that `parseExpression()` can safely require a '}' after `parseOptions()` finishes.
-
-Note that when "backtracking" really just means early exit, since only whitespace
-is involved and there's no state to save.
-*/
-
-    while(true) {
-        // If the next character is not whitespace, that means we've already
-        // parsed the entire options list (which may have been empty) and there's
-        // no trailing whitespace. In that case, exit.
-        if (!isWhitespace(source[index])) {
-            break;
-        }
-
-        // In any case other than an empty options list, there must be at least
-        // one whitespace character.
-        parseRequiredWhitespace(errorCode);
-        // Restore precondition
-        if (!inBounds(source, index)) {
-            ERROR(parseError, errorCode, index);
-            break;
-        }
-
-        // If a name character follows, then at least one more option remains
-        // in the list.
-        // Otherwise, we've consumed all the options and any trailing whitespace,
-        // and can exit.
-        // Note that exiting is sort of like backtracking: "(s option)" doesn't apply,
-        // so we back out to [s].
-        if (!isNameStart(source[index])) {
-            // We've consumed all the options (meaning that either we consumed non-empty
-            // whitespace, or consumed at least one option.)
-            // Done.
-            // Remove the whitespace from normalizedInput
-            normalizedInput.truncate(normalizedInput.length() - 1);
-            break;
-        }
-        parseOption(*optionVec, errorCode);
-    }
-
-    return OptionMap(*optionVec, errorCode);
-}
-
 /*
   Consume optional whitespace followed by a sequence of attributes
   (possibly empty), separated by whitespace
 */
-OptionMap Parser::parseAttributes(UErrorCode& errorCode) {
-    OptionMap attributes;
+template<class T>
+void Parser::parseAttributes(AttributeAdder<T>& attrAdder, UErrorCode& errorCode) {
 
     // Early exit if out of bounds -- no more work is possible
     if (!inBounds(source, index)) {
         ERROR(parseError, errorCode, index);
-        return attributes;
-    }
-
-    // Initialize options vector, which supports equality on elements
-    LocalPointer<UVector> options(createStringUVector(errorCode));
-    if (U_FAILURE(errorCode)) {
-        return attributes;
+        return;
     }
 
 /*
@@ -1297,10 +1138,8 @@ Arbitrary lookahead is required to parse attribute lists, similarly to option li
             normalizedInput.truncate(normalizedInput.length() - 1);
             break;
         }
-        parseAttribute(*options, errorCode);
+        parseAttribute(attrAdder, errorCode);
     }
-
-    return OptionMap(*options, errorCode);
 }
 
 void Parser::parseReservedEscape(UnicodeString &str, UErrorCode& errorCode) {
@@ -1490,7 +1329,6 @@ Reserved Parser::parseReservedBody(Reserved::Builder& builder, UErrorCode& statu
     return builder.build(status);
 }
 
-
 /*
   Consume a function call or reserved string, matching the `annotation`
   nonterminal in the grammar
@@ -1507,8 +1345,10 @@ Operator Parser::parseAnnotation(UErrorCode& status) {
         // Consume the function name
         FunctionName func = parseFunction(status);
         ratorBuilder.setFunctionName(std::move(func));
+
+        OptionAdder<Operator::Builder> addOptions(ratorBuilder);
         // Consume the options (which may be empty)
-        parseOptions(ratorBuilder, status);
+        parseOptions(addOptions, status);
     } else {
       // Must be reserved
       // A reserved sequence is not a parse error, but might be a formatting error
@@ -1683,8 +1523,8 @@ Expression Parser::parseExpression(UErrorCode& status) {
     }
 
     // Parse attributes
-    OptionMap attributes = parseAttributes(status);
-    exprBuilder.setAttributeMap(std::move(attributes));
+    AttributeAdder attrAdder(exprBuilder);
+    parseAttributes(attrAdder, status);
 
     // Parse optional space
     // (the last [s] in e.g. "{" [s] literal [s annotation] *(s attribute) [s] "}")
@@ -2138,13 +1978,15 @@ Markup Parser::parseMarkup(UErrorCode& status) {
     // Parse the options, which must begin with a ' '
     // if present
     if (inBounds(source, index) && isWhitespace(source[index])) {
-        builder.setOptionMap(parseOptions(status));
+        OptionAdder<Markup::Builder> optionAdder(builder);
+        parseOptions(optionAdder, status);
     }
 
     // Parse the attributes, which also must begin
     // with a ' '
     if (inBounds(source, index) && isWhitespace(source[index])) {
-        builder.setAttributeMap(parseAttributes(status));
+        AttributeAdder attrAdder(builder);
+        parseAttributes(attrAdder, status);
     }
 
     parseOptionalWhitespace(status);
diff --git a/icu4c/source/i18n/messageformat2_parser.h b/icu4c/source/i18n/messageformat2_parser.h
index 81a81abf6fa..92c0475d67d 100644
--- a/icu4c/source/i18n/messageformat2_parser.h
+++ b/icu4c/source/i18n/messageformat2_parser.h
@@ -26,6 +26,34 @@ namespace message2 {
 
     using namespace data_model;
 
+    // Used for parameterizing options parsing code
+    // over the two builders that use it (Operator and Markup)
+    template <class T>
+    class OptionAdder {
+        private:
+            T& builder;
+        public:
+            OptionAdder(T& b) : builder(b) {}
+            void addOption(const UnicodeString& k, Operand&& r, UErrorCode& s) {
+                builder.addOption(k, std::move(r), s);
+            }
+    };
+
+    // Used for parameterizing attributes parsing code
+    // over the two builders that use it (Expression and Markup)
+    // Unfortunately the same OptionAdder class can't just be reused,
+    // becaues duplicate options are forbidden while duplicate attributes are not
+    template <class T>
+    class AttributeAdder {
+        private:
+            T& builder;
+        public:
+            AttributeAdder(T& b) : builder(b) {}
+            void addAttribute(const UnicodeString& k, Operand&& r, UErrorCode& s) {
+                builder.addAttribute(k, std::move(r), s);
+            }
+    };
+
     // Parser class (private)
     class Parser : public UMemory {
     public:
@@ -98,12 +126,14 @@ namespace message2 {
         Literal parseUnquotedLiteral(UErrorCode&);
         Literal parseQuotedLiteral(UErrorCode&);
 	Literal parseLiteral(UErrorCode&);
-        void parseAttribute(UVector&, UErrorCode&);
-        OptionMap parseAttributes(UErrorCode&);
-        void parseOption(Operator::Builder&, UErrorCode&);
-        void parseOption(UVector&, UErrorCode&);
-        void parseOptions(Operator::Builder&, UErrorCode&);
-        OptionMap parseOptions(UErrorCode&);
+        template<class T>
+        void parseAttribute(AttributeAdder<T>&, UErrorCode&);
+        template<class T>
+        void parseAttributes(AttributeAdder<T>&, UErrorCode&);
+        template<class T>
+        void parseOption(OptionAdder<T>&, UErrorCode&);
+        template<class T>
+        void parseOptions(OptionAdder<T>&, UErrorCode&);
 	void parseReservedEscape(UnicodeString&, UErrorCode&);
 	void parseReservedChunk(Reserved::Builder&, UErrorCode&);
 	Reserved parseReserved(UErrorCode&);
diff --git a/icu4c/source/i18n/unicode/messageformat2_data_model.h b/icu4c/source/i18n/unicode/messageformat2_data_model.h
index 281d5bf28e5..09d7af6aef7 100644
--- a/icu4c/source/i18n/unicode/messageformat2_data_model.h
+++ b/icu4c/source/i18n/unicode/messageformat2_data_model.h
@@ -169,6 +169,10 @@ namespace message2 {
                  * @deprecated This API is for technology preview only.
                  */
                 virtual ~Builder();
+                Builder(const Builder&) = delete;
+                Builder& operator=(const Builder&) = delete;
+                Builder(Builder&&) = delete;
+                Builder& operator=(Builder&&) = delete;
             }; // class Reserved::Builder
             /**
              * Non-member swap function.
@@ -745,6 +749,10 @@ namespace message2 {
                  * @deprecated This API is for technology preview only.
                  */
                 virtual ~Builder();
+                Builder(const Builder&) = delete;
+                Builder& operator=(const Builder&) = delete;
+                Builder(Builder&&) = delete;
+                Builder& operator=(Builder&&) = delete;
             }; // class SelectorKeys::Builder
             /**
              * Less than operator. Compares the two key lists lexicographically.
@@ -804,7 +812,6 @@ namespace message2 {
              * @deprecated This API is for technology preview only.
              */
             virtual ~SelectorKeys();
-
         private:
             friend class Builder;
             friend class message2::Checker;
@@ -961,6 +968,28 @@ namespace message2 {
             OptionMap(const UVector&, UErrorCode&);
             OptionMap(Option*, int32_t);
             virtual ~OptionMap();
+
+            class U_I18N_API Builder : public UObject {
+                private:
+                    UVector* options;
+                    bool checkDuplicates = true;
+                public:
+                    Builder& add(Option&& opt, UErrorCode&);
+                    Builder(UErrorCode&);
+                    static Builder attributes(UErrorCode&);
+                    // As this class is private, build() is destructive
+                    OptionMap build(UErrorCode&);
+		    friend inline void swap(Builder& m1, Builder& m2) noexcept {
+		      using std::swap;
+
+		      swap(m1.options, m2.options);
+		      swap(m1.checkDuplicates, m2.checkDuplicates);
+		    }
+		    Builder(Builder&&);
+		    Builder(const Builder&) = delete;
+		    Builder& operator=(Builder) noexcept;
+		    virtual ~Builder();
+            }; // class OptionMap::Builder
         private:
             friend class message2::Serializer;
 
@@ -1094,13 +1123,7 @@ namespace message2 {
                 bool hasOptions = false;
                 Reserved asReserved;
                 FunctionName functionName;
-                // The next field is used internally to construct Operators
-                // and overrides the `options` vector.
-                // This is because it's easiest to share the code
-                // for parsing options between Operator::Builder and Markup::Builder.
-                OptionMap optionMap;
-                UVector* options; // Not a LocalPointer for the same reason as in `SelectorKeys::Builder`
-                Builder& setOptionMap(OptionMap&&);
+                OptionMap::Builder options;
             public:
                 /**
                  * Sets this operator to be a reserved sequence.
@@ -1176,6 +1199,10 @@ namespace message2 {
                  * @deprecated This API is for technology preview only.
                  */
                 virtual ~Builder();
+                Builder(const Builder&) = delete;
+                Builder& operator=(const Builder&) = delete;
+                Builder(Builder&&) = delete;
+                Builder& operator=(Builder&&) = delete;
             }; // class Operator::Builder
             /**
              * Copy constructor.
@@ -1224,7 +1251,6 @@ namespace message2 {
             friend class Builder;
             friend class message2::Checker;
             friend class message2::MessageFormatter;
-            friend class message2::Parser;
             friend class message2::Serializer;
 
             // Function call constructor
@@ -1358,24 +1384,11 @@ namespace message2 {
             class U_I18N_API Builder : public UMemory {
             private:
                 friend class Markup;
-                friend class message2::Parser;
 
                 UnicodeString name;
-                UVector* options; // Not a LocalPointer for the same reason as in `SelectorKeys::Builder`
-                UVector* attributes;
-                // The next two fields are used internally by the parser
-                // and override the `options` and `attributes` vectors.
-                // This is because it's easiest to share the code
-                // for parsing options between Operator::Builder and Markup::Builder.
-                OptionMap optionMap;
-                OptionMap attributeMap;
+                OptionMap::Builder options;
+                OptionMap::Builder attributes;
                 UMarkupType type = UMARKUP_COUNT;
-                Builder& setOptionMap(OptionMap&&);
-
-                // TODO: Would probably be best to make this method public (same for Markup)
-                // and not make the parser a friend class.
-                // Same for options on Operator and Markup. (addAll())
-                Builder& setAttributeMap(OptionMap&& m);
             public:
                 /**
                  * Sets the name of this markup.
@@ -1425,7 +1438,7 @@ namespace message2 {
                  * @internal ICU 75.0 technology preview
                  * @deprecated This API is for technology preview only.
                  */
-                Builder& addOption(const UnicodeString &key, Operand&& value, UErrorCode& status) noexcept;
+                Builder& addOption(const UnicodeString &key, Operand&& value, UErrorCode& status);
                 /**
                  * Adds a single attribute.
                  *
@@ -1437,7 +1450,7 @@ namespace message2 {
                  * @internal ICU 75.0 technology preview
                  * @deprecated This API is for technology preview only.
                  */
-                Builder& addAttribute(const UnicodeString &key, Operand&& value, UErrorCode& status) noexcept;
+                Builder& addAttribute(const UnicodeString &key, Operand&& value, UErrorCode& status);
                 /**
                  * Constructs a new immutable `Markup` using the name and type
                  * and (optionally) options and attributes that were previously set.
@@ -1473,6 +1486,10 @@ namespace message2 {
                  * @deprecated This API is for technology preview only.
                  */
                 virtual ~Builder();
+                Builder(const Builder&) = delete;
+                Builder& operator=(const Builder&) = delete;
+                Builder(Builder&&) = delete;
+                Builder& operator=(Builder&&) = delete;
             }; // class Markup::Builder
 
         private:
@@ -1582,21 +1599,12 @@ namespace message2 {
             class U_I18N_API Builder : public UMemory {
             private:
                 friend class Expression;
-                friend class message2::Parser;
 
                 bool hasOperand = false;
                 bool hasOperator = false;
                 Operand rand;
                 Operator rator;
-                UVector* attributes; // Not a LocalPointer for the same reason as in `SelectorKeys::builder`
-                // The next field is used internally by the parser
-                // and overrides `attributes` vector.
-                // This is because it's easiest to share the code
-                // for parsing options between Expression::Builder and Markup::Builder.
-                OptionMap attributeMap;
-                // Provided as a private method so that the parser
-                // can use the same attribute-parsing code for Expression and Markup
-                Builder& setAttributeMap(OptionMap&& m);
+                OptionMap::Builder attributes;
             public:
                 /**
                  * Sets the operand of this expression.
@@ -1665,6 +1673,10 @@ namespace message2 {
                  * @deprecated This API is for technology preview only.
                  */
                 virtual ~Builder();
+                Builder(const Builder&) = delete;
+                Builder& operator=(const Builder&) = delete;
+                Builder(Builder&&) = delete;
+                Builder& operator=(Builder&&) = delete;
             }; // class Expression::Builder
             /**
              * Non-member swap function.
@@ -1895,6 +1907,10 @@ namespace message2 {
                  * @deprecated This API is for technology preview only.
                  */
                 virtual ~Builder();
+                Builder(const Builder&) = delete;
+                Builder& operator=(const Builder&) = delete;
+                Builder(Builder&&) = delete;
+                Builder& operator=(Builder&&) = delete;
             }; // class UnsupportedStatement::Builder
             /**
              * Non-member swap function.
@@ -2249,6 +2265,10 @@ namespace message2 {
                  * @deprecated This API is for technology preview only.
                  */
                 virtual ~Builder();
+                Builder(const Builder&) = delete;
+                Builder& operator=(const Builder&) = delete;
+                Builder(Builder&&) = delete;
+                Builder& operator=(Builder&&) = delete;
             }; // class Pattern::Builder
 
             /**
@@ -3026,6 +3046,10 @@ namespace message2 {
              * @deprecated This API is for technology preview only.
              */
             virtual ~Builder();
+            Builder(const Builder&) = delete;
+            Builder& operator=(const Builder&) = delete;
+            Builder(Builder&&) = delete;
+            Builder& operator=(Builder&&) = delete;
         }; // class Builder
 
     private: