assume extern functions return tainted values

When we have no type information, it is safest to assume that extern function return values are non-local. Closes #2365 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=177312604
google · Nov 30, 2017 · f88d939 · f88d939
1 parent 8b9c010
commit f88d939
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 1 deletion.
diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java
@@ -923,7 +923,10 @@ private void updateSideEffectsFromExtern(Node externFunction, AbstractCompiler c
       // Handle externs.
       TypeI typei = externFunction.getTypeI();
       FunctionTypeI functionType = typei == null ? null : typei.toMaybeFunctionType();
-      if (functionType != null) {
+      if (functionType == null) {
+        // Assume extern functions return tainted values when we have no type info to say otherwise.
+        setTaintsReturn();
+      } else {
         TypeI retType = functionType.getReturnType();
         if (!PureFunctionIdentifier.isLocalValueType(retType, compiler)) {
           setTaintsReturn();

diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java
@@ -47,6 +47,20 @@ public final class IntegrationTest extends IntegrationTestCase {
   private static final String CLOSURE_COMPILED =
       "var COMPILED = true; var goog$exportSymbol = function() {};";
 
+  public void testIssue2365() {
+    CompilerOptions options = createCompilerOptions();
+    CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
+    options.addWarningsGuard(new DiagnosticGroupWarningsGuard(
+        DiagnosticGroups.CHECK_TYPES, CheckLevel.OFF));
+
+    // With type checking disabled we should assume that extern functions (even ones known not to
+    // have any side effects) return a non-local value, so it isn't safe to remove assignments to
+    // properties on them.
+    // noSideEffects() and the property 'value' are declared in the externs defined in
+    // IntegrationTestCase.
+    testSame(options, "noSideEffects().value = 'something';");
+  }
+
   public void testBug65688660() {
     CompilerOptions options = createCompilerOptions();
     options.setLanguageIn(LanguageMode.ECMASCRIPT_2017);

diff --git a/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java b/test/com/google/javascript/jscomp/PureFunctionIdentifierTest.java
@@ -760,13 +760,23 @@ public void testReturnLocalityMultipleDefinitionsSameName() {
   }
 
   public void testExternCalls() throws Exception {
+    testExternCallsForTypeInferenceMode(TypeInferenceMode.BOTH);
+  }
+
+  public void testExternCallsNoTypeChecking() throws Exception {
+    testExternCallsForTypeInferenceMode(TypeInferenceMode.NEITHER);
+  }
+
+  private void testExternCallsForTypeInferenceMode(TypeInferenceMode typeInferenceMode) {
+    mode = typeInferenceMode;
     String prefix = "function f(){";
     String suffix = "} f()";
 
     assertPureCallsMarked(prefix + "externNsef1()" + suffix,
                      ImmutableList.of("externNsef1", "f"));
     assertPureCallsMarked(prefix + "externObj.nsef1()" + suffix,
                      ImmutableList.of("externObj.nsef1", "f"));
+    checkLocalityOfMarkedCalls("externNsef1(); externObj.nsef1()", ImmutableList.of());
 
     assertNoPureCalls(prefix + "externSef1()" + suffix);
     assertNoPureCalls(prefix + "externObj.sef1()" + suffix);