dart2js: Allow to encode side-effects in the spec-string of JS and similar built-ins.

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 42 matching lines @@
   /// [DartType]s or [SpecialType]s instantiated by the native element.
   final List typesInstantiated = [];
 
   // If this behavior is for a JS expression, [codeTemplate] contains the
   // parsed tree.
   js.Template codeTemplate;
 
   final SideEffects sideEffects = new SideEffects.empty();
 
   /// Processes the type specification string of a call to JS and stores the
   /// result in the [typesReturned] and [typesInstantiated].

[sra1, 2015/03/03 01:49:49]

mention setSideEffects?

[floitsch, 2015/03/03 19:53:19]

Done.

   ///
   /// Two forms of the string is supported:
   /// 1) A single type string of the form 'void', '', 'var' or 'T1|...|Tn'
   ///    which defines the types returned and for the later form also created by
   ///    the call to JS.
-  /// 2) A sequence of the form '<tag>:<type-string>;' where <tag> is either
-  ///    'returns' or 'creates' and where <type-string> is a type string like in
-  ///    1). The type string marked by 'returns' defines the types returned and
-  ///    'creates' defines the types created by the call to JS. Each tag kind
-  ///    can only occur once in the sequence.
+  /// 2) A sequence of the form

[sra1, 2015/03/03 01:49:50]

It is OK to describe this here, but please make a more beautiful comment in
js_lib/foreign_helper.dart.

Please don't change that comment to ///. It will break my heart.

BTW is there somewhere we can view the formatted versions of the doc comments in
this codebase?

[floitsch, 2015/03/03 19:53:19]

I don't know of any place where the doc can be viewed.
Done.

+  ///    '<type-tag>:<type-string>;<effect-tag>:<effect-string>'
+  ///    where <type-tag> is either 'returns' or 'creates' and where
+  ///    <type-string> is a type string like in 1). The type string marked by
+  ///    'returns' defines the types returned and 'creates' defines the types
+  ///    created by the call to JS.
+  ///    The <effect-tag> is either 'effects' or 'depends' and
+  ///    <effect-string> is either 'all', 'empty' or a comma-separated list of

[sra1, 2015/03/03 01:49:49]

effects:none would read easier than effects:empty

[floitsch, 2015/03/03 19:53:19]

Done.

+  ///    '-index', '-instance', '-static'. The flag 'all' indicates

[sra1, 2015/03/03 01:49:49]

So you might write
effects:-index
Why the '-'?  Is it to reserve identifers for future use?

[floitsch, 2015/03/03 19:53:19]

It's to indicate that this effect is *not* happening.
I prefer to go the negative route.
Changed to "no-X".

+  ///    that the call affects/depends on every side-effect. The flag
+  ///    'empty' indicates that the call does not affect (resp. depends on)
+  ///    anything.
+  ///    '-index' indicates that the call does not do any array index-store (for
+  ///    'effects'), or depends on any value in an array (for 'depends').
+  ///    The flag '-instance' indicates that the call does not modify (resp.
+  ///    depends on) any instance variable. Similarly static variables are

[sra1, 2015/03/03 01:49:49]

The instance/index seems a bit dangerous for the DOM due collections that mirror
other state (e.g. directly available attributes and the attribute map)
We should not try to guess behaviour unless we are extremely conservative.

[floitsch, 2015/03/03 19:53:19]

agreed. But it's still an option here.

+  ///    indicated with '-static'. The flags 'effects' and 'depends' must be
+  ///    used in unison (either both are present or none is).
+  ///    Each tag kind (including the 'type-tag's) can only occur once in the
+  ///    sequence.

[sra1, 2015/03/03 01:49:49]

The formating of this comment is extremely dense. A few blank lines would help.

[floitsch, 2015/03/03 19:53:19]

My fault. I wasn't sure if paragraphs were allowed inside a list. But MD
supports it. -> made it less dense.

   ///
   /// [specString] is the specification string, [resolveType] resolves named
   /// types into type values, [typesReturned] and [typesInstantiated] collects
   /// the types defined by the specification string, and [objectType] and
   /// [nullType] define the types for `Object` and `Null`, respectively. The
   /// latter is used for the type strings of the form '' and 'var'.
   // TODO(johnniwinther): Use ';' as a separator instead of a terminator.
   static void processSpecString(
       DiagnosticListener listener,
       Spannable spannable,
       String specString,
-      {dynamic resolveType(String typeString),
+      {void setSideEffects(SideEffects newEffects),
+       dynamic resolveType(String typeString),
        List typesReturned, List typesInstantiated,
        objectType, nullType}) {
 
     /// 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 Ti.
     void resolveTypesString(String typesString,
                             {onVoid(), onVar(), onType(type)}) {
       // Various things that are not in fact types.
       if (typesString == 'void') {
         if (onVoid != null) {
           onVoid();
         }
         return;
       }
       if (typesString == '' || typesString == 'var') {
         if (onVar != null) {
           onVar();
         }
         return;
       }
       for (final typeString in typesString.split('|')) {
         onType(resolveType(typeString));
       }
     }
 
+
     if (specString.contains(':')) {
-      /// Find and remove a substring of the form 'tag:<type-string>;' from
+      /// Find and remove a substring of the form 'tag:<string>;' from
       /// [specString].
-      String getTypesString(String tag) {
+      String getTagString(String tag) {
         String marker = '$tag:';
         int startPos = specString.indexOf(marker);
         if (startPos == -1) return null;
         int endPos = specString.indexOf(';', startPos);
         if (endPos == -1) return null;
         String typeString =
             specString.substring(startPos + marker.length, endPos);
         specString = '${specString.substring(0, startPos)}'
                      '${specString.substring(endPos + 1)}'.trim();
         return typeString;
       }
 
-      String returns = getTypesString('returns');
+      String returns = getTagString('returns');
       if (returns != null) {
         resolveTypesString(returns, onVar: () {
           typesReturned.add(objectType);
           typesReturned.add(nullType);
         }, onType: (type) {
           typesReturned.add(type);
         });
       }
 
-      String creates = getTypesString('creates');
+      String creates = getTagString('creates');
       if (creates != null) {
         resolveTypesString(creates, onVoid: () {
           listener.internalError(spannable,
               "Invalid type string 'creates:$creates'");
         }, onVar: () {
           listener.internalError(spannable,
               "Invalid type string 'creates:$creates'");
         }, onType: (type) {
           typesInstantiated.add(type);
         });
       }
 
+      String effects = getTagString('effects');
+      String depends = getTagString('depends');
+      if (effects != null && depends == null ||
+          effects == null && depends != null) {
+        listener.internalError(spannable,
+            "Invalid JS spec string. "
+            "'effects' and 'depends' must occur together");
+      }
+
+      if (effects != null) {
+        SideEffects sideEffects = new SideEffects();
+        if (effects == "empty") {
+          sideEffects.clearAllSideEffects();
+        } else if (effects == "all") {
+          // Don't do anything.
+        } else {
+          List<String> splitEffects = effects.split(",");
+          if (splitEffects.isEmpty) {
+            listener.internalError(spannable, "Empty side-effect flag");

[sra1, 2015/03/03 01:49:49]

Empty -> Missing

[floitsch, 2015/03/03 19:53:19]

Done.

+          }
+          for (String effect in splitEffects) {
+            switch (effect) {
+              case "-index":
+                sideEffects.clearChangesIndex();
+                break;
+              case "-instance":
+                sideEffects.clearChangesInstanceProperty();
+                break;
+              case "-static":
+                sideEffects.clearChangesStaticProperty();
+                break;
+              default:
+                listener.internalError(spannable,
+                    "Unrecognized side-effect flag: $effect");
+            }
+          }
+        }
+
+        if (depends == "empty") {
+          sideEffects.clearAllDependencies();
+        } else if (depends == "all") {
+          // Don't do anything.
+        } else {
+          List<String> splitDependencies = depends.split(",");
+          if (splitDependencies.isEmpty) {
+            listener.internalError(spannable, "Empty side-effect flag");

[sra1, 2015/03/03 01:49:50]

"Missing side-effect dependency flag"

[floitsch, 2015/03/03 19:53:19]

Done.

+          }
+          for (String dependency in splitDependencies) {
+            switch (dependency) {
+              case "-index":
+                sideEffects.clearDependsOnIndexStore();
+                break;
+              case "-instance":
+                sideEffects.clearDependsOnInstancePropertyStore();
+                break;
+              case "-static":
+                sideEffects.clearDependsOnStaticPropertyStore();
+                break;
+              default:
+                listener.internalError(spannable,
+                    "Unrecognized side-effect flag: $dependency");
+            }
+          }
+        }
+
+        setSideEffects(sideEffects);
+      }
+
       if (!specString.isEmpty) {
-        listener.internalError(spannable, "Invalid JS type string.");
+        listener.internalError(spannable, "Invalid JS spec string.");
       }
     } else {
       resolveTypesString(specString, onVar: () {
         typesReturned.add(objectType);
         typesReturned.add(nullType);
       }, onType: (type) {
         typesInstantiated.add(type);
         typesReturned.add(type);
       });
     }
@@ skipping 19 matching lines @@
     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.");
     }
 
     NativeBehavior behavior = new NativeBehavior();
     behavior.codeTemplate =
         js.js.parseForeignJS(code.dartString.slowToString());
-    new SideEffectsVisitor(behavior.sideEffects)
-        .visit(behavior.codeTemplate.ast);
 
     String specString = specLiteral.dartString.slowToString();
 
-    resolveType(String typeString) {
+    DartType resolveType(String typeString) {
       return _parseType(
           typeString,
           compiler,
           (name) => resolver.resolveTypeFromString(specLiteral, name),
           jsCall);
     }
 
+    bool sideEffectsAreEncodedInSpecString = false;
+
+    void setSideEffects(SideEffects newEffects) {
+      sideEffectsAreEncodedInSpecString = true;
+      behavior.sideEffects.setTo(newEffects);
+    }
+
     processSpecString(compiler, jsCall,
                       specString,
+                      setSideEffects: setSideEffects,
                       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);
+    }
+
     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 20 matching lines @@
     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 first argument.");
     }
 
     NativeBehavior behavior = new NativeBehavior();
 
     String specString = specLiteral.dartString.slowToString();
 
-    resolveType(String typeString) {
+    void resolveType(String typeString) {
       return _parseType(
           typeString,
           compiler,
           (name) => resolver.resolveTypeFromString(specLiteral, name),
           jsGlobalCall);
     }
 
+    void setSideEffects(SideEffects newEffects) {
+      compiler.internalError(jsGlobalCall,
+          'Embedded global calls may not have any side-effect overwrites: '
+          '$specString');
+    }
+
     processSpecString(compiler, jsGlobalCall,
                       specString,
+                      setSideEffects: setSideEffects,
                       resolveType: resolveType,
                       typesReturned: behavior.typesReturned,
                       typesInstantiated: behavior.typesInstantiated,
                       objectType: compiler.objectClass.computeType(compiler),
                       nullType: compiler.nullClass.computeType(compiler));
 
     return behavior;
   }
 
   static NativeBehavior ofMethod(FunctionElement method, Compiler compiler) {
@@ skipping 123 matching lines @@
       FunctionType functionType = type;
       _capture(functionType.returnType, compiler);
       for (DartType parameter in functionType.parameterTypes) {
         _escape(parameter, compiler);
       }
     } else {
       typesInstantiated.add(type);
     }
   }
 
-  static _parseType(String typeString, Compiler compiler,
+  static DartType _parseType(String typeString, Compiler compiler,
       lookup(name), locationNodeOrElement) {
     if (typeString == '=Object') return SpecialType.JsObject;
     if (typeString == 'dynamic') {
       return const DynamicType();
     }
     DartType type = lookup(typeString);
     if (type != null) return type;
 
     int index = typeString.indexOf('<');
     if (index < 1) {
       compiler.internalError(
           _errorNode(locationNodeOrElement, compiler),
           "Type '$typeString' not found.");
     }
     type = lookup(typeString.substring(0, index));
     if (type != null)  {
       // TODO(sra): Parse type parameters.
       return type;
     }
     compiler.internalError(
         _errorNode(locationNodeOrElement, compiler),
         "Type '$typeString' not found.");
   }
 
   static _errorNode(locationNodeOrElement, compiler) {
     if (locationNodeOrElement is Node) return locationNodeOrElement;
     return locationNodeOrElement.parseNode(compiler);
   }
 }