From ba13db1ced3157832eecfaae52f9157988a41ee6 Mon Sep 17 00:00:00 2001
From: Tim Chevalier <tjc@igalia.com>
Date: Fri, 29 Mar 2024 14:54:26 -0700
Subject: [PATCH 1/5] 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.
---
 .../source/i18n/messageformat2_data_model.cpp | 165 +++++++-------
 icu4c/source/i18n/messageformat2_parser.cpp   | 202 ++----------------
 icu4c/source/i18n/messageformat2_parser.h     |  42 +++-
 .../i18n/unicode/messageformat2_data_model.h  |  54 ++---
 4 files changed, 155 insertions(+), 308 deletions(-)

diff --git a/icu4c/source/i18n/messageformat2_data_model.cpp b/icu4c/source/i18n/messageformat2_data_model.cpp
index b29f29e81342..b8ba4f5a35d7 100644
--- a/icu4c/source/i18n/messageformat2_data_model.cpp
+++ b/icu4c/source/i18n/messageformat2_data_model.cpp
@@ -304,14 +304,53 @@ 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);
+}
+
+/* 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::~OptionMap() {}
+OptionMap::Builder::~Builder() {
+    if (options != nullptr) {
+        delete options;
+    }
+}
 
 const Reserved& Operator::asReserved() const {
     U_ASSERT(isReserved());
@@ -332,13 +371,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 +393,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 +423,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 +440,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,26 +455,24 @@ 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;
-    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::Builder& Markup::Builder::setOptionMap(OptionMap&& m) {
-    optionMap = std::move(m);
-    delete options;
-    options = nullptr;
+Markup::Builder& Markup::Builder::addOption(const UnicodeString &key,
+                                            Operand&& value,
+                                            UErrorCode& errorCode) {
+    options.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::Builder& Markup::Builder::addAttribute(const UnicodeString &key,
+                                               Operand&& value,
+                                               UErrorCode& errorCode) {
+    attributes.add(Option(key, std::move(value)), errorCode);
+    return *this;
 }
 
 Markup Markup::Builder::build(UErrorCode& errorCode) {
@@ -479,34 +488,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 +546,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 +565,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 +586,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 1e3995fea979..15b185762dca 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,69 +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;
-}
-
-/*
-  Consume a name-value pair, matching the `option` nonterminal in the grammar.
-
-  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);
-    }
+    attrAdder.addAttribute(lhs, std::move(rand), errorCode);
 }
 
 /*
@@ -1020,7 +959,8 @@ void Parser::parseOption(UVector& options, UErrorCode& errorCode) {
 
   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);
-    }
-}
-
-/*
-  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);
+        parseOption(addOption, 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 81a81abf6fa6..fcc66f46a62e 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 OptionAddr 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 281d5bf28e56..249e698d2296 100644
--- a/icu4c/source/i18n/unicode/messageformat2_data_model.h
+++ b/icu4c/source/i18n/unicode/messageformat2_data_model.h
@@ -961,6 +961,19 @@ namespace message2 {
             OptionMap(const UVector&, UErrorCode&);
             OptionMap(Option*, int32_t);
             virtual ~OptionMap();
+
+            class 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&);
+                    virtual ~Builder();
+            }; // class OptionMap::Builder
         private:
             friend class message2::Serializer;
 
@@ -1094,13 +1107,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.
@@ -1224,7 +1231,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 +1364,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 +1418,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 +1430,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.
@@ -1582,21 +1575,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.

From 5c0c9d5278399ce4b22a4ffdb0c0a3797f1291e8 Mon Sep 17 00:00:00 2001
From: Tim Chevalier <tjc@igalia.com>
Date: Fri, 29 Mar 2024 15:22:51 -0700
Subject: [PATCH 2/5] Make OptionMap::Builder exported

This is necessary for compilation on Windows, even though it's
an internal class
---
 icu4c/source/i18n/unicode/messageformat2_data_model.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/icu4c/source/i18n/unicode/messageformat2_data_model.h b/icu4c/source/i18n/unicode/messageformat2_data_model.h
index 249e698d2296..ab8048538cd7 100644
--- a/icu4c/source/i18n/unicode/messageformat2_data_model.h
+++ b/icu4c/source/i18n/unicode/messageformat2_data_model.h
@@ -962,7 +962,7 @@ namespace message2 {
             OptionMap(Option*, int32_t);
             virtual ~OptionMap();
 
-            class Builder : public UObject {
+            class U_I18N_API Builder : public UObject {
                 private:
                     UVector* options;
                     bool checkDuplicates = true;

From 558bc6988903ab860622e26f230d30e1cfa369f1 Mon Sep 17 00:00:00 2001
From: Tim Chevalier <tjc@igalia.com>
Date: Wed, 3 Apr 2024 15:07:48 -0700
Subject: [PATCH 3/5] Update icu4c/source/i18n/messageformat2_parser.h

Co-authored-by: Elango Cheran <elango@unicode.org>
---
 icu4c/source/i18n/messageformat2_parser.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/icu4c/source/i18n/messageformat2_parser.h b/icu4c/source/i18n/messageformat2_parser.h
index fcc66f46a62e..92c0475d67db 100644
--- a/icu4c/source/i18n/messageformat2_parser.h
+++ b/icu4c/source/i18n/messageformat2_parser.h
@@ -41,7 +41,7 @@ namespace message2 {
 
     // Used for parameterizing attributes parsing code
     // over the two builders that use it (Expression and Markup)
-    // Unfortunately the same OptionAddr class can't just be reused,
+    // 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 {

From 2497b11a957914b63cbea1323fc18c9a28fbfdd5 Mon Sep 17 00:00:00 2001
From: Tim Chevalier <tjc@igalia.com>
Date: Thu, 4 Apr 2024 18:22:51 -0700
Subject: [PATCH 4/5] Increase timeout for MSVC Debug CI jobs

---
 .ci-builds/.azure-pipelines-icu4c.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.ci-builds/.azure-pipelines-icu4c.yml b/.ci-builds/.azure-pipelines-icu4c.yml
index a749764931ef..10a3af1e3f8a 100644
--- a/.ci-builds/.azure-pipelines-icu4c.yml
+++ b/.ci-builds/.azure-pipelines-icu4c.yml
@@ -381,7 +381,7 @@ jobs:
 #-------------------------------------------------------------------------
 - job: ICU4C_MSVC_x64_Debug
   displayName: 'C: MSVC x64 Debug (VS 2022)'
-  timeoutInMinutes: 35
+  timeoutInMinutes: 60
   pool:
     vmImage: 'windows-2022'
     demands: 
@@ -487,7 +487,7 @@ jobs:
 #-------------------------------------------------------------------------
 - job: ICU4C_MSVC_x86_Debug_VS2019
   displayName: 'C: MSVC 32-bit Debug (VS 2019)'
-  timeoutInMinutes: 60
+  timeoutInMinutes: 90
   pool:
     vmImage: 'windows-2019'
     demands: 

From 51f7e548817f29ec6e33f0a1f717e9bcc887233b Mon Sep 17 00:00:00 2001
From: Tim Chevalier <tjc@igalia.com>
Date: Mon, 8 Apr 2024 17:07:21 -0700
Subject: [PATCH 5/5] ICU-22261 Refactor error handling for internal MF2
 array-allocating functions

Several functions used to copy between vectors and arrays were written
not to take a UErrorCode, but rather to take an int argument by reference
and set it to 0 to indicate a memory allocation error. To be more
consistent with ICU style, change these functions to take a UErrorCode
argument.

Several data model classes (Reserved, Matcher and Pattern) are also
modified to add a `bogus` flag to indicate that a copy failed. Previously,
the "bogus" state was denoted by setting an internal length field to 0,
but that was wrong because a valid instance of these classes can have
length 0 and that state should be distinguished from an uninitialized
state.
---
 icu4c/source/i18n/messageformat2_allocation.h |  36 ++--
 .../source/i18n/messageformat2_data_model.cpp | 160 ++++++++++++------
 .../source/i18n/messageformat2_evaluation.cpp |   6 +-
 .../i18n/unicode/messageformat2_data_model.h  |  48 ++++--
 4 files changed, 158 insertions(+), 92 deletions(-)

diff --git a/icu4c/source/i18n/messageformat2_allocation.h b/icu4c/source/i18n/messageformat2_allocation.h
index 431862cecef5..7be27e222520 100644
--- a/icu4c/source/i18n/messageformat2_allocation.h
+++ b/icu4c/source/i18n/messageformat2_allocation.h
@@ -24,16 +24,15 @@ namespace message2 {
     // Helpers
 
     template<typename T>
-    static T* copyArray(const T* source, int32_t& len) { // `len` is an in/out param
-        if (source == nullptr) {
-            len = 0;
+    static T* copyArray(const T* source, int32_t len, UErrorCode& status) {
+        if (U_FAILURE(status)) {
             return nullptr;
         }
+        U_ASSERT(source != nullptr);
+        U_ASSERT(len >= 0);
         T* dest = new T[len];
         if (dest == nullptr) {
-            // Set length to 0 to prevent the
-            // array from being accessed
-            len = 0;
+            status = U_MEMORY_ALLOCATION_ERROR;
         } else {
             for (int32_t i = 0; i < len; i++) {
                 dest[i] = source[i];
@@ -43,13 +42,14 @@ namespace message2 {
     }
 
     template<typename T>
-    static T* copyVectorToArray(const UVector& source, int32_t& len) {
-        len = source.size();
+    static T* copyVectorToArray(const UVector& source, UErrorCode& status) {
+        if (U_FAILURE(status)) {
+            return nullptr;
+        }
+        int32_t len = source.size();
         T* dest = new T[len];
         if (dest == nullptr) {
-            // Set length to 0 to prevent the
-            // array from being accessed
-            len = 0;
+            status = U_MEMORY_ALLOCATION_ERROR;
         } else {
             for (int32_t i = 0; i < len; i++) {
                 dest[i] = *(static_cast<T*>(source.elementAt(i)));
@@ -59,19 +59,21 @@ namespace message2 {
     }
 
     template<typename T>
-    static T* moveVectorToArray(UVector& source, int32_t& len) {
-        len = source.size();
+    static T* moveVectorToArray(UVector& source, UErrorCode& status) {
+        if (U_FAILURE(status)) {
+            return nullptr;
+        }
+
+        int32_t len = source.size();
         T* dest = new T[len];
         if (dest == nullptr) {
-            // Set length to 0 to prevent the
-            // array from being accessed
-            len = 0;
+            status = U_MEMORY_ALLOCATION_ERROR;
         } else {
             for (int32_t i = 0; i < len; i++) {
                 dest[i] = std::move(*static_cast<T*>(source.elementAt(i)));
             }
+            source.removeAllElements();
         }
-        source.removeAllElements();
         return dest;
     }
 
diff --git a/icu4c/source/i18n/messageformat2_data_model.cpp b/icu4c/source/i18n/messageformat2_data_model.cpp
index b8ba4f5a35d7..b12dd31c2052 100644
--- a/icu4c/source/i18n/messageformat2_data_model.cpp
+++ b/icu4c/source/i18n/messageformat2_data_model.cpp
@@ -77,15 +77,10 @@ SelectorKeys::Builder::~Builder() {
 }
 
 SelectorKeys::SelectorKeys(const UVector& ks, UErrorCode& status) : len(ks.size()) {
+    Key* result = copyVectorToArray<Key>(ks, status);
     if (U_FAILURE(status)) {
         return;
     }
-    Key* result = copyVectorToArray<Key>(ks, len);
-    if (result == nullptr) {
-        status = U_MEMORY_ALLOCATION_ERROR;
-        len = 0;
-        return;
-    }
     keys.adoptInstead(result);
 }
 
@@ -95,7 +90,13 @@ SelectorKeys& SelectorKeys::operator=(SelectorKeys other) noexcept {
 }
 
 SelectorKeys::SelectorKeys(const SelectorKeys& other) : len(other.len) {
-    keys.adoptInstead(copyArray(other.keys.getAlias(), len));
+    UErrorCode localErrorCode = U_ZERO_ERROR;
+    if (len != 0) {
+        keys.adoptInstead(copyArray(other.keys.getAlias(), len, localErrorCode));
+    }
+    if (U_FAILURE(localErrorCode)) {
+        len = 0;
+    }
 }
 
 SelectorKeys::~SelectorKeys() {
@@ -198,9 +199,18 @@ Key::~Key() {}
 // ------------ Reserved
 
 // Copy constructor
-Reserved::Reserved(const Reserved& other) {
-    len = other.len;
-    parts.adoptInstead(copyArray(other.parts.getAlias(), len));
+Reserved::Reserved(const Reserved& other) : len(other.len) {
+    U_ASSERT(!other.bogus);
+
+    UErrorCode localErrorCode = U_ZERO_ERROR;
+    if (len == 0) {
+        parts.adoptInstead(nullptr);
+    } else {
+        parts.adoptInstead(copyArray(other.parts.getAlias(), len, localErrorCode));
+    }
+    if (U_FAILURE(localErrorCode)) {
+        bogus = true;
+    }
 }
 
 Reserved& Reserved::operator=(Reserved other) noexcept {
@@ -212,14 +222,16 @@ Reserved::Reserved(const UVector& ps, UErrorCode& status) noexcept : len(ps.size
     if (U_FAILURE(status)) {
         return;
     }
-    parts = LocalArray<Literal>(copyVectorToArray<Literal>(ps, len));
+    parts = LocalArray<Literal>(copyVectorToArray<Literal>(ps, status));
 }
 
 int32_t Reserved::numParts() const {
+    U_ASSERT(!bogus);
     return len;
 }
 
 const Literal& Reserved::getPart(int32_t i) const {
+    U_ASSERT(!bogus);
     U_ASSERT(i < numParts());
     return parts[i];
 }
@@ -257,14 +269,10 @@ Reserved::~Reserved() {
 
 //------------------------ Operator
 
-OptionMap::OptionMap(const UVector& opts, UErrorCode& status) {
-    CHECK_ERROR(status);
-
-    len = opts.size();
-    Option* result = copyVectorToArray<Option>(opts, len);
-    if (result == nullptr) {
+OptionMap::OptionMap(const UVector& opts, UErrorCode& status) : len(opts.size()) {
+    Option* result = copyVectorToArray<Option>(opts, status);
+    if (U_FAILURE(status)) {
         bogus = true;
-        status = U_MEMORY_ALLOCATION_ERROR;
         return;
     }
     options.adoptInstead(result);
@@ -273,8 +281,9 @@ OptionMap::OptionMap(const UVector& opts, UErrorCode& status) {
 
 OptionMap::OptionMap(const OptionMap& other) : len(other.len) {
     U_ASSERT(!other.bogus);
-    Option* result = copyArray(other.options.getAlias(), len);
-    if (result == nullptr && len != 0) {
+    UErrorCode localErrorCode = U_ZERO_ERROR;
+    Option* result = copyArray(other.options.getAlias(), len, localErrorCode);
+    if (U_FAILURE(localErrorCode)) {
         bogus = true;
         return;
     }
@@ -642,16 +651,13 @@ const Reserved* UnsupportedStatement::getBody(UErrorCode& errorCode) const {
 UnsupportedStatement::UnsupportedStatement(const UnicodeString& k,
                                            const std::optional<Reserved>& r,
                                            const UVector& es,
-                                           UErrorCode& status) : keyword(k), body(r) {
+                                           UErrorCode& status)
+    : keyword(k), body(r), expressionsLen(es.size()) {
     CHECK_ERROR(status);
 
-    U_ASSERT(es.size() >= 1);
-    Expression* result = copyVectorToArray<Expression>(es, expressionsLen);
-    if (result == nullptr) {
-        status = U_MEMORY_ALLOCATION_ERROR;
-        expressionsLen = 0;
-        return;
-    }
+    U_ASSERT(expressionsLen >= 1);
+    Expression* result = copyVectorToArray<Expression>(es, status);
+    CHECK_ERROR(status);
     expressions.adoptInstead(result);
 }
 
@@ -660,7 +666,11 @@ UnsupportedStatement::UnsupportedStatement(const UnsupportedStatement& other) {
     body = other.body;
     expressionsLen = other.expressionsLen;
     U_ASSERT(expressionsLen > 0);
-    expressions.adoptInstead(copyArray(other.expressions.getAlias(), expressionsLen));
+    UErrorCode localErrorCode = U_ZERO_ERROR;
+    expressions.adoptInstead(copyArray(other.expressions.getAlias(), expressionsLen, localErrorCode));
+    if (U_FAILURE(localErrorCode)) {
+        expressionsLen = 0;
+    }
 }
 
 UnsupportedStatement& UnsupportedStatement::operator=(UnsupportedStatement other) noexcept {
@@ -711,21 +721,32 @@ Pattern::Pattern(const UVector& ps, UErrorCode& status) : len(ps.size()) {
     if (U_FAILURE(status)) {
         return;
     }
-    PatternPart* result = copyVectorToArray<PatternPart>(ps, len);
-    if (result == nullptr) {
-        status = U_MEMORY_ALLOCATION_ERROR;
-        return;
-    }
+    PatternPart* result = copyVectorToArray<PatternPart>(ps, status);
+    CHECK_ERROR(status);
     parts.adoptInstead(result);
 }
 
 // Copy constructor
-Pattern::Pattern(const Pattern& other) noexcept : len(other.len) {
-    parts.adoptInstead(copyArray(other.parts.getAlias(), len));
+Pattern::Pattern(const Pattern& other) : len(other.len) {
+    U_ASSERT(!other.bogus);
+    UErrorCode localErrorCode = U_ZERO_ERROR;
+    if (len == 0) {
+        parts.adoptInstead(nullptr);
+    } else {
+        parts.adoptInstead(copyArray(other.parts.getAlias(), len, localErrorCode));
+    }
+    if (U_FAILURE(localErrorCode)) {
+        bogus = true;
+    }
+}
+
+int32_t Pattern::numParts() const {
+    U_ASSERT(!bogus);
+    return len;
 }
 
 const PatternPart& Pattern::getPart(int32_t i) const {
-    U_ASSERT(i < numParts());
+    U_ASSERT(!bogus && i < numParts());
     return parts[i];
 }
 
@@ -864,12 +885,26 @@ Matcher& Matcher::operator=(Matcher other) {
 }
 
 Matcher::Matcher(const Matcher& other) {
+    U_ASSERT(!other.bogus);
     numSelectors = other.numSelectors;
     numVariants = other.numVariants;
-    selectors.adoptInstead(copyArray<Expression>(other.selectors.getAlias(), numSelectors));
-    variants.adoptInstead(copyArray<Variant>(other.variants.getAlias(), numVariants));
+    UErrorCode localErrorCode = U_ZERO_ERROR;
+    selectors.adoptInstead(copyArray<Expression>(other.selectors.getAlias(),
+                                                 numSelectors,
+                                                 localErrorCode));
+    variants.adoptInstead(copyArray<Variant>(other.variants.getAlias(),
+                                             numVariants,
+                                             localErrorCode));
+    if (U_FAILURE(localErrorCode)) {
+        bogus = true;
+    }
 }
 
+Matcher::Matcher(Expression* ss, int32_t ns, Variant* vs, int32_t nv)
+    : selectors(ss), numSelectors(ns), variants(vs), numVariants(nv) {}
+
+Matcher::~Matcher() {}
+
 // --------------- MFDataModel
 
 const Pattern& MFDataModel::getPattern() const {
@@ -1003,29 +1038,34 @@ MFDataModel::Builder& MFDataModel::Builder::setPattern(Pattern&& pat) {
 MFDataModel::MFDataModel(const MFDataModel& other) : body(Pattern()) {
     U_ASSERT(!other.bogus);
 
+    UErrorCode localErrorCode = U_ZERO_ERROR;
+
     if (other.hasPattern()) {
-        body.emplace<Pattern>(Pattern(*std::get_if<Pattern>(&other.body)));
+        //        body.emplace<Pattern>(Pattern(*std::get_if<Pattern>(&other.body)));
+        body = *std::get_if<Pattern>(&other.body);
     } else {
         const Expression* otherSelectors = other.getSelectorsInternal();
         const Variant* otherVariants = other.getVariantsInternal();
         int32_t numSelectors = other.numSelectors();
         int32_t numVariants = other.numVariants();
-        Expression* copiedSelectors = copyArray(otherSelectors, numSelectors);
-        Variant* copiedVariants = copyArray(otherVariants, numVariants);
-        if (!(copiedSelectors != nullptr && copiedVariants != nullptr)) {
+        Expression* copiedSelectors = copyArray(otherSelectors, numSelectors, localErrorCode);
+        Variant* copiedVariants = copyArray(otherVariants, numVariants, localErrorCode);
+        if (U_FAILURE(localErrorCode)) {
             bogus = true;
+            return;
         }
-        body.emplace<Matcher>(Matcher(copiedSelectors, numSelectors, copiedVariants, numVariants));
+        //        body.emplace<Matcher>(Matcher(copiedSelectors, numSelectors, copiedVariants, numVariants));
+        body = Matcher(copiedSelectors, numSelectors, copiedVariants, numVariants);
     }
 
     bindingsLen = other.bindingsLen;
-    bindings.adoptInstead(copyArray(other.bindings.getAlias(), bindingsLen));
-    if (!bindings.isValid()) {
+    bindings.adoptInstead(copyArray(other.bindings.getAlias(), bindingsLen, localErrorCode));
+    if (U_FAILURE(localErrorCode)) {
         bogus = true;
     }
     unsupportedStatementsLen = other.unsupportedStatementsLen;
-    unsupportedStatements.adoptInstead(copyArray(other.unsupportedStatements.getAlias(), unsupportedStatementsLen));
-    if (!unsupportedStatements.isValid()) {
+    unsupportedStatements.adoptInstead(copyArray(other.unsupportedStatements.getAlias(), unsupportedStatementsLen, localErrorCode));
+    if (U_FAILURE(localErrorCode)) {
         bogus = true;
     }
 }
@@ -1036,25 +1076,33 @@ MFDataModel::MFDataModel(const MFDataModel::Builder& builder, UErrorCode& errorC
     if (builder.hasPattern) {
         body.emplace<Pattern>(builder.pattern);
     } else {
-        int32_t numVariants = builder.variants == nullptr ? 0 : builder.variants->size();
-        int32_t numSelectors = builder.selectors == nullptr ? 0 : builder.selectors->size();
-        Variant* variants = copyVectorToArray<Variant>(*builder.variants, numVariants);
-        Expression* selectors = copyVectorToArray<Expression>(*builder.selectors, numSelectors);
-        bogus &= (variants != nullptr && selectors != nullptr);
+        U_ASSERT(builder.variants != nullptr);
+        U_ASSERT(builder.selectors != nullptr);
+        int32_t numVariants = builder.variants->size();
+        int32_t numSelectors = builder.selectors->size();
+        Variant* variants = copyVectorToArray<Variant>(*builder.variants, errorCode);
+        Expression* selectors = copyVectorToArray<Expression>(*builder.selectors, errorCode);
+        if (U_FAILURE(errorCode)) {
+            bogus = true;
+            return;
+        }
         body.emplace<Matcher>(Matcher(selectors, numSelectors, variants, numVariants));
     }
 
     U_ASSERT(builder.bindings != nullptr);
     bindingsLen = builder.bindings->size();
-    bindings.adoptInstead(copyVectorToArray<Binding>(*builder.bindings, bindingsLen));
+    bindings.adoptInstead(copyVectorToArray<Binding>(*builder.bindings, errorCode));
     unsupportedStatementsLen = builder.unsupportedStatements->size();
-    unsupportedStatements.adoptInstead(copyVectorToArray<UnsupportedStatement>(*builder.unsupportedStatements, unsupportedStatementsLen));
-    bogus &= ((bool) (bindings.isValid() && unsupportedStatements.isValid()));
+    unsupportedStatements.adoptInstead(copyVectorToArray<UnsupportedStatement>(*builder.unsupportedStatements, errorCode));
+    if (U_FAILURE(errorCode)) {
+        bogus = true;
+    }
 }
 
 MFDataModel::MFDataModel() : body(Pattern()) {}
 
 MFDataModel& MFDataModel::operator=(MFDataModel other) noexcept {
+    U_ASSERT(!other.bogus);
     swap(*this, other);
     return *this;
 }
diff --git a/icu4c/source/i18n/messageformat2_evaluation.cpp b/icu4c/source/i18n/messageformat2_evaluation.cpp
index a735521f5bbf..5fd099c41b0b 100644
--- a/icu4c/source/i18n/messageformat2_evaluation.cpp
+++ b/icu4c/source/i18n/messageformat2_evaluation.cpp
@@ -41,10 +41,8 @@ const ResolvedFunctionOption* FunctionOptions::getResolvedFunctionOptions(int32_
 FunctionOptions::FunctionOptions(UVector&& optionsVector, UErrorCode& status) {
     CHECK_ERROR(status);
 
-    options = moveVectorToArray<ResolvedFunctionOption>(optionsVector, functionOptionsLen);
-    if (options == nullptr) {
-        status = U_MEMORY_ALLOCATION_ERROR;
-    }
+    functionOptionsLen = optionsVector.size();
+    options = moveVectorToArray<ResolvedFunctionOption>(optionsVector, status);
 }
 
 UBool FunctionOptions::getFunctionOption(const UnicodeString& key, Formattable& option) const {
diff --git a/icu4c/source/i18n/unicode/messageformat2_data_model.h b/icu4c/source/i18n/unicode/messageformat2_data_model.h
index ab8048538cd7..8945d29bd1bc 100644
--- a/icu4c/source/i18n/unicode/messageformat2_data_model.h
+++ b/icu4c/source/i18n/unicode/messageformat2_data_model.h
@@ -181,6 +181,7 @@ namespace message2 {
             friend inline void swap(Reserved& r1, Reserved& r2) noexcept {
                 using std::swap;
 
+                swap(r1.bogus, r2.bogus);
                 swap(r1.parts, r2.parts);
                 swap(r1.len, r2.len);
             }
@@ -217,11 +218,16 @@ namespace message2 {
             friend class Builder;
             friend class Operator;
 
+            // True if a copy failed; this has to be distinguished
+            // from a valid `Reserved` with empty parts
+            bool bogus = false;
+
             // Possibly-empty list of parts
             // `literal` reserved as a quoted literal; `reserved-char` / `reserved-escape`
             // strings represented as unquoted literals
             /* const */ LocalArray<Literal> parts;
             int32_t len = 0;
+
             Reserved(const UVector& parts, UErrorCode& status) noexcept;
             // Helper
             static void initLiterals(Reserved&, const Reserved&);
@@ -2254,6 +2260,7 @@ namespace message2 {
             friend inline void swap(Pattern& p1, Pattern& p2) noexcept {
                 using std::swap;
 
+                swap(p1.bogus, p2.bogus);
                 swap(p1.len, p2.len);
                 swap(p1.parts, p2.parts);
             }
@@ -2263,7 +2270,7 @@ namespace message2 {
              * @internal ICU 75.0 technology preview
              * @deprecated This API is for technology preview only.
              */
-            Pattern(const Pattern& other) noexcept;
+            Pattern(const Pattern& other);
             /**
              * Assignment operator
              *
@@ -2335,6 +2342,11 @@ namespace message2 {
             friend class message2::MessageFormatter;
             friend class message2::Serializer;
 
+            // Set to true if a copy constructor fails;
+            // needed in order to distinguish an uninitialized
+            // Pattern from a 0-length pattern
+            bool bogus = false;
+
             // Possibly-empty array of parts
             int32_t len = 0;
             LocalArray<PatternPart> parts;
@@ -2351,7 +2363,7 @@ namespace message2 {
              * @internal ICU 75.0 technology preview
              * @deprecated This API is for technology preview only.
              */
-            int32_t numParts() const { return len; }
+            int32_t numParts() const;
             /**
              * Returns the `i`th part in the pattern.
              * Precondition: i < numParts()
@@ -2366,10 +2378,8 @@ namespace message2 {
 
             // Gets around not being able to declare Pattern::Iterator as a friend
             // in PatternPart
-            static const std::variant<UnicodeString, Expression, Markup>& patternContents(const PatternPart& p) {
-                return p.piece;
-            }
-
+            static const std::variant<UnicodeString, Expression, Markup>&
+                patternContents(const PatternPart& p) { return p.piece; }
         }; // class Pattern
 
         /**
@@ -2629,7 +2639,7 @@ namespace message2 {
     class MFDataModel;
 
     #ifndef U_IN_DOXYGEN
-    class Matcher {
+    class Matcher : public UObject {
     public:
         Matcher& operator=(Matcher);
         Matcher(const Matcher&);
@@ -2644,24 +2654,32 @@ namespace message2 {
         friend inline void swap(Matcher& m1, Matcher& m2) noexcept {
             using std::swap;
 
+            if (m1.bogus) {
+                m2.bogus = true;
+                return;
+            }
+            if (m2.bogus) {
+                m1.bogus = true;
+                return;
+            }
             swap(m1.selectors, m2.selectors);
             swap(m1.numSelectors, m2.numSelectors);
             swap(m1.variants, m2.variants);
             swap(m1.numVariants, m2.numVariants);
         }
+        virtual ~Matcher();
     private:
 
         friend class MFDataModel;
 
-        Matcher(Expression* ss, int32_t ns, Variant* vs, int32_t nv) : selectors(ss), numSelectors(ns), variants(vs), numVariants(nv) {
-            if (selectors == nullptr) {
-                numSelectors = 0;
-            }
-            if (variants == nullptr) {
-                numVariants = 0;
-            }
-        }
+        Matcher(Expression* ss, int32_t ns, Variant* vs, int32_t nv);
         Matcher() {}
+
+        // A Matcher may have numSelectors=0 and numVariants=0
+        // (this is a data model error, but it's representable).
+        // So we have to keep a separate flag to track failed copies.
+        bool bogus = false;
+
         // The expressions that are being matched on.
         LocalArray<Expression> selectors;
         // The number of selectors