Improve error handling of JS-calls

pkg/compiler/lib/src/native/behavior.dart

 // Copyright (c) 2014, 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.
 
 part of native;
 
 /// This class is a temporary work-around until we get a more powerful DartType.
 class SpecialType {
   final String name;
   const SpecialType._(this.name);
@@ skipping 158 matching lines @@
       {Iterable<String> validTags,
        void setSideEffects(SideEffects newEffects),
        void setThrows(NativeThrowBehavior throwKind),
        void setIsAllocation(bool isAllocation),
        void setUseGvn(bool useGvn),
        dynamic resolveType(String typeString),
        List typesReturned,
        List typesInstantiated,
        objectType, nullType}) {
 
+
+    bool seenError = false;
+
+    void reportError(String message) {
+      seenError = true;
+      listener.reportError(spannable, MessageKind.GENERIC, {'text': message});
+    }
+
     const List<String> knownTags = const [
           'creates', 'returns', 'depends', 'effects',
           'throws', 'gvn', 'new'];
 
     /// Resolve a type string of one of the three forms:
     /// *  'void' - in which case [onVoid] is called,
     /// *  '' or 'var' - in which case [onVar] is called,
     /// *  'T1|...|Tn' - in which case [onType] is called for each resolved Ti.
     void resolveTypesString(String typesString,
                             {onVoid(), onVar(), onType(type)}) {
@@ skipping 34 matching lines @@
 
     assert(validTags == null ||
            (validTags.toSet()..removeAll(validTags)).isEmpty);
     if (validTags == null) validTags = knownTags;
 
     Map<String, String> values = <String, String>{};
 
     for (String spec in specs) {
       List<String> tagAndValue = spec.split(':');
       if (tagAndValue.length != 2) {
-        listener.internalError(spannable,
-            "Invalid <tag>:<value> pair '$spec'.");
+        reportError("Invalid <tag>:<value> pair '$spec'.");
+        continue;
       }
       String tag = tagAndValue[0].trim();
       String value = tagAndValue[1].trim();
 
       if (validTags.contains(tag)) {
         if (values[tag] == null) {
           values[tag] = value;
         } else {
-          listener.internalError(spannable, "Duplicate tag '$tag'.");
+          reportError("Duplicate tag '$tag'.");
         }
       } else {
         if (knownTags.contains(tag)) {
-          listener.internalError(spannable, "Tag '$tag' is not valid here.");
+          reportError("Tag '$tag' is not valid here.");
         } else {
-          listener.internalError(spannable, "Unknown tag '$tag'.");
+          reportError("Unknown tag '$tag'.");
         }
       }
     }
 
     // Enum-like tags are looked up in a map. True signature is:
     //
     //  T tagValueLookup<T>(String tag, Map<String, T> map);
     //
     dynamic tagValueLookup(String tag, Map<String, dynamic> map) {
       String tagString = values[tag];
       if (tagString == null) return null;
       var value = map[tagString];
       if (value == null) {
-        listener.internalError(spannable,
-            "Unknown '$tag' specification: '$tagString'");
+        reportError("Unknown '$tag' specification: '$tagString'.");
       }
       return value;
     }
 
     String returns = values['returns'];
     if (returns != null) {
       resolveTypesString(returns, onVar: () {
         typesReturned.add(objectType);
         typesReturned.add(nullType);
       }, onType: (type) {
         typesReturned.add(type);
       });
     }
 
     String creates = values['creates'];
     if (creates != null) {
       resolveTypesString(creates, onVoid: () {
-        listener.internalError(spannable,
-            "Invalid type string 'creates:$creates'");
+        reportError("Invalid type string 'creates:$creates'");
       }, onVar: () {
-        listener.internalError(spannable,
-            "Invalid type string 'creates:$creates'");
+        reportError("Invalid type string 'creates:$creates'");
       }, onType: (type) {
         typesInstantiated.add(type);
       });
     }
 
     const throwsOption = const <String, NativeThrowBehavior>{
       'never': NativeThrowBehavior.NEVER,
       'null(1)': NativeThrowBehavior.MAY_THROW_ONLY_ON_FIRST_ARGUMENT_ACCESS,
       'may': NativeThrowBehavior.MAY,
       'must': NativeThrowBehavior.MUST };
 
     const boolOptions = const<String, bool>{'true': true, 'false': false};
 
-    SideEffects sideEffects = processEffects(listener, spannable,
+    SideEffects sideEffects = processEffects(reportError,
         values['effects'], values['depends']);
     NativeThrowBehavior throwsKind = tagValueLookup('throws', throwsOption);
     bool isAllocation = tagValueLookup('new', boolOptions);
     bool useGvn = tagValueLookup('gvn', boolOptions);
 
     if (isAllocation == true && useGvn == true) {
-      listener.internalError(spannable, "'new' and 'gvn' are incompatible");
+      reportError("'new' and 'gvn' are incompatible");
     }
 
+    if (seenError) return;  // Avoid callbacks.
+
+    // TODO(sra): Simplify [throwBehavior] using [sideEffects].
+
     if (sideEffects != null) setSideEffects(sideEffects);
     if (throwsKind != null) setThrows(throwsKind);
     if (isAllocation != null) setIsAllocation(isAllocation);
     if (useGvn != null) setUseGvn(useGvn);
   }
 
   static SideEffects processEffects(
-      DiagnosticListener listener,
-      Spannable spannable,
+      void reportError(String message),
       String effects,
       String depends) {
 
     if (effects == null && depends == null) return null;
 
     if (effects == null || depends == null) {
-      listener.internalError(spannable,
-          "Invalid JS spec string. "
-          "'effects' and 'depends' must occur together.");
+      reportError("'effects' and 'depends' must occur together.");
       return null;
     }
 
     SideEffects sideEffects = new SideEffects();
     if (effects == "none") {
       sideEffects.clearAllSideEffects();
     } else if (effects == "all") {
       // Don't do anything.
     } else {
       List<String> splitEffects = effects.split(",");
       if (splitEffects.isEmpty) {
-        listener.internalError(spannable, "Missing side-effect flag.");
+        reportError("Missing side-effect flag.");
       }
       for (String effect in splitEffects) {
         switch (effect) {
           case "no-index":
             sideEffects.clearChangesIndex();
             break;
           case "no-instance":
             sideEffects.clearChangesInstanceProperty();
             break;
           case "no-static":
             sideEffects.clearChangesStaticProperty();
             break;
           default:
-            listener.internalError(spannable,
-                "Unrecognized side-effect flag: '$effect'.");
+            reportError("Unrecognized side-effect flag: '$effect'.");
         }
       }
     }
 
     if (depends == "none") {
       sideEffects.clearAllDependencies();
     } else if (depends == "all") {
       // Don't do anything.
     } else {
       List<String> splitDependencies = depends.split(",");
       if (splitDependencies.isEmpty) {
-        listener.internalError(spannable,
-            "Missing side-effect dependency flag.");
+        reportError("Missing side-effect dependency flag.");
       }
       for (String dependency in splitDependencies) {
         switch (dependency) {
           case "no-index":
             sideEffects.clearDependsOnIndexStore();
             break;
           case "no-instance":
             sideEffects.clearDependsOnInstancePropertyStore();
             break;
           case "no-static":
             sideEffects.clearDependsOnStaticPropertyStore();
             break;
           default:
-            listener.internalError(spannable,
-                "Unrecognized side-effect flag: '$dependency'.");
+            reportError("Unrecognized side-effect flag: '$dependency'.");
         }
       }
     }
 
     return sideEffects;
   }
 
   static NativeBehavior ofJsCall(Send jsCall, Compiler compiler, resolver) {
     // The first argument of a JS-call is a string encoding various attributes
     // of the code.
     //
     //  'Type1|Type2'.  A union type.
     //  '=Object'.      A JavaScript Object, no subtype.
 
+    NativeBehavior behavior = new NativeBehavior();
+
     var argNodes = jsCall.arguments;
-    if (argNodes.isEmpty) {
-      compiler.internalError(jsCall, "JS expression has no type.");
+    if (argNodes.isEmpty || argNodes.tail.isEmpty) {
+      compiler.reportError(jsCall, MessageKind.GENERIC,
+          {'text': "JS expression takes two or more arguments."});
+      return behavior;
     }
 
-    var code = argNodes.tail.head;
-    if (code is !StringNode || code.isInterpolation) {
-      compiler.internalError(code, 'JS code must be a string literal.');
+    var specArgument = argNodes.head;
+    if (specArgument is !StringNode || specArgument.isInterpolation) {
+      compiler.reportError(specArgument, MessageKind.GENERIC,
+          {'text': "JS first argument must be a string literal."});
+      return behavior;
     }
 
-    LiteralString specLiteral = argNodes.head.asLiteralString();
-    if (specLiteral == null) {
-      // TODO(sra): We could accept a type identifier? e.g. JS(bool, '1<2').  It
-      // is not very satisfactory because it does not work for void, dynamic.
-      compiler.internalError(argNodes.head, "Unexpected JS first argument.");
+    var codeArgument = argNodes.tail.head;
+    if (codeArgument is !StringNode || codeArgument.isInterpolation) {
+      compiler.reportError(codeArgument, MessageKind.GENERIC,
+          {'text': "JS second argument must be a string literal."});
+      return behavior;
     }
 
-    NativeBehavior behavior = new NativeBehavior();
     behavior.codeTemplate =
-        js.js.parseForeignJS(code.dartString.slowToString());
+        js.js.parseForeignJS(codeArgument.dartString.slowToString());
 
-    String specString = specLiteral.dartString.slowToString();
+    String specString = specArgument.dartString.slowToString();
 
     dynamic resolveType(String typeString) {
       return _parseType(
           typeString,
           compiler,
-          (name) => resolver.resolveTypeFromString(specLiteral, name),
-          jsCall);
+          (name) => resolver.resolveTypeFromString(specArgument, name),
+          specArgument);
     }
 
     bool sideEffectsAreEncodedInSpecString = false;
 
     void setSideEffects(SideEffects newEffects) {
       sideEffectsAreEncodedInSpecString = true;
       behavior.sideEffects.setTo(newEffects);
     }
 
     bool throwBehaviorFromSpecString = false;
     void setThrows(NativeThrowBehavior throwBehavior) {
       throwBehaviorFromSpecString = true;
       behavior.throwBehavior = throwBehavior;
     }
 
     void setIsAllocation(bool isAllocation) {
       behavior.isAllocation = isAllocation;
     }
 
     void setUseGvn(bool useGvn) {
       behavior.useGvn = useGvn;
     }
 
-    processSpecString(compiler, jsCall,
+    processSpecString(compiler, specArgument,
         specString,
         setSideEffects: setSideEffects,
         setThrows: setThrows,
         setIsAllocation: setIsAllocation,
         setUseGvn: setUseGvn,
         resolveType: resolveType,
         typesReturned: behavior.typesReturned,
         typesInstantiated: behavior.typesInstantiated,
         objectType: compiler.objectClass.computeType(compiler),
         nullType: compiler.nullClass.computeType(compiler));
 
     if (!sideEffectsAreEncodedInSpecString) {
       new SideEffectsVisitor(behavior.sideEffects)
           .visit(behavior.codeTemplate.ast);
     }
 
-    // TODO(sra): Simplify [throwBehavior] using [sideEffects].
-
     return behavior;
   }
 
   static NativeBehavior ofJsEmbeddedGlobalCall(Send jsGlobalCall,
                                                Compiler compiler,
                                                resolver) {
     // The first argument of a JS-embedded global call is a string encoding
     // the type of the code.
     //
     //  'Type1|Type2'.  A union type.
@@ skipping 185 matching lines @@
       lookup(name), locationNodeOrElement) {
     if (typeString == '=Object') return SpecialType.JsObject;
     if (typeString == 'dynamic') {
       return const DynamicType();
     }
     var type = lookup(typeString);
     if (type != null) return type;
 
     int index = typeString.indexOf('<');
     if (index < 1) {
-      compiler.internalError(
+      compiler.reportError(
           _errorNode(locationNodeOrElement, compiler),
-          "Type '$typeString' not found.");
+          MessageKind.GENERIC,
+          {'text': "Type '$typeString' not found."});
+      return const DynamicType();
     }
     type = lookup(typeString.substring(0, index));
     if (type != null)  {
       // TODO(sra): Parse type parameters.
       return type;
     }
-    compiler.internalError(
+    compiler.reportError(
         _errorNode(locationNodeOrElement, compiler),
-        "Type '$typeString' not found.");
-    return null;
+        MessageKind.GENERIC,
+        {'text': "Type '$typeString' not found."});
+    return const DynamicType();
   }
 
   static _errorNode(locationNodeOrElement, compiler) {
     if (locationNodeOrElement is Node) return locationNodeOrElement;
     return locationNodeOrElement.parseNode(compiler);
   }
 }