Skip to content

Commit

Permalink
Track CLASS scopes in CheckAccessControls#currentClassStack
Browse files Browse the repository at this point in the history 
This removes spurious warnings from subclasses accessing protected members of their superclasses, but does not yet add correct enforcement of private access.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=206412321
  • Loading branch information
@shicks @blickly
shicks authored and blickly committed Jul 30, 2018
1 parent 3681d4d commit a98a012
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 60 deletions.
38 changes: 18 additions & 20 deletions src/com/google/javascript/jscomp/CheckAccessControls.java
View file
Expand Up @@ -31,7 +31,8 @@
import com.google.javascript.rhino.jstype.JSTypeNative;
import com.google.javascript.rhino.jstype.JSTypeRegistry;
import com.google.javascript.rhino.jstype.ObjectType;
import java.util.ArrayDeque;
import java.util.Deque;
import java.util.LinkedList;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -134,8 +135,10 @@ class CheckAccessControls extends AbstractPostOrderCallback

// State about the current traversal.
private int deprecatedDepth = 0;
private final ArrayDeque<JSType> currentClassStack = new ArrayDeque<>();
private final JSType noTypeSentinel;
// NOTE: LinkedList is almost always the wrong choice, but in this case we have at most a small
// handful of elements, it provides the smoothest API (push, pop, and a peek that doesn't throw
// on empty), and (unlike ArrayDeque) is null-permissive. No other option meets all these needs.
private final Deque<JSType> currentClassStack = new LinkedList<JSType>();

private ImmutableMap<StaticSourceFile, Visibility> defaultVisibilityForFiles;
private final Multimap<JSType, String> initializedConstantProperties;
Expand All @@ -147,7 +150,6 @@ class CheckAccessControls extends AbstractPostOrderCallback
this.typeRegistry = compiler.getTypeRegistry();
this.initializedConstantProperties = HashMultimap.create();
this.enforceCodingConventions = enforceCodingConventions;
this.noTypeSentinel = typeRegistry.getNativeType(JSTypeNative.NO_TYPE);
}

@Override
Expand Down Expand Up @@ -179,15 +181,17 @@ public void enterScope(NodeTraversal t) {
if (isDeprecatedFunction(n)) {
deprecatedDepth++;
}
JSType prevClass = getCurrentClass();
JSType prevClass = currentClassStack.peek();
JSType currentClass = prevClass == null
? getClassOfMethod(n, parent)
: prevClass;
// ArrayDeques can't handle nulls, so we reuse the bottom type
// as a null sentinel.
currentClassStack.addFirst(currentClass == null
? noTypeSentinel
: currentClass);
currentClassStack.push(currentClass);
} else if (n.isClass()) {
FunctionType ctor = JSType.toMaybeFunctionType(n.getJSType());
JSType instance = ctor != null && ctor.isConstructor() ? ctor.getInstanceType() : null;
// TODO(sdh): We should probably handle nested classes better, allowing them to access
// protected members of any enclosing class.
currentClassStack.push(instance);
}
}

Expand All @@ -199,12 +203,13 @@ public void exitScope(NodeTraversal t) {
deprecatedDepth--;
}
currentClassStack.pop();
} else if (n.isClass()) {
currentClassStack.pop();
}
}

/**
* Gets the type of the class that "owns" a method, or null if
* we know that its un-owned.
* Gets the instance type of the class that "owns" a method, or null if we know that its un-owned.
*/
private JSType getClassOfMethod(Node n, Node parent) {
checkState(n.isFunction(), n);
Expand Down Expand Up @@ -831,13 +836,6 @@ private void checkPackagePropertyVisibility(
}
}

@Nullable private JSType getCurrentClass() {
JSType cur = currentClassStack.peekFirst();
return cur == noTypeSentinel
? null
: cur;
}

private void checkPrivatePropertyVisibility(
NodeTraversal t,
Node getprop,
Expand Down Expand Up @@ -873,7 +871,7 @@ private void checkProtectedPropertyVisibility(
// 2) Overriding the property in a subclass
// 3) Accessing the property from inside a subclass
// The first two have already been checked for.
JSType currentClass = getCurrentClass();
JSType currentClass = currentClassStack.peek();
if (currentClass == null || !currentClass.isSubtypeOf(ownerType)) {
String propertyName = getprop.getLastChild().getString();
compiler.report(
Expand Down
93 changes: 53 additions & 40 deletions test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java
View file
Expand Up @@ -757,9 +757,7 @@ public void testProtectedAccessForProperties2() {
" constructor() {",
" this.bar();",
" }",
"}")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"}")));
}

public void testProtectedAccessForProperties3() {
Expand All @@ -775,9 +773,7 @@ public void testProtectedAccessForProperties3() {
lines(
"class SubFoo extends Foo {", //
" static baz() { (new Foo()).bar(); }",
"}")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"}")));
}

public void testProtectedAccessForProperties4() {
Expand All @@ -791,9 +787,7 @@ public void testProtectedAccessForProperties4() {
lines(
"class SubFoo extends Foo {", //
" static foo() { super.bar(); }",
"}")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"}")));
}

public void testProtectedAccessForProperties5() {
Expand All @@ -811,9 +805,7 @@ public void testProtectedAccessForProperties5() {
" super();",
" this.bar();",
" }",
"}")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"}")));
}

public void testProtectedAccessForProperties6() {
Expand All @@ -832,9 +824,7 @@ public void testProtectedAccessForProperties6() {
" super();",
" this.bar();",
" }",
"};")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"};")));
}

public void testProtectedAccessForProperties7() {
Expand All @@ -853,9 +843,7 @@ public void testProtectedAccessForProperties7() {
" }",
"};",
"",
"SubFoo.prototype.moo = function() { this.bar(); };")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"SubFoo.prototype.moo = function() { this.bar(); };")));
}

public void testProtectedAccessForProperties8() {
Expand All @@ -869,25 +857,23 @@ public void testProtectedAccessForProperties8() {
lines(
"var SubFoo = class extends Foo {", //
" get moo() { this.bar(); }",
"};")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"};")));
}

public void testProtectedAccessForProperties9() {
test(
srcs(
lines(
"var Foo = class {};", //
"/** @unrestricted */", //
"var Foo = class {};",
"",
"/** @protected */",
"Foo.prototype.bar = function() {};"),
lines(
"var SubFoo = class extends Foo {", //
"/** @unrestricted */", //
"var SubFoo = class extends Foo {",
" set moo(val) { this.x = this.bar(); }",
"};")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"};")));
}

public void testProtectedAccessForProperties10() {
Expand Down Expand Up @@ -933,9 +919,7 @@ public void testProtectedAccessForProperties11() {
" constructor() {",
" Foo.prop;",
" }",
"};"))),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"};"))));
}

public void testProtectedAccessForProperties12() {
Expand Down Expand Up @@ -963,9 +947,7 @@ public void testProtectedAccessForProperties12() {
"",
" this.prop.length;",
" }",
"};"))),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"};"))));
}

// FYI: Java warns for the b1.method access in c.js.
Expand Down Expand Up @@ -1016,9 +998,7 @@ public void testProtectedAccessForProperties13() {
" constructor(b1) {",
" var x = b1.method();",
" }",
"};"))),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"};"))));
}

public void testProtectedAccessForProperties14() {
Expand All @@ -1039,9 +1019,7 @@ public void testProtectedAccessForProperties14() {
" }",
"};",
"",
"OtherFoo.prototype.moo = function() { new Foo().bar(); };")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS));
"OtherFoo.prototype.moo = function() { new Foo().bar(); };")));
}

public void testProtectedAccessForProperties15() {
Expand Down Expand Up @@ -1078,9 +1056,44 @@ public void testProtectedAccessForProperties16() {
"class OtherFoo extends Foo {",
" constructor() { super(); var f = () => this.bar(); }",
" baz() { return () => this.bar(); }",
"}")));
}

public void testProtectedAccessFromFunctionNestedInsideClass() {
test(
srcs(
lines(
"class Foo {", //
" /** @protected */ bar() {}",
"}"),
lines(
"class Bar extends Foo {",
" foo(/** Foo */ foo) {",
" function f() {",
" foo.bar();",
" }",
" }",
"}")));
}

public void testProtectedAccessFromClassNestedInsideClass() {
test(
srcs(
lines(
"class Foo {", //
" /** @protected */ bar() {}",
"}"),
lines(
"class Bar extends Foo {",
" foo() {",
" class Nested {",
" foo(/** Foo */ foo) {",
" foo.bar();",
" }",
" }",
" }",
"}")),
// TODO(b/80580110): This should pass without warning.
error(BAD_PROTECTED_PROPERTY_ACCESS),
// TODO(sdh): This should not be an error.
error(BAD_PROTECTED_PROPERTY_ACCESS));
}

Expand Down

0 comments on commit a98a012

Please sign in to comment.