JsonSchemaCompiler: Better error generation for array parse failures.

This CL improves the error messages for array parse failures in
generated code.

BUG=1113513

Change-Id: Ic1edfab5e005f363b8670b1f798a241673b856ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2520799Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827471}

chrome/common/extensions/manifest_handlers/content_scripts_manifest_unittest.cc

@@ -40,8 +40,9 @@ TEST_F(ContentScriptsManifestTest, MatchPattern) {
 
       // Match paterns must be strings.
       Testcase("content_script_match_pattern_not_string.json",
-               "Error at key 'content_scripts'. Parsing array failed: expected "
-               "string, got integer; unable to populate array 'matches'.")};
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: Error at key 'matches': Parsing array failed at index 0: "
+               "expected string, got integer")};
   RunTestcases(testcases, base::size(testcases), EXPECT_TYPE_ERROR);
 
   LoadAndExpectSuccess("ports_in_content_scripts.json");

chrome/common/extensions/manifest_handlers/exclude_matches_manifest_unittest.cc

@@ -21,8 +21,8 @@ TEST_F(ExcludeMatchesManifestTest, ExcludeMatchPatterns) {
 
   Testcase testcases2[] = {
       Testcase("exclude_matches_not_list.json",
-               "Error at key 'content_scripts'. Parsing array failed: "
-               "'exclude_matches': expected list, got string."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: 'exclude_matches': expected list, got string"),
       Testcase("exclude_matches_invalid_host.json",
                "Invalid value for 'content_scripts[0].exclude_matches[0]': "
                "Invalid host wildcard.")};

chrome/common/extensions/manifest_tests/extension_manifests_initvalue_unittest.cc

@@ -52,37 +52,40 @@ TEST_F(InitValueManifestTest, InitFromValueInvalid) {
                "Error at key 'content_scripts'. Type is invalid. Expected "
                "list, found integer."),
       Testcase("init_invalid_script_item_invalid.json",
-               "Error at key 'content_scripts'. Parsing array failed: expected "
-               "dictionary, got integer."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: expected dictionary, got integer"),
       Testcase("init_invalid_script_matches_missing.json",
-               "Error at key 'content_scripts'. Parsing array failed: "
-               "'matches' is required."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: 'matches' is required"),
       Testcase("init_invalid_script_matches_invalid.json",
-               "Error at key 'content_scripts'. Parsing array failed: "
-               "'matches': expected list, got integer."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: 'matches': expected list, got integer"),
       Testcase("init_invalid_script_matches_empty.json",
                errors::kInvalidMatchCount),
       Testcase("init_invalid_script_match_item_invalid.json",
-               "Error at key 'content_scripts'. Parsing array failed: expected "
-               "string, got integer; unable to populate array 'matches'."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: Error at key 'matches': Parsing array failed at index 0: "
+               "expected string, got integer"),
       Testcase("init_invalid_script_match_item_invalid_2.json",
                errors::kInvalidMatch),
       Testcase("init_invalid_script_files_missing.json", errors::kMissingFile),
       Testcase("init_invalid_files_js_invalid.json",
-               "Error at key 'content_scripts'. Parsing array failed: 'js': "
-               "expected list, got integer."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: 'js': expected list, got integer"),
       Testcase("init_invalid_files_empty.json", errors::kMissingFile),
       Testcase("init_invalid_files_js_empty_css_missing.json",
                errors::kMissingFile),
       Testcase("init_invalid_files_js_item_invalid.json",
-               "Error at key 'content_scripts'. Parsing array failed: expected "
-               "string, got integer; unable to populate array 'js'."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: Error at key 'js': Parsing array failed at index 0: "
+               "expected string, got integer"),
       Testcase("init_invalid_files_css_invalid.json",
-               "Error at key 'content_scripts'. Parsing array failed: 'css': "
-               "expected list, got integer."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: 'css': expected list, got integer"),
       Testcase("init_invalid_files_css_item_invalid.json",
-               "Error at key 'content_scripts'. Parsing array failed: expected "
-               "string, got integer; unable to populate array 'css'."),
+               "Error at key 'content_scripts'. Parsing array failed at index "
+               "0: Error at key 'css': Parsing array failed at index 0: "
+               "expected string, got integer"),
       Testcase("init_invalid_permissions_invalid.json",
                errors::kInvalidPermissions),
       Testcase("init_invalid_host_permissions_invalid.json",

extensions/common/api/declarative_net_request/dnr_manifest_unittest.cc

@@ -156,7 +156,7 @@ TEST_F(DNRManifestTest, InvalidRulesFileFormat) {
 
   LoadAndExpectError(
       "Error at key 'declarative_net_request.rule_resources'. Parsing array "
-      "failed: expected dictionary, got string.");
+      "failed at index 0: expected dictionary, got string");
 }
 
 TEST_F(DNRManifestTest, ZeroRulesets) {

tools/json_schema_compiler/cc_generator.py

@@ -999,11 +999,21 @@ class _Generator(object):
                      failure_value,
                      is_ptr=is_ptr))
       else:
-        c.Sblock('if (!%s(%s)) {' % (
+        args = ['*list', '&%(dst_var)s']
+        if self._generate_error_messages:
+          c.Append('base::string16 array_parse_error;')
+          args.append('&array_parse_error')
+
+        c.Append('if (!%s(%s)) {' % (
             self._util_cc_helper.PopulateArrayFromListFunction(is_ptr),
-            self._GenerateArgs(('*list', '&%(dst_var)s'))))
-        c.Concat(self._GenerateError(
-            '"unable to populate array \'%%(key)s\'"'))
+            self._GenerateArgs(args, generate_error_messages=False)))
+        c.Sblock()
+        if self._generate_error_messages:
+          c.Append(
+            'array_parse_error = base::UTF8ToUTF16("Error at key '
+            '\'%(key)s\': ") + array_parse_error;'
+          )
+          c.Concat(self._AppendError16('array_parse_error'))
         c.Append('return %(failure_value)s;')
         c.Eblock('}')
       c.Eblock('}')
@@ -1258,19 +1268,23 @@ class _Generator(object):
         self._type_helper.GetEnumNoneValue(prop.type_)))
     return c
 
-  def _GenerateError(self, body):
-    """Generates an error message pertaining to population failure.
-
-    E.g 'expected bool, got int'
+  def _AppendError16(self, error16):
+    """Appends the given |error16| expression/variable to |error|.
     """
     c = Code()
     if not self._generate_error_messages:
       return c
     (c.Append('if (error->length())')
       .Append('  error->append(UTF8ToUTF16("; "));')
-      .Append('error->append(UTF8ToUTF16(%s));' % body))
+      .Append('error->append(%s);' % error16))
     return c
 
+  def _GenerateError(self, body):
+    """Generates an error message pertaining to population failure.
+    E.g 'expected bool, got int'
+    """
+    return self._AppendError16('UTF8ToUTF16(%s)' % body)
+
   def _GenerateParams(self, params, generate_error_messages=None):
     """Builds the parameter list for a function, given an array of parameters.
     If |generate_error_messages| is specified, it overrides
@@ -1282,9 +1296,13 @@ class _Generator(object):
       params = list(params) + ['base::string16* error']
     return ', '.join(str(p) for p in params)
 
-  def _GenerateArgs(self, args):
+  def _GenerateArgs(self, args, generate_error_messages=None):
     """Builds the argument list for a function, given an array of arguments.
+    If |generate_error_messages| is specified, it overrides
+    |self._generate_error_messages|.
     """
-    if self._generate_error_messages:
+    if generate_error_messages is None:
+      generate_error_messages = self._generate_error_messages
+    if generate_error_messages:
       args = list(args) + ['error']
     return ', '.join(str(a) for a in args)

tools/json_schema_compiler/manifest_parse_util.cc

@@ -40,20 +40,6 @@ bool ParseHelper(const base::DictionaryValue& dict,
 
 }  // namespace
 
-void PopulateArrayParseError(
-    base::StringPiece key,
-    base::string16* error,
-    std::vector<base::StringPiece>* error_path_reversed) {
-  DCHECK(error);
-  DCHECK(error_path_reversed);
-  DCHECK(!error->empty());
-  DCHECK(error_path_reversed->empty());
-
-  error_path_reversed->push_back(key);
-  *error = base::ASCIIToUTF16(base::StringPrintf(
-      "Parsing array failed: %s.", base::UTF16ToASCII(*error).c_str()));
-}
-
 void PopulateInvalidEnumValueError(
     base::StringPiece key,
     const std::string& value,

tools/json_schema_compiler/manifest_parse_util.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/check.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_piece_forward.h"
 #include "base/values.h"
@@ -17,14 +18,6 @@ namespace manifest_parse_util {
 
 // This file contains helpers used by auto-generated manifest parsing code.
 
-// Populates |error| and |error_path_reversed| denoting array parse error at the
-// given |key|. Note |error| should already contain the specific parse error for
-// the array item.
-void PopulateArrayParseError(
-    base::StringPiece key,
-    base::string16* error,
-    std::vector<base::StringPiece>* error_path_reversed);
-
 // Populates |error| and |error_path_reversed| denoting the given invalid enum
 // |value| at the given |key|.
 void PopulateInvalidEnumValueError(
@@ -109,8 +102,11 @@ bool ParseFromDictionary(const base::DictionaryValue& dict,
 
   bool result = json_schema_compiler::util::PopulateArrayFromList(
       value->AsListValue(*value), out_ptr, error);
-  if (!result)
-    PopulateArrayParseError(key, error, error_path_reversed);
+  if (!result) {
+    DCHECK(error_path_reversed);
+    DCHECK(error_path_reversed->empty());
+    error_path_reversed->push_back(key);
+  }
 
   return result;
 }

tools/json_schema_compiler/test/error_generation_unittest.cc

@@ -174,7 +174,8 @@ TEST(JsonSchemaCompilerErrorTest, UnableToPopulateArray) {
     std::unique_ptr<base::ListValue> params_value =
         List(std::make_unique<Value>(5), std::make_unique<Value>(false));
     EXPECT_TRUE(EqualsUtf16(
-        "expected integer, got boolean; unable to populate array 'integers'",
+        "Error at key 'integers': Parsing array failed at index 1: expected "
+        "integer, got boolean",
         GetPopulateError<errors::ChoiceType::Integers>(*params_value)));
   }
 }
@@ -300,9 +301,10 @@ TEST(JsonSchemaCompilerErrorTest, OptionalUnableToPopulateArray) {
     base::string16 error;
     EXPECT_FALSE(errors::OptionalChoiceType::Integers::Populate(*params_value,
                                                                 &out, &error));
-    EXPECT_TRUE(EqualsUtf16(
-        "expected integer, got boolean; unable to populate array 'integers'",
-        error));
+    EXPECT_TRUE(
+        EqualsUtf16("Error at key 'integers': Parsing array failed at index 1: "
+                    "expected integer, got boolean",
+                    error));
     EXPECT_EQ(NULL, out.as_integer.get());
   }
 }

tools/json_schema_compiler/test/simple_api_unittest.cc

@@ -275,8 +275,8 @@ TEST(JsonSchemaCompilerSimpleTest, ManifestKeyParsing_ArrayParseError) {
   std::string error;
   ASSERT_NO_FATAL_FAILURE(GetManifestParseError(kPartialManifestJson, &error));
   EXPECT_EQ(
-      "Error at key 'key_ref.array'. Parsing array failed: expected string, "
-      "got integer.",
+      "Error at key 'key_ref.array'. Parsing array failed at index 2: expected "
+      "string, got integer",
       error);
 }
 

tools/json_schema_compiler/util.h

@@ -10,6 +10,9 @@
 #include <utility>
 #include <vector>
 
+#include "base/format_macros.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 
 namespace json_schema_compiler {
@@ -83,14 +86,22 @@ bool PopulateArrayFromList(const base::ListValue& list, std::vector<T>* out) {
 // Populates |out| with |list|. Returns false and sets |error| if there is no
 // list at the specified key or if the list has anything other than |T|.
 template <class T>
-bool PopulateArrayFromList(const base::ListValue& list,
+bool PopulateArrayFromList(const base::ListValue& list_value,
                            std::vector<T>* out,
                            base::string16* error) {
   out->clear();
   T item;
-  for (const auto& value : list) {
-    if (!PopulateItem(value, &item, error))
+  base::string16 item_error;
+  const auto& list = list_value.GetList();
+  for (size_t i = 0; i < list.size(); ++i) {
+    if (!PopulateItem(list[i], &item, &item_error)) {
+      if (!error->empty())
+        error->append(base::ASCIIToUTF16("; "));
+      error->append(base::ASCIIToUTF16(
+          base::StringPrintf("Parsing array failed at index %" PRIuS ": %s", i,
+                             base::UTF16ToASCII(item_error).c_str())));
       return false;
+    }
     out->push_back(std::move(item));
   }