From 8ceb3680477e61506d9e93168620abc676bb217f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=89milie=20Feral?= <emilie.feral@numworks.com>
Date: Thu, 28 May 2020 16:13:48 +0200
Subject: [PATCH] [poincare] Adding a number before orphan unit is done at
 beautification instead of at reducing (revert previous commit)

This fixes the following bug: input log(0*f(_t)) --> crash due to an
infinite loop where we remove/add 1 before the multiplication
---
 poincare/include/poincare/unit.h |  2 ++
 poincare/src/multiplication.cpp  |  4 ----
 poincare/src/power.cpp           | 15 ++++++++++-----
 poincare/src/unit.cpp            | 19 +++++++++++++++----
 4 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/poincare/include/poincare/unit.h b/poincare/include/poincare/unit.h
index d286d4bc6..5990c6320 100644
--- a/poincare/include/poincare/unit.h
+++ b/poincare/include/poincare/unit.h
@@ -166,6 +166,7 @@ public:
 
   // Simplification
   Expression shallowReduce(ReductionContext reductionContext) override;
+  Expression shallowBeautify(ReductionContext reductionContext) override;
   LayoutShape leftLayoutShape() const override { return LayoutShape::OneLetter; } // TODO
 
   const Dimension * dimension() const { return m_dimension; }
@@ -777,6 +778,7 @@ public:
 
   // Simplification
   Expression shallowReduce(ExpressionNode::ReductionContext reductionContext);
+  Expression shallowBeautify(ExpressionNode::ReductionContext reductionContext);
   static void ChooseBestRepresentativeAndPrefixForValue(Expression * units, double * value, ExpressionNode::ReductionContext reductionContext) { return ChooseBestMultipleForValue(units, value, true, reductionContext); }
   static void ChooseBestPrefixForValue(Expression * units, double * value, ExpressionNode::ReductionContext reductionContext) { return ChooseBestMultipleForValue(units, value, false, reductionContext); }
 
diff --git a/poincare/src/multiplication.cpp b/poincare/src/multiplication.cpp
index 8bef55675..ff11d453d 100644
--- a/poincare/src/multiplication.cpp
+++ b/poincare/src/multiplication.cpp
@@ -695,10 +695,6 @@ Expression Multiplication::privateShallowReduce(ExpressionNode::ReductionContext
     if (hasUnit()) {
       // Do not expand Multiplication in presence of units
       shouldExpand = false;
-      // Make sure a Multiplication is not made of (Power of) Units only
-      if (!c.isNumber()) {
-        addChildAtIndexInPlace(Rational::Builder(1), 0, numberOfChildren());
-      }
     } else if (c.type() != ExpressionNode::Type::Rational) {
     } else if (static_cast<const Rational &>(c).isZero()) {
       // Check that other children don't match inf or unit
diff --git a/poincare/src/power.cpp b/poincare/src/power.cpp
index 1e3f00342..4ab6fb07f 100644
--- a/poincare/src/power.cpp
+++ b/poincare/src/power.cpp
@@ -5,6 +5,7 @@
 #include <poincare/constant.h>
 #include <poincare/cosine.h>
 #include <poincare/division.h>
+#include <poincare/float.h>
 #include <poincare/horizontal_layout.h>
 #include <poincare/infinity.h>
 #include <poincare/matrix.h>
@@ -382,7 +383,6 @@ Expression Power::removeUnit(Expression * unit) {
     assert(child.isRationalOne());
     assert(childUnit.type() == ExpressionNode::Type::Unit);
     Power p = *this;
-    Expression result = child;
     replaceWithInPlace(child);
     p.replaceChildAtIndexInPlace(0, childUnit);
     *unit = p;
@@ -417,10 +417,6 @@ Expression Power::shallowReduce(ExpressionNode::ReductionContext reductionContex
         // The exponent must be an Integer
         return replaceWithUndefinedInPlace();
       }
-      if (parent().isUninitialized() && base.type() == ExpressionNode::Type::Unit) {
-        // A Power of Unit must not be orphan
-        return Multiplication::Builder(Rational::Builder(1), *this);
-      }
     }
   }
 
@@ -1001,6 +997,15 @@ Expression Power::shallowBeautify(ExpressionNode::ReductionContext reductionCont
     replaceWithInPlace(result);
     return result;
   }
+
+  // Step 4: Force Float(1) in front of an orphan Power of Unit
+  if (parent().isUninitialized() && childAtIndex(0).type() == ExpressionNode::Type::Unit) {
+    Multiplication m = Multiplication::Builder(Float<double>::Builder(1.0));
+    replaceWithInPlace(m);
+    m.addChildAtIndexInPlace(*this, 1, 1);
+    return std::move(m);
+  }
+
   return *this;
 }
 
diff --git a/poincare/src/unit.cpp b/poincare/src/unit.cpp
index e67ec6f10..3941fb7ca 100644
--- a/poincare/src/unit.cpp
+++ b/poincare/src/unit.cpp
@@ -222,6 +222,10 @@ Expression UnitNode::shallowReduce(ReductionContext reductionContext) {
   return Unit(this).shallowReduce(reductionContext);
 }
 
+Expression UnitNode::shallowBeautify(ReductionContext reductionContext) {
+  return Unit(this).shallowBeautify(reductionContext);
+}
+
 constexpr const Unit::Prefix
   Unit::PicoPrefix,
   Unit::NanoPrefix,
@@ -317,14 +321,21 @@ Expression Unit::shallowReduce(ExpressionNode::ReductionContext reductionContext
     Expression multiplier = Power::Builder(Rational::Builder(10), Rational::Builder(prefixMultiplier)).shallowReduce(reductionContext);
     result = Multiplication::Builder(multiplier, result).shallowReduce(reductionContext);
   }
-  if (parent().isUninitialized() && result.type() == ExpressionNode::Type::Unit) {
-    // A Unit must not be orphan
-    result = Multiplication::Builder(Rational::Builder(1), result);
-  }
   replaceWithInPlace(result);
   return result;
 }
 
+Expression Unit::shallowBeautify(ExpressionNode::ReductionContext reductionContext) {
+  // Force Float(1) in front of an orphan Unit
+  if (parent().isUninitialized()) {
+    Multiplication m = Multiplication::Builder(Float<double>::Builder(1.0));
+    replaceWithInPlace(m);
+    m.addChildAtIndexInPlace(*this, 1, 1);
+    return std::move(m);
+  }
+  return *this;
+}
+
 void Unit::ChooseBestMultipleForValue(Expression * units, double * value, bool tuneRepresentative, ExpressionNode::ReductionContext reductionContext) {
   // Identify the first Unit factor and its exponent
   Expression firstFactor = *units;