ICU-23222 Enhance unit alias handling

See #3675

Author: younies

icu4c/source/i18n/measunit_extra.cpp

@@ -100,6 +100,9 @@ enum PowerPart {
 // "fluid-ounce-imperial".
 constexpr int32_t kSimpleUnitOffset = 512;
 
+// Trie value offset for aliases, e.g. "portion" replaced by "part"
+constexpr int32_t kAliasOffset = 51200; // This will give a very big space for the units ids.
+
 const struct UnitPrefixStrings {
     const char* const string;
     UMeasurePrefix value;
@@ -255,6 +258,58 @@ class SimpleUnitIdentifiersSink : public icu::ResourceSink {
     int32_t outIndex;
 };
 
+class UnitAliasesSink : public icu::ResourceSink {
+  public:
+    /**
+     * Constructor.
+     * @param unitAliases The output vector of unit alias identifiers (CharString).
+     * @param unitReplacements The output vector of replacements for the unit aliases (CharString).
+     */
+    explicit UnitAliasesSink(MaybeStackVector<CharString> &unitAliases,
+                             MaybeStackVector<CharString> &unitReplacements)
+        : unitAliases(unitAliases), unitReplacements(unitReplacements) {}
+
+    /**
+     * Adds the unit alias key and its replacement to the unitAliases and unitReplacements vectors.
+     * @param key The unit alias identifier (e.g., "meter-and-liter").
+     * @param value Should be a ResourceTable value containing the replacement,
+     *     when ures_getAllChildrenWithFallback() is called correctly for this sink.
+     * @param noFallback Ignored.
+     * @param status The standard ICU error code output parameter.
+     */
+    void put(const char *key, ResourceValue &value, UBool /*noFallback*/,
+             UErrorCode &status) override {
+        if (U_FAILURE(status)) return;
+
+        // Add the unit alias key to the unitAliases vector
+        int32_t keyLen = static_cast<int32_t>(uprv_strlen(key));
+        unitAliases.emplaceBackAndCheckErrorCode(status)->append(key, keyLen, status);
+        if (U_FAILURE(status)) {
+            return;
+        }
+
+        // Find the replacement for this unit alias from the alias table resource.
+        ResourceTable aliasTable = value.getTable(status);
+        if (U_FAILURE(status)) {
+            return;
+        }
+
+        if (!aliasTable.findValue("replacement", value)) {
+            status = U_MISSING_RESOURCE_ERROR;
+            return;
+        }
+
+        int32_t len;
+        const char16_t *uReplacement = value.getString(len, status);
+        unitReplacements.emplaceBackAndCheckErrorCode(status)->appendInvariantChars(uReplacement,
+                                                                                    len, status);
+    }
+
+  private:
+    MaybeStackVector<CharString> &unitAliases;
+    MaybeStackVector<CharString> &unitReplacements;
+};
+
 /**
  * A ResourceSink that collects information from `unitQuantities` in the `units`
  * resource to provide key->value lookups from base unit to category, as well as
@@ -321,6 +376,11 @@ class CategoriesSink : public icu::ResourceSink {
 
 icu::UInitOnce gUnitExtrasInitOnce {};
 
+// Array of unit aliases.
+MaybeStackVector<icu::CharString> gUnitAliases;
+// Array of replacements for the unit aliases.
+MaybeStackVector<icu::CharString> gUnitReplacements;
+
 // Array of simple unit IDs.
 //
 // The array memory itself is owned by this pointer, but the individual char* in
@@ -453,6 +513,24 @@ void U_CALLCONV initUnitExtras(UErrorCode& status) {
                                              simpleUnitsCount, b, kSimpleUnitOffset);
     ures_getAllItemsWithFallback(unitsBundle.getAlias(), "convertUnits", identifierSink, status);
 
+    // Populate gUnitAliases and gUnitReplacements.
+    LocalUResourceBundlePointer aliasBundle(ures_open(U_ICUDATA_ALIAS, "metadata", &status));
+    if (U_FAILURE(status)) {
+        return;
+    }
+    UnitAliasesSink aliasSink(gUnitAliases, gUnitReplacements);
+    ures_getAllChildrenWithFallback(aliasBundle.getAlias(), "alias/unit", aliasSink, status);
+    if (U_FAILURE(status)) {
+        return;
+    }
+
+    for (int32_t i = 0; i < gUnitAliases.length(); i++) {
+        b.add(gUnitAliases[i]->data(), i + kAliasOffset, status);
+        if (U_FAILURE(status)) {
+            return;
+        }
+    }
+
     // Build the CharsTrie
     // TODO: Use SLOW or FAST here?
     StringPiece result = b.buildStringPiece(USTRINGTRIE_BUILD_FAST, status);
@@ -479,8 +557,10 @@ class Token {
           this->fType = TYPE_INITIAL_COMPOUND_PART;
       } else if (fMatch < kSimpleUnitOffset) {
           this->fType = TYPE_POWER_PART;
-      } else {
+      } else if (fMatch < kAliasOffset) {
           this->fType = TYPE_SIMPLE_UNIT;
+      } else {
+          this->fType = TYPE_ALIAS;
       }
   }
 
@@ -505,6 +585,7 @@ class Token {
       TYPE_POWER_PART,
       TYPE_SIMPLE_UNIT,
       TYPE_CONSTANT_DENOMINATOR,
+      TYPE_ALIAS,
   };
 
   // Calling getType() is invalid, resulting in an assertion failure, if Token
@@ -551,6 +632,11 @@ class Token {
         return fMatch - kSimpleUnitOffset;
     }
 
+    int32_t getAliasIndex() const {
+        U_ASSERT(getType() == TYPE_ALIAS);
+        return static_cast<int32_t>(fMatch - kAliasOffset);
+    }
+
     // TODO: Consider moving this to a separate utility class.
     // Utility function to parse a string into an unsigned long value.
     // The value must be a positive integer within the range [1, INT64_MAX].
@@ -673,6 +759,10 @@ class Parser {
             }
 
             if (singleUnitOrConstant.isConstantDenominator()) {
+                if (result.constantDenominator > 0) {
+                    status = kUnitIdentifierSyntaxError;
+                    return result;
+                }
                 result.constantDenominator = singleUnitOrConstant.getConstantDenominator();
                 result.complexity = UMEASURE_UNIT_COMPOUND;
                 continue;
@@ -728,6 +818,9 @@ class Parser {
     StringPiece fSource;
     BytesTrie fTrie;
 
+    // Storage for modified source string when aliases are expanded
+    CharString fModifiedSource;
+
     // Set to true when we've seen a "-per-" or a "per-", after which all units
     // are in the denominator. Until we find an "-and-", at which point the
     // identifier is invalid pending TODO(CLDR-13701).
@@ -830,6 +923,19 @@ class Parser {
             return {};
         }
 
+        // Handles the case where the alias replacement begins with "per-".
+        // For example:
+        //    if the alias is "permeter" and the replacement is "per-meter".
+        // NOTE: This case does not currently exist in CLDR, but this code anticipates possible future
+        // additions.
+        if (token.getType() == Token::TYPE_ALIAS) {
+            processAlias(token, status);
+            token = nextToken(status);
+            if (U_FAILURE(status)) {
+                return {};
+            }
+        }
+
         fJustSawPer = false;
 
         if (atStart) {
@@ -923,6 +1029,10 @@ class Parser {
                 singleUnitResult.index = token.getSimpleUnitIndex();
                 break;
 
+            case Token::TYPE_ALIAS:
+                processAlias(token, status);
+                break;
+
             default:
                 status = kUnitIdentifierSyntaxError;
                 return {};
@@ -945,6 +1055,48 @@ class Parser {
 
         return SingleUnitOrConstant::singleUnitValue(singleUnitResult);
     }
+
+  private:
+    /**
+     * Helper function to process alias replacement.
+     *
+     * @param token The token of TYPE_ALIAS to process
+     * @param status ICU error code
+     */
+    void processAlias(const Token &token, UErrorCode &status) {
+        if (U_FAILURE(status)) {
+            return;
+        }
+
+        auto aliasIndex = token.getAliasIndex();
+        if (aliasIndex < 0 || aliasIndex >= gUnitAliases.length() ||
+            aliasIndex >= gUnitReplacements.length()) {
+            status = kUnitIdentifierSyntaxError;
+            return;
+        }
+
+        auto replacement = gUnitReplacements[aliasIndex];
+
+        // Create new source string: replacement + remaining unparsed portion
+        fModifiedSource.clear();
+        fModifiedSource.append(replacement->data(), replacement->length(), status);
+
+        // Add the remaining unparsed portion of fSource which starts from fIndex
+        if (fIndex < fSource.length()) {
+            StringPiece remaining = fSource.substr(fIndex);
+            fModifiedSource.append(remaining.data(), remaining.length(), status);
+        }
+
+        if (U_FAILURE(status)) {
+            return;
+        }
+
+        // Update parser state with new source and reset index
+        fSource = StringPiece(fModifiedSource.data(), fModifiedSource.length());
+        fIndex = 0;
+
+        return;
+    }
 };
 
 // Sorting function wrapping SingleUnitImpl::compareTo for use with uprv_sortArray.

icu4c/source/i18n/number_longnames.cpp

@@ -438,6 +438,7 @@ void getMeasureData(const Locale &locale,
     subKey.append(unit.getType(), status);
     subKey.append("/", status);
 
+    // TODO(ICU-23226): Refactor LongNameHandler to use gUnitAliases and gUnitReplacements measunit_extra.cpp instead of local reasource bundle.
     // Check if unitSubType is an alias or not.
     LocalUResourceBundlePointer aliasBundle(ures_open(U_ICUDATA_ALIAS, "metadata", &status));
 

icu4c/source/test/intltest/numbertest_api.cpp

@@ -6063,27 +6063,85 @@ void NumberFormatterApiTest::formatUnitsAliases() {
         std::unique_ptr<MeasureUnit> measureUnit;
         const char * measureUnitString; // Only used if measureUnit is nullptr
         const UnicodeString expectedFormat;
+
+        TestCase(std::unique_ptr<MeasureUnit> mu, const UnicodeString &expected)
+            : measureUnit(std::move(mu)), measureUnitString(nullptr), expectedFormat(expected) {}
+        TestCase(const char *muStr, const UnicodeString &expected)
+            : measureUnit(nullptr), measureUnitString(muStr), expectedFormat(expected) {}
     } testCases[]{
-        // Aliases
-        {std::make_unique<MeasureUnit>(MeasureUnit::getMilligramOfglucosePerDeciliter()), nullptr, u"2 milligrams per deciliter"},
-        {std::make_unique<MeasureUnit>(MeasureUnit::getMilligramPerDeciliter()), nullptr, u"2 milligrams per deciliter"},
-        {std::make_unique<MeasureUnit>(MeasureUnit::getLiterPer100Kilometers()), nullptr,u"2 liters per 100 kilometers"},
-        {std::make_unique<MeasureUnit>(MeasureUnit::getPartPerMillion()), nullptr, u"2 parts per million"},
-        {std::make_unique<MeasureUnit>(MeasureUnit::getMillimeterOfMercury()), nullptr, u"2 millimeters of mercury"},
-
-        // Some replacements
-        {nullptr, "millimeter-ofhg", u"2 millimeters of mercury"},
-        {nullptr, "liter-per-100-kilometer", u"2 liters per 100 kilometers"},
-        {nullptr, "permillion", u"2 parts per million"},
-        {nullptr, "part-per-million", u"2 parts per million"},
-        {nullptr, "part-per-1e6", u"2 parts per million"},
+        // permillion
+        TestCase("permillion", u"2 parts per million"),
+        TestCase("part-per-million", u"2 parts per million"),
+        TestCase("portion-per-million", u"2 parts per million"),
+        TestCase("portion-per-1e6", u"2 parts per million"),
+        TestCase("part-per-1e6", u"2 parts per million"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getPartPer1E6()), u"2 parts per million"),
+
+        // part-per-billion
+        TestCase("portion-per-1e9", u"2 parts per billion"),
+        TestCase("part-per-1e9", u"2 parts per billion"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getPartPer1E9()), u"2 parts per billion"),
+
+        // pound-foot
+        TestCase("pound-foot", u"2 pound-force-feet"),
+        TestCase("pound-force-foot", u"2 pound-force-feet"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getPoundFoot()), u"2 pound-force-feet"),
+
+        // pound-force
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getPoundForce()), u"2 pounds of force"),
+        TestCase("pound-force", u"2 pounds of force"),
+
+        // pound-per-square-inch
+        TestCase("pound-per-square-inch", u"2 pounds-force per square inch"),
+        TestCase("pound-force-per-square-inch", u"2 pounds-force per square inch"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getPoundPerSquareInch()), u"2 pounds-force per square inch"),
+
+        // millimeter-of-mercury
+        TestCase("millimeter-of-mercury", u"2 millimeters of mercury"),
+        TestCase("millimeter-ofhg", u"2 millimeters of mercury"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getMillimeterOfMercury()), u"2 millimeters of mercury"),
+
+        // inch-hg
+        TestCase("inch-hg", u"2 inches of mercury"),
+        TestCase("inch-ofhg", u"2 inches of mercury"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getInchHg()), u"2 inches of mercury"),
+
+        // liter-per-100kilometers
+        TestCase("liter-per-100kilometers", u"2 liters per 100 kilometers"),
+        TestCase("liter-per-100-kilometer", u"2 liters per 100 kilometers"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getLiterPer100Kilometers()), u"2 liters per 100 kilometers"),
+        // meter-per-second-squared
+        TestCase("meter-per-second-squared", u"2 meters per second squared"),
+        TestCase("meter-per-square-second", u"2 meters per second squared"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getMeterPerSecondSquared()), u"2 meters per second squared"),
+
+        // metric-ton
+        TestCase("metric-ton", u"2 metric tons"),
+        TestCase("tonne", u"2 metric tons"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getMetricTon()), u"2 metric tons"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getTonne()), u"2 metric tons"),
+
+        // milligram-per-deciliter
+        TestCase("milligram-per-deciliter", u"2 milligrams per deciliter"),
+        TestCase("milligram-ofglucose-per-deciliter", u"2 milligrams per deciliter"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getMilligramPerDeciliter()), u"2 milligrams per deciliter"),
+        TestCase(std::make_unique<MeasureUnit>(MeasureUnit::getMilligramOfglucosePerDeciliter()), u"2 milligrams per deciliter"),
+
+        // Arbitrary
+        TestCase("meter-permillion", u"2 meter-parts per 1000000"),
+        TestCase("permillion-meter", u"2 parts per 1000000-meter"),
+        TestCase("tonne-per-second", u"2 metric tons per second"),
+        TestCase("part-per-1e6-per-100", u"expect exception"),
+        TestCase("permillion-per-100", u"expect exception"),
     };
 
     for (const auto &testCase : testCases) {
-        if (testCase.measureUnitString != nullptr && 
-            (uprv_strcmp("permillion", testCase.measureUnitString) == 0 ||
-             uprv_strcmp("part-per-million", testCase.measureUnitString) == 0)) {
-            logKnownIssue("ICU-23222", "Ensure unit aliases work correctly to avoid breaking callers");
+        if (testCase.expectedFormat == UnicodeString("expect exception")) {
+            UErrorCode exStatus = U_ZERO_ERROR;
+            MeasureUnit::forIdentifier(testCase.measureUnitString, exStatus);
+            if (U_SUCCESS(exStatus)) {
+                errln("exception expected");
+            }
             continue;
         }