[torque]: Improve error output

In the process, add a utility functions to automate printing out comma-separated
lists. Also make sure that the << operator applies to "const Type&" rather than
"const Type*" for consistency elsewhere and generally just good practice.

Bug: v8:7793
Change-Id: I488e8383c4a9496552e63601738d6bcca0ca6e80
Reviewed-on: https://chromium-review.googlesource.com/1111854
Commit-Queue: Daniel Clifford <danno@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54038}

src/torque/declarable.cc

@@ -13,37 +13,57 @@ namespace torque {
 
 std::ostream& operator<<(std::ostream& os, const Callable& m) {
   os << "callable " << m.name() << "(" << m.signature().parameter_types
-     << "): " << m.signature().return_type;
+     << "): " << *m.signature().return_type;
   return os;
 }
 
 std::ostream& operator<<(std::ostream& os, const Variable& v) {
-  os << "variable " << v.name() << ": " << v.type();
+  os << "variable " << v.name() << ": " << *v.type();
   return os;
 }
 
 std::ostream& operator<<(std::ostream& os, const Builtin& b) {
-  os << "builtin " << b.signature().return_type << " " << b.name()
+  os << "builtin " << *b.signature().return_type << " " << b.name()
      << b.signature().parameter_types;
   return os;
 }
 
 std::ostream& operator<<(std::ostream& os, const RuntimeFunction& b) {
-  os << "runtime function " << b.signature().return_type << " " << b.name()
+  os << "runtime function " << *b.signature().return_type << " " << b.name()
      << b.signature().parameter_types;
   return os;
 }
 
-std::ostream& operator<<(std::ostream& os, const Generic& g) {
-  os << "generic " << g.name() << "<";
-  bool first = true;
-  for (auto t : g.declaration()->generic_parameters) {
-    if (!first) {
-      os << ", ";
+void PrintLabel(std::ostream& os, const Label& l, bool with_names) {
+  os << l.name();
+  if (l.GetParameterCount() != 0) {
+    os << "(";
+    if (with_names) {
+      PrintCommaSeparatedList(os, l.GetParameters(),
+                              [](Variable* v) -> std::string {
+                                std::stringstream stream;
+                                stream << v->name();
+                                stream << ": ";
+                                stream << *(v->type());
+                                return stream.str();
+                              });
+    } else {
+      PrintCommaSeparatedList(
+          os, l.GetParameters(),
+          [](Variable* v) -> const Type& { return *(v->type()); });
     }
-    first = false;
-    os << t << ": type";
+    os << ")";
   }
+}
+
+std::ostream& operator<<(std::ostream& os, const Label& l) {
+  PrintLabel(os, l, true);
+  return os;
+}
+
+std::ostream& operator<<(std::ostream& os, const Generic& g) {
+  os << "generic " << g.name() << "<";
+  PrintCommaSeparatedList(os, g.declaration()->generic_parameters);
   os << ">";
 
   return os;

src/torque/declarable.h

@@ -337,9 +337,12 @@ class TypeAlias : public Declarable {
   const Type* type_;
 };
 
+void PrintLabel(std::ostream& os, const Label& l, bool with_names);
+
 std::ostream& operator<<(std::ostream& os, const Callable& m);
 std::ostream& operator<<(std::ostream& os, const Variable& v);
 std::ostream& operator<<(std::ostream& os, const Builtin& b);
+std::ostream& operator<<(std::ostream& os, const Label& l);
 std::ostream& operator<<(std::ostream& os, const RuntimeFunction& b);
 std::ostream& operator<<(std::ostream& os, const Generic& g);
 

src/torque/declaration-visitor.cc

@@ -90,7 +90,7 @@ Builtin* DeclarationVisitor::BuiltinDeclarationCommon(
           declarations()->LookupGlobalType(OBJECT_TYPE_STRING))) {
       std::stringstream stream;
       stream << "second parameter to javascript builtin " << decl->name
-             << " is " << signature.types()[1] << " but should be Object";
+             << " is " << *signature.types()[1] << " but should be Object";
       ReportError(stream.str());
     }
   }
@@ -256,7 +256,7 @@ void DeclarationVisitor::Visit(VarDeclarationStatement* stmt) {
   }
   declarations()->DeclareVariable(variable_name, type);
   if (global_context_.verbose()) {
-    std::cout << "declared variable " << variable_name << " with type " << type
+    std::cout << "declared variable " << variable_name << " with type " << *type
               << "\n";
   }
   if (stmt->initializer) {

src/torque/file-visitor.cc

@@ -25,102 +25,6 @@ Signature FileVisitor::MakeSignature(const CallableNodeSignature* signature) {
   return result;
 }
 
-namespace {
-
-void PrintMacroSignatures(std::stringstream& s,
-                          const std::vector<Macro*>& macros) {
-  for (Macro* m : macros) {
-    s << "\n    " << m->name() << m->signature();
-  }
-}
-
-}  // namespace
-
-Callable* FileVisitor::LookupCall(const std::string& name,
-                                  const Arguments& arguments) {
-  Callable* result = nullptr;
-  TypeVector parameter_types(arguments.parameters.GetTypeVector());
-  Declarable* declarable = declarations()->Lookup(name);
-  if (declarable->IsBuiltin()) {
-    result = Builtin::cast(declarable);
-  } else if (declarable->IsRuntimeFunction()) {
-    result = RuntimeFunction::cast(declarable);
-  } else if (declarable->IsMacroList()) {
-    std::vector<Macro*> candidates;
-    std::vector<Macro*> macros_with_same_name;
-    for (Macro* m : MacroList::cast(declarable)->list()) {
-      bool try_bool_context =
-          arguments.labels.size() == 0 &&
-          m->signature().return_type == TypeOracle::GetNeverType();
-      Label* true_label = nullptr;
-      Label* false_label = nullptr;
-      if (try_bool_context) {
-        true_label = declarations()->TryLookupLabel(kTrueLabelName);
-        false_label = declarations()->TryLookupLabel(kFalseLabelName);
-      }
-      if (IsCompatibleSignature(m->signature(), parameter_types,
-                                arguments.labels) ||
-          (true_label && false_label &&
-           IsCompatibleSignature(m->signature(), parameter_types,
-                                 {true_label, false_label}))) {
-        candidates.push_back(m);
-      } else {
-        macros_with_same_name.push_back(m);
-      }
-    }
-
-    if (candidates.empty() && macros_with_same_name.empty()) {
-      return nullptr;
-    } else if (candidates.empty()) {
-      std::stringstream stream;
-      stream << "cannot find macro with name \"" << name
-             << "\" and parameter type(s) (" << parameter_types
-             << "), candidates are:";
-      PrintMacroSignatures(stream, macros_with_same_name);
-      ReportError(stream.str());
-    }
-
-    auto is_better_candidate = [&](Macro* a, Macro* b) {
-      return ParameterDifference(a->signature().parameter_types.types,
-                                 parameter_types)
-          .StrictlyBetterThan(ParameterDifference(
-              b->signature().parameter_types.types, parameter_types));
-    };
-
-    Macro* best = *std::min_element(candidates.begin(), candidates.end(),
-                                    is_better_candidate);
-    for (Macro* candidate : candidates) {
-      if (candidate != best && !is_better_candidate(best, candidate)) {
-        std::stringstream s;
-        s << "ambiguous macro \"" << name << "\" with types ("
-          << parameter_types << "), candidates:";
-        PrintMacroSignatures(s, candidates);
-        ReportError(s.str());
-      }
-    }
-    result = best;
-  } else {
-    std::stringstream stream;
-    stream << "can't call " << declarable->type_name() << " " << name
-           << " because it's not callable"
-           << ": call parameters were (" << parameter_types << ")";
-    ReportError(stream.str());
-  }
-
-  size_t caller_size = parameter_types.size();
-  size_t callee_size = result->signature().types().size();
-  if (caller_size != callee_size &&
-      !result->signature().parameter_types.var_args) {
-    std::stringstream stream;
-    stream << "parameter count mismatch calling " << *result << " - expected "
-           << std::to_string(callee_size) << ", found "
-           << std::to_string(caller_size);
-    ReportError(stream.str());
-  }
-
-  return result;
-}
-
 void FileVisitor::QueueGenericSpecialization(
     const SpecializationKey& key, CallableNode* callable,
     const CallableNodeSignature* signature, base::Optional<Statement*> body) {

src/torque/file-visitor.h

@@ -67,8 +67,6 @@ class FileVisitor {
     return std::string("p_") + name;
   }
 
-  Callable* LookupCall(const std::string& name, const Arguments& arguments);
-
   Signature MakeSignature(const CallableNodeSignature* signature);
 
   struct PendingSpecialization {

src/torque/implementation-visitor.cc

@@ -392,7 +392,8 @@ VisitResult ImplementationVisitor::Visit(LogicalOrExpression* expr) {
   if (right_result.type() != left_result.type()) {
     std::stringstream stream;
     stream << "types of left and right expression of logical OR don't match (\""
-           << left_result.type() << "\" vs. \"" << right_result.type() << "\")";
+           << *left_result.type() << "\" vs. \"" << *right_result.type()
+           << "\")";
     ReportError(stream.str());
   }
   if (left_result.type()->IsConstexprBool()) {
@@ -425,7 +426,7 @@ VisitResult ImplementationVisitor::Visit(LogicalAndExpression* expr) {
     std::stringstream stream;
     stream
         << "types of left and right expression of logical AND don't match (\""
-        << left_result.type() << "\" vs. \"" << right_result.type() << "\")";
+        << *left_result.type() << "\" vs. \"" << *right_result.type() << "\")";
     ReportError(stream.str());
   }
   if (left_result.type()->IsConstexprBool()) {
@@ -567,7 +568,7 @@ const Type* ImplementationVisitor::Visit(IfStatement* stmt) {
     if (!(expression_result.type() == TypeOracle::GetConstexprBoolType())) {
       std::stringstream stream;
       stream << "expression should return type constexpr bool "
-             << "but returns type " << expression_result.type();
+             << "but returns type " << *expression_result.type();
       ReportError(stream.str());
     }
 
@@ -738,7 +739,7 @@ const Type* ImplementationVisitor::Visit(AssertStatement* stmt) {
     } else {
       if (expression_result.type() != TypeOracle::GetNeverType()) {
         std::stringstream s;
-        s << "unexpected return type " << expression_result.type()
+        s << "unexpected return type " << *expression_result.type()
           << " for branch expression";
         ReportError(s.str());
       }
@@ -777,7 +778,7 @@ const Type* ImplementationVisitor::Visit(ReturnStatement* stmt) {
     if (!stmt->value) {
       std::stringstream s;
       s << "return expression needs to be specified for a return type of "
-        << current_callable->signature().return_type;
+        << *current_callable->signature().return_type;
       ReportError(s.str());
     }
     VisitResult expression_result = Visit(*stmt->value);
@@ -1090,6 +1091,116 @@ void ImplementationVisitor::GenerateMacroFunctionDeclaration(
   o << ")";
 }
 
+namespace {
+
+void PrintMacroSignatures(std::stringstream& s, const std::string& name,
+                          const std::vector<Macro*>& macros) {
+  for (Macro* m : macros) {
+    s << "\n  " << name;
+    PrintSignature(s, m->signature(), false);
+  }
+}
+
+void FailMacroLookup(const std::string& reason, const std::string& name,
+                     const Arguments& arguments,
+                     const std::vector<Macro*>& candidates) {
+  std::stringstream stream;
+  stream << "\n"
+         << reason << ": \n  " << name << "("
+         << arguments.parameters.GetTypeVector() << ")";
+  if (arguments.labels.size() != 0) {
+    stream << " labels ";
+    for (auto l : arguments.labels) {
+      PrintLabel(stream, *l, false);
+    }
+  }
+  stream << "\ncandidates are:";
+  PrintMacroSignatures(stream, name, candidates);
+  ReportError(stream.str());
+}
+
+}  // namespace
+
+Callable* ImplementationVisitor::LookupCall(const std::string& name,
+                                            const Arguments& arguments) {
+  Callable* result = nullptr;
+  TypeVector parameter_types(arguments.parameters.GetTypeVector());
+  Declarable* declarable = declarations()->Lookup(name);
+  if (declarable->IsBuiltin()) {
+    result = Builtin::cast(declarable);
+  } else if (declarable->IsRuntimeFunction()) {
+    result = RuntimeFunction::cast(declarable);
+  } else if (declarable->IsMacroList()) {
+    std::vector<Macro*> candidates;
+    std::vector<Macro*> macros_with_same_name;
+    for (Macro* m : MacroList::cast(declarable)->list()) {
+      bool try_bool_context =
+          arguments.labels.size() == 0 &&
+          m->signature().return_type == TypeOracle::GetNeverType();
+      Label* true_label = nullptr;
+      Label* false_label = nullptr;
+      if (try_bool_context) {
+        true_label = declarations()->TryLookupLabel(kTrueLabelName);
+        false_label = declarations()->TryLookupLabel(kFalseLabelName);
+      }
+      if (IsCompatibleSignature(m->signature(), parameter_types,
+                                arguments.labels) ||
+          (true_label && false_label &&
+           IsCompatibleSignature(m->signature(), parameter_types,
+                                 {true_label, false_label}))) {
+        candidates.push_back(m);
+      } else {
+        macros_with_same_name.push_back(m);
+      }
+    }
+
+    if (candidates.empty() && macros_with_same_name.empty()) {
+      std::stringstream stream;
+      stream << "no matching declaration found for " << name;
+      ReportError(stream.str());
+    } else if (candidates.empty()) {
+      FailMacroLookup("cannot find macro with name", name, arguments,
+                      macros_with_same_name);
+    }
+
+    auto is_better_candidate = [&](Macro* a, Macro* b) {
+      return ParameterDifference(a->signature().parameter_types.types,
+                                 parameter_types)
+          .StrictlyBetterThan(ParameterDifference(
+              b->signature().parameter_types.types, parameter_types));
+    };
+
+    Macro* best = *std::min_element(candidates.begin(), candidates.end(),
+                                    is_better_candidate);
+    for (Macro* candidate : candidates) {
+      if (candidate != best && !is_better_candidate(best, candidate)) {
+        FailMacroLookup("ambiguous macro", name, arguments,
+                        macros_with_same_name);
+      }
+    }
+    result = best;
+  } else {
+    std::stringstream stream;
+    stream << "can't call " << declarable->type_name() << " " << name
+           << " because it's not callable"
+           << ": call parameters were (" << parameter_types << ")";
+    ReportError(stream.str());
+  }
+
+  size_t caller_size = parameter_types.size();
+  size_t callee_size = result->signature().types().size();
+  if (caller_size != callee_size &&
+      !result->signature().parameter_types.var_args) {
+    std::stringstream stream;
+    stream << "parameter count mismatch calling " << *result << " - expected "
+           << std::to_string(callee_size) << ", found "
+           << std::to_string(caller_size);
+    ReportError(stream.str());
+  }
+
+  return result;
+}
+
 void ImplementationVisitor::GenerateChangedVarsFromControlSplit(AstNode* node) {
   const std::set<const Variable*>& changed_vars =
       global_context_.GetControlSplitChangedVariables(
@@ -1117,7 +1228,7 @@ const Type* ImplementationVisitor::GetCommonType(const Type* left,
     common_type = right;
   } else {
     std::stringstream s;
-    s << "illegal combination of types " << left << " and " << right;
+    s << "illegal combination of types " << *left << " and " << *right;
     ReportError(s.str());
   }
   return common_type;
@@ -1270,7 +1381,7 @@ VisitResult ImplementationVisitor::GeneratePointerCall(
   if (!callee_result.type()->IsFunctionPointerType()) {
     std::stringstream stream;
     stream << "Expected a function pointer type but found "
-           << callee_result.type();
+           << *callee_result.type();
     ReportError(stream.str());
   }
   const FunctionPointerType* type =
@@ -1279,7 +1390,7 @@ VisitResult ImplementationVisitor::GeneratePointerCall(
   if (type->parameter_types().size() != parameter_types.size()) {
     std::stringstream stream;
     stream << "parameter count mismatch calling function pointer with Type: "
-           << type << " - expected "
+           << *type << " - expected "
            << std::to_string(type->parameter_types().size()) << ", found "
            << std::to_string(parameter_types.size());
     ReportError(stream.str());
@@ -1344,11 +1455,7 @@ VisitResult ImplementationVisitor::GeneratePointerCall(
 VisitResult ImplementationVisitor::GenerateCall(
     const std::string& callable_name, Arguments arguments, bool is_tailcall) {
   Callable* callable = LookupCall(callable_name, arguments);
-  if (callable == nullptr) {
-    std::stringstream stream;
-    stream << "no matching declaration found for " << callable_name;
-    ReportError(stream.str());
-  }
+
   // Operators used in a branching context can also be function calls that never
   // return but have a True and False label
   if (arguments.labels.size() == 0 &&
@@ -1446,8 +1553,8 @@ VisitResult ImplementationVisitor::GenerateCall(
       Variable* variable = label->GetParameter(j);
       if (!(variable->type() == t)) {
         std::stringstream s;
-        s << "mismatch of label parameters (expected " << t << " got "
-          << label->GetParameter(j)->type() << " for parameter "
+        s << "mismatch of label parameters (expected " << *t << " got "
+          << *label->GetParameter(j)->type() << " for parameter "
           << std::to_string(i + 1) << ")";
         ReportError(s.str());
       }
@@ -1470,7 +1577,8 @@ VisitResult ImplementationVisitor::GenerateCall(
 }
 
 void ImplementationVisitor::Visit(StandardDeclaration* decl) {
-  Visit(decl->callable, {}, decl->body);
+  Signature signature = MakeSignature(decl->callable->signature.get());
+  Visit(decl->callable, signature, decl->body);
 }
 
 void ImplementationVisitor::Visit(SpecializationDeclaration* decl) {
@@ -1580,7 +1688,7 @@ bool ImplementationVisitor::GenerateExpressionBranch(
   } else {
     if (expression_result.type() != TypeOracle::GetNeverType()) {
       std::stringstream s;
-      s << "unexpected return type " << expression_result.type()
+      s << "unexpected return type " << *expression_result.type()
         << " for branch expression";
       ReportError(s.str());
     }
@@ -1604,8 +1712,8 @@ VisitResult ImplementationVisitor::GenerateImplicitConvert(
     return VisitResult(destination_type, source.variable());
   } else {
     std::stringstream s;
-    s << "cannot use expression of type " << source.type()
-      << " as a value of type " << destination_type;
+    s << "cannot use expression of type " << *source.type()
+      << " as a value of type " << *destination_type;
     ReportError(s.str());
   }
   return VisitResult(TypeOracle::GetVoidType(), "");

src/torque/implementation-visitor.h

@@ -173,6 +173,8 @@ class ImplementationVisitor : public FileVisitor {
     ImplementationVisitor* visitor_;
   };
 
+  Callable* LookupCall(const std::string& name, const Arguments& arguments);
+
   void GenerateChangedVarsFromControlSplit(AstNode* node);
 
   const Type* GetCommonType(const Type* left, const Type* right);

src/torque/types.cc

@@ -83,15 +83,8 @@ std::string AbstractType::GetGeneratedTNodeTypeName() const {
 std::string FunctionPointerType::ToExplicitString() const {
   std::stringstream result;
   result << "builtin (";
-  bool first = true;
-  for (const Type* t : parameter_types_) {
-    if (!first) {
-      result << ", ";
-      first = false;
-    }
-    result << t;
-  }
-  result << ") => " << return_type_;
+  PrintCommaSeparatedList(result, parameter_types_);
+  result << ") => " << *return_type_;
   return result.str();
 }
 
@@ -116,7 +109,7 @@ std::string UnionType::ToExplicitString() const {
       result << " | ";
     }
     first = false;
-    result << t;
+    result << *t;
   }
   result << ")";
   return result.str();
@@ -148,21 +141,23 @@ std::string UnionType::GetGeneratedTNodeTypeName() const {
   return parent()->GetGeneratedTNodeTypeName();
 }
 
-std::ostream& operator<<(std::ostream& os, const Signature& sig) {
+void PrintSignature(std::ostream& os, const Signature& sig, bool with_names) {
   os << "(";
   for (size_t i = 0; i < sig.parameter_types.types.size(); ++i) {
     if (i > 0) os << ", ";
-    if (!sig.parameter_names.empty()) os << sig.parameter_names[i] << ": ";
-    os << sig.parameter_types.types[i];
+    if (with_names && !sig.parameter_names.empty()) {
+      os << sig.parameter_names[i] << ": ";
+    }
+    os << *sig.parameter_types.types[i];
   }
   if (sig.parameter_types.var_args) {
     if (sig.parameter_names.size()) os << ", ";
     os << "...";
   }
   os << ")";
-  os << ": " << sig.return_type;
+  os << ": " << *sig.return_type;
 
-  if (sig.labels.empty()) return os;
+  if (sig.labels.empty()) return;
 
   os << " labels ";
   for (size_t i = 0; i < sig.labels.size(); ++i) {
@@ -171,22 +166,20 @@ std::ostream& operator<<(std::ostream& os, const Signature& sig) {
 
     if (sig.labels[i].types.size() > 0) os << "(" << sig.labels[i].types << ")";
   }
+}
+
+std::ostream& operator<<(std::ostream& os, const Signature& sig) {
+  PrintSignature(os, sig, true);
   return os;
 }
 
 std::ostream& operator<<(std::ostream& os, const TypeVector& types) {
-  for (size_t i = 0; i < types.size(); ++i) {
-    if (i > 0) os << ", ";
-    os << types[i];
-  }
+  PrintCommaSeparatedList(os, types);
   return os;
 }
 
 std::ostream& operator<<(std::ostream& os, const ParameterTypes& p) {
-  for (size_t i = 0; i < p.types.size(); ++i) {
-    if (i > 0) os << ", ";
-    os << p.types[i];
-  }
+  PrintCommaSeparatedList(os, p.types);
   if (p.var_args) {
     if (p.types.size() > 0) os << ", ";
     os << "...";

src/torque/types.h

@@ -268,8 +268,8 @@ class UnionType final : public Type {
   std::set<const Type*, TypeLess> types_;
 };
 
-inline std::ostream& operator<<(std::ostream& os, const Type* t) {
-  os << t->ToString();
+inline std::ostream& operator<<(std::ostream& os, const Type& t) {
+  os << t.ToString();
   return os;
 }
 
@@ -344,6 +344,7 @@ struct Arguments {
   std::vector<Label*> labels;
 };
 
+void PrintSignature(std::ostream& os, const Signature& sig, bool with_names);
 std::ostream& operator<<(std::ostream& os, const Signature& sig);
 
 bool IsAssignableFrom(const Type* to, const Type* from);

src/torque/utils.h

@@ -36,6 +36,49 @@ class Deduplicator {
   std::unordered_set<T, base::hash<T>> storage_;
 };
 
+template <class C, class T>
+void PrintCommaSeparatedList(std::ostream& os, const T& list, C transform) {
+  bool first = true;
+  for (auto& e : list) {
+    if (first) {
+      first = false;
+    } else {
+      os << ", ";
+    }
+    os << transform(e);
+  }
+}
+
+template <class T,
+          typename std::enable_if<
+              std::is_pointer<typename T::value_type>::value, int>::type = 0>
+void PrintCommaSeparatedList(std::ostream& os, const T& list) {
+  bool first = true;
+  for (auto& e : list) {
+    if (first) {
+      first = false;
+    } else {
+      os << ", ";
+    }
+    os << *e;
+  }
+}
+
+template <class T,
+          typename std::enable_if<
+              !std::is_pointer<typename T::value_type>::value, int>::type = 0>
+void PrintCommaSeparatedList(std::ostream& os, const T& list) {
+  bool first = true;
+  for (auto& e : list) {
+    if (first) {
+      first = false;
+    } else {
+      os << ", ";
+    }
+    os << e;
+  }
+}
+
 }  // namespace torque
 }  // namespace internal
 }  // namespace v8