ICU-23297 Do not use lookupMatcher in UnicodeSet parsing

eggrobin · eggrobin · commit 982db0338540 · 2025-12-31T00:15:53.000+01:00
diff --git a/icu4c/source/common/rbbirb.h b/icu4c/source/common/rbbirb.h
@@ -65,15 +65,10 @@ class RBBISymbolTable : public UMemory, public SymbolTable {
     UHashtable               *fHashTable;
     RBBIRuleScanner          *fRuleScanner;
 
-    // These next two fields are part of the mechanism for passing references to
-    //   already-constructed UnicodeSets back to the UnicodeSet constructor
-    //   when the pattern includes $variable references.
-    const UnicodeString      ffffString;      // = "/uffff"
-    UnicodeSet              *fCachedSetLookup;
-
 public:
     //  API inherited from class SymbolTable
     virtual const UnicodeString*  lookup(const UnicodeString& s) const override;
+    virtual const UnicodeSet* lookupSet(const UnicodeString& s) const override;
     virtual const UnicodeFunctor* lookupMatcher(UChar32 ch) const override;
     virtual UnicodeString parseReference(const UnicodeString& text,
                                          ParsePosition& pos, int32_t limit) const override;
@@ -91,7 +86,7 @@ class RBBISymbolTable : public UMemory, public SymbolTable {
     // A do-nothing inline function for non-debug builds.  Member funcs can't be empty
     //  or the call sites won't compile.
     int32_t fFakeField;
-    #define rbbiSymtablePrint() fFakeField=0; 
+    #define rbbiSymtablePrint() fFakeField=0;
 #endif
 
 private:
@@ -220,7 +215,7 @@ typedef std::pair<int32_t, int32_t> IntPair;
 #define RBBIDebugPrintf printf
 #define RBBIDebugPuts puts
 #else
-#undef RBBIDebugPrintf 
+#undef RBBIDebugPrintf
 #define RBBIDebugPuts(arg)
 #endif
 
diff --git a/icu4c/source/common/rbbistbl.cpp b/icu4c/source/common/rbbistbl.cpp
@@ -41,11 +41,10 @@ U_CDECL_END
 U_NAMESPACE_BEGIN
 
 RBBISymbolTable::RBBISymbolTable(RBBIRuleScanner *rs, const UnicodeString &rules, UErrorCode &status)
-    : fRules(rules), fRuleScanner(rs), ffffString(static_cast<char16_t>(0xffff))
+    : fRules(rules), fRuleScanner(rs)
 {
     fHashTable       = nullptr;
-    fCachedSetLookup = nullptr;
-    
+
     fHashTable = uhash_open(uhash_hashUnicodeString, uhash_compareUnicodeString, nullptr, &status);
     // uhash_open checks status
     if (U_FAILURE(status)) {
@@ -71,60 +70,47 @@ RBBISymbolTable::~RBBISymbolTable()
 //
 const UnicodeString  *RBBISymbolTable::lookup(const UnicodeString& s) const
 {
-    RBBISymbolTableEntry  *el;
-    RBBINode              *varRefNode;
-    RBBINode              *exprNode;
-    RBBINode              *usetNode;
-    const UnicodeString   *retString;
-    RBBISymbolTable       *This = const_cast<RBBISymbolTable*>(this); // cast off const
-
-    el = static_cast<RBBISymbolTableEntry*>(uhash_get(fHashTable, &s));
+    const RBBISymbolTableEntry* const el =
+        static_cast<const RBBISymbolTableEntry*>(uhash_get(fHashTable, &s));
     if (el == nullptr) {
         return nullptr;
     }
+    const RBBINode& exprNode = *el->val->fLeftChild; // Root node of expression for variable
+    // Return the original source string for the expression.
+    // Note that for set-valued variables used in UnicodeSet expressions, this would be rejected by
+    // the UnicodeSet parser if the source itself contains variable references.  For instance, with
+    //     $CaseIgnorable   = [[:Mn:][:Me:][:Cf:][:Lm:][:Sk:] \u0027 \u00AD \u2019];
+    //     $Cased = [[:Upper_Case:][:Lower_Case:][:Lt:] - $CaseIgnorable];
+    // If lookupSet were not overridden, when parsing the right-hand side of
+    //     $NotCased        = [[^ $Cased] - $CaseIgnorable];
+    // there would be a call to lookup("Cased") which would return
+    //     "[[:Upper_Case:][:Lower_Case:][:Lt:]-$CaseIgnorable]". This contains a variable, which is
+    // disallowed by the UnicodeSet parser inside a variable expansion.
+    // However, set-valued variables are pre-parsed, and returned by lookupSet instead, so this call
+    // to lookup() never happens; instead, lookupSet("CaseIgnorable") is called when computing
+    // $Cased and returns the non-null value of $CaseIgnorable, and then when computing $NotCased,
+    // lookupSet("Cased") returns the value computed for $Cased.
+    return &exprNode.fText;
+}
 
-    varRefNode = el->val;
-    exprNode   = varRefNode->fLeftChild;     // Root node of expression for variable
-    if (exprNode->fType == RBBINode::setRef) {
-        // The $variable refers to a single UnicodeSet
-        //   return the ffffString, which will subsequently be interpreted as a
-        //   stand-in character for the set by RBBISymbolTable::lookupMatcher()
-        usetNode = exprNode->fLeftChild;
-        This->fCachedSetLookup = usetNode->fInputSet;
-        retString = &ffffString;
+const UnicodeSet* RBBISymbolTable::lookupSet(const UnicodeString& s) const {
+    const RBBISymbolTableEntry* const el = static_cast<const RBBISymbolTableEntry*>(uhash_get(fHashTable, &s));
+    if (el == nullptr) {
+        return nullptr;
     }
-    else
-    {
-        // The variable refers to something other than just a set.
-        // return the original source string for the expression
-        retString = &exprNode->fText;
-        This->fCachedSetLookup = nullptr;
+    const RBBINode& exprNode = *el->val->fLeftChild;
+    if (exprNode.fType == RBBINode::setRef) {
+        return exprNode.fLeftChild->fInputSet;
+    } else {
+        return nullptr;
     }
-    return retString;
 }
 
 
 
-//
-//  RBBISymbolTable::lookupMatcher   This function from the abstract symbol table
-//                                   interface maps a single stand-in character to a
-//                                   pointer to a Unicode Set.   The Unicode Set code uses this
-//                                   mechanism to get all references to the same $variable
-//                                   name to refer to a single common Unicode Set instance.
-//
-//    This implementation cheats a little, and does not maintain a map of stand-in chars
-//    to sets.  Instead, it takes advantage of the fact that  the UnicodeSet
-//    constructor will always call this function right after calling lookup(),
-//    and we just need to remember what set to return between these two calls.
-const UnicodeFunctor *RBBISymbolTable::lookupMatcher(UChar32 ch) const
-{
-    UnicodeSet *retVal = nullptr;
-    RBBISymbolTable *This = const_cast<RBBISymbolTable*>(this); // cast off const
-    if (ch == 0xffff) {
-        retVal = fCachedSetLookup;
-        This->fCachedSetLookup = nullptr;
-    }
-    return retVal;
+//  No longer used, see ICU-23297.
+const UnicodeFunctor* RBBISymbolTable::lookupMatcher(UChar32 /*ch*/) const {
+    return nullptr;
 }
 
 //
diff --git a/icu4c/source/common/unicode/symtable.h b/icu4c/source/common/unicode/symtable.h
@@ -19,11 +19,11 @@
 #include "unicode/uobject.h"
 
 /**
- * \file 
+ * \file
  * \brief C++ API: An interface that defines both lookup protocol and parsing of
  * symbolic names.
  */
- 
+
 U_NAMESPACE_BEGIN
 
 class ParsePosition;
@@ -82,6 +82,16 @@ class U_COMMON_API SymbolTable /* not : public UObject because this is an interf
      */
     virtual const UnicodeString* lookup(const UnicodeString& s) const = 0;
 
+    /**
+     * Returns a pointer to a pre-parsed set associated with the variable with
+     * the given name, or null if there is no such set (as is the case for an
+     * undefined or string-valued variable).
+     * @internal
+     */
+    virtual const UnicodeSet* lookupSet(const UnicodeString& /*name*/) const {
+        return nullptr;
+    }
+
     /**
      * Lookup the UnicodeMatcher associated with the given character, and
      * return it.  Return <tt>nullptr</tt> if not found.
diff --git a/icu4c/source/common/uniset_props.cpp b/icu4c/source/common/uniset_props.cpp
@@ -475,11 +475,23 @@ class UnicodeSet::Lexer {
         }
         if (first == u'$' && symbols_ != nullptr) {
             auto nameEnd = parsePosition_;
+            // The SymbolTable defines the lexing of variable names past the $.
             if (UnicodeString name = symbols_->parseReference(pattern_, nameEnd, pattern_.length());
                 !name.isEmpty()) {
                 chars_.jumpahead(nameEnd.getIndex() - (start + 1));
                 const std::u16string_view source =
                     std::u16string_view(pattern_).substr(start, parsePosition_.getIndex() - start);
+                const UnicodeSet *precomputedSet = symbols_->lookupSet(name);
+                if (precomputedSet != nullptr) {
+                    return LexicalElement(LexicalElement::VARIABLE, {}, getPos(), U_ZERO_ERROR,
+                                          precomputedSet, /*set=*/{}, source);
+                }
+                // The variable was not a precomputed set.  Use the old-fashioned `lookup`, which
+                // should give us its source text; if that parses as a single set or element, use
+                // it.  Note that variables are not allowed in that expansion.
+                // Implementers of higher-level syntaxes that pre-parse UnicodeSet-valued variables
+                // can use variables in their variable definitions, but those that simply use the
+                // source text substitution API cannot.
                 const UnicodeString *const expression = symbols_->lookup(name);
                 if (expression == nullptr) {
                     return LexicalElement(
@@ -636,22 +648,6 @@ class UnicodeSet::Lexer {
             }
             switch (variableToken.category_) {
             case LexicalElement::LITERAL_ELEMENT:
-                // Until ICU-23297 is fixed, a variable may expand to a single (normally private-use)
-                // code point that stands for a precomputed set.  As a change from ICU 78 and earlier,
-                // this mechanism is not used outside of variable expansion, and is effectively an
-                // implementation detail to make a variable represent a set without reparsing that set
-                // every time the variable is substituted.
-                if (U_SUCCESS(variableToken.errorCode())) {
-                    UnicodeSet const *standIn = dynamic_cast<const UnicodeSet *>(
-                        symbols_->lookupMatcher(*variableToken.codePoint()));
-                    if (standIn != nullptr) {
-                        return LexicalElement(LexicalElement::VARIABLE, {}, getPos(), U_ZERO_ERROR,
-                                              /*precomputedSet=*/standIn, {}, source);
-                    }
-                }
-                // The character was not a stand-in, this variable actually expands to a literal-element;
-                // fall through to the case where we bubble up the lexical element.
-                [[fallthrough]];
             case LexicalElement::ESCAPED_ELEMENT:
             case LexicalElement::NAMED_ELEMENT:
             case LexicalElement::BRACKETED_ELEMENT:
diff --git a/icu4c/source/i18n/rbt_pars.cpp b/icu4c/source/i18n/rbt_pars.cpp
diff --git a/icu4c/source/test/intltest/usettest.cpp b/icu4c/source/test/intltest/usettest.cpp
