Enables access to protected properties from syntactically nested types.

The new approach also has substantially simpler logic. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=209184112
google · Aug 18, 2018 · 7e4b3ce · 7e4b3ce
1 parent 3f9dda2
commit 7e4b3ce
Show file tree

Hide file tree

Showing 3 changed files with 114 additions and 27 deletions.
diff --git a/src/com/google/javascript/jscomp/CheckAccessControls.java b/src/com/google/javascript/jscomp/CheckAccessControls.java
@@ -184,17 +184,8 @@ public void enterScope(NodeTraversal t) {
       deprecationDepth++;
     }
 
-    if (n.isFunction()) {
+    if (isFunctionOrClass(n)) {
-      JSType prevClass = currentClassStack.peek();
+      currentClassStack.push(bestInstanceTypeForMethodOrCtor(n, n.getParent()));
-      JSType currentClass =
-          (prevClass == null) ? bestInstanceTypeForMethodOrCtor(n, n.getParent()) : prevClass;
-      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);
     }
   }
 
@@ -223,6 +214,8 @@ public void exitScope(NodeTraversal t) {
    *   <li>Constructors => The type that constructor instantiates
    *   <li>Object literal members => {@code null}
    * </ul>
+   *
+   * TODO(nickreid): Remove the {@code parent} parameter.
    */
   @Nullable
   private JSType bestInstanceTypeForMethodOrCtor(Node n, Node parent) {
@@ -882,15 +875,20 @@ 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 = currentClassStack.peek();
+    for (JSType scopeType : currentClassStack) {
-    if (currentClass == null || !currentClass.isSubtypeOf(ownerType)) {
+      if (scopeType == null) {
-      compiler.report(
+        continue;
-          t.makeError(
+      } else if (scopeType.isSubtypeOf(ownerType)) {
-              propRef.getSourceNode(),
+        return;
-              BAD_PROTECTED_PROPERTY_ACCESS,
+      }
-              propRef.getName(),
-              propRef.getReadableTypeNameOrDefault()));
     }
+
+    compiler.report(
+        t.makeError(
+            propRef.getSourceNode(),
+            BAD_PROTECTED_PROPERTY_ACCESS,
+            propRef.getName(),
+            propRef.getReadableTypeNameOrDefault()));
   }
 
   /**

diff --git a/test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java b/test/com/google/javascript/jscomp/CheckAccessControlsEs6ClassTest.java
@@ -1056,7 +1056,7 @@ public void testProtectedAccessForProperties16() {
                 "}")));
   }
 
-  public void testProtectedAccessFromFunctionNestedInsideClass() {
+  public void testProtectedAccessThroughNestedFunction() {
     test(
         srcs(
             lines(
@@ -1065,15 +1065,15 @@ public void testProtectedAccessFromFunctionNestedInsideClass() {
                 "}"),
             lines(
                 "class Bar extends Foo {",
-                "  foo(/** Foo */ foo) {",
+                "  constructor(/** Foo */ foo) {",
                 "    function f() {",
                 "      foo.bar();",
                 "    }",
                 "  }",
                 "}")));
   }
 
-  public void testProtectedAccessFromClassNestedInsideClass() {
+  public void testProtectedAccessThroughNestedEs5Class() {
     test(
         srcs(
             lines(
@@ -1082,16 +1082,35 @@ public void testProtectedAccessFromClassNestedInsideClass() {
                 "}"),
             lines(
                 "class Bar extends Foo {",
-                "  foo() {",
+                "  constructor() {",
+                "    /** @constructor */",
+                "    const Nested = function() {}",
+                "",
+                "    /** @param {!Foo} foo */",
+                "    Nested.prototype.qux = function(foo) {",
+                "      foo.bar();",
+                "    }",
+                "  }",
+                "}")));
+  }
+
+  public void testProtectedAccessThroughNestedEs6Class() {
+    test(
+        srcs(
+            lines(
+                "class Foo {", //
+                "  /** @protected */ bar() {}",
+                "}"),
+            lines(
+                "class Bar extends Foo {",
+                "  constructor() {",
                 "    class Nested {",
-                "      foo(/** Foo */ foo) {",
+                "      qux(/** Foo */ foo) {",
                 "        foo.bar();",
                 "      }",
                 "    }",
                 "  }",
-                "}")),
+                "}")));
-        // TODO(sdh): This should not be an error.
-        error(BAD_PROTECTED_PROPERTY_ACCESS));
   }
 
   public void testNoProtectedAccessForProperties1() {

diff --git a/test/com/google/javascript/jscomp/CheckAccessControlsTest.java b/test/com/google/javascript/jscomp/CheckAccessControlsTest.java
@@ -870,6 +870,76 @@ public void testProtectedAccessForProperties16() {
         });
   }
 
+  public void testProtectedAccessThroughNestedFunction() {
+    test(
+        srcs(
+            lines(
+                "/** @constructor */", //
+                "function Foo() { }",
+                "",
+                "/** @protected */",
+                "Foo.prototype.bar = function() { };"),
+            lines(
+                "/**",
+                " * @constructor",
+                " * @extends {Foo}",
+                " */",
+                "function Bar() {",
+                "  function f(/** !Foo */ foo) {",
+                "    foo.bar();",
+                "  }",
+                "}")));
+  }
+
+  public void testProtectedAccessThroughNestedEs5Class() {
+    test(
+        srcs(
+            lines(
+                "/** @constructor */", //
+                "function Foo() { }",
+                "",
+                "/** @protected */",
+                "Foo.prototype.bar = function() { };"),
+            lines(
+                "/**",
+                " * @constructor",
+                " * @extends {Foo}",
+                " */",
+                "function Bar() {",
+                "  /** @constructor */",
+                "  var Nested = function() { }",
+                "",
+                "  /** @param {!Foo} foo */",
+                "  Nested.prototype.qux = function(foo) {",
+                "    foo.bar();",
+                "  }",
+                "}")));
+  }
+
+  public void testProtectedAccessThroughNestedEs6Class() {
+    test(
+        srcs(
+            lines(
+                "/** @constructor */", //
+                "function Foo() { }",
+                "",
+                "/** @protected */",
+                "Foo.prototype.bar = function() { };"),
+            lines(
+                "/**",
+                " * @constructor",
+                " * @extends {Foo}",
+                " */",
+                "function Bar() {",
+                "  class Nested {",
+                "    /** @param {!Foo} foo */",
+                "    qux(foo) {",
+                "      foo.bar();",
+                "    }",
+                "  }",
+                "}")));
+  }
+
   public void testNoProtectedAccessForProperties1() {
     testError(new String[] {
         "/** @constructor */ function Foo() {} "