Improve NoSuchMethodError error messages

runtime/vm/parser.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/parser.h"
 
 #include "lib/invocation_mirror.h"
 #include "vm/bigint_operations.h"
 #include "vm/bootstrap.h"
 #include "vm/class_finalizer.h"
@@ skipping 2885 matching lines @@
     // TODO(regis): For an instance function, pass the receiver to
     // NoSuchMethodError.
     current_block_->statements->Add(
         ThrowNoSuchMethodError(TokenPos(),
                                current_class(),
                                function_name,
                                NULL,   // No arguments.
                                func.is_static() ?
                                    InvocationMirror::kStatic :
                                    InvocationMirror::kDynamic,
-                               InvocationMirror::kMethod));
+                               InvocationMirror::kMethod,
+                               NULL));
     end_token_pos = TokenPos();
   } else {
     UnexpectedToken();
   }
 
   ASSERT(func.end_token_pos() == func.token_pos() ||
          func.end_token_pos() == end_token_pos);
   func.set_end_token_pos(end_token_pos);
   SequenceNode* body = CloseBlock();
   current_block_->statements->Add(body);
@@ skipping 4387 matching lines @@
                         PrivateCoreLibName(Symbols::ThrowNew()),
                         arguments);
 }
 
 
 AstNode* Parser::ThrowNoSuchMethodError(intptr_t call_pos,
                                         const Class& cls,
                                         const String& function_name,
                                         ArgumentListNode* function_arguments,
                                         InvocationMirror::Call im_call,
-                                        InvocationMirror::Type im_type) {
+                                        InvocationMirror::Type im_type,
+                                        Function* func) {
   ArgumentListNode* arguments = new ArgumentListNode(call_pos);
   // Object receiver.
   // TODO(regis): For now, we pass a class literal of the unresolved
   // method's owner, but this is not specified and will probably change.
   Type& type = Type::ZoneHandle(
       Type::New(cls, TypeArguments::Handle(), call_pos, Heap::kOld));
   type ^= ClassFinalizer::FinalizeType(
       current_class(), type, ClassFinalizer::kCanonicalize);
   arguments->Add(new LiteralNode(call_pos, type));
   // String memberName.
@@ skipping 15 matching lines @@
                                      Type::ZoneHandle(Type::ArrayType()),
                                      function_arguments->nodes());
     arguments->Add(array);
   }
   // List argumentNames.
   if (function_arguments == NULL) {
     arguments->Add(new LiteralNode(call_pos, Array::ZoneHandle()));
   } else {
     arguments->Add(new LiteralNode(call_pos, function_arguments->names()));
   }
+
   // List existingArgumentNames.
-  // Check if there exists a function with the same name.
-  Function& function =
-     Function::Handle(cls.LookupStaticFunction(function_name));
-  if (function.IsNull()) {
-    arguments->Add(new LiteralNode(call_pos, Array::ZoneHandle()));
+  // Check if there exists a function with the same name unless caller
+  // has done the lookup already. If there is a function with the same
+  // name but incompatible parameters, inform the NoSuchMethodError what the
+  // expected parameters are.
+  Function& function = Function::Handle();
+  if (func != NULL) {
+    function = func->raw();
   } else {
-    const int total_num_parameters = function.NumParameters();
-    Array& array =
-        Array::ZoneHandle(Array::New(total_num_parameters, Heap::kOld));
-    // Skip receiver.
-    for (int i = 0; i < total_num_parameters; i++) {
-      array.SetAt(i, String::Handle(function.ParameterNameAt(i)));
-    }
-    arguments->Add(new LiteralNode(call_pos, array));
+    function = cls.LookupStaticFunction(function_name);
   }
+  Array& array = Array::ZoneHandle();
+  if (!function.IsNull()) {
+    // The constructor for NoSuchMethodError takes a list of existing
+    // parameter names to produce a descriptive error message explaining
+    // the parameter mismatch. The problem is that the array of names
+    // does not describe which parameters are optional positional or
+    // named, which can lead to confusing error messages.
+    // Since the NoSuchMethodError class only uses the list to produce
+    // a string describing the expected parameters, we construct a more
+    // descriptive string here and pass it as the only element of the
+    // "existingArgumentNames" array of the NoSuchMethodError constructor.
+    // TODO(13471): Separate the implementations of NoSuchMethodError
+    // between dart2js and VM. Update the constructor to accept a string
+    // describing the formal parameters of an incompatible call target.
+    array = Array::New(1, Heap::kOld);
+    array.SetAt(0, String::Handle(function.UserVisibleParameterSignature()));
+  }
+  arguments->Add(new LiteralNode(call_pos, array));
+
   return MakeStaticCall(Symbols::NoSuchMethodError(),
                         PrivateCoreLibName(Symbols::ThrowNew()),
                         arguments);
 }
 
 
 AstNode* Parser::ParseBinaryExpr(int min_preced) {
   TRACE_PARSER("ParseBinaryExpr");
   ASSERT(min_preced >= Token::Precedence(Token::kOR));
   AstNode* left_operand = ParseUnaryExpr();
@@ skipping 267 matching lines @@
       name = left_ident->raw();
     }
     if (name.IsNull()) {
       ErrorMsg(left_pos, "expression is not assignable");
     }
     result = ThrowNoSuchMethodError(original->token_pos(),
                                     current_class(),
                                     name,
                                     NULL,  // No arguments.
                                     InvocationMirror::kStatic,
-                                    InvocationMirror::kSetter);
+                                    InvocationMirror::kSetter,
+                                    NULL);

[regis, 2013/09/24 01:10:24]

As we add a comment for the 4th null argument, you could add:
// No existing function.

[hausner, 2013/09/24 16:17:09]

Done.

   } else if (result->IsStoreIndexedNode() ||
              result->IsInstanceSetterNode() ||
              result->IsStaticSetterNode() ||
              result->IsStoreStaticFieldNode() ||
              result->IsStoreLocalNode()) {
     // Ensure that the expression temp is allocated for nodes that may need it.
     EnsureExpressionTemp();
   }
   return result;
 }
@@ skipping 273 matching lines @@
       EnsureSavedCurrentContext();
       closure = GenerateStaticFieldLookup(field, call_pos);
       return new ClosureCallNode(call_pos, closure, arguments);
     }
     // Could not resolve static method: throw a NoSuchMethodError.
     return ThrowNoSuchMethodError(ident_pos,
                                   cls,
                                   func_name,
                                   arguments,
                                   InvocationMirror::kStatic,
-                                  InvocationMirror::kMethod);
+                                  InvocationMirror::kMethod,
+                                  NULL);
   } else if (cls.IsTopLevel() &&
       (cls.library() == Library::CoreLibrary()) &&
       (func.name() == Symbols::Identical().raw())) {
     // This is the predefined toplevel function identical(a,b). Create
     // a comparison node instead.
     ASSERT(num_arguments == 2);
     return new ComparisonNode(ident_pos,
                               Token::kEQ_STRICT,
                               arguments->NodeAt(0),
                               arguments->NodeAt(1));
@@ skipping 80 matching lines @@
                                      kNumArguments,
                                      Object::empty_array(),
                                      Resolver::kIsQualified);
       if (func.IsNull()) {
         // No field or explicit setter function, throw a NoSuchMethodError.
         return ThrowNoSuchMethodError(ident_pos,
                                       cls,
                                       field_name,
                                       NULL,  // No arguments.
                                       InvocationMirror::kStatic,
-                                      InvocationMirror::kField);
+                                      InvocationMirror::kField,
+                                      NULL);
       }
 
       // Explicit setter function for the field found, field does not exist.
       // Create a getter node first in case it is needed. If getter node
       // is used as part of, e.g., "+=", and the explicit getter does not
       // exist, and error will be reported by the code generator.
       access = new StaticGetterNode(call_pos,
                                     NULL,
                                     false,
                                     Class::ZoneHandle(cls.raw()),
@@ skipping 24 matching lines @@
         // We might be referring to an implicit closure, check to see if
         // there is a function of the same name.
         func = cls.LookupStaticFunction(field_name);
         if (func.IsNull()) {
           // No field or explicit getter function, throw a NoSuchMethodError.
           return ThrowNoSuchMethodError(ident_pos,
                                         cls,
                                         field_name,
                                         NULL,  // No arguments.
                                         InvocationMirror::kStatic,
-                                        InvocationMirror::kGetter);
+                                        InvocationMirror::kGetter,
+                                        NULL);
         }
         access = CreateImplicitClosureNode(func, call_pos, NULL);
       } else {
         ASSERT(func.kind() != RawFunction::kImplicitStaticFinalGetter);
         access = new StaticGetterNode(call_pos,
                                       NULL,
                                       false,
                                       Class::ZoneHandle(cls.raw()),
                                       field_name);
       }
@@ skipping 19 matching lines @@
     // In an instance method, we convert this into a getter call
     // for a field (which may be defined in a subclass.)
     String& name = String::CheckedZoneHandle(primary->primary().raw());
     if (current_function().is_static() ||
         current_function().IsInFactoryScope()) {
       return ThrowNoSuchMethodError(primary->token_pos(),
                                     current_class(),
                                     name,
                                     NULL,  // No arguments.
                                     InvocationMirror::kStatic,
-                                    InvocationMirror::kField);
+                                    InvocationMirror::kField,
+                                    NULL);
     } else {
       AstNode* receiver = LoadReceiver(primary->token_pos());
       return CallGetter(node->token_pos(), receiver, name);
     }
   }
   return primary;
 }
 
 
 AstNode* Parser::LoadClosure(PrimaryNode* primary) {
@@ skipping 151 matching lines @@
           if (primary->IsSuper()) {
             ErrorMsg(primary->token_pos(), "illegal use of super");
           }
           String& name = String::CheckedZoneHandle(primary->primary().raw());
           if (current_function().is_static()) {
             selector = ThrowNoSuchMethodError(primary->token_pos(),
                                               current_class(),
                                               name,
                                               NULL,  // No arguments.
                                               InvocationMirror::kStatic,
-                                              InvocationMirror::kMethod);
+                                              InvocationMirror::kMethod,
+                                              NULL);
           } else {
             // Treat as call to unresolved (instance) method.
             AstNode* receiver = LoadReceiver(primary->token_pos());
             selector = ParseInstanceCall(receiver, name);
           }
         } else if (primary->primary().IsTypeParameter()) {
           // TODO(regis): Issue 13134.  Make sure the error message is the
           // one we want here and add a test covering this code.
           const String& name = String::ZoneHandle(
               TypeParameter::Cast(primary->primary()).name());
           selector = ThrowNoSuchMethodError(primary->token_pos(),
                                             current_class(),
                                             name,
                                             NULL,  // No arguments.
                                             InvocationMirror::kStatic,
-                                            InvocationMirror::kMethod);
+                                            InvocationMirror::kMethod,
+                                            NULL);
         } else if (primary->primary().IsClass()) {
           const Class& type_class = Class::Cast(primary->primary());
           Type& type = Type::ZoneHandle(
               Type::New(type_class, TypeArguments::Handle(),
                         primary->token_pos(), Heap::kOld));
           type ^= ClassFinalizer::FinalizeType(
               current_class(), type, ClassFinalizer::kCanonicalize);
           ASSERT(!type.IsMalformed());
           selector = new TypeNode(primary->token_pos(), type);
         } else {
@@ skipping 713 matching lines @@
       // method, evaluation of an unresolved identifier causes a
       // NoSuchMethodError to be thrown. In an instance method, we convert
       // the unresolved name to an instance field access, since a
       // subclass might define a field with this name.
       if (current_function().is_static()) {
         resolved = ThrowNoSuchMethodError(ident_pos,
                                           current_class(),
                                           ident,
                                           NULL,  // No arguments.
                                           InvocationMirror::kStatic,
-                                          InvocationMirror::kField);
+                                          InvocationMirror::kField,
+                                          NULL);
       } else {
         // Treat as call to unresolved instance field.
         resolved = CallGetter(ident_pos, LoadReceiver(ident_pos), ident);
       }
     } else if (primary->primary().IsFunction()) {
       if (allow_closure_names) {
         resolved = LoadClosure(primary);
       } else {
         ErrorMsg(ident_pos, "illegal reference to method '%s'",
                  ident.ToCString());
@@ skipping 681 matching lines @@
             String::Handle(type_class.Name()).ToCString(),
             external_constructor_name.ToCString());
         const Error& error = Error::Handle(type.malformed_error());
         ErrorMsg(error);
       }
       return ThrowNoSuchMethodError(call_pos,
                                     type_class,
                                     external_constructor_name,
                                     arguments,
                                     InvocationMirror::kConstructor,
-                                    InvocationMirror::kMethod);
+                                    InvocationMirror::kMethod,
+                                    &constructor);
     } else if (constructor.IsRedirectingFactory()) {
       ClassFinalizer::ResolveRedirectingFactory(type_class, constructor);
       Type& redirect_type = Type::Handle(constructor.RedirectionType());
       if (!redirect_type.IsMalformed() && !redirect_type.IsInstantiated()) {
         // The type arguments of the redirection type are instantiated from the
         // type arguments of the parsed type of the 'new' or 'const' expression.
         Error& malformed_error = Error::Handle();
         redirect_type ^= redirect_type.InstantiateFrom(type_arguments,
                                                        &malformed_error);
         if (!malformed_error.IsNull()) {
@@ skipping 47 matching lines @@
                "for class '%s': %s",
                external_constructor_name.ToCString(),
                String::Handle(type_class.Name()).ToCString(),
                error_message.ToCString());
     }
     return ThrowNoSuchMethodError(call_pos,
                                   type_class,
                                   external_constructor_name,
                                   arguments,
                                   InvocationMirror::kConstructor,
-                                  InvocationMirror::kMethod);
+                                  InvocationMirror::kMethod,
+                                  &constructor);
   }
 
   // Return a throw in case of a malformed type or report a compile-time error
   // if the constructor is const.
   if (type.IsMalformed()) {
     if (is_const) {
       const Error& error = Error::Handle(type.malformed_error());
       ErrorMsg(error);
     }
     return ThrowTypeError(type_pos, type);
   }
   type_arguments ^= type_arguments.Canonicalize();
   // Make the constructor call.
   AstNode* new_object = NULL;
   if (is_const) {
     if (!constructor.is_const()) {
-      ErrorMsg("'const' requires const constructor: '%s'",
-          String::Handle(constructor.name()).ToCString());
+      const String& external_constructor_name =
+          (named_constructor ? constructor_name : type_class_name);
+      ErrorMsg("non-const constructor '%s' cannot be used in "
+               "const object creation",
+               external_constructor_name.ToCString());
     }
     const Object& constructor_result = Object::Handle(
         EvaluateConstConstructorCall(type_class,
                                      type_arguments,
                                      constructor,
                                      arguments));
     if (constructor_result.IsUnhandledException()) {
       new_object = GenerateRethrow(new_pos, constructor_result);
     } else {
       const Instance& const_instance = Instance::Cast(constructor_result);
@@ skipping 203 matching lines @@
         InvocationMirror::Type call_type =
             CurrentToken() == Token::kLPAREN ?
                InvocationMirror::kMethod : InvocationMirror::kGetter;
         const String& unresolved_name =
             String::Cast(primary->AsPrimaryNode()->primary());
         primary = ThrowNoSuchMethodError(primary->token_pos(),
                                         current_class(),
                                         unresolved_name,
                                         NULL,  // No arguments.
                                         InvocationMirror::kTopLevel,
-                                        call_type);
+                                        call_type,
+                                        NULL);
       }
     }
     ASSERT(primary != NULL);
   } else if (CurrentToken() == Token::kTHIS) {
     LocalVariable* local = LookupLocalScope(Symbols::This());
     if (local == NULL) {
       ErrorMsg("receiver 'this' is not in scope");
     }
     primary = new LoadLocalNode(TokenPos(), local);
     ConsumeToken();
@@ skipping 439 matching lines @@
 void Parser::SkipQualIdent() {
   ASSERT(IsIdentifier());
   ConsumeToken();
   if (CurrentToken() == Token::kPERIOD) {
     ConsumeToken();  // Consume the kPERIOD token.
     ExpectIdentifier("identifier expected after '.'");
   }
 }
 
 }  // namespace dart