Skip to content

Commit

Permalink
Introduce an options for `--assume_getters_and_setters_are_side_effec…
Browse files Browse the repository at this point in the history 
…t_free`.

The assumption is implemented by suppressing getter/setter property collection

The default value is initially `true` as that's the current assumption of the compiler, and we don't have code to operate any differently. However, once such code is added and vetted the default will be changed to `false`.

This option is not intended to be set by most users.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=255697780
  • Loading branch information
@nreid260 @blickly
nreid260 authored and blickly committed Jul 1, 2019
1 parent 1628dfb commit 9b397f1
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 45 deletions.
24 changes: 24 additions & 0 deletions src/com/google/javascript/jscomp/CompilerOptions.java
View file
Expand Up @@ -1128,6 +1128,27 @@ public boolean shouldProtectHiddenSideEffects() {
return protectHiddenSideEffects && !checksOnly && !allowHotswapReplaceScript;
}

/**
* Ignore the possibility of side-effects from getter and setter invocations.
*
* <p>When {@code true}, it doesn't necessarily mean that all gets/sets are considered
* side-effectful. Gets/sets that can be proven to be side-effect free may still be considered as
* such.
*
* <p>Recall that object-spread is capable of triggering getters. Since the syntax doesn't
* explicitly specifiy a property, it is essentailly impossible to prove it has no side-effects
* without this assumption.
*/
private boolean assumeGettersAndSettersAreSideEffectFree = true;

public void setAssumeGettersAndSettersAreSideEffectFree(boolean x) {
this.assumeGettersAndSettersAreSideEffectFree = x;
}

public boolean getAssumeGettersAndSettersAreSideEffectFree() {
return assumeGettersAndSettersAreSideEffectFree;
}

/**
* Data holder Alias Transformation information accumulated during a compile.
*/
Expand Down Expand Up @@ -2871,6 +2892,9 @@ public String toString() {
.add("angularPass", angularPass)
.add("anonymousFunctionNaming", anonymousFunctionNaming)
.add("assumeClosuresOnlyCaptureReferences", assumeClosuresOnlyCaptureReferences)
.add(
"assumeGettersAndSettersAreSideEffectFree",
assumeGettersAndSettersAreSideEffectFree)
.add("assumeStrictThis", assumeStrictThis())
.add("browserResolverPrefixReplacements", browserResolverPrefixReplacements)
.add("brokenClosureRequiresLevel", brokenClosureRequiresLevel)
Expand Down
7 changes: 7 additions & 0 deletions src/com/google/javascript/jscomp/GatherGetterAndSetterProperties.java
View file
Expand Up @@ -43,11 +43,18 @@ public void process(Node externs, Node root) {

/** Gathers all getters and setters in the AST. */
static void update(AbstractCompiler compiler, Node externs, Node root) {
if (compiler.getOptions().getAssumeGettersAndSettersAreSideEffectFree()) {
return;
}

// TODO(nickreid): We probably don't need to re-gather from the externs. They don't change so
// the first collection should be good forever.
compiler.setExternGetterAndSetterProperties(gather(compiler, externs));
compiler.setSourceGetterAndSetterProperties(gather(compiler, root));
}

static ImmutableMap<String, PropertyAccessKind> gather(AbstractCompiler compiler, Node root) {

GatherCallback gatherCallback = new GatherCallback();
NodeTraversal.traverse(compiler, root, gatherCallback);
return ImmutableMap.copyOf(gatherCallback.properties);
Expand Down
66 changes: 31 additions & 35 deletions test/com/google/javascript/jscomp/CompilerTestCase.java
View file
Expand Up @@ -30,12 +30,9 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.MapDifference;
import com.google.common.collect.Maps;
import com.google.common.truth.Correspondence;
import com.google.errorprone.annotations.ForOverride;
import com.google.errorprone.annotations.OverridingMethodsMustInvokeSuper;
import com.google.javascript.jscomp.AbstractCompiler.PropertyAccessKind;
import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
import com.google.javascript.jscomp.deps.ModuleLoader;
import com.google.javascript.jscomp.deps.ModuleLoader.ResolutionMode;
Expand Down Expand Up @@ -670,6 +667,7 @@ protected CompilerOptions getOptions() {
options.setLanguageOut(languageOut);
options.setModuleResolutionMode(moduleResolutionMode);
options.setPreserveTypeAnnotations(true);
options.setAssumeGettersAndSettersAreSideEffectFree(false); // Default to the complex case.

// This doesn't affect whether checkSymbols is run--it just affects
// whether variable warnings are filtered.
Expand Down Expand Up @@ -1627,38 +1625,7 @@ private void testInternal(
changeVerifier.checkRecordedChanges(mainRoot);
}

if (verifyGetterAndSetterUpdates) {
assertWithMessage("Pass did not update extern getters / setters")
.that(compiler.getExternGetterAndSetterProperties())
.isEqualTo(GatherGetterAndSetterProperties.gather(compiler, externsRoot));
assertWithMessage("Pass did not update source getters / setters")
.that(compiler.getSourceGetterAndSetterProperties())
.isEqualTo(GatherGetterAndSetterProperties.gather(compiler, mainRoot));
}

if (verifyNoNewGettersOrSetters) {
MapDifference<String, PropertyAccessKind> externsDifference =
Maps.difference(
compiler.getExternGetterAndSetterProperties(),
GatherGetterAndSetterProperties.gather(compiler, externsRoot));
assertWithMessage("Pass did not update new extern getters / setters")
.that(externsDifference.entriesOnlyOnRight())
.isEmpty();
assertWithMessage("Pass did not update new extern getters / setters")
.that(externsDifference.entriesDiffering())
.isEmpty();

MapDifference<String, PropertyAccessKind> sourceDifference =
Maps.difference(
compiler.getSourceGetterAndSetterProperties(),
GatherGetterAndSetterProperties.gather(compiler, mainRoot));
assertWithMessage("Pass did not update new source getters / setters")
.that(sourceDifference.entriesOnlyOnRight())
.isEmpty();
assertWithMessage("Pass did not update new source getters / setters")
.that(sourceDifference.entriesDiffering())
.isEmpty();
}
verifyGetterAndSetterCollection(compiler, externsRoot, mainRoot);

if (astValidationEnabled) {
new AstValidator(compiler, scriptFeatureValidationEnabled)
Expand Down Expand Up @@ -1897,6 +1864,35 @@ private static void normalizeActualCode(Compiler compiler, Node externsRoot, Nod
normalize.process(externsRoot, mainRoot);
}

private void verifyGetterAndSetterCollection(Compiler compiler, Node externsRoot, Node mainRoot) {
if (compiler.getOptions().getAssumeGettersAndSettersAreSideEffectFree()) {
assertWithMessage("Should not be validated when getter / setters are side-effect free")
.that(verifyGetterAndSetterUpdates)
.isFalse();
assertWithMessage("Should not be validated when getter / setters are side-effect free")
.that(verifyNoNewGettersOrSetters)
.isFalse();

assertThat(compiler.getExternGetterAndSetterProperties()).isEmpty();
assertThat(compiler.getSourceGetterAndSetterProperties()).isEmpty();
} else if (verifyGetterAndSetterUpdates) {
assertWithMessage("Pass did not update extern getters / setters")
.that(compiler.getExternGetterAndSetterProperties())
.containsExactlyEntriesIn(GatherGetterAndSetterProperties.gather(compiler, externsRoot));
assertWithMessage("Pass did not update source getters / setters")
.that(compiler.getSourceGetterAndSetterProperties())
.containsExactlyEntriesIn(GatherGetterAndSetterProperties.gather(compiler, mainRoot));
} else if (verifyNoNewGettersOrSetters) {
// If the above assertions hold then these two must also hold.
assertWithMessage("Pass did not update new extern getters / setters")
.that(compiler.getExternGetterAndSetterProperties())
.containsAtLeastEntriesIn(GatherGetterAndSetterProperties.gather(compiler, externsRoot));
assertWithMessage("Pass did not update new source getters / setters")
.that(compiler.getSourceGetterAndSetterProperties())
.containsAtLeastEntriesIn(GatherGetterAndSetterProperties.gather(compiler, mainRoot));
}
}

protected Node parseExpectedJs(String expected) {
return parseExpectedJs(
ImmutableList.of(
Expand Down
47 changes: 37 additions & 10 deletions test/com/google/javascript/jscomp/GatherGetterAndSetterPropertiesTest.java
View file
Expand Up @@ -17,6 +17,7 @@

import static com.google.common.truth.Truth.assertThat;

import com.google.common.collect.ImmutableMap;
import com.google.javascript.jscomp.AbstractCompiler.PropertyAccessKind;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -26,11 +27,20 @@
@RunWith(JUnit4.class)
public class GatherGetterAndSetterPropertiesTest extends CompilerTestCase {

private boolean assumeGettersAndSettersAreSideEffectFree = false;

@Override
protected CompilerPass getProcessor(Compiler compiler) {
return new GatherGetterAndSetterProperties(compiler);
}

@Override
protected CompilerOptions getOptions() {
CompilerOptions options = super.getOptions();
options.setAssumeGettersAndSettersAreSideEffectFree(assumeGettersAndSettersAreSideEffectFree);
return options;
}

@Test
public void defaultToNormal() {
testSame("");
Expand All @@ -40,12 +50,15 @@ public void defaultToNormal() {
@Test
public void findGettersAndSettersInExterns() {
testSame(externs("({get x() {}, set y(v) {}})"), srcs(""));
assertThat(getLastCompiler().getExternGetterAndSetterProperties().get("x"))
.isEqualTo(PropertyAccessKind.GETTER_ONLY);
assertThat(getLastCompiler().getExternGetterAndSetterProperties().get("y"))
.isEqualTo(PropertyAccessKind.SETTER_ONLY);
assertThat(getLastCompiler().getExternGetterAndSetterProperties()).hasSize(2);

assertThat(getLastCompiler().getExternGetterAndSetterProperties())
.containsExactlyEntriesIn(
ImmutableMap.of(
"x", PropertyAccessKind.GETTER_ONLY, //
"y", PropertyAccessKind.SETTER_ONLY));

assertThat(getLastCompiler().getSourceGetterAndSetterProperties()).isEmpty();

assertThat(getLastCompiler().getPropertyAccessKind("x"))
.isEqualTo(PropertyAccessKind.GETTER_ONLY);
assertThat(getLastCompiler().getPropertyAccessKind("y"))
Expand All @@ -55,12 +68,15 @@ public void findGettersAndSettersInExterns() {
@Test
public void findGettersAndSettersInSources() {
testSame(srcs("({get x() {}, set y(v) {}})"));
assertThat(getLastCompiler().getSourceGetterAndSetterProperties().get("x"))
.isEqualTo(PropertyAccessKind.GETTER_ONLY);
assertThat(getLastCompiler().getSourceGetterAndSetterProperties().get("y"))
.isEqualTo(PropertyAccessKind.SETTER_ONLY);
assertThat(getLastCompiler().getSourceGetterAndSetterProperties()).hasSize(2);

assertThat(getLastCompiler().getSourceGetterAndSetterProperties())
.containsExactlyEntriesIn(
ImmutableMap.of(
"x", PropertyAccessKind.GETTER_ONLY, //
"y", PropertyAccessKind.SETTER_ONLY));

assertThat(getLastCompiler().getExternGetterAndSetterProperties()).isEmpty();

assertThat(getLastCompiler().getPropertyAccessKind("x"))
.isEqualTo(PropertyAccessKind.GETTER_ONLY);
assertThat(getLastCompiler().getPropertyAccessKind("y"))
Expand Down Expand Up @@ -354,4 +370,15 @@ public void detectJscompGlobalObject() {
assertThat(getLastCompiler().getPropertyAccessKind("prop"))
.isEqualTo(PropertyAccessKind.NORMAL);
}

@Test
public void gatheringIsSkipped_ifGetterAndSettersAreAssumedSideEffectFree() {
assumeGettersAndSettersAreSideEffectFree = true;
disableGetterAndSetterUpdateValidation();

testSame(externs("({get x() {}, set y(v) {}})"), srcs("({get x() {}, set y(v) {}})"));

assertThat(getLastCompiler().getExternGetterAndSetterProperties()).isEmpty();
assertThat(getLastCompiler().getSourceGetterAndSetterProperties()).isEmpty();
}
}

0 comments on commit 9b397f1

Please sign in to comment.