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

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

Index: pkg/compiler/lib/src/native/behavior.dart
diff --git a/pkg/compiler/lib/src/native/behavior.dart b/pkg/compiler/lib/src/native/behavior.dart
index 5cd5c604a5781d66908c40a3325f1adccba1e586..a82dab764838acce9a304080aefb31339202d9c0 100644
--- a/pkg/compiler/lib/src/native/behavior.dart
+++ b/pkg/compiler/lib/src/native/behavior.dart
@@ -66,11 +66,26 @@ class NativeBehavior {
   /// 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
+  ///    '<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
+  ///    '-index', '-instance', '-static'. The flag 'all' indicates
+  ///    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
+  ///    indicated with '-static'. The flags 'effects' and 'depends' must be
+  ///    used in unison (either both are present or none is).

[herhut, 2015/03/02 21:34:51]

Why are they both required? Especially as it is opt out, why not make all the
default when no annotation is present? For opt in I would understand the
requirement to avoid missing an annotation. However, here it is always safe to
leave them out.

Not a showstopper though.

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

The thinking is: if you provide one, you should think about the other one. You
can always safely write "all".

What is missing (but I don't think we will add it): a way to specify a 
dependency, but let the side-effect visitor figure out the effects.

+  ///    Each tag kind (including the 'type-tag's) can only occur once in the
+  ///    sequence.
   ///
   /// [specString] is the specification string, [resolveType] resolves named
   /// types into type values, [typesReturned] and [typesInstantiated] collects
@@ -82,7 +97,8 @@ class NativeBehavior {
       DiagnosticListener listener,
       Spannable spannable,
       String specString,
-      {dynamic resolveType(String typeString),
+      {void setSideEffects(SideEffects newEffects),
+       dynamic resolveType(String typeString),
        List typesReturned, List typesInstantiated,
        objectType, nullType}) {
 
@@ -110,10 +126,11 @@ class NativeBehavior {
       }
     }
 
+
     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;
@@ -126,7 +143,7 @@ class NativeBehavior {
         return typeString;
       }
 
-      String returns = getTypesString('returns');
+      String returns = getTagString('returns');
       if (returns != null) {
         resolveTypesString(returns, onVar: () {
           typesReturned.add(objectType);
@@ -136,7 +153,7 @@ class NativeBehavior {
         });
       }
 
-      String creates = getTypesString('creates');
+      String creates = getTagString('creates');
       if (creates != null) {
         resolveTypesString(creates, onVoid: () {
           listener.internalError(spannable,
@@ -149,8 +166,76 @@ class NativeBehavior {
         });
       }
 
+      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");

[herhut, 2015/03/02 21:34:51]

Add a . at end of sentence.

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

Done.

+      }
+
+      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");

[herhut, 2015/03/02 21:34:51]

Dito.

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

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");

[herhut, 2015/03/02 21:34:51]

Depends flag and add a . at the end.

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

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: () {
@@ -190,12 +275,10 @@ class NativeBehavior {
     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) {
+    void resolveType(String typeString) {
       return _parseType(
           typeString,
           compiler,
@@ -203,14 +286,27 @@ class NativeBehavior {
           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;
   }
 
@@ -251,7 +347,7 @@ class NativeBehavior {
 
     String specString = specLiteral.dartString.slowToString();
 
-    resolveType(String typeString) {
+    void resolveType(String typeString) {
       return _parseType(
           typeString,
           compiler,
@@ -259,8 +355,15 @@ class NativeBehavior {
           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,