Better user error detection in assert()

Reorders the assert() function usage checks for clarity, and detects
incorrect usage of assert (when the message is not a string) even when
the assertion condition is true.

Change-Id: Ia6e6f897d265bc76ca3d5a6d2b6acf6d3462831a
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/6340
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>

tools/gn/functions.cc

diff --git a/tools/gn/functions.cc b/tools/gn/functions.cc
index b08dbd0..ce76280 100644
--- a/tools/gn/functions.cc
+++ b/tools/gn/functions.cc
@@ -231,43 +231,55 @@
                 const FunctionCallNode* function,
                 const std::vector<Value>& args,
                 Err* err) {
+  // Check usage: Assert takes 1 or 2 arguments.
   if (args.size() != 1 && args.size() != 2) {
     *err = Err(function->function(), "Wrong number of arguments.",
-               "assert() takes one or two argument, "
-               "were you expecting somethig else?");
-  } else if (args[0].type() != Value::BOOLEAN) {
-    *err = Err(function->function(), "Assertion value not a bool.");
-  } else if (!args[0].boolean_value()) {
-    if (args.size() == 2) {
-      // Optional string message.
-      if (args[1].type() != Value::STRING) {
-        *err = Err(function->function(), "Assertion failed.",
-                   "<<<ERROR MESSAGE IS NOT A STRING>>>");
-      } else {
-        *err = Err(function->function(), "Assertion failed.",
-                   args[1].string_value());
-      }
-    } else {
-      *err = Err(function->function(), "Assertion failed.");
-    }
+               "assert() takes one or two arguments, "
+               "were you expecting something else?");
+    return Value();
+  }
 
-    if (args[0].origin()) {
-      // If you do "assert(foo)" we'd ideally like to show you where foo was
-      // set, and in this case the origin of the args will tell us that.
-      // However, if you do "assert(foo && bar)" the source of the value will
-      // be the assert like, which isn't so helpful.
-      //
-      // So we try to see if the args are from the same line or not. This will
-      // break if you do "assert(\nfoo && bar)" and we may show the second line
-      // as the source, oh well. The way around this is to check to see if the
-      // origin node is inside our function call block.
-      Location origin_location = args[0].origin()->GetRange().begin();
-      if (origin_location.file() != function->function().location().file() ||
-          origin_location.line_number() !=
-              function->function().location().line_number()) {
-        err->AppendSubErr(
-            Err(args[0].origin()->GetRange(), "", "This is where it was set."));
-      }
+  // Check usage: The first argument must be a boolean.
+  if (args[0].type() != Value::BOOLEAN) {
+    *err = Err(function->function(), "Assertion value not a bool.");
+    return Value();
+  }
+  bool assertion_passed = args[0].boolean_value();
+
+  // Check usage: The second argument, if present, must be a string.
+  if (args.size() == 2 && args[1].type() != Value::STRING) {
+    *err = Err(function->function(), "Assertion message is not a string.");
+    return Value();
+  }
+
+  // Assertion passed: there is nothing to do, so return an empty value.
+  if (assertion_passed) {
+    return Value();
+  }
+
+  // Assertion failed; try to make a useful message and report it.
+  if (args.size() == 2) {
+    *err = Err(function->function(), "Assertion failed.",
+               args[1].string_value());
+  } else {
+    *err = Err(function->function(), "Assertion failed.");
+  }
+  if (args[0].origin()) {
+    // If you do "assert(foo)" we'd ideally like to show you where foo was
+    // set, and in this case the origin of the args will tell us that.
+    // However, if you do "assert(foo && bar)" the source of the value will
+    // be the assert like, which isn't so helpful.
+    //
+    // So we try to see if the args are from the same line or not. This will
+    // break if you do "assert(\nfoo && bar)" and we may show the second line
+    // as the source, oh well. The way around this is to check to see if the
+    // origin node is inside our function call block.
+    Location origin_location = args[0].origin()->GetRange().begin();
+    if (origin_location.file() != function->function().location().file() ||
+        origin_location.line_number() !=
+            function->function().location().line_number()) {
+      err->AppendSubErr(
+          Err(args[0].origin()->GetRange(), "", "This is where it was set."));
     }
   }
   return Value();