From: Younies <younies.mahmoud@gmail.com>
Date: Wed, 22 Sep 2021 14:07:43 +0000 (+0200)
Subject: ICU-21544 Throw argument error when the units are not convertible.
X-Git-Tag: release-70-rc~16
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f1f0b22a2afe6c80a9d2d21652a5de6aaac87554;p=icu

ICU-21544 Throw argument error when the units are not convertible.
---

diff --git a/icu4c/source/i18n/units_converter.cpp b/icu4c/source/i18n/units_converter.cpp
index 4858cbd233c..7e946e584bb 100644
--- a/icu4c/source/i18n/units_converter.cpp
+++ b/icu4c/source/i18n/units_converter.cpp
@@ -461,7 +461,7 @@ Convertibility U_I18N_API extractConvertibility(const MeasureUnitImpl &source,
 
     if (source.complexity == UMeasureUnitComplexity::UMEASURE_UNIT_MIXED ||
         target.complexity == UMeasureUnitComplexity::UMEASURE_UNIT_MIXED) {
-        status = U_INTERNAL_PROGRAM_ERROR;
+        status = U_ARGUMENT_TYPE_MISMATCH;
         return UNCONVERTIBLE;
     }
 
@@ -514,7 +514,7 @@ void UnitsConverter::init(const ConversionRates &ratesInfo, UErrorCode &status)
 
     if (this->conversionRate_.source.complexity == UMeasureUnitComplexity::UMEASURE_UNIT_MIXED ||
         this->conversionRate_.target.complexity == UMeasureUnitComplexity::UMEASURE_UNIT_MIXED) {
-        status = U_INTERNAL_PROGRAM_ERROR;
+        status = U_ARGUMENT_TYPE_MISMATCH;
         return;
     }
 
@@ -522,7 +522,7 @@ void UnitsConverter::init(const ConversionRates &ratesInfo, UErrorCode &status)
                                                       this->conversionRate_.target, ratesInfo, status);
     if (U_FAILURE(status)) return;
     if (unitsState == Convertibility::UNCONVERTIBLE) {
-        status = U_INTERNAL_PROGRAM_ERROR;
+        status = U_ARGUMENT_TYPE_MISMATCH;
         return;
     }
 
@@ -540,7 +540,7 @@ int32_t UnitsConverter::compareTwoUnits(const MeasureUnitImpl &firstUnit,
 
     if (firstUnit.complexity == UMeasureUnitComplexity::UMEASURE_UNIT_MIXED ||
         secondUnit.complexity == UMeasureUnitComplexity::UMEASURE_UNIT_MIXED) {
-        status = U_INTERNAL_PROGRAM_ERROR;
+        status = U_ARGUMENT_TYPE_MISMATCH;
         return 0;
     }
 
@@ -550,7 +550,7 @@ int32_t UnitsConverter::compareTwoUnits(const MeasureUnitImpl &firstUnit,
     }
 
     if (unitsState == Convertibility::UNCONVERTIBLE || unitsState == Convertibility::RECIPROCAL) {
-        status = U_INTERNAL_PROGRAM_ERROR;
+        status = U_ARGUMENT_TYPE_MISMATCH;
         return 0;
     }
 
diff --git a/icu4c/source/test/intltest/numbertest.h b/icu4c/source/test/intltest/numbertest.h
index be6fe89eb15..d6d71543fb6 100644
--- a/icu4c/source/test/intltest/numbertest.h
+++ b/icu4c/source/test/intltest/numbertest.h
@@ -67,6 +67,7 @@ class NumberFormatterApiTest : public IntlTestWithFieldPosition {
     void unitCurrency();
     void unitInflections();
     void unitGender();
+    void unitNotConvertible();
     void unitPercent();
     void percentParity();
     void roundingFraction();
diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp
index 8d26b902a6b..cf8fecc0a28 100644
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -88,6 +88,7 @@ void NumberFormatterApiTest::runIndexedTest(int32_t index, UBool exec, const cha
         TESTCASE_AUTO(unitCurrency);
         TESTCASE_AUTO(unitInflections);
         TESTCASE_AUTO(unitGender);
+        TESTCASE_AUTO(unitNotConvertible);
         TESTCASE_AUTO(unitPercent);
         if (!quick) {
             // Slow test: run in exhaustive mode only
@@ -2699,6 +2700,31 @@ void NumberFormatterApiTest::unitGender() {
     assertEquals("getGender for a genderless language", "", fn.getGender(status));
 }
 
+void NumberFormatterApiTest::unitNotConvertible() {
+    IcuTestErrorCode status(*this, "unitNotConvertible");
+    const double randomNumber = 1234;
+
+    NumberFormatter::with()
+        .unit(MeasureUnit::forIdentifier("meter-and-liter", status))
+        .locale("en_US")
+        .formatDouble(randomNumber, status);
+    assertEquals(u"error must be returned", status.errorName(), u"U_ARGUMENT_TYPE_MISMATCH");
+
+    status.reset();
+    NumberFormatter::with()
+        .unit(MeasureUnit::forIdentifier("month-and-week", status))
+        .locale("en_US")
+        .formatDouble(randomNumber, status);
+    assertEquals(u"error must be returned", status.errorName(), u"U_ARGUMENT_TYPE_MISMATCH");
+
+    status.reset();
+    NumberFormatter::with()
+        .unit(MeasureUnit::forIdentifier("week-and-day", status))
+        .locale("en_US")
+        .formatDouble(randomNumber, status);
+    assertTrue(u"no error", !U_FAILURE(status));
+}
+
 void NumberFormatterApiTest::unitPercent() {
     assertFormatDescending(
             u"Percent",
diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java
index 9f80f439246..acece19279e 100644
--- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java
+++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java
@@ -9,6 +9,7 @@ import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.regex.Pattern;
 
+import com.ibm.icu.impl.IllegalIcuArgumentException;
 import com.ibm.icu.util.MeasureUnit;
 
 public class UnitsConverter {
@@ -48,8 +49,9 @@ public class UnitsConverter {
      */
     public UnitsConverter(MeasureUnitImpl source, MeasureUnitImpl target, ConversionRates conversionRates) {
         Convertibility convertibility = extractConvertibility(source, target, conversionRates);
-        // TODO(icu-units#82): throw exception if conversion between incompatible types was requested?
-        assert (convertibility == Convertibility.CONVERTIBLE || convertibility == Convertibility.RECIPROCAL);
+        if (convertibility != Convertibility.CONVERTIBLE && convertibility != Convertibility.RECIPROCAL) {
+            throw new IllegalIcuArgumentException("input units must be convertible or reciprocal");
+        }
 
         Factor sourceToBase = conversionRates.getFactorToBase(source);
         Factor targetToBase = conversionRates.getFactorToBase(target);
diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
index 9c2a0918ab1..c0ab0dadeb3 100644
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java
@@ -2690,6 +2690,40 @@ public class NumberFormatterApiTest extends TestFmwk {
         assertEquals("getGender for a genderless language", "", fn.getGender());
     }
 
+    @Test
+    public void unitNotConvertible() {
+        final double randomNumber = 1234;
+
+        try {
+            NumberFormatter.with()
+                    .unit(MeasureUnit.forIdentifier("meter-and-liter"))
+                    .locale(new ULocale("en_US"))
+                    .format(randomNumber);
+        } catch (Exception e) {
+            assertEquals("error must be thrown", "class com.ibm.icu.impl.IllegalIcuArgumentException", e.getClass().toString());
+        }
+
+        try {
+            NumberFormatter.with()
+                    .unit(MeasureUnit.forIdentifier("month-and-week"))
+                    .locale(new ULocale("en_US"))
+                    .format(randomNumber);
+        } catch (Exception e) {
+            assertEquals("error must be thrown", "class com.ibm.icu.impl.IllegalIcuArgumentException", e.getClass().toString());
+        }
+
+        try {
+            NumberFormatter.with()
+                    .unit(MeasureUnit.forIdentifier("day-and-hour"))
+                    .locale(new ULocale("en_US"))
+                    .format(2.5);
+        } catch (Exception e) {
+            // No errors.
+            assert false;
+        }
+
+    }
+
     @Test
     public void unitPercent() {
         assertFormatDescending(