From 7e4b3ce19c9f991ad01a2d8f159e23ee0a0718bf Mon Sep 17 00:00:00 2001 From: nickreid Date: Fri, 17 Aug 2018 11:54:37 -0700 Subject: [PATCH] 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 --- .../jscomp/CheckAccessControls.java | 36 +++++----- .../CheckAccessControlsEs6ClassTest.java | 35 +++++++--- .../jscomp/CheckAccessControlsTest.java | 70 +++++++++++++++++++ 3 files changed, 114 insertions(+), 27 deletions(-) diff --git a/src/com/google/javascript/jscomp/CheckAccessControls.java b/src/com/google/javascript/jscomp/CheckAccessControls.java index 8183fea9482..249f2a84a27 100644 --- 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()) { - JSType prevClass = currentClassStack.peek(); - 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); + if (isFunctionOrClass(n)) { + currentClassStack.push(bestInstanceTypeForMethodOrCtor(n, n.getParent())); } } @@ -223,6 +214,8 @@ public void exitScope(NodeTraversal t) { *
  • Constructors => The type that constructor instantiates *
  • Object literal members => {@code null} * + * + * 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(); - if (currentClass == null || !currentClass.isSubtypeOf(ownerType)) { - compiler.report( - t.makeError( - propRef.getSourceNode(), - BAD_PROTECTED_PROPERTY_ACCESS, - propRef.getName(), - propRef.getReadableTypeNameOrDefault())); + for (JSType scopeType : currentClassStack) { + if (scopeType == null) { + continue; + } else if (scopeType.isSubtypeOf(ownerType)) { + return; + } } + + 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 index 90a43ef61ed..104120a2bbc 100644 --- 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,7 +1065,7 @@ public void testProtectedAccessFromFunctionNestedInsideClass() { "}"), lines( "class Bar extends Foo {", - " foo(/** Foo */ foo) {", + " constructor(/** Foo */ foo) {", " function f() {", " foo.bar();", " }", @@ -1073,7 +1073,7 @@ public void testProtectedAccessFromFunctionNestedInsideClass() { "}"))); } - 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 index 8568fcf57b7..d34f0e02195 100644 --- 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() {} "