Skip to content

Commit

Permalink
Update CheckAccessControls to bring checks on class-syntax up to pa…
Browse files Browse the repository at this point in the history 
…rity with old-style classes.

This change isn't intended to improve coverage of the checks, which are known to miss some cases. Such a change would be much harder to land and require more thought as to what the correct behaviour is in some ambiguous cases.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=208274192
  • Loading branch information
@nreid260 @lauraharker
nreid260 authored and lauraharker committed Aug 14, 2018
1 parent 50dfe7e commit 2425c50
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 91 deletions.
147 changes: 95 additions & 52 deletions src/com/google/javascript/jscomp/CheckAccessControls.java
View file
Expand Up @@ -15,7 +15,7 @@
*/ */
package com.google.javascript.jscomp; package com.google.javascript.jscomp;


import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Preconditions.checkState;


import com.google.auto.value.AutoValue; import com.google.auto.value.AutoValue;
Expand Down Expand Up @@ -137,7 +137,7 @@ class CheckAccessControls extends AbstractPostOrderCallback
private final boolean enforceCodingConventions; private final boolean enforceCodingConventions;


// State about the current traversal. // State about the current traversal.
private int deprecatedDepth = 0; private int deprecationDepth = 0;
// NOTE: LinkedList is almost always the wrong choice, but in this case we have at most a small // 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 // 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. // on empty), and (unlike ArrayDeque) is null-permissive. No other option meets all these needs.
Expand Down Expand Up @@ -179,15 +179,15 @@ public void hotSwapScript(Node scriptRoot, Node originalRoot) {
@Override @Override
public void enterScope(NodeTraversal t) { public void enterScope(NodeTraversal t) {
Node n = t.getScopeRoot(); Node n = t.getScopeRoot();

if (isDeprecationScopeRoot(n)) {
deprecationDepth++;
}

if (n.isFunction()) { if (n.isFunction()) {
Node parent = n.getParent();
if (isDeprecatedFunction(n)) {
deprecatedDepth++;
}
JSType prevClass = currentClassStack.peek(); JSType prevClass = currentClassStack.peek();
JSType currentClass = prevClass == null JSType currentClass =
? getClassOfMethod(n, parent) (prevClass == null) ? bestInstanceTypeForMethodOrCtor(n, n.getParent()) : prevClass;
: prevClass;
currentClassStack.push(currentClass); currentClassStack.push(currentClass);
} else if (n.isClass()) { } else if (n.isClass()) {
FunctionType ctor = JSType.toMaybeFunctionType(n.getJSType()); FunctionType ctor = JSType.toMaybeFunctionType(n.getJSType());
Expand All @@ -201,42 +201,56 @@ public void enterScope(NodeTraversal t) {
@Override @Override
public void exitScope(NodeTraversal t) { public void exitScope(NodeTraversal t) {
Node n = t.getScopeRoot(); Node n = t.getScopeRoot();

if (isDeprecationScopeRoot(n)) {
deprecationDepth--;
}

if (n.isFunction()) { if (n.isFunction()) {
if (isDeprecatedFunction(n)) {
deprecatedDepth--;
}
currentClassStack.pop(); currentClassStack.pop();
} else if (n.isClass()) { } else if (n.isClass()) {
currentClassStack.pop(); currentClassStack.pop();
} }
} }


/** /**
* Gets the instance type of the class that "owns" a method, or null if we know that its un-owned. * Returns the instance object type that best represents a method or constructor definition, or
* {@code null} if there is no representative type.
*
* <ul>
* <li>Prototype methods => The instance type having that prototype
* <li>Instance methods => The type the method is attached to
* <li>Constructors => The type that constructor instantiates
* <li>Object literal members => {@code null}
* </ul>
*/ */
private JSType getClassOfMethod(Node n, Node parent) { @Nullable
checkState(n.isFunction(), n); private JSType bestInstanceTypeForMethodOrCtor(Node n, Node parent) {
checkState(isFunctionOrClass(n), n);

if (parent.isAssign()) { if (parent.isAssign()) {
Node lValue = parent.getFirstChild(); Node lValue = parent.getFirstChild();
if (NodeUtil.isGet(lValue)) { if (NodeUtil.isGet(lValue)) {
// We have an assignment of the form "a.b = ...". // We have an assignment of the form `a.b = ...`.
JSType lValueType = lValue.getJSType(); JSType lValueType = lValue.getJSType();
if (lValueType != null && (lValueType.isConstructor() || lValueType.isInterface())) { if (lValueType != null && (lValueType.isConstructor() || lValueType.isInterface())) {
// If a.b is a constructor, then everything in this function // Case `a.B = ...`
// belongs to the "a.b" type. return normalizeClassType(lValueType);
return (lValueType.toMaybeFunctionType()).getInstanceType();
} else if (NodeUtil.isPrototypeProperty(lValue)) { } else if (NodeUtil.isPrototypeProperty(lValue)) {
return normalizeClassType( // Case `a.B.prototype = ...`
NodeUtil.getPrototypeClassName(lValue).getJSType()); return normalizeClassType(NodeUtil.getPrototypeClassName(lValue).getJSType());
} else { } else {
// Case `a.b = ...`
return normalizeClassType(lValue.getFirstChild().getJSType()); return normalizeClassType(lValue.getFirstChild().getJSType());
} }
} else { } else {
// We have an assignment of the form "a = ...", so pull the // We have an assignment of the form "a = ...", so pull the
// type off the "a". // type off the "a".
return normalizeClassType(lValue.getJSType()); return normalizeClassType(lValue.getJSType());
} }
} else if (NodeUtil.isFunctionDeclaration(n) || parent.isName()) { } else if (NodeUtil.isFunctionDeclaration(n)
|| NodeUtil.isClassDeclaration(n)
|| parent.isName()) {
return normalizeClassType(n.getJSType()); return normalizeClassType(n.getJSType());
} else if (parent.isStringKey() } else if (parent.isStringKey()
|| parent.isGetterDef() || parent.isGetterDef()
Expand Down Expand Up @@ -303,6 +317,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
checkConstructorDeprecation(t, n); checkConstructorDeprecation(t, n);
break; break;
case FUNCTION: case FUNCTION:
case CLASS:
checkFinalClassOverrides(t, n, parent); checkFinalClassOverrides(t, n, parent);
break; break;
default: default:
Expand All @@ -319,12 +334,16 @@ public void visit(NodeTraversal t, Node n, Node parent) {


/** Checks the given NEW node to ensure that access restrictions are obeyed. */ /** Checks the given NEW node to ensure that access restrictions are obeyed. */
private void checkConstructorDeprecation(NodeTraversal t, Node n) { private void checkConstructorDeprecation(NodeTraversal t, Node n) {
if (!shouldEmitDeprecationWarning(t, n)) {
return;
}

JSType type = n.getJSType(); JSType type = n.getJSType();


if (type != null) { if (type != null) {
String deprecationInfo = getTypeDeprecationInfo(type); String deprecationInfo = getTypeDeprecationInfo(type);


if (deprecationInfo != null && shouldEmitDeprecationWarning(t, n)) { if (deprecationInfo != null) {


if (!deprecationInfo.isEmpty()) { if (!deprecationInfo.isEmpty()) {
compiler.report( compiler.report(
Expand All @@ -342,6 +361,10 @@ private void checkConstructorDeprecation(NodeTraversal t, Node n) {
* Checks the given NAME node to ensure that access restrictions are obeyed. * Checks the given NAME node to ensure that access restrictions are obeyed.
*/ */
private void checkNameDeprecation(NodeTraversal t, Node n, Node parent) { private void checkNameDeprecation(NodeTraversal t, Node n, Node parent) {
if (!shouldEmitDeprecationWarning(t, n)) {
return;
}

// Don't bother checking definitions or constructors. // Don't bother checking definitions or constructors.
if (parent.isFunction() || NodeUtil.isNameDeclaration(parent) || parent.isNew()) { if (parent.isFunction() || NodeUtil.isNameDeclaration(parent) || parent.isNew()) {
return; return;
Expand All @@ -350,7 +373,7 @@ private void checkNameDeprecation(NodeTraversal t, Node n, Node parent) {
Var var = t.getScope().getVar(n.getString()); Var var = t.getScope().getVar(n.getString());
JSDocInfo docInfo = var == null ? null : var.getJSDocInfo(); JSDocInfo docInfo = var == null ? null : var.getJSDocInfo();


if (docInfo != null && docInfo.isDeprecated() && shouldEmitDeprecationWarning(t, n)) { if (docInfo != null && docInfo.isDeprecated()) {
if (docInfo.getDeprecationReason() != null) { if (docInfo.getDeprecationReason() != null) {
compiler.report( compiler.report(
t.makeError(n, DEPRECATED_NAME_REASON, n.getString(), t.makeError(n, DEPRECATED_NAME_REASON, n.getString(),
Expand All @@ -364,6 +387,10 @@ private void checkNameDeprecation(NodeTraversal t, Node n, Node parent) {


/** Checks the given GETPROP node to ensure that access restrictions are obeyed. */ /** Checks the given GETPROP node to ensure that access restrictions are obeyed. */
private void checkPropertyDeprecation(NodeTraversal t, PropertyReference propRef) { private void checkPropertyDeprecation(NodeTraversal t, PropertyReference propRef) {
if (!shouldEmitDeprecationWarning(t, propRef)) {
return;
}

// Don't bother checking constructors. // Don't bother checking constructors.
if (propRef.getSourceNode().getParent().isNew()) { if (propRef.getSourceNode().getParent().isNew()) {
return; return;
Expand All @@ -376,7 +403,7 @@ private void checkPropertyDeprecation(NodeTraversal t, PropertyReference propRef
String deprecationInfo String deprecationInfo
= getPropertyDeprecationInfo(objectType, propertyName); = getPropertyDeprecationInfo(objectType, propertyName);


if (deprecationInfo != null && shouldEmitDeprecationWarning(t, propRef)) { if (deprecationInfo != null) {


if (!deprecationInfo.isEmpty()) { if (!deprecationInfo.isEmpty()) {
compiler.report( compiler.report(
Expand Down Expand Up @@ -540,27 +567,29 @@ private void checkOverriddenPropertyVisibilityMismatch(
} }
} }


@Nullable private static Visibility getOverridingPropertyVisibility(Node parent) { @Nullable
JSDocInfo overridingInfo = parent.getJSDocInfo(); private static Visibility getOverridingPropertyVisibility(PropertyReference propRef) {
JSDocInfo overridingInfo = propRef.getJSDocInfo();
return overridingInfo == null || !overridingInfo.isOverride() return overridingInfo == null || !overridingInfo.isOverride()
? null ? null
: overridingInfo.getVisibility(); : overridingInfo.getVisibility();
} }


/** Checks if a constructor is trying to override a final class. */
private void checkFinalClassOverrides(NodeTraversal t, Node ctor, Node parent) {
checkArgument(isFunctionOrClass(ctor), ctor);



JSType type = ctor.getJSType().toMaybeFunctionType();
/**
* Checks if a constructor is trying to override a final class.
*/
private void checkFinalClassOverrides(NodeTraversal t, Node fn, Node parent) {
checkState(fn.isFunction(), fn);
JSType type = fn.getJSType().toMaybeFunctionType();
if (type != null && type.isConstructor()) { if (type != null && type.isConstructor()) {
JSType finalParentClass = getSuperClassInstanceIfFinal(getClassOfMethod(fn, parent)); JSType finalParentClass =
getSuperClassInstanceIfFinal(bestInstanceTypeForMethodOrCtor(ctor, parent));
if (finalParentClass != null) { if (finalParentClass != null) {
compiler.report( compiler.report(
t.makeError(fn, EXTEND_FINAL_CLASS, t.makeError(
type.getDisplayName(), finalParentClass.getDisplayName())); ctor,
EXTEND_FINAL_CLASS,
type.getDisplayName(),
finalParentClass.getDisplayName()));
} }
} }
} }
Expand Down Expand Up @@ -690,7 +719,7 @@ private void checkPropertyVisibility(NodeTraversal t, PropertyReference propRef)
enforceCodingConventions ? compiler.getCodingConvention() : null); enforceCodingConventions ? compiler.getCodingConvention() : null);


if (isOverride) { if (isOverride) {
Visibility overriding = getOverridingPropertyVisibility(propRef.getParentNode()); Visibility overriding = getOverridingPropertyVisibility(propRef);
if (overriding != null) { if (overriding != null) {
checkOverriddenPropertyVisibilityMismatch( checkOverriddenPropertyVisibilityMismatch(
overriding, visibility, fileOverviewVisibility, t, propRef); overriding, visibility, fileOverviewVisibility, t, propRef);
Expand Down Expand Up @@ -923,7 +952,8 @@ private boolean shouldEmitDeprecationWarning(NodeTraversal t, PropertyReference
} }


// Don't warn if the node is just declaring the property, not reading it. // Don't warn if the node is just declaring the property, not reading it.
if (propRef.isDeclaration() && propRef.getJSDocInfo().isDeprecated()) { JSDocInfo jsdoc = propRef.getJSDocInfo();
if (propRef.isDeclaration() && (jsdoc != null) && jsdoc.isDeprecated()) {
return false; return false;
} }


Expand All @@ -949,20 +979,25 @@ private boolean canAccessDeprecatedTypes(NodeTraversal t) {


return return
// Case #1 // Case #1
(deprecatedDepth > 0) (deprecationDepth > 0)
// Case #2 // Case #2
|| (getTypeDeprecationInfo(getTypeOfThis(scopeRoot)) != null) || (getTypeDeprecationInfo(getTypeOfThis(scopeRoot)) != null)
// Case #3 // Case #3
|| (scopeRootParent != null || (scopeRootParent != null
&& scopeRootParent.isAssign() && scopeRootParent.isAssign()
&& getTypeDeprecationInfo(getClassOfMethod(scopeRoot, scopeRootParent)) != null); && getTypeDeprecationInfo(bestInstanceTypeForMethodOrCtor(scopeRoot, scopeRootParent))
!= null);
} }


/** /**
* Returns whether this is a function node annotated as deprecated. * Returns whether this node roots a subtree under which references to deprecated constructs are
* allowed.
*/ */
private static boolean isDeprecatedFunction(Node n) { private static boolean isDeprecationScopeRoot(Node n) {
checkState(n.isFunction(), n); if (!isFunctionOrClass(n)) {
return false;
}

return getDeprecationReason(NodeUtil.getBestJSDocInfo(n)) != null; return getDeprecationReason(NodeUtil.getBestJSDocInfo(n)) != null;
} }


Expand Down Expand Up @@ -1061,13 +1096,18 @@ private static ObjectType castToObject(@Nullable JSType type) {
return type == null ? null : type.toMaybeObjectType(); return type == null ? null : type.toMaybeObjectType();
} }


static boolean isFunctionOrClass(Node n) {
return n.isFunction() || n.isClass();
}

@Nullable @Nullable
private JSType getTypeOfThis(Node scopeRoot) { private JSType getTypeOfThis(Node scopeRoot) {
if (scopeRoot.isRoot()) { if (scopeRoot.isRoot() || scopeRoot.isScript()) {
return castToObject(scopeRoot.getJSType()); return castToObject(scopeRoot.getJSType());
} }


checkState(scopeRoot.isFunction(), scopeRoot); checkArgument(scopeRoot.isFunction(), scopeRoot);

JSType nodeType = scopeRoot.getJSType(); JSType nodeType = scopeRoot.getJSType();
if (nodeType != null && nodeType.isFunctionType()) { if (nodeType != null && nodeType.isFunctionType()) {
return nodeType.toMaybeFunctionType().getTypeOfThis(); return nodeType.toMaybeFunctionType().getTypeOfThis();
Expand Down Expand Up @@ -1200,19 +1240,22 @@ private PropertyReference createPropertyReference(Node sourceNode) {
return null; return null;
} }


FunctionType ctorType =
checkNotNull(parent.getParent().getJSType().toMaybeFunctionType());

builder builder
.setName(sourceNode.getString()) .setName(sourceNode.getString())
.setReceiverType( .setReceiverType((ObjectType) typeRegistry.getNativeType(JSTypeNative.UNKNOWN_TYPE))
sourceNode.isStaticMember() ? ctorType : ctorType.getPrototypeObject())
.setMutation(true) .setMutation(true)
.setDeclaration(true) .setDeclaration(true)
// TODO(nickreid): This definition is way too loose. It was used to prevent breakages // TODO(nickreid): This definition is way too loose. It was used to prevent breakages
// during refactoring and should be tightened. // during refactoring and should be tightened.
.setOverride((jsdoc != null)) .setOverride(jsdoc != null)
.setReadableTypeName(() -> ""); // The default is fine for class types. .setReadableTypeName(() -> ""); // The default is fine for class types.

JSType ctorType = parent.getParent().getJSType();
if (ctorType != null && ctorType.isFunctionType()) {
FunctionType ctorFunctionType = ctorType.toMaybeFunctionType();
builder.setReceiverType(
sourceNode.isStaticMember() ? ctorFunctionType : ctorFunctionType.getPrototype());
}
} }
break; break;


Expand Down

0 comments on commit 2425c50

Please sign in to comment.